From 51b7c07571d4353c84c9b001591f0af8777a9cc4 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 14 Jan 2025 15:41:00 -0500 Subject: [PATCH 1/3] chore(ci): Check various inliner aggressiveness setttings in Brillig reports (#7049) --- .github/workflows/reports.yml | 30 ++++++++++++++++--- test_programs/gates_report_brillig.sh | 2 +- .../gates_report_brillig_execution.sh | 2 +- 3 files changed, 28 insertions(+), 6 deletions(-) diff --git a/.github/workflows/reports.yml b/.github/workflows/reports.yml index b21b4a6baec..e14f116d105 100644 --- a/.github/workflows/reports.yml +++ b/.github/workflows/reports.yml @@ -116,8 +116,19 @@ jobs: - name: Generate Brillig bytecode size report working-directory: ./test_programs run: | - ./gates_report_brillig.sh - mv gates_report_brillig.json ../gates_report_brillig.json + mkdir ./reports + + ./gates_report_brillig.sh 9223372036854775807 + jq '.programs |= map(.package_name |= (. + "_inliner_max"))' gates_report_brillig.json > ./reports/gates_report_brillig_inliner_max.json + + ./gates_report_brillig.sh 0 + jq '.programs |= map(.package_name |= (. + "_inliner_zero"))' gates_report_brillig.json > ./reports/gates_report_brillig_inliner_zero.json + + ./gates_report_brillig.sh -9223372036854775808 + jq '.programs |= map(.package_name |= (. + "_inliner_min"))' gates_report_brillig.json > ./reports/gates_report_brillig_inliner_min.json + + # Merge all reports + jq -s '{ programs: map(.programs) | add }' ./reports/* > ../gates_report_brillig.json - name: Compare Brillig bytecode size reports id: brillig_bytecode_diff @@ -165,8 +176,19 @@ jobs: - name: Generate Brillig execution report working-directory: ./test_programs run: | - ./gates_report_brillig_execution.sh - mv gates_report_brillig_execution.json ../gates_report_brillig_execution.json + mkdir ./reports + + ./gates_report_brillig_execution.sh 9223372036854775807 + jq '.programs |= map(.package_name |= (. + "_inliner_max"))' gates_report_brillig_execution.json > ./reports/gates_report_brillig_execution_inliner_max.json + + ./gates_report_brillig_execution.sh 0 + jq '.programs |= map(.package_name |= (. + "_inliner_zero"))' gates_report_brillig_execution.json > ./reports/gates_report_brillig_execution_inliner_zero.json + + ./gates_report_brillig_execution.sh -9223372036854775808 + jq '.programs |= map(.package_name |= (. + "_inliner_min"))' gates_report_brillig_execution.json > ./reports/gates_report_brillig_execution_inliner_min.json + + # Merge all reports + jq -s '{ programs: map(.programs) | add }' ./reports/* > ../gates_report_brillig_execution.json - name: Compare Brillig execution reports id: brillig_execution_diff diff --git a/test_programs/gates_report_brillig.sh b/test_programs/gates_report_brillig.sh index 7343857a3c5..3f2a4542735 100755 --- a/test_programs/gates_report_brillig.sh +++ b/test_programs/gates_report_brillig.sh @@ -28,6 +28,6 @@ done echo "]" >> Nargo.toml -nargo info --force-brillig --json | jq -r ".programs[].functions = []" > gates_report_brillig.json +nargo info --silence-warnings --force-brillig --json --inliner-aggressiveness $1 | jq -r ".programs[].functions = []" > gates_report_brillig.json rm Nargo.toml diff --git a/test_programs/gates_report_brillig_execution.sh b/test_programs/gates_report_brillig_execution.sh index 219fbb5c11a..c6a904f8b93 100755 --- a/test_programs/gates_report_brillig_execution.sh +++ b/test_programs/gates_report_brillig_execution.sh @@ -42,6 +42,6 @@ done echo "]" >> Nargo.toml -nargo info --profile-execution --json | jq -r ".programs[].functions = []" > gates_report_brillig_execution.json +nargo info --silence-warnings --profile-execution --json --inliner-aggressiveness $1 | jq -r ".programs[].functions = []" > gates_report_brillig_execution.json rm Nargo.toml From 14a7e37d7f18f1d6e9148ecc77059999d5f9b14b Mon Sep 17 00:00:00 2001 From: jfecher Date: Tue, 14 Jan 2025 15:12:16 -0600 Subject: [PATCH 2/3] feat!: Handle generic fields in `StructDefinition::fields` and move old functionality to `StructDefinition::fields_as_written` (#7067) Co-authored-by: Michael J Klein --- .../src/hir/comptime/interpreter/builtin.rs | 54 +++++++++++++++++-- .../noir/standard_library/meta/struct_def.md | 13 ++++- noir_stdlib/src/cmp.nr | 2 +- noir_stdlib/src/meta/mod.nr | 6 +-- noir_stdlib/src/meta/struct_def.nr | 16 ++++-- .../comptime_struct_definition/src/main.nr | 22 +++++++- .../comptime_type/src/main.nr | 2 +- .../derive_impl/src/main.nr | 2 +- 8 files changed, 102 insertions(+), 15 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs index 3d3d0721c65..ace2d29431c 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs +++ b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs @@ -181,7 +181,10 @@ impl<'local, 'context> Interpreter<'local, 'context> { "struct_def_add_generic" => struct_def_add_generic(interner, arguments, location), "struct_def_as_type" => struct_def_as_type(interner, arguments, location), "struct_def_eq" => struct_def_eq(arguments, location), - "struct_def_fields" => struct_def_fields(interner, arguments, location), + "struct_def_fields" => struct_def_fields(interner, arguments, location, call_stack), + "struct_def_fields_as_written" => { + struct_def_fields_as_written(interner, arguments, location) + } "struct_def_generics" => struct_def_generics(interner, arguments, location), "struct_def_has_named_attribute" => { struct_def_has_named_attribute(interner, arguments, location) @@ -482,12 +485,57 @@ fn struct_def_has_named_attribute( Ok(Value::Bool(has_named_attribute(&name, interner.struct_attributes(&struct_id)))) } -/// fn fields(self) -> [(Quoted, Type)] -/// Returns (name, type) pairs of each field of this StructDefinition +/// fn fields(self, generic_args: [Type]) -> [(Quoted, Type)] +/// Returns (name, type) pairs of each field of this StructDefinition. +/// Applies the given generic arguments to each field. fn struct_def_fields( interner: &mut NodeInterner, arguments: Vec<(Value, Location)>, location: Location, + call_stack: &im::Vector, +) -> IResult { + let (typ, generic_args) = check_two_arguments(arguments, location)?; + let struct_id = get_struct(typ)?; + let struct_def = interner.get_struct(struct_id); + let struct_def = struct_def.borrow(); + + let args_location = generic_args.1; + let generic_args = get_slice(interner, generic_args)?.0; + let generic_args = try_vecmap(generic_args, |arg| get_type((arg, args_location)))?; + + let actual = generic_args.len(); + let expected = struct_def.generics.len(); + if actual != expected { + let s = if expected == 1 { "" } else { "s" }; + let was_were = if actual == 1 { "was" } else { "were" }; + let message = Some(format!("`StructDefinition::fields` expected {expected} generic{s} for `{}` but {actual} {was_were} given", struct_def.name)); + let location = args_location; + let call_stack = call_stack.clone(); + return Err(InterpreterError::FailingConstraint { message, location, call_stack }); + } + + let mut fields = im::Vector::new(); + + for (field_name, field_type) in struct_def.get_fields(&generic_args) { + let name = Value::Quoted(Rc::new(vec![Token::Ident(field_name)])); + fields.push_back(Value::Tuple(vec![name, Value::Type(field_type)])); + } + + let typ = Type::Slice(Box::new(Type::Tuple(vec![ + Type::Quoted(QuotedType::Quoted), + Type::Quoted(QuotedType::Type), + ]))); + Ok(Value::Slice(fields, typ)) +} + +/// fn fields_as_written(self) -> [(Quoted, Type)] +/// Returns (name, type) pairs of each field of this StructDefinition. +/// +/// Note that any generic arguments won't be applied: if you need them to be, use `fields`. +fn struct_def_fields_as_written( + interner: &mut NodeInterner, + arguments: Vec<(Value, Location)>, + location: Location, ) -> IResult { let argument = check_one_argument(arguments, location)?; let struct_id = get_struct(argument)?; diff --git a/docs/docs/noir/standard_library/meta/struct_def.md b/docs/docs/noir/standard_library/meta/struct_def.md index 535708e0353..8c6e50ba23a 100644 --- a/docs/docs/noir/standard_library/meta/struct_def.md +++ b/docs/docs/noir/standard_library/meta/struct_def.md @@ -66,7 +66,18 @@ comptime fn example(foo: StructDefinition) { #include_code fields noir_stdlib/src/meta/struct_def.nr rust -Returns each field of this struct as a pair of (field name, field type). +Returns (name, type) pairs of each field in this struct. +Any generic types used in each field type is automatically substituted with the +provided generic arguments. + +### fields_as_written + +#include_code fields_as_written noir_stdlib/src/meta/struct_def.nr rust + +Returns (name, type) pairs of each field in this struct. Each type is as-is +with any generic arguments unchanged. Unless the field types are not needed, +users should generally prefer to use `StructDefinition::fields` over this +function if possible. ### has_named_attribute diff --git a/noir_stdlib/src/cmp.nr b/noir_stdlib/src/cmp.nr index ae515150a4d..7f19594c30e 100644 --- a/noir_stdlib/src/cmp.nr +++ b/noir_stdlib/src/cmp.nr @@ -12,7 +12,7 @@ comptime fn derive_eq(s: StructDefinition) -> Quoted { let signature = quote { fn eq(_self: Self, _other: Self) -> bool }; let for_each_field = |name| quote { (_self.$name == _other.$name) }; let body = |fields| { - if s.fields().len() == 0 { + if s.fields_as_written().len() == 0 { quote { true } } else { fields diff --git a/noir_stdlib/src/meta/mod.nr b/noir_stdlib/src/meta/mod.nr index f6a1f4838ee..21c1b14cc96 100644 --- a/noir_stdlib/src/meta/mod.nr +++ b/noir_stdlib/src/meta/mod.nr @@ -101,7 +101,7 @@ pub comptime fn make_trait_impl( let where_clause = s.generics().map(|name| quote { $name: $trait_name }).join(quote {,}); // `for_each_field(field1) $join_fields_with for_each_field(field2) $join_fields_with ...` - let fields = s.fields().map(|f: (Quoted, Type)| { + let fields = s.fields_as_written().map(|f: (Quoted, Type)| { let name = f.0; for_each_field(name) }); @@ -155,7 +155,7 @@ mod tests { comptime fn derive_field_count(s: StructDefinition) -> Quoted { let typ = s.as_type(); - let field_count = s.fields().len(); + let field_count = s.fields_as_written().len(); quote { impl FieldCount for $typ { fn field_count() -> u32 { @@ -174,7 +174,7 @@ mod tests { comptime fn assert_field_is_type(s: StructDefinition, typ: Type) { // Assert the first field in `s` has type `typ` - let fields = s.fields(); + let fields = s.fields([]); assert_eq(fields[0].1, typ); } // docs:end:annotation-arguments-example diff --git a/noir_stdlib/src/meta/struct_def.nr b/noir_stdlib/src/meta/struct_def.nr index ba5d0289e73..0e64a537f1f 100644 --- a/noir_stdlib/src/meta/struct_def.nr +++ b/noir_stdlib/src/meta/struct_def.nr @@ -27,13 +27,23 @@ impl StructDefinition { pub comptime fn generics(self) -> [Type] {} // docs:end:generics - /// Returns (name, type) pairs of each field in this struct. Each type is as-is - /// with any generic arguments unchanged. + /// Returns (name, type) pairs of each field in this struct. + /// Any generic types used in each field type is automatically substituted with the + /// provided generic arguments. #[builtin(struct_def_fields)] // docs:start:fields - pub comptime fn fields(self) -> [(Quoted, Type)] {} + pub comptime fn fields(self, generic_args: [Type]) -> [(Quoted, Type)] {} // docs:end:fields + /// Returns (name, type) pairs of each field in this struct. Each type is as-is + /// with any generic arguments unchanged. Unless the field types are not needed, + /// users should generally prefer to use `StructDefinition::fields` over this + /// function if possible. + #[builtin(struct_def_fields_as_written)] + // docs:start:fields_as_written + pub comptime fn fields_as_written(self) -> [(Quoted, Type)] {} + // docs:end:fields_as_written + #[builtin(struct_def_module)] // docs:start:module pub comptime fn module(self) -> Module {} diff --git a/test_programs/compile_success_empty/comptime_struct_definition/src/main.nr b/test_programs/compile_success_empty/comptime_struct_definition/src/main.nr index 686ac26025d..75d48c6f1dd 100644 --- a/test_programs/compile_success_empty/comptime_struct_definition/src/main.nr +++ b/test_programs/compile_success_empty/comptime_struct_definition/src/main.nr @@ -12,7 +12,7 @@ pub struct I32AndField { comptime fn my_comptime_fn(typ: StructDefinition) { let _ = typ.as_type(); assert_eq(typ.generics().len(), 3); - assert_eq(typ.fields().len(), 2); + assert_eq(typ.fields_as_written().len(), 2); assert_eq(typ.name(), quote { MyType }); } @@ -44,4 +44,22 @@ mod foo { // docs:end:add-generic-example } -fn main() {} +fn main() { + comptime { + let typ = quote { MyType }.as_type(); + let (struct_def, generics) = typ.as_struct().unwrap(); + + let fields = struct_def.fields(generics); + assert_eq(fields.len(), 2); + + let (field1_name, field1_type) = fields[0]; + let (field2_name, field2_type) = fields[1]; + + assert_eq(field1_name, quote { field1 }); + assert_eq(field2_name, quote { field2 }); + + // Ensure .fields(generics) actually performs substitutions on generics + assert_eq(field1_type, quote { [i8; 10] }.as_type()); + assert_eq(field2_type, quote { (i16, i32) }.as_type()); + } +} diff --git a/test_programs/compile_success_empty/comptime_type/src/main.nr b/test_programs/compile_success_empty/comptime_type/src/main.nr index 887690cb6cb..6a2453ff0f2 100644 --- a/test_programs/compile_success_empty/comptime_type/src/main.nr +++ b/test_programs/compile_success_empty/comptime_type/src/main.nr @@ -100,7 +100,7 @@ fn main() { let foo = Foo { x: 0 }; let foo_type = type_of(foo); let (struct_definition, generics) = foo_type.as_struct().unwrap(); - let fields = struct_definition.fields(); + let fields = struct_definition.fields(generics); assert_eq(fields.len(), 1); assert_eq(generics.len(), 1); diff --git a/test_programs/compile_success_empty/derive_impl/src/main.nr b/test_programs/compile_success_empty/derive_impl/src/main.nr index 4396169235d..fe7a7140280 100644 --- a/test_programs/compile_success_empty/derive_impl/src/main.nr +++ b/test_programs/compile_success_empty/derive_impl/src/main.nr @@ -7,7 +7,7 @@ comptime fn derive_default(typ: StructDefinition) -> Quoted { ); let type_name = typ.as_type(); - let fields = typ.fields(); + let fields = typ.fields_as_written(); let fields = join(make_field_exprs(fields)); From 3b8d1da2ac9f751b8fff3eaad0c64cbba8d01575 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 14 Jan 2025 18:47:52 -0300 Subject: [PATCH 3/3] feat(LSP): code action to import trait in a method call (#7066) --- tooling/lsp/src/requests/code_action.rs | 3 + .../src/requests/code_action/import_trait.rs | 252 ++++++++++++++++++ tooling/lsp/src/requests/code_action/tests.rs | 2 +- 3 files changed, 256 insertions(+), 1 deletion(-) create mode 100644 tooling/lsp/src/requests/code_action/import_trait.rs diff --git a/tooling/lsp/src/requests/code_action.rs b/tooling/lsp/src/requests/code_action.rs index 38cc6bddf64..24ed327393d 100644 --- a/tooling/lsp/src/requests/code_action.rs +++ b/tooling/lsp/src/requests/code_action.rs @@ -33,6 +33,7 @@ use super::{process_request, to_lsp_location}; mod fill_struct_fields; mod implement_missing_members; mod import_or_qualify; +mod import_trait; mod remove_bang_from_call; mod remove_unused_import; mod tests; @@ -285,6 +286,8 @@ impl<'a> Visitor for CodeActionFinder<'a> { self.remove_bang_from_call(method_call.method_name.span()); } + self.import_trait_in_method_call(method_call); + true } } diff --git a/tooling/lsp/src/requests/code_action/import_trait.rs b/tooling/lsp/src/requests/code_action/import_trait.rs new file mode 100644 index 00000000000..1e731aa563b --- /dev/null +++ b/tooling/lsp/src/requests/code_action/import_trait.rs @@ -0,0 +1,252 @@ +use noirc_errors::Location; +use noirc_frontend::{ + ast::MethodCallExpression, + hir::{def_map::ModuleDefId, resolution::visibility::trait_member_is_visible}, + hir_def::traits::Trait, + node_interner::ReferenceId, +}; + +use crate::{ + modules::relative_module_full_path, + use_segment_positions::{ + use_completion_item_additional_text_edits, UseCompletionItemAdditionTextEditsRequest, + }, +}; + +use super::CodeActionFinder; + +impl<'a> CodeActionFinder<'a> { + pub(super) fn import_trait_in_method_call(&mut self, method_call: &MethodCallExpression) { + // First see if the method name already points to a function. + let name_location = Location::new(method_call.method_name.span(), self.file); + if let Some(ReferenceId::Function(func_id)) = self.interner.find_referenced(name_location) { + // If yes, it could be that the compiler is issuing a warning because there's + // only one possible trait that the method could be coming from, but it's not imported + let func_meta = self.interner.function_meta(&func_id); + let Some(trait_impl_id) = func_meta.trait_impl else { + return; + }; + + let trait_impl = self.interner.get_trait_implementation(trait_impl_id); + let trait_id = trait_impl.borrow().trait_id; + let trait_ = self.interner.get_trait(trait_id); + + // Check if the trait is currently imported. If so, no need to suggest anything + let module_data = + &self.def_maps[&self.module_id.krate].modules()[self.module_id.local_id.0]; + if !module_data.scope().find_name(&trait_.name).is_none() { + return; + } + + self.push_import_trait_code_action(trait_); + return; + } + + // Find out the type of the object + let object_location = Location::new(method_call.object.span, self.file); + let Some(typ) = self.interner.type_at_location(object_location) else { + return; + }; + + for (func_id, trait_id) in + self.interner.lookup_trait_methods(&typ, &method_call.method_name.0.contents, true) + { + let visibility = self.interner.function_modifiers(&func_id).visibility; + if !trait_member_is_visible(trait_id, visibility, self.module_id, self.def_maps) { + continue; + } + + let trait_ = self.interner.get_trait(trait_id); + self.push_import_trait_code_action(trait_); + } + } + + fn push_import_trait_code_action(&mut self, trait_: &Trait) { + let trait_id = trait_.id; + + let module_def_id = ModuleDefId::TraitId(trait_id); + let current_module_parent_id = self.module_id.parent(self.def_maps); + let Some(module_full_path) = relative_module_full_path( + module_def_id, + self.module_id, + current_module_parent_id, + self.interner, + ) else { + return; + }; + let full_path = format!("{}::{}", module_full_path, trait_.name); + + let title = format!("Import {}", full_path); + + let text_edits = use_completion_item_additional_text_edits( + UseCompletionItemAdditionTextEditsRequest { + full_path: &full_path, + files: self.files, + file: self.file, + lines: &self.lines, + nesting: self.nesting, + auto_import_line: self.auto_import_line, + }, + &self.use_segment_positions, + ); + + let code_action = self.new_quick_fix_multiple_edits(title, text_edits); + self.code_actions.push(code_action); + } +} + +#[cfg(test)] +mod tests { + use tokio::test; + + use crate::requests::code_action::tests::assert_code_action; + + #[test] + async fn test_import_trait_in_method_call_when_one_option_but_not_in_scope() { + let title = "Import moo::Foo"; + + let src = r#"mod moo { + pub trait Foo { + fn foobar(self); + } + + impl Foo for Field { + fn foobar(self) {} + } +} + +fn main() { + let x: Field = 1; + x.foo>||| CodeActionResponse { ) .await .expect("Could not execute on_code_action_request") - .unwrap() + .expect("Expected to get a CodeActionResponse, got None") } pub(crate) async fn assert_code_action(title: &str, src: &str, expected: &str) {