From c4512d9c09fd16a937653a5cd836600456ee375c Mon Sep 17 00:00:00 2001 From: jfecher Date: Thu, 9 Jan 2025 08:55:25 -0600 Subject: [PATCH 01/10] chore: Remove resolve_is_unconstrained pass (#7004) --- compiler/noirc_evaluator/src/ssa.rs | 1 - compiler/noirc_evaluator/src/ssa/ir/dfg.rs | 1 + .../src/ssa/ir/instruction/call.rs | 5 +- .../src/ssa/opt/constant_folding.rs | 16 +++--- compiler/noirc_evaluator/src/ssa/opt/mod.rs | 1 - .../src/ssa/opt/resolve_is_unconstrained.rs | 55 ------------------- 6 files changed, 12 insertions(+), 67 deletions(-) delete mode 100644 compiler/noirc_evaluator/src/ssa/opt/resolve_is_unconstrained.rs diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index 02425618c3d..206fe8d9084 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -153,7 +153,6 @@ fn optimize_all(builder: SsaBuilder, options: &SsaEvaluatorOptions) -> Result SimplifyResult::None, - Intrinsic::IsUnconstrained => SimplifyResult::None, + Intrinsic::IsUnconstrained => { + let result = dfg.runtime().is_brillig().into(); + SimplifyResult::SimplifiedTo(dfg.make_constant(result, NumericType::bool())) + } Intrinsic::DerivePedersenGenerators => { if let Some(Type::Array(_, len)) = return_type.clone() { simplify_derive_generators(dfg, arguments, len, block, call_stack) diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index 8dca867fc6d..db249f3bc3a 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -1557,7 +1557,6 @@ mod test { // After EnableSideEffectsIf removal: brillig(inline) fn main f0 { b0(v0: Field, v1: Field, v2: u1): - v4 = call is_unconstrained() -> u1 v7 = call to_be_radix(v0, u32 256) -> [u8; 1] // `a.to_be_radix(256)`; inc_rc v7 v8 = call to_be_radix(v0, u32 256) -> [u8; 1] // duplicate load of `a` @@ -1574,14 +1573,13 @@ mod test { let expected = " brillig(inline) fn main f0 { b0(v0: Field, v1: Field, v2: u1): - v4 = call is_unconstrained() -> u1 - v7 = call to_be_radix(v0, u32 256) -> [u8; 1] - inc_rc v7 - inc_rc v7 - v8 = cast v2 as Field - v9 = mul v0, v8 - v10 = call to_be_radix(v9, u32 256) -> [u8; 1] - inc_rc v10 + v5 = call to_be_radix(v0, u32 256) -> [u8; 1] + inc_rc v5 + inc_rc v5 + v6 = cast v2 as Field + v7 = mul v0, v6 + v8 = call to_be_radix(v7, u32 256) -> [u8; 1] + inc_rc v8 enable_side_effects v2 return } diff --git a/compiler/noirc_evaluator/src/ssa/opt/mod.rs b/compiler/noirc_evaluator/src/ssa/opt/mod.rs index 58dbc25e869..1105e15c30e 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mod.rs @@ -21,7 +21,6 @@ mod remove_bit_shifts; mod remove_enable_side_effects; mod remove_if_else; mod remove_unreachable; -mod resolve_is_unconstrained; mod simplify_cfg; mod unrolling; diff --git a/compiler/noirc_evaluator/src/ssa/opt/resolve_is_unconstrained.rs b/compiler/noirc_evaluator/src/ssa/opt/resolve_is_unconstrained.rs deleted file mode 100644 index 3ac7535a1c4..00000000000 --- a/compiler/noirc_evaluator/src/ssa/opt/resolve_is_unconstrained.rs +++ /dev/null @@ -1,55 +0,0 @@ -use crate::ssa::{ - ir::{ - function::{Function, RuntimeType}, - instruction::{Instruction, Intrinsic}, - types::NumericType, - value::Value, - }, - ssa_gen::Ssa, -}; -use fxhash::FxHashSet as HashSet; - -impl Ssa { - /// An SSA pass to find any calls to `Intrinsic::IsUnconstrained` and replacing any uses of the result of the intrinsic - /// with the resolved boolean value. - /// Note that this pass must run after the pass that does runtime separation, since in SSA generation an ACIR function can end up targeting brillig. - #[tracing::instrument(level = "trace", skip(self))] - pub(crate) fn resolve_is_unconstrained(mut self) -> Self { - for func in self.functions.values_mut() { - func.replace_is_unconstrained_result(); - } - self - } -} - -impl Function { - pub(crate) fn replace_is_unconstrained_result(&mut self) { - let mut is_unconstrained_calls = HashSet::default(); - // Collect all calls to is_unconstrained - for block_id in self.reachable_blocks() { - for &instruction_id in self.dfg[block_id].instructions() { - let target_func = match &self.dfg[instruction_id] { - Instruction::Call { func, .. } => *func, - _ => continue, - }; - - if let Value::Intrinsic(Intrinsic::IsUnconstrained) = &self.dfg[target_func] { - is_unconstrained_calls.insert(instruction_id); - } - } - } - - let is_unconstrained = matches!(self.runtime(), RuntimeType::Brillig(_)).into(); - let is_within_unconstrained = self.dfg.make_constant(is_unconstrained, NumericType::bool()); - for instruction_id in is_unconstrained_calls { - let call_returns = self.dfg.instruction_results(instruction_id); - let original_return_id = call_returns[0]; - - // Replace all uses of the original return value with the constant - self.dfg.set_value_from_id(original_return_id, is_within_unconstrained); - - // Now remove the original instruction - self.dfg.remove_instruction(instruction_id); - } - } -} From fc61c2140e24ddc534184e6273619ce516af7243 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 9 Jan 2025 10:49:41 -0500 Subject: [PATCH 02/10] chore: Only resolved globals monomorphization (#7006) --- compiler/noirc_frontend/src/monomorphization/mod.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/compiler/noirc_frontend/src/monomorphization/mod.rs b/compiler/noirc_frontend/src/monomorphization/mod.rs index ce855f62526..c3a32f979d4 100644 --- a/compiler/noirc_frontend/src/monomorphization/mod.rs +++ b/compiler/noirc_frontend/src/monomorphization/mod.rs @@ -934,11 +934,9 @@ impl<'interner> Monomorphizer<'interner> { .into_hir_expression(self.interner, global.location) .map_err(MonomorphizationError::InterpreterError)? } else { - let let_ = self.interner.get_global_let_statement(*global_id).expect( - "Globals should have a corresponding let statement by monomorphization", - ); - let_.expression + unreachable!("All global values should be resolved at compile time and before monomorphization"); }; + self.expr(expr)? } DefinitionKind::Local(_) => match self.lookup_captured_expr(ident.id) { From e0d6840c56847915508b75800aed604c18ab8ee7 Mon Sep 17 00:00:00 2001 From: jfecher Date: Thu, 9 Jan 2025 10:08:30 -0600 Subject: [PATCH 03/10] chore: Add short circuit in ssa-gen for known if conditions (#7007) --- .../noirc_evaluator/src/ssa/ssa_gen/mod.rs | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs index 56a2723a038..10b8129e8db 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -2,6 +2,7 @@ pub(crate) mod context; mod program; mod value; +use acvm::AcirField; use noirc_frontend::token::FmtStrFragment; pub(crate) use program::Ssa; @@ -595,6 +596,9 @@ impl<'a> FunctionContext<'a> { /// ``` fn codegen_if(&mut self, if_expr: &ast::If) -> Result { let condition = self.codegen_non_tuple_expression(&if_expr.condition)?; + if let Some(result) = self.try_codegen_constant_if(condition, if_expr) { + return result; + } let then_block = self.builder.insert_block(); let else_block = self.builder.insert_block(); @@ -633,6 +637,25 @@ impl<'a> FunctionContext<'a> { Ok(result) } + /// If the condition is known, skip codegen for the then/else branch and only compile the + /// relevant branch. + fn try_codegen_constant_if( + &mut self, + condition: ValueId, + if_expr: &ast::If, + ) -> Option> { + let condition = self.builder.current_function.dfg.get_numeric_constant(condition)?; + + Some(if condition.is_zero() { + match if_expr.alternative.as_ref() { + Some(alternative) => self.codegen_expression(alternative), + None => Ok(Self::unit_value()), + } + } else { + self.codegen_expression(&if_expr.consequence) + }) + } + fn codegen_tuple(&mut self, tuple: &[Expression]) -> Result { Ok(Tree::Branch(try_vecmap(tuple, |expr| self.codegen_expression(expr))?)) } From a7eea81e8418a90ba415a1f37157371f56ea4f1d Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Thu, 9 Jan 2025 16:31:36 +0000 Subject: [PATCH 04/10] feat(comptime): Implement to_be_bits and to_le_bits in the interpreter (#7008) Co-authored-by: jfecher --- compiler/noirc_evaluator/src/acir/mod.rs | 2 +- .../noirc_frontend/src/elaborator/types.rs | 2 +- .../src/hir/comptime/interpreter/builtin.rs | 22 +++++++++++++++++++ .../noirc_frontend/src/hir/comptime/tests.rs | 2 ++ compiler/noirc_frontend/src/tests.rs | 13 ++++++----- .../noir_test_success/global_eval/Nargo.toml | 7 ++++++ .../noir_test_success/global_eval/src/main.nr | 20 +++++++++++++++++ 7 files changed, 61 insertions(+), 7 deletions(-) create mode 100644 test_programs/noir_test_success/global_eval/Nargo.toml create mode 100644 test_programs/noir_test_success/global_eval/src/main.nr diff --git a/compiler/noirc_evaluator/src/acir/mod.rs b/compiler/noirc_evaluator/src/acir/mod.rs index 7ecc4d28032..e57f5d18830 100644 --- a/compiler/noirc_evaluator/src/acir/mod.rs +++ b/compiler/noirc_evaluator/src/acir/mod.rs @@ -2169,7 +2169,7 @@ impl<'a> Context<'a> { let Type::Array(result_type, array_length) = dfg.type_of_value(result_ids[0]) else { - unreachable!("ICE: ToRadix result must be an array"); + unreachable!("ICE: ToBits result must be an array"); }; self.acir_context diff --git a/compiler/noirc_frontend/src/elaborator/types.rs b/compiler/noirc_frontend/src/elaborator/types.rs index 0e26d0b5bd8..98b08266f67 100644 --- a/compiler/noirc_frontend/src/elaborator/types.rs +++ b/compiler/noirc_frontend/src/elaborator/types.rs @@ -1526,7 +1526,7 @@ impl<'context> Elaborator<'context> { ) -> Option { let func_id = match self.current_item { Some(DependencyId::Function(id)) => id, - _ => panic!("unexpected method outside a function"), + _ => panic!("unexpected method outside a function: {method_name}"), }; let func_meta = self.interner.function_meta(&func_id); diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs index 8a9fcf12e16..730bcd1be6c 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs +++ b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs @@ -192,6 +192,8 @@ impl<'local, 'context> Interpreter<'local, 'context> { "struct_def_set_fields" => struct_def_set_fields(interner, arguments, location), "to_be_radix" => to_be_radix(arguments, return_type, location), "to_le_radix" => to_le_radix(arguments, return_type, location), + "to_be_bits" => to_be_bits(arguments, return_type, location), + "to_le_bits" => to_le_bits(arguments, return_type, location), "trait_constraint_eq" => trait_constraint_eq(arguments, location), "trait_constraint_hash" => trait_constraint_hash(arguments, location), "trait_def_as_trait_constraint" => { @@ -776,6 +778,26 @@ fn quoted_tokens(arguments: Vec<(Value, Location)>, location: Location) -> IResu )) } +fn to_be_bits( + arguments: Vec<(Value, Location)>, + return_type: Type, + location: Location, +) -> IResult { + let value = check_one_argument(arguments, location)?; + let radix = (Value::U32(2), value.1); + to_be_radix(vec![value, radix], return_type, location) +} + +fn to_le_bits( + arguments: Vec<(Value, Location)>, + return_type: Type, + location: Location, +) -> IResult { + let value = check_one_argument(arguments, location)?; + let radix = (Value::U32(2), value.1); + to_le_radix(vec![value, radix], return_type, location) +} + fn to_be_radix( arguments: Vec<(Value, Location)>, return_type: Type, diff --git a/compiler/noirc_frontend/src/hir/comptime/tests.rs b/compiler/noirc_frontend/src/hir/comptime/tests.rs index ce8d43e56d5..342d0a616a0 100644 --- a/compiler/noirc_frontend/src/hir/comptime/tests.rs +++ b/compiler/noirc_frontend/src/hir/comptime/tests.rs @@ -19,6 +19,8 @@ use crate::node_interner::FuncId; use crate::parse_program; /// Create an interpreter for a code snippet and pass it to a test function. +/// +/// The stdlib is not made available as a dependency. pub(crate) fn with_interpreter( src: &str, f: impl FnOnce(&mut Interpreter, FuncId, &[(CompilationError, FileId)]) -> T, diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index c657a839a33..c1b86b5dcf7 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -65,6 +65,9 @@ pub(crate) fn get_program(src: &str) -> (ParsedModule, Context, Vec<(Compilation ) } +/// Compile a program. +/// +/// The stdlib is not available for these snippets. pub(crate) fn get_program_with_maybe_parser_errors( src: &str, allow_parser_errors: bool, @@ -3894,10 +3897,10 @@ fn errors_on_cyclic_globals() { #[test] fn warns_on_unneeded_unsafe() { let src = r#" - fn main() { + fn main() { /// Safety: test unsafe { - foo() + foo() } } @@ -3914,12 +3917,12 @@ fn warns_on_unneeded_unsafe() { #[test] fn warns_on_nested_unsafe() { let src = r#" - fn main() { + fn main() { /// Safety: test - unsafe { + unsafe { /// Safety: test unsafe { - foo() + foo() } } } diff --git a/test_programs/noir_test_success/global_eval/Nargo.toml b/test_programs/noir_test_success/global_eval/Nargo.toml new file mode 100644 index 00000000000..1dfe3a9660a --- /dev/null +++ b/test_programs/noir_test_success/global_eval/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "global_eval" +type = "bin" +authors = [""] +compiler_version = ">=0.27.0" + +[dependencies] diff --git a/test_programs/noir_test_success/global_eval/src/main.nr b/test_programs/noir_test_success/global_eval/src/main.nr new file mode 100644 index 00000000000..6ec366c4cd6 --- /dev/null +++ b/test_programs/noir_test_success/global_eval/src/main.nr @@ -0,0 +1,20 @@ +use std::uint128::U128; + +// These definitions require `to_be_bits` and `to_le_bits` to be supported at comptime. +global BITS_BE_13: [u1; 4] = (13 as Field).to_be_bits(); +global BITS_LE_13: [u1; 4] = (13 as Field).to_le_bits(); + +// Examples from #6691 which use the above behind the scenes. +global POW64_A: Field = 2.pow_32(64); +global POW64_B: Field = (U128::one() << 64).to_integer(); + +#[test] +fn test_be_and_le_bits() { + assert_eq(BITS_BE_13, [1,1,0,1]); + assert_eq(BITS_LE_13, [1,0,1,1]); +} + +#[test] +fn test_pow64() { + assert_eq(POW64_A, POW64_B); +} From ae9f2aed62a81c75026f47abaaf41cd2ec533275 Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Thu, 9 Jan 2025 16:58:57 +0000 Subject: [PATCH 05/10] chore: turn on averaging for protocol circuits metrics in CI (#6999) --- .../{.failures.jsonl.does_not_compile => .failures.jsonl} | 0 .github/workflows/reports.yml | 8 ++++---- 2 files changed, 4 insertions(+), 4 deletions(-) rename .github/critical_libraries_status/noir-lang/noir_rsa/{.failures.jsonl.does_not_compile => .failures.jsonl} (100%) diff --git a/.github/critical_libraries_status/noir-lang/noir_rsa/.failures.jsonl.does_not_compile b/.github/critical_libraries_status/noir-lang/noir_rsa/.failures.jsonl similarity index 100% rename from .github/critical_libraries_status/noir-lang/noir_rsa/.failures.jsonl.does_not_compile rename to .github/critical_libraries_status/noir-lang/noir_rsa/.failures.jsonl diff --git a/.github/workflows/reports.yml b/.github/workflows/reports.yml index 811f4d6f37b..b18ea731358 100644 --- a/.github/workflows/reports.yml +++ b/.github/workflows/reports.yml @@ -303,11 +303,11 @@ jobs: - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/private-kernel-inner, take_average: true } - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/private-kernel-tail, take_average: true } - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/private-kernel-reset, take_average: true } - - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-root } + - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-root, take_average: true } #- 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-merge } - - 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, take_average: true } + - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-base-private, take_average: true } + - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-base-public, take_average: true } name: External repo compilation and execution reports - ${{ matrix.project.repo }}/${{ matrix.project.path }} steps: From 4d38a8849c2731182bef11f4e616561c498a76d7 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 9 Jan 2025 14:18:48 -0300 Subject: [PATCH 06/10] feat(lsp): use trait method docs for trait impl method docs on hover (#7003) --- tooling/lsp/Cargo.toml | 4 +-- tooling/lsp/src/requests/hover.rs | 32 +++++++++++++++++-- .../test_programs/workspace/two/src/lib.nr | 9 ++++++ 3 files changed, 40 insertions(+), 5 deletions(-) diff --git a/tooling/lsp/Cargo.toml b/tooling/lsp/Cargo.toml index 04c8edf7ea9..65b59552b32 100644 --- a/tooling/lsp/Cargo.toml +++ b/tooling/lsp/Cargo.toml @@ -3,7 +3,7 @@ name = "noir_lsp" description = "Language server for Noir" version.workspace = true authors.workspace = true -edition.workspace = true# +edition.workspace = true rust-version.workspace = true license.workspace = true @@ -16,7 +16,7 @@ workspace = true acvm.workspace = true codespan-lsp.workspace = true lsp-types.workspace = true -nargo.workspace = true +nargo = { workspace = true, features = ["rpc"] } nargo_fmt.workspace = true nargo_toml.workspace = true noirc_driver.workspace = true diff --git a/tooling/lsp/src/requests/hover.rs b/tooling/lsp/src/requests/hover.rs index e2e2d2881dc..9da24fd1c8a 100644 --- a/tooling/lsp/src/requests/hover.rs +++ b/tooling/lsp/src/requests/hover.rs @@ -410,8 +410,10 @@ fn format_function(id: FuncId, args: &ProcessRequestCallbackArgs) -> String { string.push_str("comptime "); } + let func_name = &func_name_definition_id.name; + string.push_str("fn "); - string.push_str(&func_name_definition_id.name); + string.push_str(func_name); format_generics(&func_meta.direct_generics, &mut string); string.push('('); let parameters = &func_meta.parameters; @@ -442,7 +444,20 @@ fn format_function(id: FuncId, args: &ProcessRequestCallbackArgs) -> String { string.push_str(&go_to_type_links(return_type, args.interner, args.files)); - append_doc_comments(args.interner, ReferenceId::Function(id), &mut string); + let had_doc_comments = + append_doc_comments(args.interner, ReferenceId::Function(id), &mut string); + if !had_doc_comments { + // If this function doesn't have doc comments, but it's a trait impl method, + // use the trait method doc comments. + if let Some(trait_impl_id) = func_meta.trait_impl { + let trait_impl = args.interner.get_trait_implementation(trait_impl_id); + let trait_impl = trait_impl.borrow(); + let trait_ = args.interner.get_trait(trait_impl.trait_id); + if let Some(func_id) = trait_.method_ids.get(func_name) { + append_doc_comments(args.interner, ReferenceId::Function(*func_id), &mut string); + } + } + } string } @@ -776,13 +791,16 @@ fn format_link(name: String, location: lsp_types::Location) -> String { ) } -fn append_doc_comments(interner: &NodeInterner, id: ReferenceId, string: &mut String) { +fn append_doc_comments(interner: &NodeInterner, id: ReferenceId, string: &mut String) -> bool { if let Some(doc_comments) = interner.doc_comments(id) { string.push_str("\n\n---\n\n"); for comment in doc_comments { string.push_str(comment); string.push('\n'); } + true + } else { + false } } @@ -1139,4 +1157,12 @@ mod hover_tests { pub fn bar_stuff(self)" )); } + + #[test] + async fn hover_on_trait_impl_method_uses_docs_from_trait_method() { + let hover_text = + get_hover_text("workspace", "two/src/lib.nr", Position { line: 92, character: 8 }) + .await; + assert!(hover_text.contains("Some docs")); + } } diff --git a/tooling/lsp/test_programs/workspace/two/src/lib.nr b/tooling/lsp/test_programs/workspace/two/src/lib.nr index 2dec902f327..d18a663b276 100644 --- a/tooling/lsp/test_programs/workspace/two/src/lib.nr +++ b/tooling/lsp/test_programs/workspace/two/src/lib.nr @@ -84,3 +84,12 @@ fn bar_stuff() { foo.bar_stuff(); } +trait TraitWithDocs { + /// Some docs + fn foo(); +} + +impl TraitWithDocs for Field { + fn foo() {} +} + From 56c931a8e59e9e67e8bd5aae4a114e85fe40ff98 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 9 Jan 2025 15:56:42 -0300 Subject: [PATCH 07/10] feat!: require trait primitive functions/calls to have their trait in scope (#6901) Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> --- .../noirc_frontend/src/elaborator/types.rs | 77 ++++++------- compiler/noirc_frontend/src/hir_def/types.rs | 2 +- compiler/noirc_frontend/src/node_interner.rs | 106 ++++++------------ compiler/noirc_frontend/src/tests/traits.rs | 65 +++++++++++ noir_stdlib/src/meta/ctstring.nr | 2 + noir_stdlib/src/meta/mod.nr | 1 + noir_stdlib/src/meta/op.nr | 2 + .../src/main.nr | 2 + .../comptime_trait_constraint/src/main.nr | 2 +- .../ctstring/src/main.nr | 2 + .../regression_5428/src/main.nr | 2 + 11 files changed, 144 insertions(+), 119 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/types.rs b/compiler/noirc_frontend/src/elaborator/types.rs index 98b08266f67..9b3df4631b4 100644 --- a/compiler/noirc_frontend/src/elaborator/types.rs +++ b/compiler/noirc_frontend/src/elaborator/types.rs @@ -34,8 +34,7 @@ use crate::{ TraitImplKind, TraitMethodId, }, token::SecondaryAttribute, - Generics, Kind, ResolvedGeneric, Shared, StructType, Type, TypeBinding, TypeBindings, - UnificationError, + Generics, Kind, ResolvedGeneric, Type, TypeBinding, TypeBindings, UnificationError, }; use super::{lints, path_resolution::PathResolutionItem, Elaborator, UnsafeBlockStatus}; @@ -1320,9 +1319,6 @@ impl<'context> Elaborator<'context> { has_self_arg: bool, ) -> Option { match object_type.follow_bindings() { - Type::Struct(typ, _args) => { - self.lookup_struct_method(object_type, method_name, span, has_self_arg, typ) - } // TODO: We should allow method calls on `impl Trait`s eventually. // For now it is fine since they are only allowed on return types. Type::TraitAsType(..) => { @@ -1338,11 +1334,9 @@ impl<'context> Elaborator<'context> { } // Mutable references to another type should resolve to methods of their element type. // This may be a struct or a primitive type. - Type::MutableReference(element) => self - .interner - .lookup_primitive_trait_method_mut(element.as_ref(), method_name, has_self_arg) - .map(HirMethodReference::FuncId) - .or_else(|| self.lookup_method(&element, method_name, span, has_self_arg)), + Type::MutableReference(element) => { + self.lookup_method(&element, method_name, span, has_self_arg) + } // If we fail to resolve the object to a struct type, we have no way of type // checking its arguments as we can't even resolve the name of the function @@ -1354,39 +1348,29 @@ impl<'context> Elaborator<'context> { None } - other => match self.interner.lookup_primitive_method(&other, method_name, has_self_arg) - { - Some(method_id) => Some(HirMethodReference::FuncId(method_id)), - None => { - // It could be that this type is a composite type that is bound to a trait, - // for example `x: (T, U) ... where (T, U): SomeTrait` - // (so this case is a generalization of the NamedGeneric case) - self.lookup_method_in_trait_constraints(object_type, method_name, span) - } - }, + other => { + self.lookup_struct_or_primitive_method(&other, method_name, span, has_self_arg) + } } } - fn lookup_struct_method( + fn lookup_struct_or_primitive_method( &mut self, object_type: &Type, method_name: &str, span: Span, has_self_arg: bool, - typ: Shared, ) -> Option { - let id = typ.borrow().id; - // First search in the struct methods if let Some(method_id) = - self.interner.lookup_direct_method(object_type, id, method_name, has_self_arg) + self.interner.lookup_direct_method(object_type, method_name, has_self_arg) { return Some(HirMethodReference::FuncId(method_id)); } // Next lookup all matching trait methods. let trait_methods = - self.interner.lookup_trait_methods(object_type, id, method_name, has_self_arg); + self.interner.lookup_trait_methods(object_type, method_name, has_self_arg); if trait_methods.is_empty() { // If we couldn't find any trait methods, search in @@ -1397,26 +1381,31 @@ impl<'context> Elaborator<'context> { return Some(HirMethodReference::FuncId(func_id)); } - // Otherwise it's an error - let has_field_with_function_type = typ - .borrow() - .get_fields_as_written() - .into_iter() - .any(|field| field.name.0.contents == method_name && field.typ.is_function()); - if has_field_with_function_type { - self.push_err(TypeCheckError::CannotInvokeStructFieldFunctionType { - method_name: method_name.to_string(), - object_type: object_type.clone(), - span, - }); + if let Type::Struct(struct_type, _) = object_type { + let has_field_with_function_type = + struct_type.borrow().get_fields_as_written().into_iter().any(|field| { + field.name.0.contents == method_name && field.typ.is_function() + }); + if has_field_with_function_type { + self.push_err(TypeCheckError::CannotInvokeStructFieldFunctionType { + method_name: method_name.to_string(), + object_type: object_type.clone(), + span, + }); + } else { + self.push_err(TypeCheckError::UnresolvedMethodCall { + method_name: method_name.to_string(), + object_type: object_type.clone(), + span, + }); + } + return None; } else { - self.push_err(TypeCheckError::UnresolvedMethodCall { - method_name: method_name.to_string(), - object_type: object_type.clone(), - span, - }); + // It could be that this type is a composite type that is bound to a trait, + // for example `x: (T, U) ... where (T, U): SomeTrait` + // (so this case is a generalization of the NamedGeneric case) + return self.lookup_method_in_trait_constraints(object_type, method_name, span); } - return None; } // We found some trait methods... but is only one of them currently in scope? diff --git a/compiler/noirc_frontend/src/hir_def/types.rs b/compiler/noirc_frontend/src/hir_def/types.rs index 0ed6399296b..c0dbf6f9500 100644 --- a/compiler/noirc_frontend/src/hir_def/types.rs +++ b/compiler/noirc_frontend/src/hir_def/types.rs @@ -1862,7 +1862,7 @@ impl Type { if let (Type::Array(_size, element1), Type::Slice(element2)) = (&this, &target) { // We can only do the coercion if the `as_slice` method exists. // This is usually true, but some tests don't have access to the standard library. - if let Some(as_slice) = interner.lookup_primitive_method(&this, "as_slice", true) { + if let Some(as_slice) = interner.lookup_direct_method(&this, "as_slice", true) { // Still have to ensure the element types match. // Don't need to issue an error here if not, it will be done in unify_with_coercions let mut bindings = TypeBindings::new(); diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index 599558bb91a..c1a405535f2 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -183,16 +183,13 @@ pub struct NodeInterner { next_type_variable_id: std::cell::Cell, - /// A map from a struct type and method name to a function id for the method. + /// A map from a type and method name to a function id for the method. /// This can resolve to potentially multiple methods if the same method name is /// specialized for different generics on the same type. E.g. for `Struct`, we /// may have both `impl Struct { fn foo(){} }` and `impl Struct { fn foo(){} }`. /// If this happens, the returned Vec will have 2 entries and we'll need to further /// disambiguate them by checking the type of each function. - struct_methods: HashMap>, - - /// Methods on primitive types defined in the stdlib. - primitive_methods: HashMap>, + methods: HashMap>, // For trait implementation functions, this is their self type and trait they belong to func_id_to_trait: HashMap, @@ -666,8 +663,7 @@ impl Default for NodeInterner { next_type_variable_id: std::cell::Cell::new(0), globals: Vec::new(), global_attributes: HashMap::default(), - struct_methods: HashMap::default(), - primitive_methods: HashMap::default(), + methods: HashMap::default(), type_alias_ref: Vec::new(), type_ref_locations: Vec::new(), quoted_types: Default::default(), @@ -1214,27 +1210,8 @@ impl NodeInterner { self.structs[&id].clone() } - pub fn get_struct_methods(&self, id: StructId) -> Option<&HashMap> { - self.struct_methods.get(&id) - } - - fn get_primitive_methods(&self, key: TypeMethodKey) -> Option<&HashMap> { - self.primitive_methods.get(&key) - } - pub fn get_type_methods(&self, typ: &Type) -> Option<&HashMap> { - match typ { - Type::Struct(struct_type, _) => { - let struct_type = struct_type.borrow(); - self.get_struct_methods(struct_type.id) - } - Type::Alias(type_alias, generics) => { - let type_alias = type_alias.borrow(); - let typ = type_alias.get_type(generics); - self.get_type_methods(&typ) - } - _ => get_type_method_key(typ).and_then(|key| self.get_primitive_methods(key)), - } + get_type_method_key(typ).and_then(|key| self.methods.get(&key)) } pub fn get_trait(&self, id: TraitId) -> &Trait { @@ -1394,39 +1371,27 @@ impl NodeInterner { trait_id: Option, ) -> Option { match self_type { - Type::Struct(struct_type, _generics) => { - let id = struct_type.borrow().id; - - if trait_id.is_none() { - if let Some(existing) = - self.lookup_direct_method(self_type, id, &method_name, true) - { - return Some(existing); - } - } - - self.struct_methods - .entry(id) - .or_default() - .entry(method_name) - .or_default() - .add_method(method_id, None, trait_id); - None - } Type::Error => None, Type::MutableReference(element) => { self.add_method(element, method_name, method_id, trait_id) } - - other => { + _ => { let key = get_type_method_key(self_type).unwrap_or_else(|| { - unreachable!("Cannot add a method to the unsupported type '{}'", other) + unreachable!("Cannot add a method to the unsupported type '{}'", self_type) }); + + if trait_id.is_none() && matches!(self_type, Type::Struct(..)) { + if let Some(existing) = self.lookup_direct_method(self_type, &method_name, true) + { + return Some(existing); + } + } + // Only remember the actual type if it's FieldOrInt, // so later we can disambiguate on calls like `u32::call`. let typ = if key == TypeMethodKey::FieldOrInt { Some(self_type.clone()) } else { None }; - self.primitive_methods + self.methods .entry(key) .or_default() .entry(method_name) @@ -1785,12 +1750,13 @@ impl NodeInterner { pub fn lookup_direct_method( &self, typ: &Type, - id: StructId, method_name: &str, has_self_arg: bool, ) -> Option { - self.struct_methods - .get(&id) + let key = get_type_method_key(typ)?; + + self.methods + .get(&key) .and_then(|h| h.get(method_name)) .and_then(|methods| methods.find_direct_method(typ, has_self_arg, self)) } @@ -1799,15 +1765,19 @@ impl NodeInterner { pub fn lookup_trait_methods( &self, typ: &Type, - id: StructId, method_name: &str, has_self_arg: bool, ) -> Vec<(FuncId, TraitId)> { - self.struct_methods - .get(&id) - .and_then(|h| h.get(method_name)) - .map(|methods| methods.find_trait_methods(typ, has_self_arg, self)) - .unwrap_or_default() + let key = get_type_method_key(typ); + if let Some(key) = key { + self.methods + .get(&key) + .and_then(|h| h.get(method_name)) + .map(|methods| methods.find_trait_methods(typ, has_self_arg, self)) + .unwrap_or_default() + } else { + Vec::new() + } } /// Select the 1 matching method with an object type matching `typ` @@ -1833,8 +1803,7 @@ impl NodeInterner { method_name: &str, has_self_arg: bool, ) -> Option { - let global_methods = - self.primitive_methods.get(&TypeMethodKey::Generic)?.get(method_name)?; + let global_methods = self.methods.get(&TypeMethodKey::Generic)?.get(method_name)?; global_methods.find_matching_method(typ, has_self_arg, self) } @@ -1846,20 +1815,10 @@ impl NodeInterner { has_self_arg: bool, ) -> Option { let key = get_type_method_key(typ)?; - let methods = self.primitive_methods.get(&key)?.get(method_name)?; + let methods = self.methods.get(&key)?.get(method_name)?; self.find_matching_method(typ, Some(methods), method_name, has_self_arg) } - pub fn lookup_primitive_trait_method_mut( - &self, - typ: &Type, - method_name: &str, - has_self_arg: bool, - ) -> Option { - let typ = Type::MutableReference(Box::new(typ.clone())); - self.lookup_primitive_method(&typ, method_name, has_self_arg) - } - /// Returns what the next trait impl id is expected to be. pub fn next_trait_impl_id(&mut self) -> TraitImplId { let next_id = self.next_trait_implementation_id; @@ -2462,6 +2421,7 @@ enum TypeMethodKey { Function, Generic, Quoted(QuotedType), + Struct(StructId), } fn get_type_method_key(typ: &Type) -> Option { @@ -2489,12 +2449,12 @@ fn get_type_method_key(typ: &Type) -> Option { Type::Quoted(quoted) => Some(Quoted(*quoted)), Type::MutableReference(element) => get_type_method_key(element), Type::Alias(alias, _) => get_type_method_key(&alias.borrow().typ), + Type::Struct(struct_type, _) => Some(Struct(struct_type.borrow().id)), // We do not support adding methods to these types Type::Forall(_, _) | Type::Constant(..) | Type::Error - | Type::Struct(_, _) | Type::InfixExpr(..) | Type::CheckedCast { .. } | Type::TraitAsType(..) => None, diff --git a/compiler/noirc_frontend/src/tests/traits.rs b/compiler/noirc_frontend/src/tests/traits.rs index faacd9aeee3..f5730451311 100644 --- a/compiler/noirc_frontend/src/tests/traits.rs +++ b/compiler/noirc_frontend/src/tests/traits.rs @@ -1101,3 +1101,68 @@ fn type_checks_trait_default_method_and_does_not_error_using_self() { "#; assert_no_errors(src); } + +#[test] +fn warns_if_trait_is_not_in_scope_for_primitive_function_call_and_there_is_only_one_trait_method() { + let src = r#" + fn main() { + let _ = Field::foo(); + } + + mod private_mod { + pub trait Foo { + fn foo() -> i32; + } + + impl Foo for Field { + fn foo() -> i32 { + 42 + } + } + } + "#; + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + + let CompilationError::ResolverError(ResolverError::PathResolutionError( + PathResolutionError::TraitMethodNotInScope { ident, trait_name }, + )) = &errors[0].0 + else { + panic!("Expected a 'trait method not in scope' error"); + }; + assert_eq!(ident.to_string(), "foo"); + assert_eq!(trait_name, "private_mod::Foo"); +} + +#[test] +fn warns_if_trait_is_not_in_scope_for_primitive_method_call_and_there_is_only_one_trait_method() { + let src = r#" + fn main() { + let x: Field = 1; + let _ = x.foo(); + } + + mod private_mod { + pub trait Foo { + fn foo(self) -> i32; + } + + impl Foo for Field { + fn foo(self) -> i32 { + self as i32 + } + } + } + "#; + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + + let CompilationError::ResolverError(ResolverError::PathResolutionError( + PathResolutionError::TraitMethodNotInScope { ident, trait_name }, + )) = &errors[0].0 + else { + panic!("Expected a 'trait method not in scope' error"); + }; + assert_eq!(ident.to_string(), "foo"); + assert_eq!(trait_name, "private_mod::Foo"); +} diff --git a/noir_stdlib/src/meta/ctstring.nr b/noir_stdlib/src/meta/ctstring.nr index b414b3418d9..e23567ece7d 100644 --- a/noir_stdlib/src/meta/ctstring.nr +++ b/noir_stdlib/src/meta/ctstring.nr @@ -86,6 +86,8 @@ comptime fn ctstring_eq(_first: CtString, _second: CtString) -> bool {} comptime fn ctstring_hash(_string: CtString) -> Field {} mod test { + use super::AsCtString; + #[test] fn as_quoted_str_example() { comptime { diff --git a/noir_stdlib/src/meta/mod.nr b/noir_stdlib/src/meta/mod.nr index 4edda9a3120..f6a1f4838ee 100644 --- a/noir_stdlib/src/meta/mod.nr +++ b/noir_stdlib/src/meta/mod.nr @@ -117,6 +117,7 @@ pub comptime fn make_trait_impl( } mod tests { + use crate::meta::ctstring::AsCtString; use crate::meta::derive_via; // docs:start:quote-example diff --git a/noir_stdlib/src/meta/op.nr b/noir_stdlib/src/meta/op.nr index afe230429f8..67c9b4cdb64 100644 --- a/noir_stdlib/src/meta/op.nr +++ b/noir_stdlib/src/meta/op.nr @@ -1,3 +1,5 @@ +use crate::hash::Hash; + pub struct UnaryOp { op: Field, } diff --git a/test_programs/compile_success_empty/comptime_change_type_each_iteration/src/main.nr b/test_programs/compile_success_empty/comptime_change_type_each_iteration/src/main.nr index 976987fec9e..153eb308911 100644 --- a/test_programs/compile_success_empty/comptime_change_type_each_iteration/src/main.nr +++ b/test_programs/compile_success_empty/comptime_change_type_each_iteration/src/main.nr @@ -1,3 +1,5 @@ +use std::meta::ctstring::AsCtString; + fn main() { comptime { for i in 9..11 { diff --git a/test_programs/compile_success_empty/comptime_trait_constraint/src/main.nr b/test_programs/compile_success_empty/comptime_trait_constraint/src/main.nr index 2f24675e8c7..39ac1b5cde1 100644 --- a/test_programs/compile_success_empty/comptime_trait_constraint/src/main.nr +++ b/test_programs/compile_success_empty/comptime_trait_constraint/src/main.nr @@ -1,4 +1,4 @@ -use std::hash::Hasher; +use std::hash::{Hash, Hasher}; trait TraitWithGenerics { fn foo(self) -> (A, B); diff --git a/test_programs/compile_success_empty/ctstring/src/main.nr b/test_programs/compile_success_empty/ctstring/src/main.nr index 61cd848a436..818ae7f94e8 100644 --- a/test_programs/compile_success_empty/ctstring/src/main.nr +++ b/test_programs/compile_success_empty/ctstring/src/main.nr @@ -1,3 +1,5 @@ +use std::meta::ctstring::AsCtString; + fn main() { comptime { let msg1 = "msg1"; diff --git a/test_programs/compile_success_empty/regression_5428/src/main.nr b/test_programs/compile_success_empty/regression_5428/src/main.nr index f01b89cbea4..e9e6c8a5dc8 100644 --- a/test_programs/compile_success_empty/regression_5428/src/main.nr +++ b/test_programs/compile_success_empty/regression_5428/src/main.nr @@ -1,3 +1,5 @@ +use std::append::Append; + fn main() { assert_true!(); } From 5300ec321fb99ddaad32e83f751aed28e175736f Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 9 Jan 2025 18:16:57 -0300 Subject: [PATCH 08/10] fix: require generic trait impls to be in scope to call them (#6913) Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> --- .../noirc_frontend/src/elaborator/types.rs | 80 +++++++++++-------- compiler/noirc_frontend/src/node_interner.rs | 45 +++-------- compiler/noirc_frontend/src/tests/traits.rs | 70 ++++++++++++++++ 3 files changed, 126 insertions(+), 69 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/types.rs b/compiler/noirc_frontend/src/elaborator/types.rs index 9b3df4631b4..72e46d36c2c 100644 --- a/compiler/noirc_frontend/src/elaborator/types.rs +++ b/compiler/noirc_frontend/src/elaborator/types.rs @@ -1361,7 +1361,7 @@ impl<'context> Elaborator<'context> { span: Span, has_self_arg: bool, ) -> Option { - // First search in the struct methods + // First search in the type methods. If there is one, that's the one. if let Some(method_id) = self.interner.lookup_direct_method(object_type, method_name, has_self_arg) { @@ -1372,43 +1372,55 @@ impl<'context> Elaborator<'context> { let trait_methods = self.interner.lookup_trait_methods(object_type, method_name, has_self_arg); - if trait_methods.is_empty() { - // If we couldn't find any trait methods, search in - // impls for all types `T`, e.g. `impl Foo for T` - if let Some(func_id) = - self.interner.lookup_generic_method(object_type, method_name, has_self_arg) - { - return Some(HirMethodReference::FuncId(func_id)); - } + // If there's at least one matching trait method we need to see if only one is in scope. + if !trait_methods.is_empty() { + return self.return_trait_method_in_scope(&trait_methods, method_name, span); + } - if let Type::Struct(struct_type, _) = object_type { - let has_field_with_function_type = - struct_type.borrow().get_fields_as_written().into_iter().any(|field| { - field.name.0.contents == method_name && field.typ.is_function() - }); - if has_field_with_function_type { - self.push_err(TypeCheckError::CannotInvokeStructFieldFunctionType { - method_name: method_name.to_string(), - object_type: object_type.clone(), - span, - }); - } else { - self.push_err(TypeCheckError::UnresolvedMethodCall { - method_name: method_name.to_string(), - object_type: object_type.clone(), - span, - }); - } - return None; + // If we couldn't find any trait methods, search in + // impls for all types `T`, e.g. `impl Foo for T` + let generic_methods = + self.interner.lookup_generic_methods(object_type, method_name, has_self_arg); + if !generic_methods.is_empty() { + return self.return_trait_method_in_scope(&generic_methods, method_name, span); + } + + if let Type::Struct(struct_type, _) = object_type { + let has_field_with_function_type = struct_type + .borrow() + .get_fields_as_written() + .into_iter() + .any(|field| field.name.0.contents == method_name && field.typ.is_function()); + if has_field_with_function_type { + self.push_err(TypeCheckError::CannotInvokeStructFieldFunctionType { + method_name: method_name.to_string(), + object_type: object_type.clone(), + span, + }); } else { - // It could be that this type is a composite type that is bound to a trait, - // for example `x: (T, U) ... where (T, U): SomeTrait` - // (so this case is a generalization of the NamedGeneric case) - return self.lookup_method_in_trait_constraints(object_type, method_name, span); + self.push_err(TypeCheckError::UnresolvedMethodCall { + method_name: method_name.to_string(), + object_type: object_type.clone(), + span, + }); } + None + } else { + // It could be that this type is a composite type that is bound to a trait, + // for example `x: (T, U) ... where (T, U): SomeTrait` + // (so this case is a generalization of the NamedGeneric case) + self.lookup_method_in_trait_constraints(object_type, method_name, span) } + } - // We found some trait methods... but is only one of them currently in scope? + /// Given a list of functions and the trait they belong to, returns the one function + /// that is in scope. + fn return_trait_method_in_scope( + &mut self, + trait_methods: &[(FuncId, TraitId)], + method_name: &str, + span: Span, + ) -> Option { let module_id = self.module_id(); let module_data = self.get_module(module_id); @@ -1490,7 +1502,7 @@ impl<'context> Elaborator<'context> { fn trait_hir_method_reference( &self, trait_id: TraitId, - trait_methods: Vec<(FuncId, TraitId)>, + trait_methods: &[(FuncId, TraitId)], method_name: &str, span: Span, ) -> HirMethodReference { diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index c1a405535f2..a2b1d7fc8f0 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -1746,7 +1746,7 @@ impl NodeInterner { Ok(()) } - /// Looks up a method that's directly defined in the given struct. + /// Looks up a method that's directly defined in the given type. pub fn lookup_direct_method( &self, typ: &Type, @@ -1761,7 +1761,7 @@ impl NodeInterner { .and_then(|methods| methods.find_direct_method(typ, has_self_arg, self)) } - /// Looks up a methods that apply to the given struct but are defined in traits. + /// Looks up a methods that apply to the given type but are defined in traits. pub fn lookup_trait_methods( &self, typ: &Type, @@ -1780,43 +1780,18 @@ impl NodeInterner { } } - /// Select the 1 matching method with an object type matching `typ` - fn find_matching_method( - &self, - typ: &Type, - methods: Option<&Methods>, - method_name: &str, - has_self_arg: bool, - ) -> Option { - if let Some(method) = methods.and_then(|m| m.find_matching_method(typ, has_self_arg, self)) - { - Some(method) - } else { - self.lookup_generic_method(typ, method_name, has_self_arg) - } - } - - /// Looks up a method at impls for all types `T`, e.g. `impl Foo for T` - pub fn lookup_generic_method( + /// Looks up methods at impls for all types `T`, e.g. `impl Foo for T` + pub fn lookup_generic_methods( &self, typ: &Type, method_name: &str, has_self_arg: bool, - ) -> Option { - let global_methods = self.methods.get(&TypeMethodKey::Generic)?.get(method_name)?; - global_methods.find_matching_method(typ, has_self_arg, self) - } - - /// Looks up a given method name on the given primitive type. - pub fn lookup_primitive_method( - &self, - typ: &Type, - method_name: &str, - has_self_arg: bool, - ) -> Option { - let key = get_type_method_key(typ)?; - let methods = self.methods.get(&key)?.get(method_name)?; - self.find_matching_method(typ, Some(methods), method_name, has_self_arg) + ) -> Vec<(FuncId, TraitId)> { + self.methods + .get(&TypeMethodKey::Generic) + .and_then(|h| h.get(method_name)) + .map(|methods| methods.find_trait_methods(typ, has_self_arg, self)) + .unwrap_or_default() } /// Returns what the next trait impl id is expected to be. diff --git a/compiler/noirc_frontend/src/tests/traits.rs b/compiler/noirc_frontend/src/tests/traits.rs index f5730451311..3a55fc2b67d 100644 --- a/compiler/noirc_frontend/src/tests/traits.rs +++ b/compiler/noirc_frontend/src/tests/traits.rs @@ -879,6 +879,43 @@ fn errors_if_multiple_trait_methods_are_in_scope_for_function_call() { assert_eq!(traits, vec!["private_mod::Foo", "private_mod::Foo2"]); } +#[test] +fn warns_if_trait_is_not_in_scope_for_method_call_and_there_is_only_one_trait_method() { + let src = r#" + fn main() { + let bar = Bar { x: 42 }; + let _ = bar.foo(); + } + + pub struct Bar { + x: i32, + } + + mod private_mod { + pub trait Foo { + fn foo(self) -> i32; + } + + impl Foo for super::Bar { + fn foo(self) -> i32 { + self.x + } + } + } + "#; + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + + let CompilationError::ResolverError(ResolverError::PathResolutionError( + PathResolutionError::TraitMethodNotInScope { ident, trait_name }, + )) = &errors[0].0 + else { + panic!("Expected a 'trait method not in scope' error"); + }; + assert_eq!(ident.to_string(), "foo"); + assert_eq!(trait_name, "private_mod::Foo"); +} + #[test] fn calls_trait_method_if_it_is_in_scope() { let src = r#" @@ -1166,3 +1203,36 @@ fn warns_if_trait_is_not_in_scope_for_primitive_method_call_and_there_is_only_on assert_eq!(ident.to_string(), "foo"); assert_eq!(trait_name, "private_mod::Foo"); } + +#[test] +fn warns_if_trait_is_not_in_scope_for_generic_function_call_and_there_is_only_one_trait_method() { + let src = r#" + fn main() { + let x: i32 = 1; + let _ = x.foo(); + } + + mod private_mod { + pub trait Foo { + fn foo(self) -> i32; + } + + impl Foo for T { + fn foo(self) -> i32 { + 42 + } + } + } + "#; + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + + let CompilationError::ResolverError(ResolverError::PathResolutionError( + PathResolutionError::TraitMethodNotInScope { ident, trait_name }, + )) = &errors[0].0 + else { + panic!("Expected a 'trait method not in scope' error"); + }; + assert_eq!(ident.to_string(), "foo"); + assert_eq!(trait_name, "private_mod::Foo"); +} From 2903052b5a7e74b0a14464d26d8b687bff4454eb Mon Sep 17 00:00:00 2001 From: Lisa <106527861+sthwnd@users.noreply.github.com> Date: Fri, 10 Jan 2025 14:26:16 +0200 Subject: [PATCH 09/10] chore: clarity fix in docs (#7016) Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> --- docs/docs/getting_started/quick_start.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/docs/getting_started/quick_start.md b/docs/docs/getting_started/quick_start.md index 7deeae12fd9..c980b5e7ffc 100644 --- a/docs/docs/getting_started/quick_start.md +++ b/docs/docs/getting_started/quick_start.md @@ -98,7 +98,7 @@ bb prove -b ./target/hello_world.json -w ./target/hello_world.gz -o ./target/pro :::tip -Naming can be confusing, specially as you pass them to the `bb` commands. If unsure, it won't hurt to delete the target folder and start anew to make sure you're using the most recent versions of the compiled circuit and witness. +Naming can be confusing, specially as you pass them to the `bb` commands. If unsure, it won't hurt to delete the target folder and start fresh to make sure you're using the most recent versions of the compiled circuit and witness. ::: From 24433fec92c4e0cab6a1f4a49c633c3e33219107 Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Fri, 10 Jan 2025 12:58:46 +0000 Subject: [PATCH 10/10] chore: mark `aztec-nr` as expected to compile (#7015) --- ...nr.failures.jsonl.does_not_compile => aztec-nr.failures.jsonl} | 0 ...lures.jsonl.does_not_compile => noir-contracts.failures.jsonl} | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename .github/critical_libraries_status/AztecProtocol/aztec-packages/noir-projects/{aztec-nr.failures.jsonl.does_not_compile => aztec-nr.failures.jsonl} (100%) rename .github/critical_libraries_status/AztecProtocol/aztec-packages/noir-projects/{noir-contracts.failures.jsonl.does_not_compile => noir-contracts.failures.jsonl} (100%) diff --git a/.github/critical_libraries_status/AztecProtocol/aztec-packages/noir-projects/aztec-nr.failures.jsonl.does_not_compile b/.github/critical_libraries_status/AztecProtocol/aztec-packages/noir-projects/aztec-nr.failures.jsonl similarity index 100% rename from .github/critical_libraries_status/AztecProtocol/aztec-packages/noir-projects/aztec-nr.failures.jsonl.does_not_compile rename to .github/critical_libraries_status/AztecProtocol/aztec-packages/noir-projects/aztec-nr.failures.jsonl diff --git a/.github/critical_libraries_status/AztecProtocol/aztec-packages/noir-projects/noir-contracts.failures.jsonl.does_not_compile b/.github/critical_libraries_status/AztecProtocol/aztec-packages/noir-projects/noir-contracts.failures.jsonl similarity index 100% rename from .github/critical_libraries_status/AztecProtocol/aztec-packages/noir-projects/noir-contracts.failures.jsonl.does_not_compile rename to .github/critical_libraries_status/AztecProtocol/aztec-packages/noir-projects/noir-contracts.failures.jsonl