diff --git a/.noir-sync-commit b/.noir-sync-commit index 53551d013539..b8333111c299 100644 --- a/.noir-sync-commit +++ b/.noir-sync-commit @@ -1 +1 @@ -d8e549aad6bae0f96621c57b58a301693463f732 +60c770f5f2594eea31ac75c852980edefa40d9eb diff --git a/noir/noir-repo/Cargo.lock b/noir/noir-repo/Cargo.lock index dabac0a7570b..3add28d39391 100644 --- a/noir/noir-repo/Cargo.lock +++ b/noir/noir-repo/Cargo.lock @@ -2587,6 +2587,7 @@ dependencies = [ "termcolor", "termion", "test-binary", + "test-case", "thiserror", "tokio", "tokio-util 0.7.10", @@ -4423,6 +4424,39 @@ dependencies = [ "thiserror", ] +[[package]] +name = "test-case" +version = "3.3.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "eb2550dd13afcd286853192af8601920d959b14c401fcece38071d53bf0768a8" +dependencies = [ + "test-case-macros", +] + +[[package]] +name = "test-case-core" +version = "3.3.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "adcb7fd841cd518e279be3d5a3eb0636409487998a4aff22f3de87b81e88384f" +dependencies = [ + "cfg-if 1.0.0", + "proc-macro2", + "quote", + "syn 2.0.64", +] + +[[package]] +name = "test-case-macros" +version = "3.3.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5c89e72a01ed4c579669add59014b9a524d609c0c88c6a585ce37485879f6ffb" +dependencies = [ + "proc-macro2", + "quote", + "syn 2.0.64", + "test-case-core", +] + [[package]] name = "textwrap" version = "0.13.4" diff --git a/noir/noir-repo/Cargo.toml b/noir/noir-repo/Cargo.toml index 36b4d4c2ce1a..222e7dce859d 100644 --- a/noir/noir-repo/Cargo.toml +++ b/noir/noir-repo/Cargo.toml @@ -146,6 +146,7 @@ num-bigint = "0.4" num-traits = "0.2" similar-asserts = "1.5.0" tempfile = "3.6.0" +test-case = "3.3.1" jsonrpc = { version = "0.16.0", features = ["minreq_http"] } flate2 = "1.0.24" color-eyre = "0.6.2" diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/array_set.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/array_set.rs index 6ae13bc085ac..7035345436e1 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/array_set.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/array_set.rs @@ -53,7 +53,9 @@ struct Context<'f> { is_brillig_runtime: bool, array_to_last_use: HashMap, instructions_that_can_be_made_mutable: HashSet, - arrays_from_load: HashSet, + // Mapping of an array that comes from a load and whether the address + // it was loaded from is a reference parameter. + arrays_from_load: HashMap, inner_nested_arrays: HashMap, } @@ -64,7 +66,7 @@ impl<'f> Context<'f> { is_brillig_runtime, array_to_last_use: HashMap::default(), instructions_that_can_be_made_mutable: HashSet::default(), - arrays_from_load: HashSet::default(), + arrays_from_load: HashMap::default(), inner_nested_arrays: HashMap::default(), } } @@ -113,9 +115,13 @@ impl<'f> Context<'f> { array_in_terminator = true; } }); - if (!self.arrays_from_load.contains(&array) || is_return_block) - && !array_in_terminator - { + if let Some(is_from_param) = self.arrays_from_load.get(&array) { + // If the array was loaded from a reference parameter, we cannot + // safely mark that array mutable as it may be shared by another value. + if !is_from_param && is_return_block { + self.instructions_that_can_be_made_mutable.insert(*instruction_id); + } + } else if !array_in_terminator { self.instructions_that_can_be_made_mutable.insert(*instruction_id); } } @@ -133,10 +139,12 @@ impl<'f> Context<'f> { } } } - Instruction::Load { .. } => { + Instruction::Load { address } => { let result = self.dfg.instruction_results(*instruction_id)[0]; if matches!(self.dfg.type_of_value(result), Array { .. } | Slice { .. }) { - self.arrays_from_load.insert(result); + let is_reference_param = + self.dfg.block_parameters(block_id).contains(address); + self.arrays_from_load.insert(result, is_reference_param); } } _ => (), diff --git a/noir/noir-repo/compiler/noirc_frontend/src/ast/statement.rs b/noir/noir-repo/compiler/noirc_frontend/src/ast/statement.rs index 038a7b76bacf..a25b87b73950 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/ast/statement.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/ast/statement.rs @@ -327,6 +327,7 @@ pub enum PathKind { pub struct UseTree { pub prefix: Path, pub kind: UseTreeKind, + pub span: Span, } impl Display for UseTree { diff --git a/noir/noir-repo/compiler/noirc_frontend/src/ast/visitor.rs b/noir/noir-repo/compiler/noirc_frontend/src/ast/visitor.rs index 632c56561374..16ee0fc4c026 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/ast/visitor.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/ast/visitor.rs @@ -22,8 +22,8 @@ use crate::{ use super::{ ForBounds, FunctionReturnType, GenericTypeArgs, IntegerBitSize, ItemVisibility, Pattern, - Signedness, TraitImplItemKind, TypePath, UnresolvedGenerics, UnresolvedTraitConstraint, - UnresolvedType, UnresolvedTypeData, UnresolvedTypeExpression, + Signedness, TraitBound, TraitImplItemKind, TypePath, UnresolvedGenerics, + UnresolvedTraitConstraint, UnresolvedType, UnresolvedTypeData, UnresolvedTypeExpression, }; #[derive(Debug, Copy, Clone, PartialEq, Eq)] @@ -438,6 +438,14 @@ pub trait Visitor { true } + fn visit_trait_bound(&mut self, _: &TraitBound) -> bool { + true + } + + fn visit_unresolved_trait_constraint(&mut self, _: &UnresolvedTraitConstraint) -> bool { + true + } + fn visit_pattern(&mut self, _: &Pattern) -> bool { true } @@ -555,6 +563,12 @@ impl NoirFunction { param.typ.accept(visitor); } + self.def.return_type.accept(visitor); + + for constraint in &self.def.where_clause { + constraint.accept(visitor); + } + self.def.body.accept(None, visitor); } } @@ -645,6 +659,14 @@ impl NoirTrait { attribute.accept(AttributeTarget::Trait, visitor); } + for bound in &self.bounds { + bound.accept(visitor); + } + + for constraint in &self.where_clause { + constraint.accept(visitor); + } + for item in &self.items { item.item.accept(visitor); } @@ -686,7 +708,7 @@ impl TraitItem { return_type.accept(visitor); for unresolved_trait_constraint in where_clause { - unresolved_trait_constraint.typ.accept(visitor); + unresolved_trait_constraint.accept(visitor); } if let Some(body) = body { @@ -1346,6 +1368,32 @@ impl FunctionReturnType { } } +impl TraitBound { + pub fn accept(&self, visitor: &mut impl Visitor) { + if visitor.visit_trait_bound(self) { + self.accept_children(visitor); + } + } + + pub fn accept_children(&self, visitor: &mut impl Visitor) { + self.trait_path.accept(visitor); + self.trait_generics.accept(visitor); + } +} + +impl UnresolvedTraitConstraint { + pub fn accept(&self, visitor: &mut impl Visitor) { + if visitor.visit_unresolved_trait_constraint(self) { + self.accept_children(visitor); + } + } + + pub fn accept_children(&self, visitor: &mut impl Visitor) { + self.typ.accept(visitor); + self.trait_bound.accept(visitor); + } +} + impl Pattern { pub fn accept(&self, visitor: &mut impl Visitor) { if visitor.visit_pattern(self) { diff --git a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/scope.rs b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/scope.rs index b83a1494ab92..d38b7a501759 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/scope.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/scope.rs @@ -269,12 +269,26 @@ impl<'context> Elaborator<'context> { } } - match self.lookup(path) { - Ok(struct_id) => { + let span = path.span; + match self.resolve_path_or_error(path) { + Ok(ModuleDefId::TypeId(struct_id)) => { let struct_type = self.get_struct(struct_id); let generics = struct_type.borrow().instantiate(self.interner); Some(Type::Struct(struct_type, generics)) } + Ok(ModuleDefId::TypeAliasId(alias_id)) => { + let alias = self.interner.get_type_alias(alias_id); + let alias = alias.borrow(); + Some(alias.instantiate(self.interner)) + } + Ok(other) => { + self.push_err(ResolverError::Expected { + expected: StructId::description(), + got: other.as_str().to_owned(), + span, + }); + None + } Err(error) => { self.push_err(error); None diff --git a/noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/interpreter.rs b/noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/interpreter.rs index ffb759e74a2f..7205242ead94 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/interpreter.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/interpreter.rs @@ -554,8 +554,8 @@ impl<'local, 'interner> Interpreter<'local, 'interner> { match &definition.kind { DefinitionKind::Function(function_id) => { let typ = self.elaborator.interner.id_type(id).follow_bindings(); - let bindings = - Rc::new(self.elaborator.interner.get_instantiation_bindings(id).clone()); + let bindings = self.elaborator.interner.try_get_instantiation_bindings(id); + let bindings = Rc::new(bindings.map_or(TypeBindings::default(), Clone::clone)); Ok(Value::Function(*function_id, typ, bindings)) } DefinitionKind::Local(_) => self.lookup(&ident), diff --git a/noir/noir-repo/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/noir/noir-repo/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index 16fd43ba2a28..658812be3240 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -303,7 +303,12 @@ impl DefCollector { def_map.extern_prelude.insert(dep.as_name(), module_id); let location = dep_def_map[dep_def_root].location; - let attributes = ModuleAttributes { name: dep.as_name(), location, parent: None }; + let attributes = ModuleAttributes { + name: dep.as_name(), + location, + parent: None, + visibility: ItemVisibility::Public, + }; context.def_interner.add_module_attributes(module_id, attributes); } diff --git a/noir/noir-repo/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/noir/noir-repo/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index b9ce8f361f7f..825a8414fe0f 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -885,6 +885,7 @@ fn push_child_module( name: mod_name.0.contents.clone(), location: mod_location, parent: Some(parent), + visibility, }, ); diff --git a/noir/noir-repo/compiler/noirc_frontend/src/hir_def/types.rs b/noir/noir-repo/compiler/noirc_frontend/src/hir_def/types.rs index fa2a455c06dc..77030b0e0487 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/hir_def/types.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/hir_def/types.rs @@ -590,6 +590,11 @@ impl TypeAlias { self.typ.substitute(&substitutions) } + + pub fn instantiate(&self, interner: &NodeInterner) -> Type { + let args = vecmap(&self.generics, |_| interner.next_type_variable()); + self.get_type(&args) + } } /// A shared, mutable reference to some T. diff --git a/noir/noir-repo/compiler/noirc_frontend/src/node_interner.rs b/noir/noir-repo/compiler/noirc_frontend/src/node_interner.rs index 5fe88ed4e23d..68510f2ffbd9 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/node_interner.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/node_interner.rs @@ -56,6 +56,7 @@ pub struct ModuleAttributes { pub name: String, pub location: Location, pub parent: Option, + pub visibility: ItemVisibility, } type StructAttributes = Vec; diff --git a/noir/noir-repo/compiler/noirc_frontend/src/parser/parser/use_tree.rs b/noir/noir-repo/compiler/noirc_frontend/src/parser/parser/use_tree.rs index bc95c04d46b2..08834383f99c 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/parser/parser/use_tree.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/parser/parser/use_tree.rs @@ -53,13 +53,17 @@ impl<'a> Parser<'a> { Self::parse_use_tree_in_list, ); - UseTree { prefix, kind: UseTreeKind::List(use_trees) } + UseTree { + prefix, + kind: UseTreeKind::List(use_trees), + span: self.span_since(start_span), + } } else { self.expected_token(Token::LeftBrace); - self.parse_path_use_tree_end(prefix, nested) + self.parse_path_use_tree_end(prefix, nested, start_span) } } else { - self.parse_path_use_tree_end(prefix, nested) + self.parse_path_use_tree_end(prefix, nested, start_span) } } @@ -71,6 +75,7 @@ impl<'a> Parser<'a> { return Some(UseTree { prefix: Path { segments: Vec::new(), kind: PathKind::Plain, span: start_span }, kind: UseTreeKind::Path(Ident::new("self".to_string(), start_span), None), + span: start_span, }); } @@ -89,25 +94,46 @@ impl<'a> Parser<'a> { } } - pub(super) fn parse_path_use_tree_end(&mut self, mut prefix: Path, nested: bool) -> UseTree { + pub(super) fn parse_path_use_tree_end( + &mut self, + mut prefix: Path, + nested: bool, + start_span: Span, + ) -> UseTree { if prefix.segments.is_empty() { if nested { self.expected_identifier(); } else { self.expected_label(ParsingRuleLabel::UseSegment); } - UseTree { prefix, kind: UseTreeKind::Path(Ident::default(), None) } + UseTree { + prefix, + kind: UseTreeKind::Path(Ident::default(), None), + span: self.span_since(start_span), + } } else { let ident = prefix.segments.pop().unwrap().ident; if self.eat_keyword(Keyword::As) { if let Some(alias) = self.eat_ident() { - UseTree { prefix, kind: UseTreeKind::Path(ident, Some(alias)) } + UseTree { + prefix, + kind: UseTreeKind::Path(ident, Some(alias)), + span: self.span_since(start_span), + } } else { self.expected_identifier(); - UseTree { prefix, kind: UseTreeKind::Path(ident, None) } + UseTree { + prefix, + kind: UseTreeKind::Path(ident, None), + span: self.span_since(start_span), + } } } else { - UseTree { prefix, kind: UseTreeKind::Path(ident, None) } + UseTree { + prefix, + kind: UseTreeKind::Path(ident, None), + span: self.span_since(start_span), + } } } } diff --git a/noir/noir-repo/compiler/noirc_frontend/src/tests.rs b/noir/noir-repo/compiler/noirc_frontend/src/tests.rs index e06881127fdd..56c4b5e9a120 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/tests.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/tests.rs @@ -3546,3 +3546,19 @@ fn uses_self_in_import() { "#; assert_no_errors(src); } + +#[test] +fn alias_in_let_pattern() { + let src = r#" + struct Foo { x: T } + + type Bar = Foo; + + fn main() { + let Bar { x } = Foo { x: [0] }; + // This is just to show the compiler knows this is an array. + let _: [Field; 1] = x; + } + "#; + assert_no_errors(src); +} diff --git a/noir/noir-repo/compiler/noirc_frontend/src/tests/aliases.rs b/noir/noir-repo/compiler/noirc_frontend/src/tests/aliases.rs index 2ca460ca6358..5239abcb3663 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/tests/aliases.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/tests/aliases.rs @@ -31,22 +31,3 @@ fn allows_usage_of_type_alias_as_return_type() { "#; assert_no_errors(src); } - -// This is a regression test for https://github.com/noir-lang/noir/issues/6347 -#[test] -#[should_panic = r#"ResolverError(Expected { span: Span(Span { start: ByteIndex(95), end: ByteIndex(98) }), expected: "type", got: "type alias" }"#] -fn allows_destructuring_a_type_alias_of_a_struct() { - let src = r#" - struct Foo { - inner: Field - } - - type Bar = Foo; - - fn main() { - let Bar { inner } = Foo { inner: 42 }; - assert_eq(inner, 42); - } - "#; - assert_no_errors(src); -} diff --git a/noir/noir-repo/compiler/noirc_frontend/src/tests/metaprogramming.rs b/noir/noir-repo/compiler/noirc_frontend/src/tests/metaprogramming.rs index a5241cc26ed4..82c40203244f 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/tests/metaprogramming.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/tests/metaprogramming.rs @@ -1,6 +1,15 @@ -use crate::hir::{ - def_collector::dc_crate::CompilationError, resolution::errors::ResolverError, - type_check::TypeCheckError, +use noirc_errors::Spanned; + +use crate::{ + ast::Ident, + hir::{ + def_collector::{ + dc_crate::CompilationError, + errors::{DefCollectorErrorKind, DuplicateType}, + }, + resolution::errors::ResolverError, + type_check::TypeCheckError, + }, }; use super::{assert_no_errors, get_program_errors}; @@ -96,3 +105,39 @@ fn allows_references_to_structs_generated_by_macros() { assert_no_errors(src); } + +#[test] +fn errors_if_macros_inject_functions_with_name_collisions() { + let src = r#" + comptime fn make_colliding_functions(_s: StructDefinition) -> Quoted { + quote { + fn foo() {} + } + } + + #[make_colliding_functions] + struct Foo {} + + #[make_colliding_functions] + struct Bar {} + + fn main() { + let _ = Foo {}; + let _ = Bar {}; + foo(); + } + "#; + + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + assert!(matches!( + &errors[0].0, + CompilationError::DefinitionError( + DefCollectorErrorKind::Duplicate { + typ: DuplicateType::Function, + first_def: Ident(Spanned { contents, .. }), + .. + }, + ) if contents == "foo" + )); +} diff --git a/noir/noir-repo/compiler/noirc_frontend/src/tests/traits.rs b/noir/noir-repo/compiler/noirc_frontend/src/tests/traits.rs index 88138ecde4d9..dd6430a94cc5 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/tests/traits.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/tests/traits.rs @@ -261,3 +261,24 @@ fn errors_if_impl_trait_constraint_is_not_satisfied() { assert_eq!(typ, "SomeGreeter"); assert_eq!(impl_trait, "Foo"); } + +#[test] +fn removes_assumed_parent_traits_after_function_ends() { + let src = r#" + trait Foo {} + trait Bar: Foo {} + + pub fn foo() + where + T: Bar, + {} + + pub fn bar() + where + T: Foo, + {} + + fn main() {} + "#; + assert_no_errors(src); +} diff --git a/noir/noir-repo/docs/docs/noir/concepts/comptime.md b/noir/noir-repo/docs/docs/noir/concepts/comptime.md index cea3a896c17f..37457d47b464 100644 --- a/noir/noir-repo/docs/docs/noir/concepts/comptime.md +++ b/noir/noir-repo/docs/docs/noir/concepts/comptime.md @@ -5,7 +5,7 @@ keywords: [Noir, comptime, compile-time, metaprogramming, macros, quote, unquote sidebar_position: 15 --- -# Overview +## Overview Metaprogramming in Noir is comprised of three parts: 1. `comptime` code @@ -21,7 +21,7 @@ for greater analysis and modification of programs. --- -# Comptime +## Comptime `comptime` is a new keyword in Noir which marks an item as executing or existing at compile-time. It can be used in several ways: @@ -32,11 +32,11 @@ for greater analysis and modification of programs. - `comptime let` to define a variable whose value is evaluated at compile-time. - `comptime for` to run a for loop at compile-time. Syntax sugar for `comptime { for .. }`. -## Scoping +### Scoping Note that while in a `comptime` context, any runtime variables _local to the current function_ are never visible. -## Evaluating +### Evaluating Evaluation rules of `comptime` follows the normal unconstrained evaluation rules for other Noir code. There are a few things to note though: @@ -62,7 +62,7 @@ For example, using globals to generate unique ids should be fine but relying on function itself was called at compile-time previously, it will already be resolved and cannot be modified. To prevent accidentally calling functions you wish to modify at compile-time, it may be helpful to sort your `comptime` annotation functions into a different crate along with any dependencies they require. -## Lowering +### Lowering When a `comptime` value is used in runtime code it must be lowered into a runtime value. This means replacing the expression with the literal that it evaluated to. For example, the code: @@ -102,7 +102,7 @@ comptime fn get_type() -> Type { ... } --- -# (Quasi) Quote +## (Quasi) Quote Macros in Noir are `comptime` functions which return code as a value which is inserted into the call site when it is lowered there. A code value in this case is of type `Quoted` and can be created by a `quote { ... }` expression. @@ -121,7 +121,7 @@ Calling such a function at compile-time without `!` will just return the `Quoted For those familiar with quoting from other languages (primarily lisps), Noir's `quote` is actually a _quasiquote_. This means we can escape the quoting by using the unquote operator to splice values in the middle of quoted code. -# Unquote +## Unquote The unquote operator `$` is usable within a `quote` expression. It takes a variable as an argument, evaluates the variable, and splices the resulting value into the quoted token stream at that point. For example, @@ -160,7 +160,7 @@ comptime { --- -# Annotations +## Annotations Annotations provide a way to run a `comptime` function on an item in the program. When you use an annotation, the function with the same name will be called with that item as an argument: @@ -188,7 +188,7 @@ For example, this is the mechanism used to insert additional trait implementatio #include_code derive-field-count-example noir_stdlib/src/meta/mod.nr rust -## Calling annotations with additional arguments +### Calling annotations with additional arguments Arguments may optionally be given to annotations. When this is done, these additional arguments are passed to the annotation function after the item argument. @@ -201,13 +201,13 @@ We can also take any number of arguments by adding the `varargs` annotation: --- -# Comptime API +## Comptime API Although `comptime`, `quote`, and unquoting provide a flexible base for writing macros, Noir's true metaprogramming ability comes from being able to interact with the compiler through a compile-time API. This API can be accessed through built-in functions in `std::meta` as well as on methods of several `comptime` types. -The following is an incomplete list of some `comptime` types along with some useful methods on them. +The following is an incomplete list of some `comptime` types along with some useful methods on them. You can see more in the standard library [Metaprogramming section](../standard_library/meta). - `Quoted`: A token stream - `Type`: The type of a Noir type @@ -238,7 +238,7 @@ The following is an incomplete list of some `comptime` types along with some use There are many more functions available by exploring the `std::meta` module and its submodules. Using these methods is the key to writing powerful metaprogramming libraries. -## `#[use_callers_scope]` +### `#[use_callers_scope]` Since certain functions such as `Quoted::as_type`, `Expression::as_type`, or `Quoted::as_trait_constraint` will attempt to resolve their contents in a particular scope - it can be useful to change the scope they resolve in. By default @@ -251,7 +251,7 @@ your attribute function and a helper function it calls use it, then they can bot --- -# Example: Derive +## Example: Derive Using all of the above, we can write a `derive` macro that behaves similarly to Rust's but is not built into the language. From the user's perspective it will look like this: diff --git a/noir/noir-repo/noir_stdlib/src/hash/mod.nr b/noir/noir-repo/noir_stdlib/src/hash/mod.nr index 15112757312a..c5e6da9d76f9 100644 --- a/noir/noir-repo/noir_stdlib/src/hash/mod.nr +++ b/noir/noir-repo/noir_stdlib/src/hash/mod.nr @@ -75,6 +75,7 @@ pub fn pedersen_hash_with_separator(input: [Field; N], separator: u3 } #[field(bn254)] +#[inline_always] pub fn derive_generators( domain_separator_bytes: [u8; M], starting_index: u32, diff --git a/noir/noir-repo/tooling/lsp/src/lib.rs b/noir/noir-repo/tooling/lsp/src/lib.rs index 39e14a74007f..a85b9d043b95 100644 --- a/noir/noir-repo/tooling/lsp/src/lib.rs +++ b/noir/noir-repo/tooling/lsp/src/lib.rs @@ -67,8 +67,10 @@ mod modules; mod notifications; mod requests; mod solver; +mod tests; mod trait_impl_method_stub_generator; mod types; +mod use_segment_positions; mod utils; mod visibility; diff --git a/noir/noir-repo/tooling/lsp/src/modules.rs b/noir/noir-repo/tooling/lsp/src/modules.rs index 9f9a826d6cac..cadf71b2eec0 100644 --- a/noir/noir-repo/tooling/lsp/src/modules.rs +++ b/noir/noir-repo/tooling/lsp/src/modules.rs @@ -1,14 +1,9 @@ -use std::collections::BTreeMap; - use noirc_frontend::{ - ast::ItemVisibility, graph::{CrateId, Dependency}, - hir::def_map::{CrateDefMap, ModuleDefId, ModuleId}, + hir::def_map::{ModuleDefId, ModuleId}, node_interner::{NodeInterner, ReferenceId}, }; -use crate::visibility::is_visible; - pub(crate) fn get_parent_module( interner: &NodeInterner, module_def_id: ModuleDefId, @@ -33,18 +28,12 @@ pub(crate) fn module_def_id_to_reference_id(module_def_id: ModuleDefId) -> Refer /// - Otherwise, that item's parent module's path is returned pub(crate) fn relative_module_full_path( module_def_id: ModuleDefId, - visibility: ItemVisibility, current_module_id: ModuleId, current_module_parent_id: Option, interner: &NodeInterner, - def_maps: &BTreeMap, ) -> Option { let full_path; if let ModuleDefId::ModuleId(module_id) = module_def_id { - if !is_visible(module_id, current_module_id, visibility, def_maps) { - return None; - } - full_path = relative_module_id_path( module_id, ¤t_module_id, @@ -56,10 +45,6 @@ pub(crate) fn relative_module_full_path( return None; }; - if !is_visible(parent_module, current_module_id, visibility, def_maps) { - return None; - } - full_path = relative_module_id_path( parent_module, ¤t_module_id, diff --git a/noir/noir-repo/tooling/lsp/src/requests/code_action.rs b/noir/noir-repo/tooling/lsp/src/requests/code_action.rs index 9299dc763686..f3e9130e17db 100644 --- a/noir/noir-repo/tooling/lsp/src/requests/code_action.rs +++ b/noir/noir-repo/tooling/lsp/src/requests/code_action.rs @@ -22,7 +22,7 @@ use noirc_frontend::{ ParsedModule, }; -use crate::{utils, LspState}; +use crate::{use_segment_positions::UseSegmentPositions, utils, LspState}; use super::{process_request, to_lsp_location}; @@ -82,6 +82,7 @@ struct CodeActionFinder<'a> { nesting: usize, /// The line where an auto_import must be inserted auto_import_line: usize, + use_segment_positions: UseSegmentPositions, /// Text edits for the "Remove all unused imports" code action unused_imports_text_edits: Vec, code_actions: Vec, @@ -121,6 +122,7 @@ impl<'a> CodeActionFinder<'a> { interner, nesting: 0, auto_import_line: 0, + use_segment_positions: UseSegmentPositions::default(), unused_imports_text_edits: vec![], code_actions: vec![], } @@ -184,10 +186,11 @@ impl<'a> CodeActionFinder<'a> { impl<'a> Visitor for CodeActionFinder<'a> { fn visit_item(&mut self, item: &Item) -> bool { - if let ItemKind::Import(..) = &item.kind { + if let ItemKind::Import(use_tree, _) = &item.kind { if let Some(lsp_location) = to_lsp_location(self.files, self.file, item.span) { self.auto_import_line = (lsp_location.range.end.line + 1) as usize; } + self.use_segment_positions.add(use_tree); } self.includes_span(item.span) diff --git a/noir/noir-repo/tooling/lsp/src/requests/code_action/import_or_qualify.rs b/noir/noir-repo/tooling/lsp/src/requests/code_action/import_or_qualify.rs index bf1fe906be34..ffc83b05a5ba 100644 --- a/noir/noir-repo/tooling/lsp/src/requests/code_action/import_or_qualify.rs +++ b/noir/noir-repo/tooling/lsp/src/requests/code_action/import_or_qualify.rs @@ -1,4 +1,4 @@ -use lsp_types::{Position, Range, TextEdit}; +use lsp_types::TextEdit; use noirc_errors::Location; use noirc_frontend::{ ast::{Ident, Path}, @@ -8,6 +8,10 @@ use noirc_frontend::{ use crate::{ byte_span_to_range, modules::{relative_module_full_path, relative_module_id_path}, + use_segment_positions::{ + use_completion_item_additional_text_edits, UseCompletionItemAdditionTextEditsRequest, + }, + visibility::module_def_id_is_visible, }; use super::CodeActionFinder; @@ -38,6 +42,17 @@ impl<'a> CodeActionFinder<'a> { } for (module_def_id, visibility, defining_module) in entries { + if !module_def_id_is_visible( + *module_def_id, + self.module_id, + *visibility, + *defining_module, + self.interner, + self.def_maps, + ) { + continue; + } + let module_full_path = if let Some(defining_module) = defining_module { relative_module_id_path( *defining_module, @@ -48,11 +63,9 @@ impl<'a> CodeActionFinder<'a> { } else { let Some(module_full_path) = relative_module_full_path( *module_def_id, - *visibility, self.module_id, current_module_parent_id, self.interner, - self.def_maps, ) else { continue; }; @@ -82,25 +95,21 @@ impl<'a> CodeActionFinder<'a> { } fn push_import_code_action(&mut self, full_path: &str) { - let line = self.auto_import_line as u32; - let character = (self.nesting * 4) as u32; - let indent = " ".repeat(self.nesting * 4); - let mut newlines = "\n"; - - // If the line we are inserting into is not an empty line, insert an extra line to make some room - if let Some(line_text) = self.lines.get(line as usize) { - if !line_text.trim().is_empty() { - newlines = "\n\n"; - } - } - let title = format!("Import {}", full_path); - let text_edit = TextEdit { - range: Range { start: Position { line, character }, end: Position { line, character } }, - new_text: format!("use {};{}{}", full_path, newlines, indent), - }; - let code_action = self.new_quick_fix(title, text_edit); + let text_edits = use_completion_item_additional_text_edits( + UseCompletionItemAdditionTextEditsRequest { + 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); } @@ -133,7 +142,7 @@ mod tests { let src = r#" mod foo { - mod bar { + pub mod bar { pub struct SomeTypeInBar {} } } @@ -143,7 +152,7 @@ mod tests { let expected = r#" mod foo { - mod bar { + pub mod bar { pub struct SomeTypeInBar {} } } @@ -159,7 +168,7 @@ mod tests { let title = "Import foo::bar::SomeTypeInBar"; let src = r#"mod foo { - mod bar { + pub mod bar { pub struct SomeTypeInBar {} } } @@ -169,7 +178,7 @@ fn foo(x: SomeType>|||<", ""), &text_edits[0]); + let result = apply_text_edits(&src.replace(">|<", ""), text_edits); if result != expected { println!("Expected:\n```\n{}\n```\n\nGot:\n```\n{}\n```", expected, result); assert_eq!(result, expected); } } - -fn apply_text_edit(src: &str, text_edit: &TextEdit) -> String { - let mut lines: Vec<_> = src.lines().collect(); - assert_eq!(text_edit.range.start.line, text_edit.range.end.line); - - let mut line = lines[text_edit.range.start.line as usize].to_string(); - line.replace_range( - text_edit.range.start.character as usize..text_edit.range.end.character as usize, - &text_edit.new_text, - ); - lines[text_edit.range.start.line as usize] = &line; - lines.join("\n") -} diff --git a/noir/noir-repo/tooling/lsp/src/requests/completion.rs b/noir/noir-repo/tooling/lsp/src/requests/completion.rs index 658033fc5260..2dd42c740cde 100644 --- a/noir/noir-repo/tooling/lsp/src/requests/completion.rs +++ b/noir/noir-repo/tooling/lsp/src/requests/completion.rs @@ -19,9 +19,9 @@ use noirc_frontend::{ Expression, ExpressionKind, ForLoopStatement, GenericTypeArgs, Ident, IfExpression, IntegerBitSize, ItemVisibility, LValue, Lambda, LetStatement, MemberAccessExpression, MethodCallExpression, NoirFunction, NoirStruct, NoirTraitImpl, Path, PathKind, Pattern, - Signedness, Statement, TraitImplItemKind, TypeImpl, TypePath, UnresolvedGeneric, - UnresolvedGenerics, UnresolvedType, UnresolvedTypeData, UnresolvedTypeExpression, UseTree, - UseTreeKind, Visitor, + Signedness, Statement, TraitBound, TraitImplItemKind, TypeImpl, TypePath, + UnresolvedGeneric, UnresolvedGenerics, UnresolvedType, UnresolvedTypeData, + UnresolvedTypeExpression, UseTree, UseTreeKind, Visitor, }, graph::{CrateId, Dependency}, hir::{ @@ -38,7 +38,8 @@ use sort_text::underscore_sort_text; use crate::{ requests::to_lsp_location, trait_impl_method_stub_generator::TraitImplMethodStubGenerator, - utils, visibility::is_visible, LspState, + use_segment_positions::UseSegmentPositions, utils, visibility::item_in_module_is_visible, + LspState, }; use super::process_request; @@ -115,6 +116,7 @@ struct NodeFinder<'a> { nesting: usize, /// The line where an auto_import must be inserted auto_import_line: usize, + use_segment_positions: UseSegmentPositions, self_type: Option, in_comptime: bool, } @@ -159,6 +161,7 @@ impl<'a> NodeFinder<'a> { suggested_module_def_ids: HashSet::new(), nesting: 0, auto_import_line: 0, + use_segment_positions: UseSegmentPositions::default(), self_type: None, in_comptime: false, } @@ -381,7 +384,7 @@ impl<'a> NodeFinder<'a> { self.builtin_types_completion(&prefix); self.type_parameters_completion(&prefix); } - RequestedItems::OnlyAttributeFunctions(..) => (), + RequestedItems::OnlyTraits | RequestedItems::OnlyAttributeFunctions(..) => (), } self.complete_auto_imports(&prefix, requested_items, function_completion_kind); } @@ -795,7 +798,12 @@ impl<'a> NodeFinder<'a> { if name_matches(name, prefix) { let per_ns = module_data.find_name(ident); if let Some((module_def_id, visibility, _)) = per_ns.types { - if is_visible(module_id, self.module_id, visibility, self.def_maps) { + if item_in_module_is_visible( + module_id, + self.module_id, + visibility, + self.def_maps, + ) { let completion_items = self.module_def_id_completion_items( module_def_id, name.clone(), @@ -811,7 +819,12 @@ impl<'a> NodeFinder<'a> { } if let Some((module_def_id, visibility, _)) = per_ns.values { - if is_visible(module_id, self.module_id, visibility, self.def_maps) { + if item_in_module_is_visible( + module_id, + self.module_id, + visibility, + self.def_maps, + ) { let completion_items = self.module_def_id_completion_items( module_def_id, name.clone(), @@ -1035,6 +1048,8 @@ impl<'a> NodeFinder<'a> { } } + /// Determine where each segment in a `use` statement is located. + fn includes_span(&self, span: Span) -> bool { span.start() as usize <= self.byte_index && self.byte_index <= span.end() as usize } @@ -1042,10 +1057,11 @@ impl<'a> NodeFinder<'a> { impl<'a> Visitor for NodeFinder<'a> { fn visit_item(&mut self, item: &Item) -> bool { - if let ItemKind::Import(..) = &item.kind { + if let ItemKind::Import(use_tree, _) = &item.kind { if let Some(lsp_location) = to_lsp_location(self.files, self.file, item.span) { self.auto_import_line = (lsp_location.range.end.line + 1) as usize; } + self.use_segment_positions.add(use_tree); } self.includes_span(item.span) @@ -1108,6 +1124,10 @@ impl<'a> Visitor for NodeFinder<'a> { noir_function.def.return_type.accept(self); + for constraint in &noir_function.def.where_clause { + constraint.accept(self); + } + self.local_variables.clear(); for param in &noir_function.def.parameters { self.collect_local_variables(¶m.pattern); @@ -1215,7 +1235,7 @@ impl<'a> Visitor for NodeFinder<'a> { return_type.accept(self); for unresolved_trait_constraint in where_clause { - unresolved_trait_constraint.typ.accept(self); + unresolved_trait_constraint.accept(self); } if let Some(body) = body { @@ -1686,6 +1706,12 @@ impl<'a> Visitor for NodeFinder<'a> { last_was_dollar = false; } } + + fn visit_trait_bound(&mut self, trait_bound: &TraitBound) -> bool { + self.find_in_path(&trait_bound.trait_path, RequestedItems::OnlyTraits); + trait_bound.trait_generics.accept(self); + false + } } fn get_field_type(typ: &Type, name: &str) -> Option { diff --git a/noir/noir-repo/tooling/lsp/src/requests/completion/auto_import.rs b/noir/noir-repo/tooling/lsp/src/requests/completion/auto_import.rs index e2dd582f2f35..0e80b284f32e 100644 --- a/noir/noir-repo/tooling/lsp/src/requests/completion/auto_import.rs +++ b/noir/noir-repo/tooling/lsp/src/requests/completion/auto_import.rs @@ -1,7 +1,12 @@ -use lsp_types::{Position, Range, TextEdit}; use noirc_frontend::hir::def_map::ModuleDefId; -use crate::modules::{relative_module_full_path, relative_module_id_path}; +use crate::{ + modules::{relative_module_full_path, relative_module_id_path}, + use_segment_positions::{ + use_completion_item_additional_text_edits, UseCompletionItemAdditionTextEditsRequest, + }, + visibility::module_def_id_is_visible, +}; use super::{ kinds::{FunctionCompletionKind, FunctionKind, RequestedItems}, @@ -29,6 +34,17 @@ impl<'a> NodeFinder<'a> { continue; } + if !module_def_id_is_visible( + *module_def_id, + self.module_id, + *visibility, + *defining_module, + self.interner, + self.def_maps, + ) { + continue; + } + let completion_items = self.module_def_id_completion_items( *module_def_id, name.clone(), @@ -54,11 +70,9 @@ impl<'a> NodeFinder<'a> { } else { let Some(module_full_path) = relative_module_full_path( *module_def_id, - *visibility, self.module_id, current_module_parent_id, self.interner, - self.def_maps, ) else { continue; }; @@ -76,27 +90,18 @@ impl<'a> NodeFinder<'a> { let mut label_details = completion_item.label_details.unwrap(); label_details.detail = Some(format!("(use {})", full_path)); completion_item.label_details = Some(label_details); - - let line = self.auto_import_line as u32; - let character = (self.nesting * 4) as u32; - let indent = " ".repeat(self.nesting * 4); - let mut newlines = "\n"; - - // If the line we are inserting into is not an empty line, insert an extra line to make some room - if let Some(line_text) = self.lines.get(line as usize) { - if !line_text.trim().is_empty() { - newlines = "\n\n"; - } - } - - completion_item.additional_text_edits = Some(vec![TextEdit { - range: Range { - start: Position { line, character }, - end: Position { line, character }, - }, - new_text: format!("use {};{}{}", full_path, newlines, indent), - }]); - + completion_item.additional_text_edits = + Some(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, + )); completion_item.sort_text = Some(auto_import_sort_text()); self.completion_items.push(completion_item); diff --git a/noir/noir-repo/tooling/lsp/src/requests/completion/completion_items.rs b/noir/noir-repo/tooling/lsp/src/requests/completion/completion_items.rs index f281f5e3abfe..49e4474738ef 100644 --- a/noir/noir-repo/tooling/lsp/src/requests/completion/completion_items.rs +++ b/noir/noir-repo/tooling/lsp/src/requests/completion/completion_items.rs @@ -35,6 +35,14 @@ impl<'a> NodeFinder<'a> { | ModuleDefId::TypeAliasId(_) | ModuleDefId::TraitId(_) => (), }, + RequestedItems::OnlyTraits => match module_def_id { + ModuleDefId::FunctionId(_) | ModuleDefId::GlobalId(_) | ModuleDefId::TypeId(_) => { + return Vec::new() + } + ModuleDefId::ModuleId(_) + | ModuleDefId::TypeAliasId(_) + | ModuleDefId::TraitId(_) => (), + }, RequestedItems::OnlyAttributeFunctions(..) => { if !matches!(module_def_id, ModuleDefId::FunctionId(..)) { return Vec::new(); diff --git a/noir/noir-repo/tooling/lsp/src/requests/completion/kinds.rs b/noir/noir-repo/tooling/lsp/src/requests/completion/kinds.rs index 6fa74ffdb1a2..ba6faada6f4f 100644 --- a/noir/noir-repo/tooling/lsp/src/requests/completion/kinds.rs +++ b/noir/noir-repo/tooling/lsp/src/requests/completion/kinds.rs @@ -25,8 +25,10 @@ pub(super) enum FunctionKind<'a> { pub(super) enum RequestedItems { // Suggest any items (types, functions, etc.). AnyItems, - // Only suggest types. + // Only suggest types (and modules, because they can contain types). OnlyTypes, + // Only suggest traits (and modules, because they can contain traits). + OnlyTraits, // Only attribute functions OnlyAttributeFunctions(AttributeTarget), } diff --git a/noir/noir-repo/tooling/lsp/src/requests/completion/tests.rs b/noir/noir-repo/tooling/lsp/src/requests/completion/tests.rs index 668255eb34d9..745dacfc1528 100644 --- a/noir/noir-repo/tooling/lsp/src/requests/completion/tests.rs +++ b/noir/noir-repo/tooling/lsp/src/requests/completion/tests.rs @@ -15,12 +15,13 @@ mod completion_tests { on_completion_request, }, test_utils, + tests::apply_text_edits, }; use lsp_types::{ CompletionItem, CompletionItemKind, CompletionItemLabelDetails, CompletionParams, - CompletionResponse, DidOpenTextDocumentParams, PartialResultParams, Position, Range, - TextDocumentIdentifier, TextDocumentItem, TextDocumentPositionParams, TextEdit, + CompletionResponse, DidOpenTextDocumentParams, PartialResultParams, Position, + TextDocumentIdentifier, TextDocumentItem, TextDocumentPositionParams, WorkDoneProgressParams, }; use tokio::test; @@ -1392,29 +1393,50 @@ mod completion_tests { #[test] async fn test_auto_imports() { - let src = r#" - mod foo { - mod bar { - pub fn hello_world() {} + let src = r#"mod foo { + pub mod bar { + pub fn hello_world() {} - struct Foo { - } + struct Foo { + } - impl Foo { - // This is here to make sure it's not offered for completion - fn hello_world() {} - } - } - } + impl Foo { + // This is here to make sure it's not offered for completion + fn hello_world() {} + } + } +} - fn main() { - hel>|< - } +fn main() { + hel>|< +} "#; - let items = get_completions(src).await; + + let expected = r#"use foo::bar::hello_world; + +mod foo { + pub mod bar { + pub fn hello_world() {} + + struct Foo { + } + + impl Foo { + // This is here to make sure it's not offered for completion + fn hello_world() {} + } + } +} + +fn main() { + hel +} + "#; + + let mut items = get_completions(src).await; assert_eq!(items.len(), 1); - let item = &items[0]; + let item = items.remove(0); assert_eq!(item.label, "hello_world()"); assert_eq!( item.label_details, @@ -1424,38 +1446,43 @@ mod completion_tests { }) ); - assert_eq!( - item.additional_text_edits, - Some(vec![TextEdit { - range: Range { - start: Position { line: 0, character: 0 }, - end: Position { line: 0, character: 0 }, - }, - new_text: "use foo::bar::hello_world;\n".to_string(), - }]) - ); - + let changed = + apply_text_edits(&src.replace(">|<", ""), &item.additional_text_edits.unwrap()); + assert_eq!(changed, expected); assert_eq!(item.sort_text, Some(auto_import_sort_text())); } #[test] async fn test_auto_imports_when_in_nested_module_and_item_is_further_nested() { - let src = r#" - #[something] - mod foo { - mod bar { - pub fn hello_world() {} - } + let src = r#"#[something] +mod foo { + pub mod bar { + pub fn hello_world() {} + } - fn foo() { - hel>|< - } - } + fn foo() { + hel>|< + } +} "#; - let items = get_completions(src).await; + + let expected = r#"#[something] +mod foo { + use bar::hello_world; + + pub mod bar { + pub fn hello_world() {} + } + + fn foo() { + hel + } +} + "#; + let mut items = get_completions(src).await; assert_eq!(items.len(), 1); - let item = &items[0]; + let item = items.remove(0); assert_eq!(item.label, "hello_world()"); assert_eq!( item.label_details, @@ -1465,37 +1492,42 @@ mod completion_tests { }) ); - assert_eq!( - item.additional_text_edits, - Some(vec![TextEdit { - range: Range { - start: Position { line: 3, character: 4 }, - end: Position { line: 3, character: 4 }, - }, - new_text: "use bar::hello_world;\n\n ".to_string(), - }]) - ); + let changed = + apply_text_edits(&src.replace(">|<", ""), &item.additional_text_edits.unwrap()); + assert_eq!(changed, expected); } #[test] async fn test_auto_imports_when_in_nested_module_and_item_is_not_further_nested() { - let src = r#" - mod foo { - mod bar { - pub fn hello_world() {} - } + let src = r#"mod foo { + pub mod bar { + pub fn hello_world() {} + } - mod baz { - fn foo() { - hel>|< - } - } - } - "#; - let items = get_completions(src).await; + pub mod baz { + fn foo() { + hel>|< + } + } +}"#; + + let expected = r#"mod foo { + pub mod bar { + pub fn hello_world() {} + } + + pub mod baz { + use super::bar::hello_world; + + fn foo() { + hel + } + } +}"#; + let mut items = get_completions(src).await; assert_eq!(items.len(), 1); - let item = &items[0]; + let item = items.remove(0); assert_eq!(item.label, "hello_world()"); assert_eq!( item.label_details, @@ -1505,23 +1537,61 @@ mod completion_tests { }) ); - assert_eq!( - item.additional_text_edits, - Some(vec![TextEdit { - range: Range { - start: Position { line: 7, character: 8 }, - end: Position { line: 7, character: 8 }, - }, - new_text: "use super::bar::hello_world;\n\n ".to_string(), - }]) - ); + let changed = + apply_text_edits(&src.replace(">|<", ""), &item.additional_text_edits.unwrap()); + assert_eq!(changed, expected); } #[test] async fn test_auto_import_inserts_after_last_use() { + let src = r#"mod foo { + pub mod bar { + pub fn hello_world() {} + } +} + +mod baz { + fn qux() {} +} + +use baz::qux; + +fn main() { + hel>|< +}"#; + + let expected = r#"mod foo { + pub mod bar { + pub fn hello_world() {} + } +} + +mod baz { + fn qux() {} +} + +use baz::qux; +use foo::bar::hello_world; + +fn main() { + hel +}"#; + let mut items = get_completions(src).await; + assert_eq!(items.len(), 1); + + let item = items.remove(0); + + let changed = + apply_text_edits(&src.replace(">|<", ""), &item.additional_text_edits.unwrap()); + assert_eq!(changed, expected); + } + + #[test] + async fn test_does_not_auto_import_test_functions() { let src = r#" mod foo { mod bar { + #[test] pub fn hello_world() {} } } @@ -1533,28 +1603,15 @@ mod completion_tests { } "#; let items = get_completions(src).await; - assert_eq!(items.len(), 1); - - let item = &items[0]; - assert_eq!( - item.additional_text_edits, - Some(vec![TextEdit { - range: Range { - start: Position { line: 8, character: 0 }, - end: Position { line: 8, character: 0 }, - }, - new_text: "use foo::bar::hello_world;\n".to_string(), - }]) - ); + assert!(items.is_empty()); } #[test] - async fn test_does_not_auto_import_test_functions() { + async fn test_does_not_auto_import_private_functions() { let src = r#" mod foo { mod bar { - #[test] - pub fn hello_world() {} + fn hello_world() {} } } @@ -1569,16 +1626,14 @@ mod completion_tests { } #[test] - async fn test_does_not_auto_import_private_functions() { + async fn test_does_not_auto_import_public_function_in_private_module() { let src = r#" mod foo { mod bar { - fn hello_world() {} + pub fn hello_world() {} } } - use foo::bar; - fn main() { hel>|< } @@ -1588,23 +1643,60 @@ mod completion_tests { } #[test] - async fn test_auto_import_suggests_modules_too() { + async fn checks_visibility_of_module_that_exports_item_if_any() { let src = r#" mod foo { - pub mod barbaz { - fn hello_world() {} + mod bar { + pub fn hello_world() {} } + + pub use bar::hello_world; } fn main() { - barb>|< + hello_w>|< } "#; - let items = get_completions(src).await; + let mut items = get_completions(src).await; assert_eq!(items.len(), 1); - let item = &items[0]; + let item = items.remove(0); + assert_eq!(item.label, "hello_world()"); + assert_eq!(item.label_details.unwrap().detail.unwrap(), "(use foo::hello_world)"); + } + + #[test] + async fn test_auto_import_suggests_modules_too() { + let src = r#"mod foo { + pub mod barbaz { + fn hello_world() {} + } + } + + fn main() { + barb>|< + } +}"#; + + let expected = r#"use foo::barbaz; + +mod foo { + pub mod barbaz { + fn hello_world() {} + } + } + + fn main() { + barb + } +}"#; + + let mut items = get_completions(src).await; + assert_eq!(items.len(), 1); + + let item = items.remove(0); assert_eq!(item.label, "barbaz"); + assert_eq!( item.label_details, Some(CompletionItemLabelDetails { @@ -1612,6 +1704,286 @@ mod completion_tests { description: None }) ); + + let changed = + apply_text_edits(&src.replace(">|<", ""), &item.additional_text_edits.unwrap()); + assert_eq!(changed, expected); + } + + #[test] + async fn test_auto_imports_expands_existing_use_before_one_segment_not_in_list() { + let src = r#"use foo::bar::one_hello_world; + +mod foo { + pub mod bar { + pub fn one_hello_world() {} + pub fn two_hello_world() {} + } +} + +fn main() { + two_hello_>|< +}"#; + + let expected = r#"use foo::bar::{one_hello_world, two_hello_world}; + +mod foo { + pub mod bar { + pub fn one_hello_world() {} + pub fn two_hello_world() {} + } +} + +fn main() { + two_hello_ +}"#; + + let mut items = get_completions(src).await; + assert_eq!(items.len(), 1); + + let item = items.remove(0); + let changed = + apply_text_edits(&src.replace(">|<", ""), &item.additional_text_edits.unwrap()); + assert_eq!(changed, expected); + assert_eq!(item.sort_text, Some(auto_import_sort_text())); + } + + #[test] + async fn test_auto_imports_expands_existing_use_before_two_segments() { + let src = r#"use foo::bar::one_hello_world; + +mod foo { + pub mod bar { + pub fn one_hello_world() {} + } + pub fn two_hello_world() {} +} + +fn main() { + two_hello_>|< +}"#; + + let expected = r#"use foo::{bar::one_hello_world, two_hello_world}; + +mod foo { + pub mod bar { + pub fn one_hello_world() {} + } + pub fn two_hello_world() {} +} + +fn main() { + two_hello_ +}"#; + + let mut items = get_completions(src).await; + assert_eq!(items.len(), 1); + + let item = items.remove(0); + let changed = + apply_text_edits(&src.replace(">|<", ""), &item.additional_text_edits.unwrap()); + assert_eq!(changed, expected); + assert_eq!(item.sort_text, Some(auto_import_sort_text())); + } + + #[test] + async fn test_auto_imports_expands_existing_use_before_one_segment_inside_list() { + let src = r#"use foo::{bar::one_hello_world, baz}; + +mod foo { + pub mod bar { + pub fn one_hello_world() {} + pub fn two_hello_world() {} + } + pub mod baz {} +} + +fn main() { + two_hello_>|< +}"#; + + let expected = r#"use foo::{bar::{one_hello_world, two_hello_world}, baz}; + +mod foo { + pub mod bar { + pub fn one_hello_world() {} + pub fn two_hello_world() {} + } + pub mod baz {} +} + +fn main() { + two_hello_ +}"#; + + let mut items = get_completions(src).await; + assert_eq!(items.len(), 1); + + let item = items.remove(0); + let changed = + apply_text_edits(&src.replace(">|<", ""), &item.additional_text_edits.unwrap()); + assert_eq!(changed, expected); + assert_eq!(item.sort_text, Some(auto_import_sort_text())); + } + + #[test] + async fn test_auto_imports_expands_existing_use_before_one_segment_checks_parents() { + let src = r#"use foo::bar::baz; + +mod foo { + pub mod bar { + pub mod baz { + pub fn one_hello_world() {} + } + pub mod qux { + pub fn two_hello_world() {} + } + } +} + +fn main() { + two_hello_>|< +}"#; + + let expected = r#"use foo::bar::{baz, qux::two_hello_world}; + +mod foo { + pub mod bar { + pub mod baz { + pub fn one_hello_world() {} + } + pub mod qux { + pub fn two_hello_world() {} + } + } +} + +fn main() { + two_hello_ +}"#; + + let mut items = get_completions(src).await; + assert_eq!(items.len(), 1); + + let item = items.remove(0); + let changed = + apply_text_edits(&src.replace(">|<", ""), &item.additional_text_edits.unwrap()); + assert_eq!(changed, expected); + assert_eq!(item.sort_text, Some(auto_import_sort_text())); + } + + #[test] + async fn test_auto_imports_expands_existing_use_last_segment() { + let src = r#"use foo::bar; + +mod foo { + pub mod bar { + pub fn one_hello_world() {} + pub fn two_hello_world() {} + } +} + +fn main() { + two_hello_>|< +}"#; + + let expected = r#"use foo::bar::{self, two_hello_world}; + +mod foo { + pub mod bar { + pub fn one_hello_world() {} + pub fn two_hello_world() {} + } +} + +fn main() { + two_hello_ +}"#; + + let mut items = get_completions(src).await; + assert_eq!(items.len(), 1); + + let item = items.remove(0); + let changed = + apply_text_edits(&src.replace(">|<", ""), &item.additional_text_edits.unwrap()); + assert_eq!(changed, expected); + assert_eq!(item.sort_text, Some(auto_import_sort_text())); + } + + #[test] + async fn test_auto_imports_expands_existing_use_before_list() { + let src = r#"use foo::bar::{one_hello_world, three_hello_world}; + +mod foo { + pub mod bar { + pub fn one_hello_world() {} + pub fn two_hello_world() {} + pub fn three_hello_world() {} + } +} + +fn main() { + two_hello_>|< +}"#; + + let expected = r#"use foo::bar::{two_hello_world, one_hello_world, three_hello_world}; + +mod foo { + pub mod bar { + pub fn one_hello_world() {} + pub fn two_hello_world() {} + pub fn three_hello_world() {} + } +} + +fn main() { + two_hello_ +}"#; + + let mut items = get_completions(src).await; + assert_eq!(items.len(), 1); + + let item = items.remove(0); + let changed = + apply_text_edits(&src.replace(">|<", ""), &item.additional_text_edits.unwrap()); + assert_eq!(changed, expected); + assert_eq!(item.sort_text, Some(auto_import_sort_text())); + } + + #[test] + async fn test_auto_imports_expands_existing_use_before_empty_list() { + let src = r#"use foo::bar::{}; + +mod foo { + pub mod bar { + pub fn two_hello_world() {} + } +} + +fn main() { + two_hello_>|< +}"#; + + let expected = r#"use foo::bar::{two_hello_world}; + +mod foo { + pub mod bar { + pub fn two_hello_world() {} + } +} + +fn main() { + two_hello_ +}"#; + + let mut items = get_completions(src).await; + assert_eq!(items.len(), 1); + + let item = items.remove(0); + let changed = + apply_text_edits(&src.replace(">|<", ""), &item.additional_text_edits.unwrap()); + assert_eq!(changed, expected); + assert_eq!(item.sort_text, Some(auto_import_sort_text())); } #[test] @@ -1901,7 +2273,7 @@ mod completion_tests { async fn test_auto_import_suggests_pub_use_for_function() { let src = r#" mod bar { - mod baz { + pub mod baz { pub fn coco() {} } @@ -2370,4 +2742,42 @@ mod completion_tests { assert_completion(src, Vec::new()).await; } + + #[test] + async fn test_suggests_trait_in_trait_parent_bounds() { + let src = r#" + trait Foobar {} + struct Foobarbaz {} + + trait Bar: Foob>|< {} + "#; + assert_completion( + src, + vec![simple_completion_item( + "Foobar", + CompletionItemKind::INTERFACE, + Some("Foobar".to_string()), + )], + ) + .await; + } + + #[test] + async fn test_suggests_trait_in_function_where_clause() { + let src = r#" + trait Foobar {} + struct Foobarbaz {} + + fn foo() where T: Foob>|< {} + "#; + assert_completion( + src, + vec![simple_completion_item( + "Foobar", + CompletionItemKind::INTERFACE, + Some("Foobar".to_string()), + )], + ) + .await; + } } diff --git a/noir/noir-repo/tooling/lsp/src/requests/mod.rs b/noir/noir-repo/tooling/lsp/src/requests/mod.rs index 597d83554684..0ee66f1f6180 100644 --- a/noir/noir-repo/tooling/lsp/src/requests/mod.rs +++ b/noir/noir-repo/tooling/lsp/src/requests/mod.rs @@ -378,7 +378,7 @@ fn character_to_line_offset(line: &str, character: u32) -> Result } } -fn to_lsp_location<'a, F>( +pub(crate) fn to_lsp_location<'a, F>( files: &'a F, file_id: F::FileId, definition_span: noirc_errors::Span, diff --git a/noir/noir-repo/tooling/lsp/src/tests.rs b/noir/noir-repo/tooling/lsp/src/tests.rs new file mode 100644 index 000000000000..7f2d48cd23ff --- /dev/null +++ b/noir/noir-repo/tooling/lsp/src/tests.rs @@ -0,0 +1,33 @@ +#![cfg(test)] + +use lsp_types::TextEdit; + +pub(crate) fn apply_text_edit(src: &str, text_edit: &TextEdit) -> String { + let mut lines: Vec<_> = src.lines().collect(); + assert_eq!(text_edit.range.start.line, text_edit.range.end.line); + + let mut line = lines[text_edit.range.start.line as usize].to_string(); + line.replace_range( + text_edit.range.start.character as usize..text_edit.range.end.character as usize, + &text_edit.new_text, + ); + lines[text_edit.range.start.line as usize] = &line; + lines.join("\n") +} + +pub(crate) fn apply_text_edits(src: &str, text_edits: &[TextEdit]) -> String { + let mut text = src.to_string(); + + // Text edits must be applied from last to first, otherwise if we apply a text edit + // that comes before another one, that other one becomes invalid (it will edit the wrong + // piece of code). + let mut text_edits = text_edits.to_vec(); + text_edits.sort_by_key(|edit| (edit.range.start.line, edit.range.start.character)); + text_edits.reverse(); + + for text_edit in text_edits { + text = apply_text_edit(&text, &text_edit); + } + + text +} diff --git a/noir/noir-repo/tooling/lsp/src/use_segment_positions.rs b/noir/noir-repo/tooling/lsp/src/use_segment_positions.rs new file mode 100644 index 000000000000..f9a3f4290294 --- /dev/null +++ b/noir/noir-repo/tooling/lsp/src/use_segment_positions.rs @@ -0,0 +1,336 @@ +use std::collections::HashMap; + +use fm::{FileId, FileMap}; +use lsp_types::{Position, Range, TextEdit}; +use noirc_errors::Span; +use noirc_frontend::ast::{PathKind, UseTree, UseTreeKind}; + +use crate::requests::to_lsp_location; + +/// The position of a segment in a `use` statement. +/// We use this to determine how an auto-import should be inserted. +#[derive(Debug, Default, Copy, Clone)] +pub(crate) enum UseSegmentPosition { + /// The segment either doesn't exist in the source code or there are multiple segments. + /// In this case auto-import will add a new use statement. + #[default] + NoneOrMultiple, + /// The segment is the last one in the `use` statement (or nested use statement): + /// + /// use foo::bar; + /// ^^^ + /// + /// Auto-import will transform it to this: + /// + /// use foo::bar::{self, baz}; + Last { span: Span }, + /// The segment happens before another simple (ident) segment: + /// + /// use foo::bar::qux; + /// ^^^ + /// + /// Auto-import will transform it to this: + /// + /// use foo::bar::{qux, baz}; + BeforeSegment { segment_span_until_end: Span }, + /// The segment happens before a list: + /// + /// use foo::bar::{qux, another}; + /// + /// Auto-import will transform it to this: + /// + /// use foo::bar::{qux, another, baz}; + BeforeList { first_entry_span: Span, list_is_empty: bool }, +} + +/// Remembers where each segment in a `use` statement is located. +/// The key is the full segment, so for `use foo::bar::baz` we'll have three +/// segments: `foo`, `foo::bar` and `foo::bar::baz`, where the span is just +/// for the last identifier (`foo`, `bar` and `baz` in the previous example). +#[derive(Default)] +pub(crate) struct UseSegmentPositions { + use_segment_positions: HashMap, +} + +impl UseSegmentPositions { + pub(crate) fn add(&mut self, use_tree: &UseTree) { + self.gather_use_tree_segments(use_tree, String::new()); + } + + /// Given a full path like `foo::bar::baz`, returns the first non-"NoneOrMultiple" segment position + /// trying each successive parent, together with the name after the parent. + /// + /// For example, first we'll check if `foo::bar` has a single position. If not, we'll try with `foo`. + pub(crate) fn get(&self, full_path: &str) -> (UseSegmentPosition, String) { + // Build a parent path to know in which full segment we need to add this import + let mut segments: Vec<_> = full_path.split("::").collect(); + let mut name = segments.pop().unwrap().to_string(); + let mut parent_path = segments.join("::"); + + loop { + let use_segment_position = + self.use_segment_positions.get(&parent_path).cloned().unwrap_or_default(); + + if let UseSegmentPosition::NoneOrMultiple = use_segment_position { + if let Some(next_name) = segments.pop() { + name = format!("{next_name}::{name}"); + parent_path = segments.join("::"); + } else { + return (UseSegmentPosition::NoneOrMultiple, String::new()); + } + } else { + return (use_segment_position, name); + } + } + } + + fn gather_use_tree_segments(&mut self, use_tree: &UseTree, mut prefix: String) { + let kind_string = match use_tree.prefix.kind { + PathKind::Crate => Some("crate".to_string()), + PathKind::Super => Some("super".to_string()), + PathKind::Dep | PathKind::Plain => None, + }; + if let Some(kind_string) = kind_string { + if let Some(segment) = use_tree.prefix.segments.first() { + self.insert_use_segment_position( + kind_string, + UseSegmentPosition::BeforeSegment { + segment_span_until_end: Span::from( + segment.ident.span().start()..use_tree.span.end() - 1, + ), + }, + ); + } else { + self.insert_use_segment_position_before_use_tree_kind(use_tree, kind_string); + } + } + + let prefix_segments_len = use_tree.prefix.segments.len(); + for (index, segment) in use_tree.prefix.segments.iter().enumerate() { + let ident = &segment.ident; + if !prefix.is_empty() { + prefix.push_str("::"); + }; + prefix.push_str(&ident.0.contents); + + if index < prefix_segments_len - 1 { + self.insert_use_segment_position( + prefix.clone(), + UseSegmentPosition::BeforeSegment { + segment_span_until_end: Span::from( + use_tree.prefix.segments[index + 1].ident.span().start() + ..use_tree.span.end() - 1, + ), + }, + ); + } else { + self.insert_use_segment_position_before_use_tree_kind(use_tree, prefix.clone()); + } + } + + match &use_tree.kind { + UseTreeKind::Path(ident, alias) => { + if !prefix.is_empty() { + prefix.push_str("::"); + } + prefix.push_str(&ident.0.contents); + + if alias.is_none() { + self.insert_use_segment_position( + prefix, + UseSegmentPosition::Last { span: ident.span() }, + ); + } else { + self.insert_use_segment_position(prefix, UseSegmentPosition::NoneOrMultiple); + } + } + UseTreeKind::List(use_trees) => { + for use_tree in use_trees { + self.gather_use_tree_segments(use_tree, prefix.clone()); + } + } + } + } + + fn insert_use_segment_position_before_use_tree_kind( + &mut self, + use_tree: &UseTree, + prefix: String, + ) { + match &use_tree.kind { + UseTreeKind::Path(ident, _alias) => { + self.insert_use_segment_position( + prefix, + UseSegmentPosition::BeforeSegment { + segment_span_until_end: Span::from( + ident.span().start()..use_tree.span.end() - 1, + ), + }, + ); + } + UseTreeKind::List(use_trees) => { + if let Some(first_use_tree) = use_trees.first() { + self.insert_use_segment_position( + prefix, + UseSegmentPosition::BeforeList { + first_entry_span: first_use_tree.prefix.span(), + list_is_empty: false, + }, + ); + } else { + self.insert_use_segment_position( + prefix, + UseSegmentPosition::BeforeList { + first_entry_span: Span::from( + use_tree.span.end() - 1..use_tree.span.end() - 1, + ), + list_is_empty: true, + }, + ); + } + } + } + } + + fn insert_use_segment_position(&mut self, segment: String, position: UseSegmentPosition) { + if self.use_segment_positions.get(&segment).is_none() { + self.use_segment_positions.insert(segment, position); + } else { + self.use_segment_positions.insert(segment, UseSegmentPosition::NoneOrMultiple); + } + } +} + +pub(crate) struct UseCompletionItemAdditionTextEditsRequest<'a> { + /// The full path of the use statement to insert + pub(crate) full_path: &'a str, + pub(crate) files: &'a FileMap, + pub(crate) file: FileId, + /// All of the current source lines + pub(crate) lines: &'a Vec<&'a str>, + /// How many nested `mod` we are in deep + pub(crate) nesting: usize, + /// The line where an auto_import must be inserted + pub(crate) auto_import_line: usize, +} + +/// Returns the text edits needed to add an auto-import for a given full path. +pub(crate) fn use_completion_item_additional_text_edits( + request: UseCompletionItemAdditionTextEditsRequest, + positions: &UseSegmentPositions, +) -> Vec { + let (use_segment_position, name) = positions.get(request.full_path); + match use_segment_position { + UseSegmentPosition::NoneOrMultiple => { + // The parent path either isn't in any use statement, or it exists in multiple + // use statements. In either case we'll add a new use statement. + + new_use_completion_item_additional_text_edits(request) + } + UseSegmentPosition::Last { span } => { + // We have + // + // use foo::bar; + // ^^^ -> span + // + // and we want to transform it to: + // + // use foo::bar::{self, baz}; + // ^^^^^^^^^^^^^ + // + // So we need one text edit: + // 1. insert "::{self, baz}" right after the span + if let Some(lsp_location) = to_lsp_location(request.files, request.file, span) { + let range = lsp_location.range; + vec![TextEdit { + new_text: format!("::{{self, {}}}", name), + range: Range { start: range.end, end: range.end }, + }] + } else { + new_use_completion_item_additional_text_edits(request) + } + } + UseSegmentPosition::BeforeSegment { segment_span_until_end } => { + // Go past the end + let segment_span_until_end = + Span::from(segment_span_until_end.start()..segment_span_until_end.end() + 1); + + // We have + // + // use foo::bar::{one, two}; + // ^^^^^^^^^^^^^^^ -> segment_span_until_end + // + // and we want to transform it to: + // + // use foo::{bar::{one, two}, baz}; + // ^ ^^^^^^ + // + // So we need two text edits: + // 1. insert "{" right before the segment span + // 2. insert ", baz}" right after the segment span + if let Some(lsp_location) = + to_lsp_location(request.files, request.file, segment_span_until_end) + { + let range = lsp_location.range; + vec![ + TextEdit { + new_text: "{".to_string(), + range: Range { start: range.start, end: range.start }, + }, + TextEdit { + new_text: format!(", {}}}", name), + range: Range { start: range.end, end: range.end }, + }, + ] + } else { + new_use_completion_item_additional_text_edits(request) + } + } + UseSegmentPosition::BeforeList { first_entry_span, list_is_empty } => { + // We have + // + // use foo::bar::{one, two}; + // ^^^ -> first_entry_span + // + // and we want to transform it to: + // + // use foo::bar::{baz, one, two}; + // ^^^^ + // + // So we need one text edit: + // 1. insert "baz, " right before the first entry span + if let Some(lsp_location) = + to_lsp_location(request.files, request.file, first_entry_span) + { + let range = lsp_location.range; + vec![TextEdit { + new_text: if list_is_empty { name } else { format!("{}, ", name) }, + range: Range { start: range.start, end: range.start }, + }] + } else { + new_use_completion_item_additional_text_edits(request) + } + } + } +} + +fn new_use_completion_item_additional_text_edits( + request: UseCompletionItemAdditionTextEditsRequest, +) -> Vec { + let line = request.auto_import_line as u32; + let character = (request.nesting * 4) as u32; + let indent = " ".repeat(request.nesting * 4); + let mut newlines = "\n"; + + // If the line we are inserting into is not an empty line, insert an extra line to make some room + if let Some(line_text) = request.lines.get(line as usize) { + if !line_text.trim().is_empty() { + newlines = "\n\n"; + } + } + + vec![TextEdit { + range: Range { start: Position { line, character }, end: Position { line, character } }, + new_text: format!("use {};{}{}", request.full_path, newlines, indent), + }] +} diff --git a/noir/noir-repo/tooling/lsp/src/visibility.rs b/noir/noir-repo/tooling/lsp/src/visibility.rs index 207302f327e0..6a21525f069a 100644 --- a/noir/noir-repo/tooling/lsp/src/visibility.rs +++ b/noir/noir-repo/tooling/lsp/src/visibility.rs @@ -4,12 +4,23 @@ use noirc_frontend::{ ast::ItemVisibility, graph::CrateId, hir::{ - def_map::{CrateDefMap, ModuleId}, + def_map::{CrateDefMap, ModuleDefId, ModuleId}, resolution::visibility::can_reference_module_id, }, + node_interner::NodeInterner, }; -pub(super) fn is_visible( +use crate::modules::get_parent_module; + +/// Returns true if an item with the given visibility in the target module +/// is visible from the current module. For example: +/// +/// mod foo { +/// ^^^ <-- target module +/// pub(crate) fn bar() {} +/// ^^^^^^^^^^ <- visibility +/// } +pub(super) fn item_in_module_is_visible( target_module_id: ModuleId, current_module_id: ModuleId, visibility: ItemVisibility, @@ -23,3 +34,45 @@ pub(super) fn is_visible( visibility, ) } + +/// Returns true if the given ModuleDefId is visible from the current module, given its visibility. +/// This will in turn check if the ModuleDefId parent modules are visible from the current module. +/// If `defining_module` is Some, it will be considered as the parent of the item to check +/// (this is the case when the item is re-exported with `pub use` or similar). +pub(super) fn module_def_id_is_visible( + module_def_id: ModuleDefId, + current_module_id: ModuleId, + mut visibility: ItemVisibility, + mut defining_module: Option, + interner: &NodeInterner, + def_maps: &BTreeMap, +) -> bool { + // First find out which module we need to check. + // If a module is trying to be referenced, it's that module. Otherwise it's the module that contains the item. + let mut target_module_id = if let ModuleDefId::ModuleId(module_id) = module_def_id { + Some(module_id) + } else { + std::mem::take(&mut defining_module).or_else(|| get_parent_module(interner, module_def_id)) + }; + + // Then check if it's visible, and upwards + while let Some(module_id) = target_module_id { + if !item_in_module_is_visible(module_id, current_module_id, visibility, def_maps) { + return false; + } + + target_module_id = std::mem::take(&mut defining_module).or_else(|| { + let module_data = &def_maps[&module_id.krate].modules()[module_id.local_id.0]; + let parent_local_id = module_data.parent; + parent_local_id.map(|local_id| ModuleId { krate: module_id.krate, local_id }) + }); + + // This is a bit strange, but the visibility is always that of the item inside another module, + // so the visibility we update here is for the next loop check. + visibility = interner + .try_module_attributes(&module_id) + .map_or(ItemVisibility::Public, |attributes| attributes.visibility); + } + + true +} diff --git a/noir/noir-repo/tooling/nargo_cli/Cargo.toml b/noir/noir-repo/tooling/nargo_cli/Cargo.toml index d72556ab936d..d48e44415b0d 100644 --- a/noir/noir-repo/tooling/nargo_cli/Cargo.toml +++ b/noir/noir-repo/tooling/nargo_cli/Cargo.toml @@ -86,6 +86,7 @@ sha2.workspace = true sha3.workspace = true iai = "0.1.1" test-binary = "3.0.2" +test-case.workspace = true light-poseidon = "0.2.0" diff --git a/noir/noir-repo/tooling/nargo_cli/tests/stdlib-tests.rs b/noir/noir-repo/tooling/nargo_cli/tests/stdlib-tests.rs index 46a241b7e0b6..bdc92e625ab9 100644 --- a/noir/noir-repo/tooling/nargo_cli/tests/stdlib-tests.rs +++ b/noir/noir-repo/tooling/nargo_cli/tests/stdlib-tests.rs @@ -1,4 +1,5 @@ //! Execute unit tests in the Noir standard library. +#![allow(clippy::items_after_test_module)] use clap::Parser; use fm::FileManager; use noirc_driver::{check_crate, file_manager_with_stdlib, CompileOptions}; @@ -12,6 +13,7 @@ use nargo::{ parse_all, prepare_package, }; use termcolor::{Color, ColorChoice, ColorSpec, StandardStream, WriteColor}; +use test_case::test_matrix; #[derive(Parser, Debug)] #[command(ignore_errors = true)] @@ -29,14 +31,22 @@ pub struct Options { impl Options { pub fn function_name_match(&self) -> FunctionNameMatch { match self.args.as_slice() { - [_, lib] => FunctionNameMatch::Contains(lib.as_str()), + [_test_name, lib] => FunctionNameMatch::Contains(lib.as_str()), _ => FunctionNameMatch::Anything, } } } -#[test] -fn run_stdlib_tests() { +/// Inliner aggressiveness results in different SSA. +/// Inlining happens if `inline_cost - retain_cost < aggressiveness` (see `inlining.rs`). +/// NB the CLI uses maximum aggressiveness. +/// +/// Even with the same inlining aggressiveness, forcing Brillig can trigger different behaviour. +#[test_matrix( + [false, true], + [i64::MIN, 0, i64::MAX] +)] +fn run_stdlib_tests(force_brillig: bool, inliner_aggressiveness: i64) { let opts = Options::parse(); let mut file_manager = file_manager_with_stdlib(&PathBuf::from(".")); @@ -80,7 +90,7 @@ fn run_stdlib_tests() { None, Some(dummy_package.root_dir.clone()), Some(dummy_package.name.to_string()), - &CompileOptions::default(), + &CompileOptions { force_brillig, inliner_aggressiveness, ..Default::default() }, ); (test_name, status) }) diff --git a/noir/noir-repo/tooling/nargo_fmt/src/chunks.rs b/noir/noir-repo/tooling/nargo_fmt/src/chunks.rs index 673cf020aed3..0e55dfad3b73 100644 --- a/noir/noir-repo/tooling/nargo_fmt/src/chunks.rs +++ b/noir/noir-repo/tooling/nargo_fmt/src/chunks.rs @@ -995,7 +995,12 @@ impl<'a> Formatter<'a> { /// Appends the string to the current buffer line by line, with some pre-checks. fn write_chunk_lines(&mut self, string: &str) { - for (index, line) in string.lines().enumerate() { + let lines: Vec<_> = string.lines().collect(); + + let mut index = 0; + while index < lines.len() { + let line = &lines[index]; + // Don't indent the first line (it should already be indented). // Also don't indent if the current line already has a space as the last char // (it means it's already indented) @@ -1015,6 +1020,14 @@ impl<'a> Formatter<'a> { } else { self.write(line); } + + index += 1; + + // If a newline comes next too, write multiple lines to preserve original formatting + while index < lines.len() && lines[index].is_empty() { + self.write_multiple_lines_without_skipping_whitespace_and_comments(); + index += 1; + } } } diff --git a/noir/noir-repo/tooling/nargo_fmt/src/formatter/comments_and_whitespace.rs b/noir/noir-repo/tooling/nargo_fmt/src/formatter/comments_and_whitespace.rs index 4aba0481e243..547d33348b85 100644 --- a/noir/noir-repo/tooling/nargo_fmt/src/formatter/comments_and_whitespace.rs +++ b/noir/noir-repo/tooling/nargo_fmt/src/formatter/comments_and_whitespace.rs @@ -643,7 +643,10 @@ mod foo; /* hello */ } "; - let expected = "struct Foo { /* hello */ }\n"; + let expected = "struct Foo { + /* hello */ +} +"; assert_format(src, expected); } diff --git a/noir/noir-repo/tooling/nargo_fmt/src/formatter/expression.rs b/noir/noir-repo/tooling/nargo_fmt/src/formatter/expression.rs index ebfa3ae78fb0..cd46b09f1906 100644 --- a/noir/noir-repo/tooling/nargo_fmt/src/formatter/expression.rs +++ b/noir/noir-repo/tooling/nargo_fmt/src/formatter/expression.rs @@ -1142,6 +1142,9 @@ impl<'a, 'b> ChunkFormatter<'a, 'b> { pub(super) fn empty_block_contents_chunk(&mut self) -> Option { let mut group = ChunkGroup::new(); group.increase_indentation(); + + let newlines_count = self.following_newlines_count(); + let mut chunk = self.chunk(|formatter| { formatter.skip_comments_and_whitespace_writing_multiple_lines_if_found(); }); @@ -1151,15 +1154,13 @@ impl<'a, 'b> ChunkFormatter<'a, 'b> { // so there's nothing to write. None } else { - if chunk.string.trim_start().starts_with("//") { - group.text(chunk); - group.decrease_indentation(); - group.line(); - } else { + // If we have a trailing comment, preserve it in the same line + if newlines_count == 0 && !chunk.string.trim_start().starts_with("//") { chunk.string = format!(" {} ", chunk.string.trim()); - group.text(chunk); - group.decrease_indentation(); } + group.text(chunk); + group.decrease_indentation(); + group.line(); Some(group) } } diff --git a/noir/noir-repo/tooling/nargo_fmt/src/formatter/module.rs b/noir/noir-repo/tooling/nargo_fmt/src/formatter/module.rs index 00ac9fe2f47e..3df9b7e0c902 100644 --- a/noir/noir-repo/tooling/nargo_fmt/src/formatter/module.rs +++ b/noir/noir-repo/tooling/nargo_fmt/src/formatter/module.rs @@ -31,27 +31,17 @@ impl<'a> Formatter<'a> { self.write_space(); self.write_identifier(submodule.name); self.write_space(); + self.write_left_brace(); if parsed_module_is_empty(&submodule.contents) { - self.write_left_brace(); - self.increase_indentation(); - - let comments_count_before = self.written_comments_count; - self.skip_comments_and_whitespace_writing_multiple_lines_if_found(); - self.decrease_indentation(); - if self.written_comments_count > comments_count_before { - self.write_line(); - self.write_indentation(); - } - self.write_right_brace(); + self.format_empty_block_contents(); } else { - self.write_left_brace(); self.increase_indentation(); self.write_line(); self.format_parsed_module(submodule.contents, self.ignore_next); self.decrease_indentation(); self.write_indentation(); - self.write_right_brace(); } + self.write_right_brace(); } } @@ -102,6 +92,18 @@ mod foo;\n"; assert_format(src, expected); } + #[test] + fn format_empty_submodule_2() { + let src = "mod foo { mod bar { + + } }"; + let expected = "mod foo { + mod bar {} +} +"; + assert_format(src, expected); + } + #[test] fn format_empty_subcontract() { let src = "contract foo { }";