From 4624549048296e34c2fe33c03fca264e35174cc1 Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Tue, 14 Jan 2025 10:54:39 +0000 Subject: [PATCH 1/9] chore: clippy warning fix (#7051) --- compiler/noirc_frontend/src/elaborator/types.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/types.rs b/compiler/noirc_frontend/src/elaborator/types.rs index 72e46d36c2..8d6447d19d 100644 --- a/compiler/noirc_frontend/src/elaborator/types.rs +++ b/compiler/noirc_frontend/src/elaborator/types.rs @@ -1434,12 +1434,8 @@ impl<'context> Elaborator<'context> { .filter_map(|trait_id| { let trait_ = self.interner.get_trait(*trait_id); let trait_name = &trait_.name; - let Some(map) = module_data.scope().types().get(trait_name) else { - return None; - }; - let Some(imported_item) = map.get(&None) else { - return None; - }; + let map = module_data.scope().types().get(trait_name)?; + let imported_item = map.get(&None)?; if imported_item.0 == ModuleDefId::TraitId(*trait_id) { Some((*trait_id, trait_name)) } else { From 36f9751b0269f218d147cc708b53700450e07530 Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Tue, 14 Jan 2025 14:39:24 +0000 Subject: [PATCH 2/9] chore: add `noir_check_shuffle` as a critical library (#7056) --- .github/critical_libraries_status/.gitignore | 3 +++ .../noir-lang/noir_check_shuffle/.failures.jsonl | 0 CRITICAL_NOIR_LIBRARIES | 1 + 3 files changed, 4 insertions(+) create mode 100644 .github/critical_libraries_status/.gitignore create mode 100644 .github/critical_libraries_status/noir-lang/noir_check_shuffle/.failures.jsonl diff --git a/.github/critical_libraries_status/.gitignore b/.github/critical_libraries_status/.gitignore new file mode 100644 index 0000000000..38a3cf9caf --- /dev/null +++ b/.github/critical_libraries_status/.gitignore @@ -0,0 +1,3 @@ +.actual.jsonl +.actual.jsonl.jq +.failures.jsonl.jq \ No newline at end of file diff --git a/.github/critical_libraries_status/noir-lang/noir_check_shuffle/.failures.jsonl b/.github/critical_libraries_status/noir-lang/noir_check_shuffle/.failures.jsonl new file mode 100644 index 0000000000..e69de29bb2 diff --git a/CRITICAL_NOIR_LIBRARIES b/CRITICAL_NOIR_LIBRARIES index c753b76a4f..7637d9ac6d 100644 --- a/CRITICAL_NOIR_LIBRARIES +++ b/CRITICAL_NOIR_LIBRARIES @@ -1,3 +1,4 @@ +https://github.com/noir-lang/noir_check_shuffle https://github.com/noir-lang/ec https://github.com/noir-lang/eddsa https://github.com/noir-lang/mimc From 31dbfe34d88a1fc826e954a6a763e665fda58511 Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Tue, 14 Jan 2025 15:36:14 +0000 Subject: [PATCH 3/9] chore: reduce number of iterations of `acvm::compiler::compile` (#7050) --- acvm-repo/acvm/src/compiler/mod.rs | 41 ++----------------- .../acvm/src/compiler/transformers/mod.rs | 23 +++++++++-- 2 files changed, 23 insertions(+), 41 deletions(-) diff --git a/acvm-repo/acvm/src/compiler/mod.rs b/acvm-repo/acvm/src/compiler/mod.rs index daedd77c4a..ee84f7bf60 100644 --- a/acvm-repo/acvm/src/compiler/mod.rs +++ b/acvm-repo/acvm/src/compiler/mod.rs @@ -16,10 +16,6 @@ pub use simulator::CircuitSimulator; use transformers::transform_internal; pub use transformers::{transform, MIN_EXPRESSION_WIDTH}; -/// We need multiple passes to stabilize the output. -/// The value was determined by running tests. -const MAX_OPTIMIZER_PASSES: usize = 3; - /// This module moves and decomposes acir opcodes. The transformation map allows consumers of this module to map /// metadata they had about the opcodes to the new opcode structure generated after the transformation. #[derive(Debug)] @@ -84,41 +80,10 @@ pub fn compile( ) -> (Circuit, AcirTransformationMap) { let acir_opcode_positions = (0..acir.opcodes.len()).collect::>(); - if MAX_OPTIMIZER_PASSES == 0 { - return (acir, AcirTransformationMap::new(&acir_opcode_positions)); - } - - let mut pass = 0; - let mut prev_opcodes_hash = fxhash::hash64(&acir.opcodes); - let mut prev_acir = acir; - let mut prev_acir_opcode_positions = acir_opcode_positions; - - // For most test programs it would be enough to only loop `transform_internal`, - // but some of them don't stabilize unless we also repeat the backend agnostic optimizations. - let (mut acir, acir_opcode_positions) = loop { - let (acir, acir_opcode_positions) = - optimize_internal(prev_acir, prev_acir_opcode_positions); - - // Stop if we have already done at least one transform and an extra optimization changed nothing. - if pass > 0 && prev_opcodes_hash == fxhash::hash64(&acir.opcodes) { - break (acir, acir_opcode_positions); - } - - let (acir, acir_opcode_positions) = - transform_internal(acir, expression_width, acir_opcode_positions); - - let opcodes_hash = fxhash::hash64(&acir.opcodes); - - // Stop if the output hasn't change in this loop or we went too long. - if pass == MAX_OPTIMIZER_PASSES - 1 || prev_opcodes_hash == opcodes_hash { - break (acir, acir_opcode_positions); - } + let (acir, acir_opcode_positions) = optimize_internal(acir, acir_opcode_positions); - pass += 1; - prev_acir = acir; - prev_opcodes_hash = opcodes_hash; - prev_acir_opcode_positions = acir_opcode_positions; - }; + let (mut acir, acir_opcode_positions) = + transform_internal(acir, expression_width, acir_opcode_positions); let transformation_map = AcirTransformationMap::new(&acir_opcode_positions); acir.assert_messages = transform_assert_messages(acir.assert_messages, &transformation_map); diff --git a/acvm-repo/acvm/src/compiler/transformers/mod.rs b/acvm-repo/acvm/src/compiler/transformers/mod.rs index a499aec1b3..77a5d2da34 100644 --- a/acvm-repo/acvm/src/compiler/transformers/mod.rs +++ b/acvm-repo/acvm/src/compiler/transformers/mod.rs @@ -14,12 +14,17 @@ mod csat; pub(crate) use csat::CSatTransformer; pub use csat::MIN_EXPRESSION_WIDTH; +use tracing::info; use super::{ - optimizers::MergeExpressionsOptimizer, transform_assert_messages, AcirTransformationMap, - MAX_OPTIMIZER_PASSES, + optimizers::{MergeExpressionsOptimizer, RangeOptimizer}, + transform_assert_messages, AcirTransformationMap, }; +/// We need multiple passes to stabilize the output. +/// The value was determined by running tests. +const MAX_TRANSFORMER_PASSES: usize = 3; + /// Applies [`ProofSystemCompiler`][crate::ProofSystemCompiler] specific optimizations to a [`Circuit`]. pub fn transform( acir: Circuit, @@ -50,12 +55,18 @@ pub(super) fn transform_internal( expression_width: ExpressionWidth, mut acir_opcode_positions: Vec, ) -> (Circuit, Vec) { + if acir.opcodes.len() == 1 && matches!(acir.opcodes[0], Opcode::BrilligCall { .. }) { + info!("Program is fully unconstrained, skipping transformation pass"); + return (acir, acir_opcode_positions); + } + // Allow multiple passes until we have stable output. let mut prev_opcodes_hash = fxhash::hash64(&acir.opcodes); // For most test programs it would be enough to loop here, but some of them // don't stabilize unless we also repeat the backend agnostic optimizations. - for _ in 0..MAX_OPTIMIZER_PASSES { + for _ in 0..MAX_TRANSFORMER_PASSES { + info!("Number of opcodes {}", acir.opcodes.len()); let (new_acir, new_acir_opcode_positions) = transform_internal_once(acir, expression_width, acir_opcode_positions); @@ -217,6 +228,12 @@ fn transform_internal_once( ..acir }; + // The `MergeOptimizer` can merge two witnesses which have range opcodes applied to them + // so we run the `RangeOptimizer` afterwards to clear these up. + let range_optimizer = RangeOptimizer::new(acir); + let (acir, new_acir_opcode_positions) = + range_optimizer.replace_redundant_ranges(new_acir_opcode_positions); + (acir, new_acir_opcode_positions) } From b6097a037fa174a8c5dd31105751ebd87c8f94bd Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 14 Jan 2025 13:16:56 -0300 Subject: [PATCH 4/9] chore(perf): try using vec-collections's VecSet in AliasSet (#7058) --- Cargo.lock | 37 +++++++++++++++++++ compiler/noirc_evaluator/Cargo.toml | 1 + .../noirc_evaluator/src/ssa/opt/mem2reg.rs | 6 ++- .../src/ssa/opt/mem2reg/alias_set.rs | 18 ++++----- 4 files changed, 50 insertions(+), 12 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 89287609a2..e82d47d690 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -644,6 +644,12 @@ version = "1.6.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8c3c1a368f70d6cf7302d78f8f7093da241fb8e8807c05cc9e51a125895a6d5b" +[[package]] +name = "binary-merge" +version = "0.1.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "597bb81c80a54b6a4381b23faba8d7774b144c94cbd1d6fe3f1329bd776554ab" + [[package]] name = "bincode" version = "1.3.3" @@ -2519,6 +2525,15 @@ dependencies = [ "libc", ] +[[package]] +name = "inplace-vec-builder" +version = "0.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cf64c2edc8226891a71f127587a2861b132d2b942310843814d5001d99a1d307" +dependencies = [ + "smallvec", +] + [[package]] name = "is-terminal" version = "0.4.13" @@ -3366,6 +3381,7 @@ dependencies = [ "thiserror", "tracing", "tracing-test", + "vec-collections", ] [[package]] @@ -4807,6 +4823,12 @@ dependencies = [ "sha1", ] +[[package]] +name = "sorted-iter" +version = "0.1.11" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bceb57dc07c92cdae60f5b27b3fa92ecaaa42fe36c55e22dbfb0b44893e0b1f7" + [[package]] name = "spin" version = "0.9.8" @@ -5497,6 +5519,21 @@ version = "0.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "830b7e5d4d90034032940e4ace0d9a9a057e7a45cd94e6c007832e39edb82f6d" +[[package]] +name = "vec-collections" +version = "0.4.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3c9965c8f2ffed1dbcd16cafe18a009642f540fa22661c6cfd6309ddb02e4982" +dependencies = [ + "binary-merge", + "inplace-vec-builder", + "lazy_static", + "num-traits", + "serde", + "smallvec", + "sorted-iter", +] + [[package]] name = "version_check" version = "0.9.5" diff --git a/compiler/noirc_evaluator/Cargo.toml b/compiler/noirc_evaluator/Cargo.toml index 15531fafff..3e30fa6673 100644 --- a/compiler/noirc_evaluator/Cargo.toml +++ b/compiler/noirc_evaluator/Cargo.toml @@ -29,6 +29,7 @@ chrono = "0.4.37" rayon.workspace = true cfg-if.workspace = true smallvec = { version = "1.13.2", features = ["serde"] } +vec-collections = "0.4.3" [dev-dependencies] proptest.workspace = true diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index 96a2458383..ce76825877 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -79,6 +79,7 @@ mod block; use std::collections::{BTreeMap, BTreeSet}; use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet}; +use vec_collections::VecSet; use crate::ssa::{ ir::{ @@ -619,7 +620,7 @@ impl<'f> PerFunctionContext<'f> { // then those parameters also alias each other. // We save parameters with repeat arguments to later mark those // parameters as aliasing one another. - let mut arg_set: HashMap> = HashMap::default(); + let mut arg_set = HashMap::default(); // Add an alias for each reference parameter for (parameter, argument) in destination_parameters.iter().zip(arguments) { @@ -632,7 +633,8 @@ impl<'f> PerFunctionContext<'f> { aliases.insert(*parameter); // Check if we have seen the same argument - let seen_parameters = arg_set.entry(argument).or_default(); + let seen_parameters = + arg_set.entry(argument).or_insert_with(VecSet::empty); // Add the current parameter to the parameters we have seen for this argument. // The previous parameters and the current one alias one another. seen_parameters.insert(*parameter); diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg/alias_set.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg/alias_set.rs index e32eaa7018..ab85fbe3a9 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg/alias_set.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg/alias_set.rs @@ -1,4 +1,4 @@ -use std::collections::BTreeSet; +use vec_collections::VecSet; use crate::ssa::ir::value::ValueId; @@ -10,7 +10,7 @@ use crate::ssa::ir::value::ValueId; /// "unknown which aliases this may refer to" - `None`. #[derive(Debug, Default, Clone)] pub(super) struct AliasSet { - aliases: Option>, + aliases: Option>, } impl AliasSet { @@ -19,12 +19,10 @@ impl AliasSet { } pub(super) fn known(value: ValueId) -> AliasSet { - let mut aliases = BTreeSet::new(); - aliases.insert(value); - Self { aliases: Some(aliases) } + Self { aliases: Some(VecSet::single(value)) } } - pub(super) fn known_multiple(values: BTreeSet) -> AliasSet { + pub(super) fn known_multiple(values: VecSet<[ValueId; 1]>) -> AliasSet { Self { aliases: Some(values) } } @@ -32,7 +30,7 @@ impl AliasSet { /// particular value will be known to be zero, which is distinct from being unknown and /// possibly referring to any alias. pub(super) fn known_empty() -> AliasSet { - Self { aliases: Some(BTreeSet::new()) } + Self { aliases: Some(VecSet::empty()) } } pub(super) fn is_unknown(&self) -> bool { @@ -44,14 +42,14 @@ impl AliasSet { pub(super) fn single_alias(&self) -> Option { self.aliases .as_ref() - .and_then(|aliases| (aliases.len() == 1).then(|| *aliases.first().unwrap())) + .and_then(|aliases| (aliases.len() == 1).then(|| *aliases.iter().next().unwrap())) } /// Unify this alias set with another. The result of this set is empty if either set is empty. /// Otherwise, it is the union of both alias sets. pub(super) fn unify(&mut self, other: &Self) { if let (Some(self_aliases), Some(other_aliases)) = (&mut self.aliases, &other.aliases) { - self_aliases.extend(other_aliases); + self_aliases.extend(other_aliases.iter().cloned()); } else { self.aliases = None; } @@ -82,6 +80,6 @@ impl AliasSet { /// The ordering is arbitrary (by lowest ValueId) so this method should only be /// used when you need an arbitrary ValueId from the alias set. pub(super) fn first(&self) -> Option { - self.aliases.as_ref().and_then(|aliases| aliases.first().copied()) + self.aliases.as_ref().and_then(|aliases| aliases.iter().next().copied()) } } From aa7b91c4657f1dbdf734dfd8071c0517272092ba Mon Sep 17 00:00:00 2001 From: jfecher Date: Tue, 14 Jan 2025 10:25:33 -0600 Subject: [PATCH 5/9] feat: Allow associated types to be ellided from trait constraints (#7026) Co-authored-by: Ary Borenszweig --- compiler/noirc_frontend/src/elaborator/mod.rs | 147 +++++++++++++++--- .../noirc_frontend/src/elaborator/traits.rs | 7 +- .../noirc_frontend/src/elaborator/types.rs | 49 +++++- .../src/hir/resolution/visibility.rs | 21 ++- compiler/noirc_frontend/src/tests.rs | 23 +++ docs/docs/noir/concepts/traits.md | 29 ++-- .../associated_types_implicit/Nargo.toml | 6 + .../associated_types_implicit/Prover.toml | 1 + .../associated_types_implicit/src/main.nr | 60 +++++++ 9 files changed, 293 insertions(+), 50 deletions(-) create mode 100644 test_programs/compile_success_empty/associated_types_implicit/Nargo.toml create mode 100644 test_programs/compile_success_empty/associated_types_implicit/Prover.toml create mode 100644 test_programs/compile_success_empty/associated_types_implicit/src/main.nr diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index 0f59de6004..d263c8245f 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -315,7 +315,7 @@ impl<'context> Elaborator<'context> { self.define_function_metas(&mut items.functions, &mut items.impls, &mut items.trait_impls); - self.collect_traits(&items.traits); + self.collect_traits(&mut items.traits); // Before we resolve any function symbols we must go through our impls and // re-collect the methods within into their proper module. This cannot be @@ -354,6 +354,7 @@ impl<'context> Elaborator<'context> { self.current_trait = Some(trait_id); self.elaborate_functions(unresolved_trait.fns_with_default_impl); } + self.current_trait = None; for impls in items.impls.into_values() { @@ -475,7 +476,7 @@ impl<'context> Elaborator<'context> { self.add_existing_variable_to_scope(name, parameter.clone(), warn_if_unused); } - self.add_trait_constraints_to_scope(&func_meta); + self.add_trait_constraints_to_scope(&func_meta.trait_constraints, func_meta.location.span); let (hir_func, body_type) = match kind { FunctionKind::Builtin @@ -501,7 +502,7 @@ impl<'context> Elaborator<'context> { // when multiple impls are available. Instead we default first to choose the Field or u64 impl. self.check_and_pop_function_context(); - self.remove_trait_constraints_from_scope(&func_meta); + self.remove_trait_constraints_from_scope(&func_meta.trait_constraints); let func_scope_tree = self.scopes.end_function(); @@ -733,8 +734,13 @@ impl<'context> Elaborator<'context> { None } - /// TODO: This is currently only respected for generic free functions - /// there's a bunch of other places where trait constraints can pop up + /// Resolve the given trait constraints and add them to scope as we go. + /// This second step is necessary to resolve subsequent constraints such + /// as `::Bar: Eq` which may lookup an impl which was assumed + /// by a previous constraint. + /// + /// If these constraints are unwanted afterward they should be manually + /// removed from the interner. fn resolve_trait_constraints( &mut self, where_clause: &[UnresolvedTraitConstraint], @@ -745,12 +751,92 @@ impl<'context> Elaborator<'context> { .collect() } - pub fn resolve_trait_constraint( + /// Expands any traits in a where clause to mention all associated types if they were + /// elided by the user. See `add_missing_named_generics` for more detail. + /// + /// Returns all newly created generics to be added to this function/trait/impl. + fn desugar_trait_constraints( + &mut self, + where_clause: &mut [UnresolvedTraitConstraint], + ) -> Vec { + where_clause + .iter_mut() + .flat_map(|constraint| self.add_missing_named_generics(&mut constraint.trait_bound)) + .collect() + } + + /// For each associated type that isn't mentioned in a trait bound, this adds + /// the type as an implicit generic to the where clause and returns the newly + /// created generics in a vector to add to the function/trait/impl later. + /// For example, this will turn a function using a trait with 2 associated types: + /// + /// `fn foo() where T: Foo { ... }` + /// + /// into: + /// `fn foo() where T: Foo { ... }` + /// + /// with a vector of `` returned so that the caller can then modify the function to: + /// `fn foo() where T: Foo { ... }` + fn add_missing_named_generics(&mut self, bound: &mut TraitBound) -> Vec { + let mut added_generics = Vec::new(); + + let Ok(item) = self.resolve_path_or_error(bound.trait_path.clone()) else { + return Vec::new(); + }; + + let PathResolutionItem::Trait(trait_id) = item else { + return Vec::new(); + }; + + let the_trait = self.get_trait_mut(trait_id); + + if the_trait.associated_types.len() > bound.trait_generics.named_args.len() { + for associated_type in &the_trait.associated_types.clone() { + if !bound + .trait_generics + .named_args + .iter() + .any(|(name, _)| name.0.contents == *associated_type.name.as_ref()) + { + // This generic isn't contained in the bound's named arguments, + // so add it by creating a fresh type variable. + let new_generic_id = self.interner.next_type_variable_id(); + let type_var = TypeVariable::unbound(new_generic_id, Kind::Normal); + + let span = bound.trait_path.span; + let name = associated_type.name.clone(); + let typ = Type::NamedGeneric(type_var.clone(), name.clone()); + let typ = self.interner.push_quoted_type(typ); + let typ = UnresolvedTypeData::Resolved(typ).with_span(span); + let ident = Ident::new(associated_type.name.as_ref().clone(), span); + + bound.trait_generics.named_args.push((ident, typ)); + added_generics.push(ResolvedGeneric { name, span, type_var }); + } + } + } + + added_generics + } + + /// Resolves a trait constraint and adds it to scope as an assumed impl. + /// This second step is necessary to resolve subsequent constraints such + /// as `::Bar: Eq` which may lookup an impl which was assumed + /// by a previous constraint. + fn resolve_trait_constraint( &mut self, constraint: &UnresolvedTraitConstraint, ) -> Option { let typ = self.resolve_type(constraint.typ.clone()); let trait_bound = self.resolve_trait_bound(&constraint.trait_bound)?; + + self.add_trait_bound_to_scope( + constraint.trait_bound.trait_path.span, + &typ, + &trait_bound, + trait_bound.trait_id, + ); + Some(TraitConstraint { typ, trait_bound }) } @@ -800,10 +886,13 @@ impl<'context> Elaborator<'context> { let has_inline_attribute = has_no_predicates_attribute || should_fold; let is_pub_allowed = self.pub_allowed(func, in_contract); self.add_generics(&func.def.generics); + let mut generics = vecmap(&self.generics, |generic| generic.type_var.clone()); + + let new_generics = self.desugar_trait_constraints(&mut func.def.where_clause); + generics.extend(new_generics.into_iter().map(|generic| generic.type_var)); let mut trait_constraints = self.resolve_trait_constraints(&func.def.where_clause); - let mut generics = vecmap(&self.generics, |generic| generic.type_var.clone()); let mut parameters = Vec::new(); let mut parameter_types = Vec::new(); let mut parameter_idents = Vec::new(); @@ -874,6 +963,9 @@ impl<'context> Elaborator<'context> { None }; + // Remove the traits assumed by `resolve_trait_constraints` from scope + self.remove_trait_constraints_from_scope(&trait_constraints); + let meta = FuncMeta { name: name_ident, kind: func.kind, @@ -1013,10 +1105,10 @@ impl<'context> Elaborator<'context> { } } - fn add_trait_constraints_to_scope(&mut self, func_meta: &FuncMeta) { - for constraint in &func_meta.trait_constraints { + fn add_trait_constraints_to_scope(&mut self, constraints: &[TraitConstraint], span: Span) { + for constraint in constraints { self.add_trait_bound_to_scope( - func_meta, + span, &constraint.typ, &constraint.trait_bound, constraint.trait_bound.trait_id, @@ -1030,7 +1122,7 @@ impl<'context> Elaborator<'context> { let self_type = self.self_type.clone().expect("Expected a self type if there's a current trait"); self.add_trait_bound_to_scope( - func_meta, + span, &self_type, &constraint.trait_bound, constraint.trait_bound.trait_id, @@ -1038,8 +1130,8 @@ impl<'context> Elaborator<'context> { } } - fn remove_trait_constraints_from_scope(&mut self, func_meta: &FuncMeta) { - for constraint in &func_meta.trait_constraints { + fn remove_trait_constraints_from_scope(&mut self, constraints: &[TraitConstraint]) { + for constraint in constraints { self.interner .remove_assumed_trait_implementations_for_trait(constraint.trait_bound.trait_id); } @@ -1052,7 +1144,7 @@ impl<'context> Elaborator<'context> { fn add_trait_bound_to_scope( &mut self, - func_meta: &FuncMeta, + span: Span, object: &Type, trait_bound: &ResolvedTraitBound, starting_trait_id: TraitId, @@ -1064,7 +1156,6 @@ impl<'context> Elaborator<'context> { if let Some(the_trait) = self.interner.try_get_trait(trait_id) { let trait_name = the_trait.name.to_string(); let typ = object.clone(); - let span = func_meta.location.span; self.push_err(TypeCheckError::UnneededTraitConstraint { trait_name, typ, span }); } } @@ -1081,12 +1172,7 @@ impl<'context> Elaborator<'context> { let parent_trait_bound = self.instantiate_parent_trait_bound(trait_bound, &parent_trait_bound); - self.add_trait_bound_to_scope( - func_meta, - object, - &parent_trait_bound, - starting_trait_id, - ); + self.add_trait_bound_to_scope(span, object, &parent_trait_bound, starting_trait_id); } } } @@ -1316,6 +1402,7 @@ impl<'context> Elaborator<'context> { self.generics = trait_impl.resolved_generics.clone(); let where_clause = self.resolve_trait_constraints(&trait_impl.where_clause); + self.remove_trait_constraints_from_scope(&where_clause); self.collect_trait_impl_methods(trait_id, trait_impl, &where_clause); @@ -1811,6 +1898,17 @@ impl<'context> Elaborator<'context> { self.add_generics(&trait_impl.generics); trait_impl.resolved_generics = self.generics.clone(); + let new_generics = self.desugar_trait_constraints(&mut trait_impl.where_clause); + for new_generic in new_generics { + trait_impl.resolved_generics.push(new_generic.clone()); + self.generics.push(new_generic); + } + + // We need to resolve the where clause before any associated types to be + // able to resolve trait as type syntax, eg. `` in case there + // is a where constraint for `T: Foo`. + let constraints = self.resolve_trait_constraints(&trait_impl.where_clause); + for (_, _, method) in trait_impl.methods.functions.iter_mut() { // Attach any trait constraints on the impl to the function method.def.where_clause.append(&mut trait_impl.where_clause.clone()); @@ -1823,17 +1921,20 @@ impl<'context> Elaborator<'context> { let impl_id = self.interner.next_trait_impl_id(); self.current_trait_impl = Some(impl_id); - // Fetch trait constraints here + let path_span = trait_impl.trait_path.span; let (ordered_generics, named_generics) = trait_impl .trait_id .map(|trait_id| { - self.resolve_type_args(trait_generics, trait_id, trait_impl.trait_path.span) + // Check for missing generics & associated types for the trait being implemented + self.resolve_trait_args_from_trait_impl(trait_generics, trait_id, path_span) }) .unwrap_or_default(); trait_impl.resolved_trait_generics = ordered_generics; self.interner.set_associated_types_for_impl(impl_id, named_generics); + self.remove_trait_constraints_from_scope(&constraints); + let self_type = self.resolve_type(unresolved_type); self.self_type = Some(self_type.clone()); trait_impl.methods.self_type = Some(self_type); diff --git a/compiler/noirc_frontend/src/elaborator/traits.rs b/compiler/noirc_frontend/src/elaborator/traits.rs index 09bd639e31..11646d52a0 100644 --- a/compiler/noirc_frontend/src/elaborator/traits.rs +++ b/compiler/noirc_frontend/src/elaborator/traits.rs @@ -21,7 +21,7 @@ use crate::{ use super::Elaborator; impl<'context> Elaborator<'context> { - pub fn collect_traits(&mut self, traits: &BTreeMap) { + pub fn collect_traits(&mut self, traits: &mut BTreeMap) { for (trait_id, unresolved_trait) in traits { self.local_module = unresolved_trait.module_id; @@ -39,8 +39,13 @@ impl<'context> Elaborator<'context> { &resolved_generics, ); + let new_generics = + this.desugar_trait_constraints(&mut unresolved_trait.trait_def.where_clause); + this.generics.extend(new_generics); + let where_clause = this.resolve_trait_constraints(&unresolved_trait.trait_def.where_clause); + this.remove_trait_constraints_from_scope(&where_clause); // Each associated type in this trait is also an implicit generic for associated_type in &this.interner.get_trait(*trait_id).associated_types { diff --git a/compiler/noirc_frontend/src/elaborator/types.rs b/compiler/noirc_frontend/src/elaborator/types.rs index 8d6447d19d..b17bc09662 100644 --- a/compiler/noirc_frontend/src/elaborator/types.rs +++ b/compiler/noirc_frontend/src/elaborator/types.rs @@ -310,11 +310,32 @@ impl<'context> Elaborator<'context> { } } + /// Identical to `resolve_type_args` but does not allow + /// associated types to be elided since trait impls must specify them. + pub(super) fn resolve_trait_args_from_trait_impl( + &mut self, + args: GenericTypeArgs, + item: TraitId, + span: Span, + ) -> (Vec, Vec) { + self.resolve_type_args_inner(args, item, span, false) + } + pub(super) fn resolve_type_args( + &mut self, + args: GenericTypeArgs, + item: impl Generic, + span: Span, + ) -> (Vec, Vec) { + self.resolve_type_args_inner(args, item, span, true) + } + + pub(super) fn resolve_type_args_inner( &mut self, mut args: GenericTypeArgs, item: impl Generic, span: Span, + allow_implicit_named_args: bool, ) -> (Vec, Vec) { let expected_kinds = item.generics(self.interner); @@ -336,7 +357,12 @@ impl<'context> Elaborator<'context> { let mut associated = Vec::new(); if item.accepts_named_type_args() { - associated = self.resolve_associated_type_args(args.named_args, item, span); + associated = self.resolve_associated_type_args( + args.named_args, + item, + span, + allow_implicit_named_args, + ); } else if !args.named_args.is_empty() { let item_kind = item.item_kind(); self.push_err(ResolverError::NamedTypeArgs { span, item_kind }); @@ -350,6 +376,7 @@ impl<'context> Elaborator<'context> { args: Vec<(Ident, UnresolvedType)>, item: impl Generic, span: Span, + allow_implicit_named_args: bool, ) -> Vec { let mut seen_args = HashMap::default(); let mut required_args = item.named_generics(self.interner); @@ -379,11 +406,19 @@ impl<'context> Elaborator<'context> { resolved.push(NamedType { name, typ }); } - // Anything that hasn't been removed yet is missing + // Anything that hasn't been removed yet is missing. + // Fill it in to avoid a panic if we allow named args to be elided, otherwise error. for generic in required_args { - let item = item.item_name(self.interner); let name = generic.name.clone(); - self.push_err(TypeCheckError::MissingNamedTypeArg { item, span, name }); + + if allow_implicit_named_args { + let name = Ident::new(name.as_ref().clone(), span); + let typ = self.interner.next_type_variable(); + resolved.push(NamedType { name, typ }); + } else { + let item = item.item_name(self.interner); + self.push_err(TypeCheckError::MissingNamedTypeArg { item, span, name }); + } } resolved @@ -1973,7 +2008,7 @@ pub(crate) fn bind_named_generics( args: &[NamedType], bindings: &mut TypeBindings, ) { - assert_eq!(params.len(), args.len()); + assert!(args.len() <= params.len()); for arg in args { let i = params @@ -1984,6 +2019,10 @@ pub(crate) fn bind_named_generics( let param = params.swap_remove(i); bind_generic(¶m, &arg.typ, bindings); } + + for unbound_param in params { + bind_generic(&unbound_param, &Type::Error, bindings); + } } fn bind_generic(param: &ResolvedGeneric, arg: &Type, bindings: &mut TypeBindings) { diff --git a/compiler/noirc_frontend/src/hir/resolution/visibility.rs b/compiler/noirc_frontend/src/hir/resolution/visibility.rs index 5e6dffa56e..557f799df8 100644 --- a/compiler/noirc_frontend/src/hir/resolution/visibility.rs +++ b/compiler/noirc_frontend/src/hir/resolution/visibility.rs @@ -138,18 +138,29 @@ pub fn method_call_is_visible( ItemVisibility::Public => true, ItemVisibility::PublicCrate | ItemVisibility::Private => { let func_meta = interner.function_meta(&func_id); - if let Some(struct_id) = func_meta.struct_id { - return struct_member_is_visible( - struct_id, + + if let Some(trait_id) = func_meta.trait_id { + return trait_member_is_visible( + trait_id, modifiers.visibility, current_module, def_maps, ); } - if let Some(trait_id) = func_meta.trait_id { + if let Some(trait_impl_id) = func_meta.trait_impl { + let trait_impl = interner.get_trait_implementation(trait_impl_id); return trait_member_is_visible( - trait_id, + trait_impl.borrow().trait_id, + modifiers.visibility, + current_module, + def_maps, + ); + } + + if let Some(struct_id) = func_meta.struct_id { + return struct_member_is_visible( + struct_id, modifiers.visibility, current_module, def_maps, diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index c1b86b5dcf..637b15e719 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -3955,3 +3955,26 @@ fn mutable_self_call() { "#; assert_no_errors(src); } + +#[test] +fn checks_visibility_of_trait_related_to_trait_impl_on_method_call() { + let src = r#" + mod moo { + pub struct Bar {} + } + + trait Foo { + fn foo(self); + } + + impl Foo for moo::Bar { + fn foo(self) {} + } + + fn main() { + let bar = moo::Bar {}; + bar.foo(); + } + "#; + assert_no_errors(src); +} diff --git a/docs/docs/noir/concepts/traits.md b/docs/docs/noir/concepts/traits.md index 1b232dcf8a..13818ecffa 100644 --- a/docs/docs/noir/concepts/traits.md +++ b/docs/docs/noir/concepts/traits.md @@ -252,36 +252,33 @@ impl MyTrait for Field { Since associated constants can also be used in a type position, its values are limited to only other expression kinds allowed in numeric generics. -Note that currently all associated types and constants must be explicitly specified in a trait constraint. -If we leave out any, we'll get an error that we're missing one: +When writing a trait constraint, you can specify all associated types and constants explicitly if +you wish: ```rust -// Error! Constraint is missing associated constant for `Bar` -fn foo(x: T) where T: MyTrait { +fn foo(x: T) where T: MyTrait { ... } ``` -Because all associated types and constants must be explicitly specified, they are essentially named generics, -although this is set to change in the future. Future versions of Noir will allow users to elide associated types -in trait constraints similar to Rust. When this is done, you may still refer to their value with the `::AssociatedType` -syntax: +Or you can also elide them since there should only be one `Foo` and `Bar` for a given implementation +of `MyTrait` for a type: ```rust -// Only valid in future versions of Noir: fn foo(x: T) where T: MyTrait { - let _: ::Foo = ...; + ... } ``` -The type as trait syntax is possible in Noir today but is less useful when each type must be explicitly specified anyway: +If you elide associated types, you can still refer to them via the type as trait syntax ``: ```rust -fn foo(x: T) where T: MyTrait { - // Works, but could just use F directly - let _: >::Foo = ...; - - let _: F = ...; +fn foo(x: T) where + T: MyTrait, + ::Foo: Default + Eq +{ + let foo_value: ::Foo = Default::default(); + assert_eq(foo_value, foo_value); } ``` diff --git a/test_programs/compile_success_empty/associated_types_implicit/Nargo.toml b/test_programs/compile_success_empty/associated_types_implicit/Nargo.toml new file mode 100644 index 0000000000..6b54cbfca6 --- /dev/null +++ b/test_programs/compile_success_empty/associated_types_implicit/Nargo.toml @@ -0,0 +1,6 @@ +[package] +name = "associated_types_implicit" +type = "bin" +authors = [""] + +[dependencies] diff --git a/test_programs/compile_success_empty/associated_types_implicit/Prover.toml b/test_programs/compile_success_empty/associated_types_implicit/Prover.toml new file mode 100644 index 0000000000..cab679b4b6 --- /dev/null +++ b/test_programs/compile_success_empty/associated_types_implicit/Prover.toml @@ -0,0 +1 @@ +a = [[0, 1], [2, 3]] diff --git a/test_programs/compile_success_empty/associated_types_implicit/src/main.nr b/test_programs/compile_success_empty/associated_types_implicit/src/main.nr new file mode 100644 index 0000000000..d04cef5186 --- /dev/null +++ b/test_programs/compile_success_empty/associated_types_implicit/src/main.nr @@ -0,0 +1,60 @@ +trait Foo { + type Bar; + + fn foo(self) -> Self::Bar; +} + +impl Foo for u64 { + type Bar = u8; + + fn foo(self) -> Self::Bar { + self as u8 + } +} + +fn main() { + // This currently requires a type annotation to find the impl + let three: u64 = 3; + call_foo(three); + + let x: Option> = Option::some(Option::some(0)); + let x_foo = x.foo(); + assert_eq(x_foo, x_foo); // ensure we don't need an additional type annotation for Bar here + + // The `as u8` is still necessary even though we know the object type, + // otherwise we try to search for `u64: Foo`. + // It seems the `Bar = u8` constraint is still there & checked for, but + // the defaulting of the polymorphic integer occurs first. + assert_eq(x.foo(), 0 as u8); +} + +// Ensure we can use `::Bar: Eq` in a function's where clause +fn call_foo(x: T) +where + T: Foo, + ::Bar: Eq, +{ + let y = x.foo(); + assert_eq(y, y); +} + +// Ensure we can use `::Bar: Eq` in a trait impl's where clause +impl Foo for Option +where + T: Foo, + ::Bar: Eq, +{ + type Bar = ::Bar; + + fn foo(self) -> Self::Bar { + self.unwrap().foo() + } +} + +// Ensure we can use `::Bar: Eq` in a trait's where clause +// TODO: Not working, see issue #7024 +// trait Baz where +// T: Foo, +// ::Bar: Eq {} +// +// impl Baz for u32 {} From a0ffedfdf445b2dc6655cafa6fe614958ddb9603 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Tue, 14 Jan 2025 17:03:59 +0000 Subject: [PATCH 6/9] fix: Reduce memory usage in mem2reg (#7053) Co-authored-by: Ary Borenszweig Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> --- .../src/ssa/opt/mem2reg/alias_set.rs | 16 +++++++++++++++- .../noirc_evaluator/src/ssa/opt/mem2reg/block.rs | 6 ++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg/alias_set.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg/alias_set.rs index ab85fbe3a9..443b21cfd1 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg/alias_set.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg/alias_set.rs @@ -1,4 +1,4 @@ -use vec_collections::VecSet; +use vec_collections::{AbstractVecSet, VecSet}; use crate::ssa::ir::value::ValueId; @@ -55,6 +55,20 @@ impl AliasSet { } } + /// Returns true if calling `unify` would change something in this alias set. + /// + /// This is an optimization to avoid having to look up an entry ready to be modified in the [Block](crate::ssa::opt::mem2reg::block::Block), + /// because doing so would involve calling `Arc::make_mut` which clones the entry, ready for modification. + pub(super) fn should_unify(&self, other: &Self) -> bool { + if let (Some(self_aliases), Some(other_aliases)) = (&self.aliases, &other.aliases) { + // `unify` would extend `self_aliases` with `other_aliases`, so if `other_aliases` is a subset, then nothing would happen. + !other_aliases.is_subset(self_aliases) + } else { + // `unify` would set `aliases` to `None`, so if it's not `Some`, then nothing would happen. + self.aliases.is_some() + } + } + /// Inserts a new alias into this set if it is not unknown pub(super) fn insert(&mut self, new_alias: ValueId) { if let Some(aliases) = &mut self.aliases { diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg/block.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg/block.rs index f4265b2466..91e27f07b8 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg/block.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg/block.rs @@ -128,6 +128,12 @@ impl Block { } for (expression, new_aliases) in &other.aliases { + // If nothing would change, then don't call `.entry(...).and_modify(...)` as it involves creating more `Arc`s. + if let Some(aliases) = self.aliases.get(expression) { + if !aliases.should_unify(new_aliases) { + continue; + } + } let expression = expression.clone(); self.aliases From 0edd9c4dbd3f9dca27019cd91113369218bc9885 Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Tue, 14 Jan 2025 19:05:52 +0000 Subject: [PATCH 7/9] chore: mark some critical libraries as good again (#7065) --- .../{.failures.jsonl.does_not_compile => .failures.jsonl} | 0 .../{.failures.jsonl.does_not_compile => .failures.jsonl} | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename .github/critical_libraries_status/noir-lang/noir_json_parser/{.failures.jsonl.does_not_compile => .failures.jsonl} (100%) rename .github/critical_libraries_status/noir-lang/noir_sort/{.failures.jsonl.does_not_compile => .failures.jsonl} (100%) diff --git a/.github/critical_libraries_status/noir-lang/noir_json_parser/.failures.jsonl.does_not_compile b/.github/critical_libraries_status/noir-lang/noir_json_parser/.failures.jsonl similarity index 100% rename from .github/critical_libraries_status/noir-lang/noir_json_parser/.failures.jsonl.does_not_compile rename to .github/critical_libraries_status/noir-lang/noir_json_parser/.failures.jsonl diff --git a/.github/critical_libraries_status/noir-lang/noir_sort/.failures.jsonl.does_not_compile b/.github/critical_libraries_status/noir-lang/noir_sort/.failures.jsonl similarity index 100% rename from .github/critical_libraries_status/noir-lang/noir_sort/.failures.jsonl.does_not_compile rename to .github/critical_libraries_status/noir-lang/noir_sort/.failures.jsonl From ba07336e5e5229eefae342dadec2526bfb0a72a2 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 14 Jan 2025 16:18:01 -0300 Subject: [PATCH 8/9] fix: don't always select trait impl when verifying trait constraints (#7041) --- .../src/elaborator/expressions.rs | 5 ++- compiler/noirc_frontend/src/elaborator/mod.rs | 9 ++++- .../noirc_frontend/src/elaborator/patterns.rs | 12 +++++- .../noirc_frontend/src/elaborator/traits.rs | 2 +- .../noirc_frontend/src/elaborator/types.rs | 15 +++++-- .../regression_7038/Nargo.toml | 7 ++++ .../regression_7038/src/main.nr | 40 +++++++++++++++++++ .../regression_7038_2/Nargo.toml | 7 ++++ .../regression_7038_2/src/main.nr | 39 ++++++++++++++++++ .../regression_7038_3/Nargo.toml | 7 ++++ .../regression_7038_3/src/main.nr | 40 +++++++++++++++++++ .../regression_7038_4/Nargo.toml | 7 ++++ .../regression_7038_4/src/main.nr | 29 ++++++++++++++ 13 files changed, 210 insertions(+), 9 deletions(-) create mode 100644 test_programs/compile_success_empty/regression_7038/Nargo.toml create mode 100644 test_programs/compile_success_empty/regression_7038/src/main.nr create mode 100644 test_programs/compile_success_empty/regression_7038_2/Nargo.toml create mode 100644 test_programs/compile_success_empty/regression_7038_2/src/main.nr create mode 100644 test_programs/compile_success_empty/regression_7038_3/Nargo.toml create mode 100644 test_programs/compile_success_empty/regression_7038_3/src/main.nr create mode 100644 test_programs/compile_success_empty/regression_7038_4/Nargo.toml create mode 100644 test_programs/compile_success_empty/regression_7038_4/src/main.nr diff --git a/compiler/noirc_frontend/src/elaborator/expressions.rs b/compiler/noirc_frontend/src/elaborator/expressions.rs index 1b3bc645cb..33af075aeb 100644 --- a/compiler/noirc_frontend/src/elaborator/expressions.rs +++ b/compiler/noirc_frontend/src/elaborator/expressions.rs @@ -775,7 +775,10 @@ impl<'context> Elaborator<'context> { span, }, }; - self.push_trait_constraint(constraint, expr_id); + self.push_trait_constraint( + constraint, expr_id, + true, // this constraint should lead to choosing a trait impl + ); self.type_check_operator_method(expr_id, trait_id, operand_type, span); } typ diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index d263c8245f..10e56364ed 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -197,7 +197,11 @@ struct FunctionContext { /// verified at the end of a function. This is because constraints arise /// on each variable, but it is only until function calls when the types /// needed for the trait constraint may become known. - trait_constraints: Vec<(TraitConstraint, ExprId)>, + /// The `select impl` bool indicates whether, after verifying the trait constraint, + /// the resulting trait impl should be the one used for a call (sometimes trait + /// constraints are verified but there's no call associated with them, like in the + /// case of checking generic arguments) + trait_constraints: Vec<(TraitConstraint, ExprId, bool /* select impl */)>, } impl<'context> Elaborator<'context> { @@ -550,7 +554,7 @@ impl<'context> Elaborator<'context> { } } - for (mut constraint, expr_id) in context.trait_constraints { + for (mut constraint, expr_id, select_impl) in context.trait_constraints { let span = self.interner.expr_span(&expr_id); if matches!(&constraint.typ, Type::MutableReference(_)) { @@ -565,6 +569,7 @@ impl<'context> Elaborator<'context> { &constraint.trait_bound.trait_generics.ordered, &constraint.trait_bound.trait_generics.named, expr_id, + select_impl, span, ); } diff --git a/compiler/noirc_frontend/src/elaborator/patterns.rs b/compiler/noirc_frontend/src/elaborator/patterns.rs index 242f5f0b49..6a672866d7 100644 --- a/compiler/noirc_frontend/src/elaborator/patterns.rs +++ b/compiler/noirc_frontend/src/elaborator/patterns.rs @@ -751,7 +751,11 @@ impl<'context> Elaborator<'context> { let function = self.interner.function_meta(&function); for mut constraint in function.trait_constraints.clone() { constraint.apply_bindings(&bindings); - self.push_trait_constraint(constraint, expr_id); + + self.push_trait_constraint( + constraint, expr_id, + false, // This constraint shouldn't lead to choosing a trait impl method + ); } } } @@ -767,7 +771,11 @@ impl<'context> Elaborator<'context> { // Currently only one impl can be selected per expr_id, so this // constraint needs to be pushed after any other constraints so // that monomorphization can resolve this trait method to the correct impl. - self.push_trait_constraint(method.constraint, expr_id); + self.push_trait_constraint( + method.constraint, + expr_id, + true, // this constraint should lead to choosing a trait impl method + ); } } diff --git a/compiler/noirc_frontend/src/elaborator/traits.rs b/compiler/noirc_frontend/src/elaborator/traits.rs index 11646d52a0..bbc1214de9 100644 --- a/compiler/noirc_frontend/src/elaborator/traits.rs +++ b/compiler/noirc_frontend/src/elaborator/traits.rs @@ -132,7 +132,7 @@ impl<'context> Elaborator<'context> { let func_id = unresolved_trait.method_ids[&name.0.contents]; let mut where_clause = where_clause.to_vec(); - // Attach any trait constraints on the trait to the function + // Attach any trait constraints on the trait to the function, where_clause.extend(unresolved_trait.trait_def.where_clause.clone()); this.resolve_trait_function( diff --git a/compiler/noirc_frontend/src/elaborator/types.rs b/compiler/noirc_frontend/src/elaborator/types.rs index b17bc09662..8fa0b21060 100644 --- a/compiler/noirc_frontend/src/elaborator/types.rs +++ b/compiler/noirc_frontend/src/elaborator/types.rs @@ -1845,6 +1845,7 @@ impl<'context> Elaborator<'context> { (expr_span, empty_function) } + #[allow(clippy::too_many_arguments)] pub fn verify_trait_constraint( &mut self, object_type: &Type, @@ -1852,6 +1853,7 @@ impl<'context> Elaborator<'context> { trait_generics: &[Type], associated_types: &[NamedType], function_ident_id: ExprId, + select_impl: bool, span: Span, ) { match self.interner.lookup_trait_implementation( @@ -1861,7 +1863,9 @@ impl<'context> Elaborator<'context> { associated_types, ) { Ok(impl_kind) => { - self.interner.select_impl_for_expression(function_ident_id, impl_kind); + if select_impl { + self.interner.select_impl_for_expression(function_ident_id, impl_kind); + } } Err(error) => self.push_trait_constraint_error(object_type, error, span), } @@ -1935,10 +1939,15 @@ impl<'context> Elaborator<'context> { /// Push a trait constraint into the current FunctionContext to be solved if needed /// at the end of the earlier of either the current function or the current comptime scope. - pub fn push_trait_constraint(&mut self, constraint: TraitConstraint, expr_id: ExprId) { + pub fn push_trait_constraint( + &mut self, + constraint: TraitConstraint, + expr_id: ExprId, + select_impl: bool, + ) { let context = self.function_context.last_mut(); let context = context.expect("The function_context stack should always be non-empty"); - context.trait_constraints.push((constraint, expr_id)); + context.trait_constraints.push((constraint, expr_id, select_impl)); } pub fn bind_generics_from_trait_constraint( diff --git a/test_programs/compile_success_empty/regression_7038/Nargo.toml b/test_programs/compile_success_empty/regression_7038/Nargo.toml new file mode 100644 index 0000000000..3c874d4b6e --- /dev/null +++ b/test_programs/compile_success_empty/regression_7038/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "regression_7038" +type = "bin" +authors = [""] +compiler_version = ">=0.30.0" + +[dependencies] diff --git a/test_programs/compile_success_empty/regression_7038/src/main.nr b/test_programs/compile_success_empty/regression_7038/src/main.nr new file mode 100644 index 0000000000..793a3f6080 --- /dev/null +++ b/test_programs/compile_success_empty/regression_7038/src/main.nr @@ -0,0 +1,40 @@ +trait BigNumTrait {} + +pub struct MyBigNum; + +impl crate::BigNumTrait for MyBigNum {} + +trait CurveParamsTrait +where + BigNum: BigNumTrait, +{ + fn one(); +} + +pub struct BN254Params; +impl CurveParamsTrait for BN254Params { + + fn one() {} +} + +trait BigCurveTrait { + fn two(); +} + +pub struct BigCurve {} + +type BN254 = BigCurve; + +impl BigCurveTrait for BigCurve +where + BigNum: BigNumTrait, + CurveParams: CurveParamsTrait, +{ + fn two() { + let _ = CurveParams::one(); + } +} + +fn main() { + let _ = BN254::two(); +} diff --git a/test_programs/compile_success_empty/regression_7038_2/Nargo.toml b/test_programs/compile_success_empty/regression_7038_2/Nargo.toml new file mode 100644 index 0000000000..f4f23683eb --- /dev/null +++ b/test_programs/compile_success_empty/regression_7038_2/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "regression_7038_2" +type = "bin" +authors = [""] +compiler_version = ">=0.30.0" + +[dependencies] diff --git a/test_programs/compile_success_empty/regression_7038_2/src/main.nr b/test_programs/compile_success_empty/regression_7038_2/src/main.nr new file mode 100644 index 0000000000..6a116bb072 --- /dev/null +++ b/test_programs/compile_success_empty/regression_7038_2/src/main.nr @@ -0,0 +1,39 @@ +trait BigNumTrait {} + +pub struct MyBigNum; + +impl crate::BigNumTrait for MyBigNum {} + +trait CurveParamsTrait +where + BigNum: BigNumTrait, +{ + // The difference between this and regression_7083 is that here + // this is a default method. + fn one() {} +} + +pub struct BN254Params; +impl CurveParamsTrait for BN254Params {} + +trait BigCurveTrait { + fn two(); +} + +pub struct BigCurve {} + +type BN254 = BigCurve; + +impl BigCurveTrait for BigCurve +where + BigNum: BigNumTrait, + CurveParams: CurveParamsTrait, +{ + fn two() { + let _ = CurveParams::one(); + } +} + +fn main() { + let _ = BN254::two(); +} diff --git a/test_programs/compile_success_empty/regression_7038_3/Nargo.toml b/test_programs/compile_success_empty/regression_7038_3/Nargo.toml new file mode 100644 index 0000000000..65bc946c55 --- /dev/null +++ b/test_programs/compile_success_empty/regression_7038_3/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "regression_7038_3" +type = "bin" +authors = [""] +compiler_version = ">=0.30.0" + +[dependencies] diff --git a/test_programs/compile_success_empty/regression_7038_3/src/main.nr b/test_programs/compile_success_empty/regression_7038_3/src/main.nr new file mode 100644 index 0000000000..1b6bf0b72d --- /dev/null +++ b/test_programs/compile_success_empty/regression_7038_3/src/main.nr @@ -0,0 +1,40 @@ +trait BigNumTrait {} + +pub struct MyBigNum; + +impl crate::BigNumTrait for MyBigNum {} + +trait CurveParamsTrait { + // The difference between this and regression_7038 and regression_7038_2 is that + // here the where clause is on the method, not the trait + fn one() + where + BigNum: BigNumTrait; +} + +pub struct BN254Params; +impl CurveParamsTrait for BN254Params { + fn one() {} +} + +trait BigCurveTrait { + fn two(); +} + +pub struct BigCurve {} + +type BN254 = BigCurve; + +impl BigCurveTrait for BigCurve +where + BigNum: BigNumTrait, + CurveParams: CurveParamsTrait, +{ + fn two() { + let _ = CurveParams::one(); + } +} + +fn main() { + let _ = BN254::two(); +} diff --git a/test_programs/compile_success_empty/regression_7038_4/Nargo.toml b/test_programs/compile_success_empty/regression_7038_4/Nargo.toml new file mode 100644 index 0000000000..435c8094c7 --- /dev/null +++ b/test_programs/compile_success_empty/regression_7038_4/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "regression_7038_4" +type = "bin" +authors = [""] +compiler_version = ">=0.30.0" + +[dependencies] diff --git a/test_programs/compile_success_empty/regression_7038_4/src/main.nr b/test_programs/compile_success_empty/regression_7038_4/src/main.nr new file mode 100644 index 0000000000..524f05a502 --- /dev/null +++ b/test_programs/compile_success_empty/regression_7038_4/src/main.nr @@ -0,0 +1,29 @@ +// This program is a reduction of regression_7038_3 that led to a monomorphizer crash +trait BigNumTrait {} + +trait CurveParamsTrait { + fn one() + where + BigNum: BigNumTrait; +} + +pub struct MyBigNum; + +impl BigNumTrait for MyBigNum {} + +pub struct Params; +impl CurveParamsTrait for Params { + + fn one() {} +} + +fn foo() +where + C: CurveParamsTrait, +{ + let _ = C::one(); +} + +fn main() { + foo::(); +} From f4d2271e048be55645a7213974c12d757b376b70 Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Tue, 14 Jan 2025 19:48:07 +0000 Subject: [PATCH 9/9] chore: reenable reports on rollup root circuits (#7061) --- .github/workflows/reports.yml | 28 ++++++++++++++++++++-------- test_programs/compilation_report.sh | 3 ++- test_programs/memory_report.sh | 5 +++-- 3 files changed, 25 insertions(+), 11 deletions(-) diff --git a/.github/workflows/reports.yml b/.github/workflows/reports.yml index 4207d1afff..b21b4a6bae 100644 --- a/.github/workflows/reports.yml +++ b/.github/workflows/reports.yml @@ -299,16 +299,16 @@ jobs: fail-fast: false matrix: include: - # - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-contracts, is_library: true } + # - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-contracts, cannot_execute: true } - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/private-kernel-inner, num_runs: 5 } - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/private-kernel-tail, num_runs: 5 } - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/private-kernel-reset, num_runs: 5 } - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-base-private, num_runs: 5 } - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-base-public, num_runs: 5 } - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-block-merge, num_runs: 5 } - #- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-block-root-empty } - #- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-block-single-tx } - #- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-block-root } + - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-block-root-empty, num_runs: 5, cannot_execute: true } + - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-block-root-single-tx, num_runs: 1, flags: "--skip-brillig-constraints-check --skip-underconstrained-check", cannot_execute: true } + - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-block-root, num_runs: 1, flags: "--skip-brillig-constraints-check --skip-underconstrained-check" } - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-merge, num_runs: 5 } - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-root, num_runs: 5 } @@ -352,10 +352,12 @@ jobs: chmod +x parse_time.sh cp /home/runner/work/noir/noir/scripts/test_programs/parse_time.sh ./parse_time.sh ./compilation_report.sh 1 ${{ matrix.project.num_runs }} - + env: + FLAGS: ${{ matrix.project.flags }} + - name: Generate execution report working-directory: ./test-repo/${{ matrix.project.path }} - if: ${{ !matrix.project.is_library }} + if: ${{ !matrix.project.cannot_execute }} run: | mv /home/runner/work/noir/noir/scripts/test_programs/execution_report.sh ./execution_report.sh mv /home/runner/work/noir/noir/scripts/test_programs/parse_time.sh ./parse_time.sh @@ -373,7 +375,7 @@ jobs: - name: Move execution report id: execution_report shell: bash - if: ${{ !matrix.project.is_library }} + if: ${{ !matrix.project.cannot_execute }} run: | PACKAGE_NAME=${{ matrix.project.path }} PACKAGE_NAME=$(basename $PACKAGE_NAME) @@ -451,10 +453,16 @@ jobs: # TODO: Bring this report back under a flag. The `noir-contracts` report takes just under 30 minutes. # - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-contracts } - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/private-kernel-inner } + - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/private-kernel-tail } - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/private-kernel-reset } - - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/private-kernel-tail } - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-base-private } - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-base-public } + - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-block-merge } + - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-block-root-empty, cannot_execute: true } + - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-block-root-single-tx, flags: "--skip-brillig-constraints-check --skip-underconstrained-check", cannot_execute: true } + - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-block-root, flags: "--skip-brillig-constraints-check --skip-underconstrained-check" } + - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-merge } + - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-root } name: External repo memory report - ${{ matrix.project.repo }}/${{ matrix.project.path }} steps: @@ -493,9 +501,12 @@ jobs: ./memory_report.sh 1 # Rename the memory report as the execution report is about to write to the same file cp memory_report.json compilation_memory_report.json + env: + FLAGS: ${{ matrix.project.flags }} - name: Generate execution memory report working-directory: ./test-repo/${{ matrix.project.path }} + if: ${{ !matrix.project.cannot_execute }} run: | ./memory_report.sh 1 1 @@ -518,6 +529,7 @@ jobs: - name: Move execution report id: execution_mem_report + if: ${{ !matrix.project.cannot_execute }} shell: bash run: | PACKAGE_NAME=${{ matrix.project.path }} diff --git a/test_programs/compilation_report.sh b/test_programs/compilation_report.sh index b12e398474..786dbd75fe 100755 --- a/test_programs/compilation_report.sh +++ b/test_programs/compilation_report.sh @@ -18,6 +18,7 @@ fi ITER="1" NUM_ARTIFACTS=${#tests_to_profile[@]} +FLAGS=${FLAGS:- ""} for dir in ${tests_to_profile[@]}; do if [[ " ${excluded_dirs[@]} " =~ " ${dir} " ]]; then @@ -41,7 +42,7 @@ for dir in ${tests_to_profile[@]}; do TOTAL_TIME=0 for ((i = 1; i <= NUM_RUNS; i++)); do - NOIR_LOG=trace NARGO_LOG_DIR=./tmp nargo compile --force --silence-warnings + NOIR_LOG=trace NARGO_LOG_DIR=./tmp nargo compile --force --silence-warnings $FLAGS done TIMES=($(jq -r '. | select(.target == "nargo::cli" and .fields.message == "close") | .fields."time.busy"' ./tmp/*)) diff --git a/test_programs/memory_report.sh b/test_programs/memory_report.sh index e501464c19..a5c13f58b0 100755 --- a/test_programs/memory_report.sh +++ b/test_programs/memory_report.sh @@ -19,6 +19,7 @@ fi FIRST="1" +FLAGS=${FLAGS:- ""} echo "{\"memory_reports\": [ " > memory_report.json for test_name in ${tests_to_profile[@]}; do @@ -35,12 +36,12 @@ for test_name in ${tests_to_profile[@]}; do test_name=$(basename $current_dir) fi - COMMAND="compile --force --silence-warnings" + COMMAND="compile --force --silence-warnings $FLAGS" if [ "$2" == "1" ]; then COMMAND="execute --silence-warnings" fi - heaptrack --output $current_dir/$test_name"_heap" $NARGO $COMMAND + heaptrack --output $current_dir/$test_name"_heap" $NARGO $COMMAND if test -f $current_dir/$test_name"_heap.gz"; then heaptrack --analyze $current_dir/$test_name"_heap.gz" > $current_dir/$test_name"_heap_analysis.txt"