diff --git a/compiler/noirc_evaluator/src/ssa/interpreter/intrinsics.rs b/compiler/noirc_evaluator/src/ssa/interpreter/intrinsics.rs index dd52aab0fa4..93c94155858 100644 --- a/compiler/noirc_evaluator/src/ssa/interpreter/intrinsics.rs +++ b/compiler/noirc_evaluator/src/ssa/interpreter/intrinsics.rs @@ -1,4 +1,4 @@ -use std::io::Write; +use std::{hash::BuildHasher, io::Write}; use acvm::{AcirField, BlackBoxFunctionSolver, BlackBoxResolutionError, FieldElement}; use bn254_blackbox_solver::derive_generators; @@ -831,11 +831,16 @@ fn values_to_fields(values: &[Value]) -> Vec { // but that's catered for the by the SSA generation: the env is passed as separate values. fields.push(FieldElement::from(id.to_u32())); } - Value::Intrinsic(x) => { - panic!("didn't expect to print intrinsics: {x}") - } Value::ForeignFunction(x) => { - panic!("didn't expect to print foreign functions: {x}") + // The actual display of functions only shows the type, but expects the ID. + // Send a hash so we can interpret the Initial SSA until we wrap these values with a normal function. + let hash = rustc_hash::FxBuildHasher.hash_one(x); + fields.push(FieldElement::from(hash)); + } + Value::Intrinsic(x) => { + // Same as foreign functions: just pass something so we can handle the initial SSA even if somehow an intrinsic makes it here. + let hash = rustc_hash::FxBuildHasher.hash_one(x); + fields.push(FieldElement::from(hash)); } } // Chamber the length for a potential vector following it. diff --git a/compiler/noirc_evaluator/src/ssa/mod.rs b/compiler/noirc_evaluator/src/ssa/mod.rs index 85be9067262..0b9b58fdb0a 100644 --- a/compiler/noirc_evaluator/src/ssa/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/mod.rs @@ -395,7 +395,7 @@ pub fn create_program_with_minimal_passes( for func in &program.functions { assert!( func.unconstrained, - "The minimum SSA pipeline only works with Brillig: '{}' needs to be unconstrained", + "The minimal SSA pipeline only works with Brillig: '{}' needs to be unconstrained", func.name ); } diff --git a/compiler/noirc_evaluator/src/ssa/opt/defunctionalize.rs b/compiler/noirc_evaluator/src/ssa/opt/defunctionalize.rs index 0313dac741d..447dd363d94 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/defunctionalize.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/defunctionalize.rs @@ -59,7 +59,7 @@ use rustc_hash::FxHashMap as HashMap; /// fn apply(function_id: Field, arg1: Field, arg2: Field) -> Field { /// match function_id { /// 0 -> function0(arg1, arg2), -/// 1 -> function0(arg1, arg2), +/// 1 -> function1(arg1, arg2), /// ... /// N -> functionN(arg1, arg2), /// } @@ -75,7 +75,7 @@ struct ApplyFunction { } /// All functions used as a value that share the same signature and runtime type -/// Maps ([Signature], [RuntimeType]) -> Vec<[FunctionId]> +/// Maps ([Signature], Caller [RuntimeType]) -> Vec<([FunctionId], Callee [RuntimeType])> type Variants = BTreeMap<(Signature, RuntimeType), Vec<(FunctionId, RuntimeType)>>; /// All generated apply functions for each grouping of function variants. /// Each apply function is handles a specific ([Signature], [RuntimeType]) group. @@ -94,6 +94,10 @@ impl Ssa { /// See [`defunctionalize`][self] module for more information. #[tracing::instrument(level = "trace", skip(self))] pub(crate) fn defunctionalize(mut self) -> Ssa { + // Check that we have removed all cases we don't handle in this pass. + #[cfg(debug_assertions)] + self.functions.values().for_each(defunctionalize_pre_check); + // Find all functions used as value that share the same signature and runtime type let variants = find_variants(&self); @@ -360,10 +364,19 @@ fn find_variants(ssa: &Ssa) -> Variants { fn find_functions_as_values(func: &Function) -> BTreeSet { let mut functions_as_values: BTreeSet = BTreeSet::new(); - let mut process_value = |value_id: ValueId| { - if let Value::Function(id) = func.dfg[value_id] { - functions_as_values.insert(id); + visit_values_other_than_call_target(func, |value| { + if let Value::Function(id) = value { + functions_as_values.insert(*id); } + }); + + functions_as_values +} + +/// Visit all values which are *not* targets of a `Call`. +fn visit_values_other_than_call_target(func: &Function, mut f: impl FnMut(&Value)) { + let mut process_value = |value_id: ValueId| { + f(&func.dfg[value_id]); }; for block_id in func.reachable_blocks() { @@ -382,8 +395,6 @@ fn find_functions_as_values(func: &Function) -> BTreeSet { block.unwrap_terminator().for_each_value(&mut process_value); } - - functions_as_values } /// Finds all dynamic dispatch signatures in the given function. @@ -546,7 +557,7 @@ fn create_apply_function( caller_runtime: RuntimeType, function_ids: Vec<(FunctionId, RuntimeType)>, ) -> FunctionId { - assert!( + debug_assert!( function_ids.len() > 1, "create_apply_function is expected to be called with two or more FunctionIds" ); @@ -754,6 +765,19 @@ fn make_dummy_return_data(function_builder: &mut FunctionBuilder, typ: &Type) -> } } +/// Check pre-execution properties. +/// +/// Panics if: +/// * Any intrinsic or foreign function is passed as a value. +#[cfg(debug_assertions)] +fn defunctionalize_pre_check(function: &Function) { + visit_values_other_than_call_target(function, |value| match value { + Value::ForeignFunction(name) => panic!("foreign function as value: {name}"), + Value::Intrinsic(intrinsic) => panic!("intrinsic function as value: {intrinsic}"), + _ => (), + }); +} + /// Check post-execution properties: /// * All blocks which took function parameters should receive a discriminator instead #[cfg(debug_assertions)] @@ -1096,7 +1120,7 @@ mod tests { acir(inline) fn main f0 { b0(): call f1() - return f1 + return f1 } acir(inline) fn bar f1 { b0(): diff --git a/compiler/noirc_frontend/src/monomorphization/ast.rs b/compiler/noirc_frontend/src/monomorphization/ast.rs index cb83501bd55..28abc2db93e 100644 --- a/compiler/noirc_frontend/src/monomorphization/ast.rs +++ b/compiler/noirc_frontend/src/monomorphization/ast.rs @@ -529,6 +529,7 @@ pub enum Type { Tuple(Vec), Slice(Box), Reference(Box, /*mutable:*/ bool), + /// `(args, ret, env, unconstrained)` Function( /*args:*/ Vec, /*ret:*/ Box, diff --git a/compiler/noirc_frontend/src/monomorphization/mod.rs b/compiler/noirc_frontend/src/monomorphization/mod.rs index a15a65bfa21..b6cb0def59c 100644 --- a/compiler/noirc_frontend/src/monomorphization/mod.rs +++ b/compiler/noirc_frontend/src/monomorphization/mod.rs @@ -49,7 +49,9 @@ mod debug; pub mod debug_types; pub mod errors; pub mod printer; +pub mod proxies; pub mod tests; +pub mod visitor; struct LambdaContext { env_ident: ast::Ident, @@ -60,7 +62,7 @@ struct LambdaContext { /// /// This struct holds the FIFO queue of functions to monomorphize, which is added to /// whenever a new (function, type) combination is encountered. -pub(super) struct Monomorphizer<'interner> { +pub struct Monomorphizer<'interner> { /// Functions are keyed by their unique ID, whether they're unconstrained, their expected type, /// and any generics they have so that we can monomorphize a new version of the function for each type. /// @@ -203,8 +205,11 @@ pub fn monomorphize_debug( debug_variables, debug_functions, debug_types, - ); - Ok(program.handle_ownership()) + ) + .handle_ownership() + .create_foreign_proxies(); + + Ok(program) } impl<'interner> Monomorphizer<'interner> { @@ -1282,7 +1287,10 @@ impl<'interner> Monomorphizer<'interner> { } /// Convert a non-tuple/struct type to a monomorphized type - fn convert_type(typ: &HirType, location: Location) -> Result { + pub fn convert_type( + typ: &HirType, + location: Location, + ) -> Result { Self::convert_type_helper(typ, location, &mut HashSet::default()) } diff --git a/compiler/noirc_frontend/src/monomorphization/proxies.rs b/compiler/noirc_frontend/src/monomorphization/proxies.rs new file mode 100644 index 00000000000..d3fe7895888 --- /dev/null +++ b/compiler/noirc_frontend/src/monomorphization/proxies.rs @@ -0,0 +1,288 @@ +//! Implement a post-monomorphization pass where builtin/intrinsic/oracle functions used as values +//! are wrapped in a proxy function which replaces them as a value and forwards calls to them. +//! +//! The reason this exists is because by the time we get to the `defunctionalize` SSA pass it would +//! not be possible to deal with function pointers to foreign functions, because unlike for normal +//! functions, we don't have type information for them in the SSA. That is, we can't tell what their +//! parameter and return types are when they are just passed around as a value. +//! +//! The type information is still available in the monomorphized AST though, so we can find all +//! instances where a foreign function is referenced by an [`Ident`](crate::monomorphization::ast::Expression::Ident) +//! without actually being the target of a [`Call`](crate::monomorphization::ast::Expression::Call), +//! and replace them with a normal function, which will preserve the information we need create +//! dispatch functions for them in the `defunctionalize` pass. + +use std::collections::HashMap; + +use iter_extended::vecmap; +use noirc_errors::Location; + +use crate::{ + hir_def::function::FunctionSignature, + monomorphization::{ + ast::{ + Call, Definition, Expression, FuncId, Function, Ident, IdentId, InlineType, LocalId, + Program, Type, + }, + visitor::visit_expr_mut, + }, + shared::Visibility, +}; + +impl Program { + /// Create proxies for foreign functions used as values. + /// + /// See the [proxies](crate::monomorphization::proxies) for details. + /// + /// This should only be called once, before converting to SSA. + pub fn create_foreign_proxies(mut self) -> Self { + let mut context = ProxyContext::new(self.functions.len() as u32); + + // Replace foreign function identifier definitions with proxy function IDs. + for function in self.functions.iter_mut() { + context.visit_expr(&mut function.body); + } + + // Add new functions. + self.functions.extend(context.into_proxies()); + + self + } +} + +struct ProxyContext { + next_func_id: u32, + replacements: HashMap<(Definition, /*unconstrained*/ bool), FuncId>, + proxies: Vec<(FuncId, (Ident, /*unconstrained*/ bool))>, +} + +impl ProxyContext { + fn new(next_func_id: u32) -> Self { + Self { next_func_id, replacements: HashMap::new(), proxies: Vec::new() } + } + + fn next_func_id(&mut self) -> FuncId { + let id = self.next_func_id; + self.next_func_id += 1; + FuncId(id) + } + + /// Visit expressions and replace foreign function identifier definitions with newly allocated + /// function IDs, collecting the type information along the way so that we can create those + /// proxies after visiting all functions. + fn visit_expr(&mut self, expr: &mut Expression) { + visit_expr_mut(expr, &mut |expr| { + // Note that if we see a function in `Call::func` then it will be an `Ident`, not a `Tuple`, + // even though its `Ident::typ` will be a `Tuple([Function, Function])`, but since we only + // handle tuples, we don't have to skip the `Call::func` to leave it in tact. + + // If this is a foreign function value, we want to replace it with proxies. + let Some(mut pair) = ForeignFunctionValue::try_from(expr) else { + return true; + }; + + // Create a separate proxy for the constrained and unconstrained version. + pair.for_each(|ident, unconstrained| { + let key = (ident.definition.clone(), unconstrained); + + let proxy_id = match self.replacements.get(&key) { + Some(id) => *id, + None => { + let func_id = self.next_func_id(); + self.replacements.insert(key, func_id); + self.proxies.push((func_id, (ident.clone(), unconstrained))); + func_id + } + }; + + ident.definition = Definition::Function(proxy_id); + }); + + true + }); + } + + /// Create proxy functions for the foreign function values we discovered. + fn into_proxies(self) -> impl IntoIterator { + self.proxies + .into_iter() + .map(|(id, (ident, unconstrained))| make_proxy(id, ident, unconstrained)) + } +} + +/// When function values are passed around in the monomorphized AST, +/// they appear as a pair (tuple) of a constrained and unconstrained +/// function. +struct ForeignFunctionValue<'a> { + items: &'a mut Vec, +} + +impl<'a> ForeignFunctionValue<'a> { + /// Check if we have a pair of identifiers of foreign functions with + /// the same name, and return both `Ident`s for modification. + fn try_from(expr: &'a mut Expression) -> Option { + let Expression::Tuple(items) = expr else { + return None; + }; + if items.len() != 2 { + return None; + } + let Expression::Ident(c) = &items[0] else { + return None; + }; + let Expression::Ident(u) = &items[1] else { + return None; + }; + if c.definition != u.definition + || !is_foreign_func(&c.definition) + || !is_foreign_func(&u.definition) + || !is_func_pair(&c.typ) + || !is_func_pair(&u.typ) + { + return None; + } + Some(Self { items }) + } + + /// Apply a function on the constrained and unconstrained identifier. + fn for_each(&mut self, mut f: impl FnMut(&mut Ident, bool)) { + let Expression::Ident(c) = &mut self.items[0] else { unreachable!() }; + f(c, false); + let Expression::Ident(u) = &mut self.items[1] else { unreachable!() }; + f(u, true); + } +} + +/// Check if the definition is that of a function defined by a "name" rather than an ID. +fn is_foreign_func(definition: &Definition) -> bool { + matches!(definition, Definition::Builtin(_) | Definition::LowLevel(_) | Definition::Oracle(_)) +} + +/// Check that the identifier is of a pair of constrained and unconstrained function types. +fn is_func_pair(typ: &Type) -> bool { + let Type::Tuple(types) = typ else { + return false; + }; + types.len() == 2 + && matches!(types[0], Type::Function(_, _, _, false)) + && matches!(types[1], Type::Function(_, _, _, true)) +} + +/// Create a proxy function definition for a foreign function based on an identifier that got replaced. +/// +/// The body of the function will be a single forwarding call to the original. +fn make_proxy(id: FuncId, ident: Ident, unconstrained: bool) -> Function { + let Type::Tuple(items) = &ident.typ else { + unreachable!("ICE: expected pair of functions; got {}", ident.typ); + }; + + // Pick the version of the function that we need to forward to. + let func_idx = if unconstrained { 1 } else { 0 }; + let func_typ = items[func_idx].clone(); + let Type::Function(args, ret, _env, _) = func_typ else { + unreachable!("ICE: expected function type; got {}", ident.typ); + }; + + let name = format!("{}_proxy", ident.name); + + let mut next_ident_id = 0u32; + let mut next_ident_id = || { + let id = next_ident_id; + next_ident_id += 1; + IdentId(id) + }; + + let parameters = vecmap(args.into_iter().enumerate(), |(i, typ)| { + let id = LocalId(i as u32); + let mutable = false; + let name = format!("p{i}"); + let vis = Visibility::Private; + (id, mutable, name, typ, vis) + }); + + // The function signature only matters for entry points. + let func_sig = FunctionSignature::default(); + + let call = { + let func = Ident { + id: next_ident_id(), + location: None, + definition: ident.definition, + mutable: ident.mutable, + name: ident.name, + // The ident type still carries both function types in its definition. + typ: ident.typ, + }; + + let arguments = vecmap(¶meters, |(id, mutable, name, typ, _)| { + let parameter_ident = Ident { + location: None, + definition: Definition::Local(*id), + mutable: *mutable, + name: name.clone(), + typ: typ.clone(), + id: next_ident_id(), + }; + Expression::Ident(parameter_ident) + }); + + Call { + func: Box::new(Expression::Ident(func)), + arguments, + return_type: *ret.clone(), + location: Location::dummy(), + } + }; + + Function { + id, + name, + parameters, + body: Expression::Call(call), + return_type: *ret, + return_visibility: Visibility::Private, + unconstrained, + inline_type: InlineType::InlineAlways, + func_sig, + } +} + +#[cfg(test)] +mod tests { + use crate::test_utils::get_monomorphized_no_emit_test; + + #[test] + fn creates_proxies_for_oracle() { + let src = " + unconstrained fn main() { + foo(bar); + } + + unconstrained fn foo(f: unconstrained fn(Field) -> ()) { + f(0); + } + + #[oracle(my_oracle)] + unconstrained fn bar(f: Field) { + } + "; + + let program = get_monomorphized_no_emit_test(src).unwrap(); + insta::assert_snapshot!(program, @r" + unconstrained fn main$f0() -> () { + foo$f1((bar$f2, bar$f3)); + } + unconstrained fn foo$f1(f$l0: (fn(Field) -> (), unconstrained fn(Field) -> ())) -> () { + f$l0.1(0); + } + #[inline_always] + fn bar_proxy$f2(p0$l0: Field) -> () { + bar$my_oracle(p0$l0) + } + #[inline_always] + unconstrained fn bar_proxy$f3(p0$l0: Field) -> () { + bar$my_oracle(p0$l0) + } + "); + } +} diff --git a/tooling/ast_fuzzer/src/program/visitor.rs b/compiler/noirc_frontend/src/monomorphization/visitor.rs similarity index 99% rename from tooling/ast_fuzzer/src/program/visitor.rs rename to compiler/noirc_frontend/src/monomorphization/visitor.rs index a3ca1711a3d..b03469e04ed 100644 --- a/tooling/ast_fuzzer/src/program/visitor.rs +++ b/compiler/noirc_frontend/src/monomorphization/visitor.rs @@ -1,4 +1,4 @@ -use noirc_frontend::monomorphization::ast::{Expression, Ident, LValue, Literal}; +use super::ast::{Expression, Ident, LValue, Literal}; /// Visit all identifiers under the [Expression]. pub fn visit_ident_mut(expr: &mut Expression, v: &mut V) diff --git a/test_programs/execution_panic/regression_10117/Nargo.toml b/test_programs/compile_success_with_bug/regression_10117/Nargo.toml similarity index 100% rename from test_programs/execution_panic/regression_10117/Nargo.toml rename to test_programs/compile_success_with_bug/regression_10117/Nargo.toml diff --git a/test_programs/compile_success_with_bug/regression_10117/Prover.toml b/test_programs/compile_success_with_bug/regression_10117/Prover.toml new file mode 100644 index 00000000000..330f366dee1 --- /dev/null +++ b/test_programs/compile_success_with_bug/regression_10117/Prover.toml @@ -0,0 +1 @@ +input = [0] diff --git a/test_programs/execution_panic/regression_10117/src/main.nr b/test_programs/compile_success_with_bug/regression_10117/src/main.nr similarity index 98% rename from test_programs/execution_panic/regression_10117/src/main.nr rename to test_programs/compile_success_with_bug/regression_10117/src/main.nr index a191f3f0f50..eed1ca659c9 100644 --- a/test_programs/execution_panic/regression_10117/src/main.nr +++ b/test_programs/compile_success_with_bug/regression_10117/src/main.nr @@ -5,4 +5,4 @@ fn main(input: [u8; 1]) -> pub [u8; 32] { |_| [0_u8; 32] }; fun(input) -} \ No newline at end of file +} diff --git a/test_programs/execution_success/regression_10156/Nargo.toml b/test_programs/execution_success/regression_10156/Nargo.toml new file mode 100644 index 00000000000..88f7b8200d4 --- /dev/null +++ b/test_programs/execution_success/regression_10156/Nargo.toml @@ -0,0 +1,6 @@ +[package] +name = "regression_10156" +type = "bin" +authors = [""] + +[dependencies] \ No newline at end of file diff --git a/test_programs/execution_success/regression_10156/Prover.toml b/test_programs/execution_success/regression_10156/Prover.toml new file mode 100644 index 00000000000..7203617ffc4 --- /dev/null +++ b/test_programs/execution_success/regression_10156/Prover.toml @@ -0,0 +1 @@ +return = 0 diff --git a/test_programs/execution_success/regression_10156/src/main.nr b/test_programs/execution_success/regression_10156/src/main.nr new file mode 100644 index 00000000000..d968ba85a7b --- /dev/null +++ b/test_programs/execution_success/regression_10156/src/main.nr @@ -0,0 +1,42 @@ +pub fn main() -> pub Field { + println(foo); // normal + println(bar); // oracle + println(std::as_witness); // intrinsic + println(std::hash::blake2s::<10>); // builtin + + // safety: + unsafe { + unconstrained_take_bar(bar); + } + take_bar(bar); + + // safety: + unsafe { + unconstrained_take_as_witness(std::as_witness); + } + take_as_witness(std::as_witness); + std::as_witness(foo()); + + take_foo(foo) +} + +fn foo() -> Field { + 0 +} + +#[oracle(barnacle)] +unconstrained fn bar() -> Field {} + +fn take_foo(foo: fn() -> Field) -> Field { + foo() +} + +fn take_bar(_bar: unconstrained fn() -> Field) {} +unconstrained fn unconstrained_take_bar(_bar: unconstrained fn() -> Field) {} + +fn take_as_witness(aw: fn(Field) -> ()) { + aw(0) +} +unconstrained fn unconstrained_take_as_witness(aw: fn(Field) -> ()) { + aw(0) +} diff --git a/tooling/ast_fuzzer/fuzz/src/targets/orig_vs_morph.rs b/tooling/ast_fuzzer/fuzz/src/targets/orig_vs_morph.rs index 5c90230f587..7afbdd9879f 100644 --- a/tooling/ast_fuzzer/fuzz/src/targets/orig_vs_morph.rs +++ b/tooling/ast_fuzzer/fuzz/src/targets/orig_vs_morph.rs @@ -8,13 +8,13 @@ use crate::{compare_results_compiled, compile_into_circuit_or_die, default_ssa_o use arbitrary::{Arbitrary, Unstructured}; use color_eyre::eyre; use noir_ast_fuzzer::compare::{CompareMorph, CompareOptions}; +use noir_ast_fuzzer::rewrite; use noir_ast_fuzzer::scope::ScopeStack; -use noir_ast_fuzzer::visitor::visit_expr_be_mut; -use noir_ast_fuzzer::{rewrite, visitor}; use noirc_frontend::ast::UnaryOp; use noirc_frontend::monomorphization::ast::{ Call, Definition, Expression, Function, Ident, IdentId, LocalId, Program, Unary, }; +use noirc_frontend::monomorphization::visitor::{visit_expr, visit_expr_be_mut}; pub fn fuzz(u: &mut Unstructured) -> eyre::Result<()> { let config = default_config(u)?; @@ -272,7 +272,7 @@ fn estimate_applicable_rules( rules: &[rules::Rule], ) -> usize { let mut count = 0; - visitor::visit_expr(expr, &mut |expr| { + visit_expr(expr, &mut |expr| { for rule in rules { if rule.matches(ctx, expr) { count += 1; @@ -624,10 +624,13 @@ mod helpers { use std::{cell::RefCell, collections::HashMap, sync::OnceLock}; use arbitrary::Unstructured; - use noir_ast_fuzzer::{Config, expr, types, visitor::visit_expr_be_mut}; + use noir_ast_fuzzer::{Config, expr, types}; use noirc_frontend::{ ast::{IntegerBitSize, UnaryOp}, - monomorphization::ast::{BinaryOp, Definition, Expression, LocalId, Type}, + monomorphization::{ + ast::{BinaryOp, Definition, Expression, LocalId, Type}, + visitor::visit_expr_be_mut, + }, shared::Signedness, }; use strum::IntoEnumIterator; diff --git a/tooling/ast_fuzzer/src/lib.rs b/tooling/ast_fuzzer/src/lib.rs index 5276a672218..9e40bca3d5d 100644 --- a/tooling/ast_fuzzer/src/lib.rs +++ b/tooling/ast_fuzzer/src/lib.rs @@ -16,7 +16,7 @@ pub use program::{ DisplayAstAsNoir, DisplayAstAsNoirComptime, arb_program, arb_program_comptime, program_wrap_expression, }; -pub use program::{expr, rewrite, scope, types, visitor}; +pub use program::{expr, rewrite, scope, types}; /// AST generation configuration. #[derive(Debug, Clone)] diff --git a/tooling/ast_fuzzer/src/program/expr.rs b/tooling/ast_fuzzer/src/program/expr.rs index 9258d716d6c..e2e5d905059 100644 --- a/tooling/ast_fuzzer/src/program/expr.rs +++ b/tooling/ast_fuzzer/src/program/expr.rs @@ -6,16 +6,19 @@ use nargo::errors::Location; use arbitrary::{Arbitrary, Unstructured}; use noirc_frontend::{ ast::{BinaryOpKind, IntegerBitSize, UnaryOp}, - monomorphization::ast::{ - ArrayLiteral, Assign, Binary, BinaryOp, Call, Cast, Definition, Expression, FuncId, Ident, - IdentId, If, LValue, Let, Literal, LocalId, Type, Unary, + monomorphization::{ + ast::{ + ArrayLiteral, Assign, Binary, BinaryOp, Call, Cast, Definition, Expression, FuncId, + Ident, IdentId, If, LValue, Let, Literal, LocalId, Type, Unary, + }, + visitor::visit_expr, }, signed_field::SignedField, }; use crate::Config; -use super::{Name, VariableId, types, visitor::visit_expr}; +use super::{Name, VariableId, types}; /// Boolean literal. pub fn lit_bool(value: bool) -> Expression { diff --git a/tooling/ast_fuzzer/src/program/func.rs b/tooling/ast_fuzzer/src/program/func.rs index 4a04c634135..fadda113a66 100644 --- a/tooling/ast_fuzzer/src/program/func.rs +++ b/tooling/ast_fuzzer/src/program/func.rs @@ -64,7 +64,7 @@ impl FunctionDeclaration { .collect(); let return_type = - (!types::is_unit(&self.return_type)).then(|| types::to_hir_type(&self.return_type)); + (!types::is_unit(&self.return_type)).then_some(types::to_hir_type(&self.return_type)); (param_types, return_type) } @@ -1466,6 +1466,7 @@ impl<'a> FunctionContext<'a> { // but it takes 2 more arguments: the type descriptor and the format string marker, // which are inserted automatically by the monomorphizer. let param_types = vec![Type::Bool, typ.clone()]; + let hir_type = types::to_hir_type(typ); let ident = self.local_ident(id); diff --git a/tooling/ast_fuzzer/src/program/mod.rs b/tooling/ast_fuzzer/src/program/mod.rs index 0e816cab701..50ea3a7445b 100644 --- a/tooling/ast_fuzzer/src/program/mod.rs +++ b/tooling/ast_fuzzer/src/program/mod.rs @@ -25,7 +25,6 @@ mod func; pub mod rewrite; pub mod scope; pub mod types; -pub mod visitor; #[cfg(test)] mod tests; diff --git a/tooling/ast_fuzzer/src/program/rewrite/limit.rs b/tooling/ast_fuzzer/src/program/rewrite/limit.rs index e53e57d64b8..e2497d6a0d8 100644 --- a/tooling/ast_fuzzer/src/program/rewrite/limit.rs +++ b/tooling/ast_fuzzer/src/program/rewrite/limit.rs @@ -4,15 +4,18 @@ use arbitrary::Unstructured; use nargo::errors::Location; use noirc_frontend::{ ast::BinaryOpKind, - monomorphization::ast::{ - Call, Definition, Expression, FuncId, Function, Ident, IdentId, LocalId, Program, Type, + monomorphization::{ + ast::{ + Call, Definition, Expression, FuncId, Function, Ident, IdentId, LocalId, Program, Type, + }, + visitor::visit_expr_mut, }, shared::Visibility, }; use crate::{ Config, - program::{Context, VariableId, expr, types, visitor::visit_expr_mut}, + program::{Context, VariableId, expr, types}, }; use super::next_local_and_ident_id; diff --git a/tooling/ast_fuzzer/src/program/rewrite/mod.rs b/tooling/ast_fuzzer/src/program/rewrite/mod.rs index 1efdccdf3a5..9e7095e4f8e 100644 --- a/tooling/ast_fuzzer/src/program/rewrite/mod.rs +++ b/tooling/ast_fuzzer/src/program/rewrite/mod.rs @@ -1,12 +1,10 @@ -use noirc_frontend::monomorphization::ast::{ - Call, Expression, Function, Ident, LocalId, Program, Type, -}; - -use super::{ - expr, types, +use noirc_frontend::monomorphization::{ + ast::{Call, Expression, Function, Ident, LocalId, Program, Type}, visitor::{visit_expr, visit_expr_mut}, }; +use super::{expr, types}; + mod limit; mod unreachable; diff --git a/tooling/ast_fuzzer/src/program/rewrite/unreachable.rs b/tooling/ast_fuzzer/src/program/rewrite/unreachable.rs index fd15c405a8e..a0bff1d2e5e 100644 --- a/tooling/ast_fuzzer/src/program/rewrite/unreachable.rs +++ b/tooling/ast_fuzzer/src/program/rewrite/unreachable.rs @@ -1,9 +1,12 @@ use std::collections::{BTreeSet, VecDeque}; use im::HashMap; -use noirc_frontend::monomorphization::ast::{Definition, Expression, FuncId, Ident, Program}; +use noirc_frontend::monomorphization::{ + ast::{Definition, Expression, FuncId, Ident, Program}, + visitor::visit_expr_mut, +}; -use crate::{expr, program::Context, visitor::visit_expr_mut}; +use crate::{expr, program::Context}; /// Remove functions that are unreachable from main. pub(crate) fn remove_unreachable_functions(ctx: &mut Context) { diff --git a/tooling/ast_fuzzer/src/program/tests.rs b/tooling/ast_fuzzer/src/program/tests.rs index 5ea15a529ee..9673a1a5230 100644 --- a/tooling/ast_fuzzer/src/program/tests.rs +++ b/tooling/ast_fuzzer/src/program/tests.rs @@ -3,14 +3,17 @@ use nargo::errors::Location; use noirc_evaluator::{assert_ssa_snapshot, ssa::ssa_gen}; use noirc_frontend::{ ast::IntegerBitSize, - monomorphization::ast::{ - Call, Definition, Expression, For, FuncId, Function, Ident, IdentId, InlineType, LocalId, - Program, Type, + monomorphization::{ + Monomorphizer, + ast::{ + Call, Definition, Expression, For, FuncId, Function, Ident, IdentId, InlineType, + LocalId, Program, Type, + }, }, shared::Visibility, }; -use crate::{Config, program::FunctionDeclaration}; +use crate::{Config, arb_program, program::FunctionDeclaration, types}; use super::{Context, DisplayAstAsNoir}; @@ -243,3 +246,40 @@ fn test_recursion_limit_rewrite() { } "); } + +/// Test that if we generate a random program, then all of the functions' HIR type signature +/// can be turned into an AST type and back and yield the same result. +/// +/// This is not generally true for real Noir programs with e.g. `struct`s in them, but for +/// HIR types that were derived from AST types, the transformation should be idempotent. +#[test] +fn test_to_hir_type_roundtrip() { + arbtest::arbtest(|u| { + let config = Config::default(); + let program = arb_program(u, config)?; + + // `program.function_signatures` only contains the `main` function. + for func in program.functions { + let hir_types = func + .func_sig + .0 + .into_iter() + .map(|(_, typ, _)| typ) + .chain(func.func_sig.1.into_iter()); + + for hir_type0 in hir_types { + let mono_type0 = + Monomorphizer::convert_type(&hir_type0, Location::dummy()).unwrap(); + let hir_type1 = types::to_hir_type(&mono_type0); + // Need a second pass to get rid of any inconsistency in the constrainedness of functions. + let mono_type1 = + Monomorphizer::convert_type(&hir_type1, Location::dummy()).unwrap(); + let hir_type2 = types::to_hir_type(&mono_type1); + assert_eq!(hir_type1, hir_type2); + } + } + + Ok(()) + }) + .run(); +} diff --git a/tooling/ast_fuzzer/src/program/types.rs b/tooling/ast_fuzzer/src/program/types.rs index d75ed79414a..30b69630a1a 100644 --- a/tooling/ast_fuzzer/src/program/types.rs +++ b/tooling/ast_fuzzer/src/program/types.rs @@ -3,7 +3,6 @@ use std::collections::HashSet; use iter_extended::vecmap; use noirc_frontend::{ ast::{BinaryOpKind, IntegerBitSize}, - hir_def, monomorphization::ast::{BinaryOp, Type}, shared::Signedness, signed_field::SignedField, @@ -141,44 +140,6 @@ pub fn types_produced(typ: &Type) -> HashSet { acc } -pub fn to_hir_type(typ: &Type) -> hir_def::types::Type { - use hir_def::types::{Kind as HirKind, Type as HirType}; - - // Meet the expectations of `Type::evaluate_to_u32`. - let size_const = |size: u32| { - Box::new(HirType::Constant( - SignedField::from(size), - HirKind::Numeric(Box::new(HirType::Integer( - Signedness::Unsigned, - IntegerBitSize::ThirtyTwo, - ))), - )) - }; - - match typ { - Type::Unit => HirType::Unit, - Type::Bool => HirType::Bool, - Type::Field => HirType::FieldElement, - Type::Integer(signedness, integer_bit_size) => { - HirType::Integer(*signedness, *integer_bit_size) - } - Type::String(size) => HirType::String(size_const(*size)), - Type::Array(size, typ) => HirType::Array(size_const(*size), Box::new(to_hir_type(typ))), - Type::Tuple(items) => HirType::Tuple(items.iter().map(to_hir_type).collect()), - Type::Function(param_types, return_type, env_type, unconstrained) => HirType::Function( - vecmap(param_types, to_hir_type), - Box::new(to_hir_type(return_type)), - Box::new(to_hir_type(env_type)), - *unconstrained, - ), - Type::Reference(typ, mutable) => HirType::Reference(Box::new(to_hir_type(typ)), *mutable), - Type::Slice(typ) => HirType::Slice(Box::new(to_hir_type(typ))), - Type::FmtString(_, _) => { - unreachable!("unexpected type converting to HIR: {}", typ) - } - } -} - /// Check if the type is a number. pub fn is_numeric(typ: &Type) -> bool { matches!(typ, Type::Field | Type::Integer(_, _)) @@ -352,3 +313,69 @@ pub fn can_binary_op_return_from_input(op: &BinaryOp, input: &Type, output: &Typ pub fn ref_mut(typ: Type) -> Type { Type::Reference(Box::new(typ), true) } + +/// Convert the type back into a HIR equivalent (not necessarily the original HIR type). +/// +/// Aims to maintain parity with [Monomorphizer::convert_type](noirc_frontend::monomorphization::Monomorphizer::convert_type). +pub fn to_hir_type(typ: &Type) -> noirc_frontend::Type { + use noirc_frontend::{Kind as HirKind, Type as HirType}; + + // Meet the expectations of `Type::evaluate_to_u32`. + fn size_const(size: u32) -> Box { + Box::new(HirType::Constant( + SignedField::from(size), + HirKind::Numeric(Box::new(HirType::Integer( + Signedness::Unsigned, + IntegerBitSize::ThirtyTwo, + ))), + )) + } + + // Inverse of HirType::Function -> Type::Tuple([Type::Function, Type::Function]) + fn maybe_func(items: &[Type]) -> Option { + if items.len() != 2 { + return None; + } + let Type::Function(args0, ret0, env0, false) = &items[0] else { + return None; + }; + let Type::Function(args1, ret1, env1, true) = &items[1] else { + return None; + }; + if args0 != args1 || ret0 != ret1 || env0 != env1 { + return None; + } + let func = HirType::Function( + vecmap(args0, to_hir_type), + Box::new(to_hir_type(ret0)), + Box::new(to_hir_type(env0)), + // Assume unconstrained. + false, + ); + Some(func) + } + + match typ { + Type::Unit => HirType::Unit, + Type::Bool => HirType::Bool, + Type::Field => HirType::FieldElement, + Type::Integer(signedness, integer_bit_size) => { + HirType::Integer(*signedness, *integer_bit_size) + } + Type::String(size) => HirType::String(size_const(*size)), + Type::Array(size, typ) => HirType::Array(size_const(*size), Box::new(to_hir_type(typ))), + Type::Reference(typ, mutable) => HirType::Reference(Box::new(to_hir_type(typ)), *mutable), + Type::Slice(typ) => HirType::Slice(Box::new(to_hir_type(typ))), + Type::FmtString(size, typ) => { + HirType::FmtString(size_const(*size), Box::new(to_hir_type(typ))) + } + Type::Tuple(items) => maybe_func(items) + .unwrap_or_else(|| HirType::Tuple(items.iter().map(to_hir_type).collect())), + Type::Function(args, ret, env, unconstrained) => HirType::Function( + vecmap(args, to_hir_type), + Box::new(to_hir_type(ret)), + Box::new(to_hir_type(env)), + *unconstrained, + ), + } +} diff --git a/tooling/ast_fuzzer/tests/calibration.rs b/tooling/ast_fuzzer/tests/calibration.rs index 71f24c3949e..b873f9b956e 100644 --- a/tooling/ast_fuzzer/tests/calibration.rs +++ b/tooling/ast_fuzzer/tests/calibration.rs @@ -11,8 +11,11 @@ use std::{collections::BTreeMap, ops::RangeInclusive}; use arbtest::arbtest; -use noir_ast_fuzzer::{Config, arb_program, visitor::visit_expr}; -use noirc_frontend::monomorphization::ast::{Expression, Type}; +use noir_ast_fuzzer::{Config, arb_program}; +use noirc_frontend::monomorphization::{ + ast::{Expression, Type}, + visitor::visit_expr, +}; #[test] fn arb_program_freqs_in_expected_range() { diff --git a/tooling/nargo_cli/build.rs b/tooling/nargo_cli/build.rs index 67ef33311f0..6f8f36aec7f 100644 --- a/tooling/nargo_cli/build.rs +++ b/tooling/nargo_cli/build.rs @@ -132,7 +132,7 @@ const IGNORED_INTERPRET_EXECUTION_TESTS: [&str; 2] = [ ]; /// `nargo execute --minimal-ssa` ignored tests -const IGNORED_MINIMAL_EXECUTION_TESTS: [&str; 13] = [ +const IGNORED_MINIMAL_EXECUTION_TESTS: [&str; 14] = [ // internal error: entered unreachable code: unsupported function call type Intrinsic(AssertConstant) // These tests contain calls to `assert_constant`, which are evaluated and removed in the full SSA // pipeline, but in the minimal they are untouched, and trying to remove them causes a failure because @@ -148,8 +148,9 @@ const IGNORED_MINIMAL_EXECUTION_TESTS: [&str; 13] = [ "pedersen_commitment", "simple_shield", "strings", - // The minimum SSA pipeline only works with Brillig: \'zeroed_lambda\' needs to be unconstrained + // The minimal SSA pipeline only works with Brillig: \'zeroed_lambda\' needs to be unconstrained "lambda_from_dynamic_if", + "regression_10156", // This relies on maximum inliner setting "reference_counts_inliner_max", ]; diff --git a/tooling/nargo_cli/tests/snapshots/compile_success_with_bug/regression_10117/execute__tests__expanded.snap b/tooling/nargo_cli/tests/snapshots/compile_success_with_bug/regression_10117/execute__tests__expanded.snap new file mode 100644 index 00000000000..85ce5ff6c9e --- /dev/null +++ b/tooling/nargo_cli/tests/snapshots/compile_success_with_bug/regression_10117/execute__tests__expanded.snap @@ -0,0 +1,12 @@ +--- +source: tooling/nargo_cli/tests/execute.rs +expression: expanded_code +--- +fn main(input: [u8; 1]) -> pub [u8; 32] { + let fun: fn([u8; 1]) -> [u8; 32] = if input[0_u32] == 0_u8 { + std::hash::blake2s + } else { + |_: [u8; 1]| -> [u8; 32] [0_u8; 32] + }; + fun(input) +} diff --git a/tooling/nargo_cli/tests/snapshots/compile_success_with_bug/regression_10117/execute__tests__stderr.snap b/tooling/nargo_cli/tests/snapshots/compile_success_with_bug/regression_10117/execute__tests__stderr.snap new file mode 100644 index 00000000000..f4cb23b8d74 --- /dev/null +++ b/tooling/nargo_cli/tests/snapshots/compile_success_with_bug/regression_10117/execute__tests__stderr.snap @@ -0,0 +1,12 @@ +--- +source: tooling/nargo_cli/tests/execute.rs +expression: stderr +--- +bug: Brillig function call isn't properly covered by a manual constraint + ┌─ src/main.nr:7:5 + │ +7 │ fun(input) + │ ---------- This Brillig call's inputs and its return values haven't been sufficiently constrained. This should be done to prevent potential soundness vulnerabilities + │ + = Call stack: + 1. src/main.nr:7:5 diff --git a/tooling/nargo_cli/tests/snapshots/execution_success/regression_10156/execute__tests__expanded.snap b/tooling/nargo_cli/tests/snapshots/execution_success/regression_10156/execute__tests__expanded.snap new file mode 100644 index 00000000000..2c1d2bb5a6f --- /dev/null +++ b/tooling/nargo_cli/tests/snapshots/execution_success/regression_10156/execute__tests__expanded.snap @@ -0,0 +1,41 @@ +--- +source: tooling/nargo_cli/tests/execute.rs +expression: expanded_code +--- +pub fn main() -> pub Field { + println(foo); + println(bar); + println(std::as_witness); + println(std::hash::blake2s::<10>); + // Safety: comment added by `nargo expand` + unsafe { unconstrained_take_bar(bar); }; + take_bar(bar); + // Safety: comment added by `nargo expand` + unsafe { unconstrained_take_as_witness(std::as_witness); }; + take_as_witness(std::as_witness); + std::as_witness(foo()); + take_foo(foo) +} + +fn foo() -> Field { + 0_Field +} + +#[oracle(barnacle)] +unconstrained fn bar() -> Field {} + +fn take_foo(foo: fn() -> Field) -> Field { + foo() +} + +fn take_bar(_bar: unconstrained fn() -> Field) {} + +unconstrained fn unconstrained_take_bar(_bar: unconstrained fn() -> Field) {} + +fn take_as_witness(aw: fn(Field)) { + aw(0_Field) +} + +unconstrained fn unconstrained_take_as_witness(aw: fn(Field)) { + aw(0_Field) +} diff --git a/tooling/nargo_cli/tests/snapshots/execution_success/regression_10156/execute__tests__stdout.snap b/tooling/nargo_cli/tests/snapshots/execution_success/regression_10156/execute__tests__stdout.snap new file mode 100644 index 00000000000..5eb8194fa08 --- /dev/null +++ b/tooling/nargo_cli/tests/snapshots/execution_success/regression_10156/execute__tests__stdout.snap @@ -0,0 +1,9 @@ +--- +source: tooling/nargo_cli/tests/execute.rs +expression: stdout +--- +(< Field>>, < Field>>) +(< Field>>, < Field>>) +(< ()>>, < ()>>) +(< [u8; 32]>>, < [u8; 32]>>) +[regression_10156] Circuit output: 0x00