From 256509e5083895b6115b110aedd5a97bd9e74fc0 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 15 Jul 2024 09:09:29 -0300 Subject: [PATCH] feat: add support for usage of `super` in import paths (#5502) # Description ## Problem Resolves #4835 ## Summary Implements `super` in `use` statements and regular paths. For example: ```rust fn some_function() {} mod foo { use super::some_function(); // used in a `use` fn another_function() { super::some_function(); // used in a path } fn foo() { some_function(); // of course this also works because of the `use` above } } ``` Note that, unlike Rust, only one `super` is allowed: `super::super::...` will give an error. ## Additional Context None. ## Documentation\* Check one: - [ ] No documentation needed. - [x] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --- compiler/noirc_frontend/src/ast/statement.rs | 2 + .../src/hir/def_collector/dc_mod.rs | 6 +- .../src/hir/resolution/import.rs | 24 ++++ compiler/noirc_frontend/src/node_interner.rs | 1 - .../noirc_frontend/src/parser/parser/path.rs | 2 + compiler/noirc_frontend/src/tests.rs | 118 +++++++++++------- .../noir/modules_packages_crates/modules.md | 34 +++++ tooling/lsp/src/requests/hover.rs | 37 +++--- tooling/lsp/src/requests/mod.rs | 13 +- tooling/nargo_fmt/src/rewrite/imports.rs | 3 + tooling/nargo_fmt/tests/expected/use_super.nr | 5 + tooling/nargo_fmt/tests/input/use_super.nr | 7 ++ 12 files changed, 186 insertions(+), 66 deletions(-) create mode 100644 tooling/nargo_fmt/tests/expected/use_super.nr create mode 100644 tooling/nargo_fmt/tests/input/use_super.nr diff --git a/compiler/noirc_frontend/src/ast/statement.rs b/compiler/noirc_frontend/src/ast/statement.rs index 3e6a140ff93..6b148cd5428 100644 --- a/compiler/noirc_frontend/src/ast/statement.rs +++ b/compiler/noirc_frontend/src/ast/statement.rs @@ -299,6 +299,7 @@ pub enum PathKind { Crate, Dep, Plain, + Super, } #[derive(Debug, PartialEq, Eq, Clone)] @@ -748,6 +749,7 @@ impl Display for PathKind { match self { PathKind::Crate => write!(f, "crate"), PathKind::Dep => write!(f, "dep"), + PathKind::Super => write!(f, "super"), PathKind::Plain => write!(f, "plain"), } } diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index 1f30b4388b6..12f4aafb304 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -732,11 +732,7 @@ impl<'a> ModCollector<'a> { context.def_interner.add_module_attributes( mod_id, - ModuleAttributes { - name: mod_name.0.contents.clone(), - location: mod_location, - parent: self.module_id, - }, + ModuleAttributes { name: mod_name.0.contents.clone(), location: mod_location }, ); } diff --git a/compiler/noirc_frontend/src/hir/resolution/import.rs b/compiler/noirc_frontend/src/hir/resolution/import.rs index e0c87e2d9e4..10e18248dec 100644 --- a/compiler/noirc_frontend/src/hir/resolution/import.rs +++ b/compiler/noirc_frontend/src/hir/resolution/import.rs @@ -41,6 +41,8 @@ pub enum PathResolutionError { Unresolved(Ident), #[error("{0} is private and not visible from the current module")] Private(Ident), + #[error("There is no super module")] + NoSuper(Span), } #[derive(Debug)] @@ -73,6 +75,9 @@ impl<'a> From<&'a PathResolutionError> for CustomDiagnostic { format!("{ident} is private"), ident.span(), ), + PathResolutionError::NoSuper(span) => { + CustomDiagnostic::simple_error(error.to_string(), String::new(), *span) + } } } } @@ -187,6 +192,25 @@ fn resolve_path_to_ns( path_references, importing_crate, ), + + crate::ast::PathKind::Super => { + if let Some(parent_module_id) = + def_maps[&crate_id].modules[import_directive.module_id.0].parent + { + resolve_name_in_module( + crate_id, + importing_crate, + import_path, + parent_module_id, + def_maps, + path_references, + ) + } else { + let span_start = import_directive.path.span().start(); + let span = Span::from(span_start..span_start + 5); // 5 == "super".len() + Err(PathResolutionError::NoSuper(span)) + } + } } } diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index e534f1bab29..6520dc6aa1f 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -48,7 +48,6 @@ const IMPL_SEARCH_RECURSION_LIMIT: u32 = 10; pub struct ModuleAttributes { pub name: String, pub location: Location, - pub parent: LocalModuleId, } type StructAttributes = Vec; diff --git a/compiler/noirc_frontend/src/parser/parser/path.rs b/compiler/noirc_frontend/src/parser/parser/path.rs index e40268af410..8957fb7c40b 100644 --- a/compiler/noirc_frontend/src/parser/parser/path.rs +++ b/compiler/noirc_frontend/src/parser/parser/path.rs @@ -17,6 +17,7 @@ pub(super) fn path() -> impl NoirParser { choice(( path_kind(Keyword::Crate, PathKind::Crate), path_kind(Keyword::Dep, PathKind::Dep), + path_kind(Keyword::Super, PathKind::Super), idents().map_with_span(make_path(PathKind::Plain)), )) } @@ -64,6 +65,7 @@ mod test { ("std", PathKind::Plain), ("hash::collections", PathKind::Plain), ("crate::std::hash", PathKind::Crate), + ("super::foo", PathKind::Super), ]; for (src, expected_path_kind) in cases { diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index 8cedeeeff0d..321ac3f549d 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -97,6 +97,13 @@ pub(crate) fn get_program_errors(src: &str) -> Vec<(CompilationError, FileId)> { get_program(src, false).2 } +fn assert_no_errors(src: &str) { + let errors = get_program_errors(src); + if !errors.is_empty() { + panic!("Expected no errors, got: {:?}", errors); + } +} + #[test] fn check_trait_implemented_for_all_t() { let src = " @@ -141,10 +148,7 @@ fn check_trait_implemented_for_all_t() { fn main(a: Foo) -> pub bool { a.is_default() }"; - - let errors = get_program_errors(src); - errors.iter().for_each(|err| println!("{:?}", err)); - assert!(errors.is_empty()); + assert_no_errors(src); } #[test] @@ -767,9 +771,7 @@ fn test_impl_self_within_default_def() { self } }"; - let errors = get_program_errors(src); - errors.iter().for_each(|err| println!("{:?}", err)); - assert!(errors.is_empty()); + assert_no_errors(src); } #[test] @@ -794,10 +796,7 @@ fn check_trait_as_type_as_fn_parameter() { fn main(a: Foo) -> pub bool { test_eq(a) }"; - - let errors = get_program_errors(src); - errors.iter().for_each(|err| println!("{:?}", err)); - assert!(errors.is_empty()); + assert_no_errors(src); } #[test] @@ -830,10 +829,7 @@ fn check_trait_as_type_as_two_fn_parameters() { fn main(a: Foo, b: u64) -> pub bool { test_eq(a, b) }"; - - let errors = get_program_errors(src); - errors.iter().for_each(|err| println!("{:?}", err)); - assert!(errors.is_empty()); + assert_no_errors(src); } fn get_program_captures(src: &str) -> Vec> { @@ -898,7 +894,7 @@ fn resolve_empty_function() { } "; - assert!(get_program_errors(src).is_empty()); + assert_no_errors(src); } #[test] fn resolve_basic_function() { @@ -908,7 +904,7 @@ fn resolve_basic_function() { assert(y == x); } "#; - assert!(get_program_errors(src).is_empty()); + assert_no_errors(src); } #[test] fn resolve_unused_var() { @@ -981,7 +977,7 @@ fn resolve_literal_expr() { assert(y == x); } "#; - assert!(get_program_errors(src).is_empty()); + assert_no_errors(src); } #[test] @@ -1028,7 +1024,7 @@ fn resolve_prefix_expr() { let _y = -x; } "#; - assert!(get_program_errors(src).is_empty()); + assert_no_errors(src); } #[test] @@ -1040,7 +1036,7 @@ fn resolve_for_expr() { }; } "#; - assert!(get_program_errors(src).is_empty()); + assert_no_errors(src); } #[test] @@ -1054,7 +1050,7 @@ fn resolve_call_expr() { x } "#; - assert!(get_program_errors(src).is_empty()); + assert_no_errors(src); } #[test] @@ -1071,7 +1067,7 @@ fn resolve_shadowing() { x } "#; - assert!(get_program_errors(src).is_empty()); + assert_no_errors(src); } #[test] @@ -1082,7 +1078,7 @@ fn resolve_basic_closure() { closure(x) } "#; - assert!(get_program_errors(src).is_empty()); + assert_no_errors(src); } #[test] @@ -1133,7 +1129,7 @@ fn resolve_complex_closures() { a + b + c + closure_with_transitive_captures(6) } "#; - assert!(get_program_errors(src).is_empty(), "there should be no errors"); + assert_no_errors(src); let expected_captures = vec![ vec![], @@ -1636,8 +1632,7 @@ fn numeric_generic_in_function_signature() { let src = r#" fn foo(arr: [Field; N]) -> [Field; N] { arr } "#; - let errors = get_program_errors(src); - assert!(errors.is_empty()); + assert_no_errors(src); } #[test] @@ -1749,12 +1744,14 @@ fn numeric_generic_used_in_nested_type_pass() { inner: [u64; N], } "#; - let errors = get_program_errors(src); - assert!(errors.is_empty()); + assert_no_errors(src); } #[test] fn numeric_generic_used_in_trait() { + // We want to make sure that `N` in `impl Deserialize` does + // not trigger `expected type, found numeric generic parameter N` as the trait + // does in fact expect a numeric generic. let src = r#" struct MyType { a: Field, @@ -1773,11 +1770,7 @@ fn numeric_generic_used_in_trait() { fn deserialize(fields: [Field; N], other: T) -> Self; } "#; - let errors = get_program_errors(src); - // We want to make sure that `N` in `impl Deserialize` does - // not trigger `expected type, found numeric generic parameter N` as the trait - // does in fact expect a numeric generic. - assert!(errors.is_empty()); + assert_no_errors(src); } #[test] @@ -1808,8 +1801,7 @@ fn numeric_generic_in_trait_impl_with_extra_impl_generics() { fn deserialize(fields: [Field; N]) -> Self; } "#; - let errors = get_program_errors(src); - assert!(errors.is_empty()); + assert_no_errors(src); } #[test] @@ -1827,8 +1819,7 @@ fn numeric_generic_used_in_where_clause() { T::deserialize(fields) } "#; - let errors = get_program_errors(src); - assert!(errors.is_empty()); + assert_no_errors(src); } #[test] @@ -1845,8 +1836,7 @@ fn numeric_generic_used_in_turbofish() { assert(double::<7 + 8>() == 30); } "#; - let errors = get_program_errors(src); - assert!(errors.is_empty()); + assert_no_errors(src); } #[test] @@ -1866,8 +1856,7 @@ fn constant_used_with_numeric_generic() { } } "#; - let errors = get_program_errors(src); - assert!(errors.is_empty()); + assert_no_errors(src); } #[test] @@ -2076,8 +2065,7 @@ fn turbofish_numeric_generic_nested_call() { let _ = bar::(); } "#; - let errors = get_program_errors(src); - assert!(errors.is_empty()); + assert_no_errors(src); // Check for turbofish numeric generics used with method calls let src = r#" @@ -2107,6 +2095,48 @@ fn turbofish_numeric_generic_nested_call() { let _ = bar::(); } "#; + assert_no_errors(src); +} + +#[test] +fn use_super() { + let src = r#" + fn some_func() {} + + mod foo { + use super::some_func; + } + "#; + assert_no_errors(src); +} + +#[test] +fn use_super_in_path() { + let src = r#" + fn some_func() {} + + mod foo { + fn func() { + super::some_func(); + } + } + "#; + assert_no_errors(src); +} + +#[test] +fn no_super() { + let src = "use super::some_func;"; let errors = get_program_errors(src); - assert!(errors.is_empty()); + assert_eq!(errors.len(), 1); + + let CompilationError::DefinitionError(DefCollectorErrorKind::PathResolutionError( + PathResolutionError::NoSuper(span), + )) = &errors[0].0 + else { + panic!("Expected a 'no super' error, got {:?}", errors[0].0); + }; + + assert_eq!(span.start(), 4); + assert_eq!(span.end(), 9); } diff --git a/docs/docs/noir/modules_packages_crates/modules.md b/docs/docs/noir/modules_packages_crates/modules.md index 9fffd925b7b..16b6307d2fd 100644 --- a/docs/docs/noir/modules_packages_crates/modules.md +++ b/docs/docs/noir/modules_packages_crates/modules.md @@ -148,4 +148,38 @@ Filename : `src/foo/bar/mod.nr` ```rust fn from_bar() {} +``` + +### Referencing a parent module + +Given a submodule, you can refer to its parent module using the `super` keyword. + +Filename : `src/main.nr` + +```rust +mod foo; + +fn main() { + foo::from_foo(); +} +``` + +Filename : `src/foo.nr` + +```rust +mod bar; + +fn from_foo() {} +``` + +Filename : `src/foo/bar.nr` + +```rust +// Same as bar::from_foo +use super::from_foo; + +fn from_bar() { + from_foo(); // invokes super::from_foo(), which is bar::from_foo() + super::from_foo(); // also invokes bar::from_foo() +} ``` \ No newline at end of file diff --git a/tooling/lsp/src/requests/hover.rs b/tooling/lsp/src/requests/hover.rs index 161fd20f555..729fe85e70e 100644 --- a/tooling/lsp/src/requests/hover.rs +++ b/tooling/lsp/src/requests/hover.rs @@ -56,14 +56,17 @@ fn format_reference(reference: ReferenceId, args: &ProcessRequestCallbackArgs) - } fn format_module(id: ModuleId, args: &ProcessRequestCallbackArgs) -> String { let module_attributes = args.interner.module_attributes(&id); + let parent_module_id = args.def_maps[&id.krate].modules()[id.local_id.0].parent; let mut string = String::new(); - if format_parent_module_from_module_id( - &ModuleId { krate: id.krate, local_id: module_attributes.parent }, - args, - &mut string, - ) { - string.push('\n'); + if let Some(parent_module_id) = parent_module_id { + if format_parent_module_from_module_id( + &ModuleId { krate: id.krate, local_id: parent_module_id }, + args, + &mut string, + ) { + string.push('\n'); + } } string.push_str(" "); string.push_str("mod "); @@ -316,7 +319,6 @@ fn format_parent_module_from_module_id( CrateId::Stdlib(_) => Some("std".to_string()), CrateId::Dummy => None, }; - let wrote_crate = if let Some(crate_name) = crate_name { string.push_str(" "); string.push_str(&crate_name); @@ -329,6 +331,9 @@ fn format_parent_module_from_module_id( return wrote_crate; }; + let modules = args.def_maps[&module.krate].modules(); + let module_data = &modules[module.local_id.0]; + if wrote_crate { string.push_str("::"); } else { @@ -336,13 +341,17 @@ fn format_parent_module_from_module_id( } let mut segments = Vec::new(); - let mut current_attributes = module_attributes; - while let Some(parent_attributes) = args.interner.try_module_attributes(&ModuleId { - krate: module.krate, - local_id: current_attributes.parent, - }) { - segments.push(&parent_attributes.name); - current_attributes = parent_attributes; + let mut current_parent = module_data.parent; + while let Some(parent) = current_parent { + let parent_attributes = args + .interner + .try_module_attributes(&ModuleId { krate: module.krate, local_id: parent }); + if let Some(parent_attributes) = parent_attributes { + segments.push(&parent_attributes.name); + current_parent = modules[module.local_id.0].parent; + } else { + break; + } } for segment in segments.iter().rev() { diff --git a/tooling/lsp/src/requests/mod.rs b/tooling/lsp/src/requests/mod.rs index e029750e589..fbdfbc1cb1a 100644 --- a/tooling/lsp/src/requests/mod.rs +++ b/tooling/lsp/src/requests/mod.rs @@ -1,4 +1,7 @@ -use std::{collections::HashMap, future::Future}; +use std::{ + collections::{BTreeMap, HashMap}, + future::Future, +}; use crate::{ parse_diff, resolve_workspace_for_source_path, @@ -14,7 +17,11 @@ use lsp_types::{ use nargo::insert_all_files_for_workspace_into_file_manager; use nargo_fmt::Config; use noirc_driver::file_manager_with_stdlib; -use noirc_frontend::{graph::Dependency, macros_api::NodeInterner}; +use noirc_frontend::{ + graph::{CrateId, Dependency}, + hir::def_map::CrateDefMap, + macros_api::NodeInterner, +}; use serde::{Deserialize, Serialize}; use crate::{ @@ -278,6 +285,7 @@ pub(crate) struct ProcessRequestCallbackArgs<'a> { interners: &'a HashMap, root_crate_name: String, root_crate_dependencies: &'a Vec, + def_maps: &'a BTreeMap, } pub(crate) fn process_request( @@ -332,6 +340,7 @@ where interners: &state.cached_definitions, root_crate_name: package.name.to_string(), root_crate_dependencies: &context.crate_graph[context.root_crate_id()].dependencies, + def_maps: &context.def_maps, })) } pub(crate) fn find_all_references_in_workspace( diff --git a/tooling/nargo_fmt/src/rewrite/imports.rs b/tooling/nargo_fmt/src/rewrite/imports.rs index 564ef3fa370..025d354259e 100644 --- a/tooling/nargo_fmt/src/rewrite/imports.rs +++ b/tooling/nargo_fmt/src/rewrite/imports.rs @@ -14,6 +14,7 @@ pub(crate) enum UseSegment { List(Vec), Dep, Crate, + Super, } impl UseSegment { @@ -50,6 +51,7 @@ impl UseSegment { } UseSegment::Dep => "dep".into(), UseSegment::Crate => "crate".into(), + UseSegment::Super => "super".into(), } } } @@ -66,6 +68,7 @@ impl UseTree { match use_tree.prefix.kind { ast::PathKind::Crate => result.path.push(UseSegment::Crate), ast::PathKind::Dep => result.path.push(UseSegment::Dep), + ast::PathKind::Super => result.path.push(UseSegment::Super), ast::PathKind::Plain => {} }; diff --git a/tooling/nargo_fmt/tests/expected/use_super.nr b/tooling/nargo_fmt/tests/expected/use_super.nr new file mode 100644 index 00000000000..91fbe7a9df1 --- /dev/null +++ b/tooling/nargo_fmt/tests/expected/use_super.nr @@ -0,0 +1,5 @@ +fn some_func() {} + +mod foo { + use super::some_func; +} diff --git a/tooling/nargo_fmt/tests/input/use_super.nr b/tooling/nargo_fmt/tests/input/use_super.nr new file mode 100644 index 00000000000..a3b7d4cb4e2 --- /dev/null +++ b/tooling/nargo_fmt/tests/input/use_super.nr @@ -0,0 +1,7 @@ +fn some_func() { + +} + +mod foo { + use super::some_func; +}