From b869697ae18e33a955a527bf7bbc73cbde95bedc Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 28 Aug 2024 17:07:34 -0300 Subject: [PATCH 01/24] feat: use visibility --- aztec_macros/src/utils/parse_utils.rs | 2 +- compiler/noirc_frontend/src/ast/mod.rs | 10 +++++++ compiler/noirc_frontend/src/ast/statement.rs | 14 +++++----- .../noirc_frontend/src/elaborator/comptime.rs | 2 +- .../src/hir/def_collector/dc_crate.rs | 9 ++++--- .../src/hir/def_collector/dc_mod.rs | 1 + .../src/hir/def_map/module_data.rs | 5 ++-- .../src/hir/resolution/import.rs | 22 +++++++++------ .../src/hir/resolution/path_resolver.rs | 11 +++++--- compiler/noirc_frontend/src/parser/mod.rs | 25 ++++++++++------- compiler/noirc_frontend/src/parser/parser.rs | 15 +++++++---- .../src/parser/parser/function.rs | 19 +++---------- .../src/parser/parser/visibility.rs | 27 +++++++++++++++++++ noir_stdlib/src/ops/mod.nr | 4 +-- noir_stdlib/src/prelude.nr | 18 ++++++------- tooling/lsp/src/requests/completion.rs | 2 +- tooling/lsp/src/requests/inlay_hint.rs | 2 +- tooling/nargo_fmt/src/rewrite/imports.rs | 16 ++++++++--- tooling/nargo_fmt/src/visitor/item.rs | 9 ++++--- 19 files changed, 140 insertions(+), 73 deletions(-) create mode 100644 compiler/noirc_frontend/src/parser/parser/visibility.rs diff --git a/aztec_macros/src/utils/parse_utils.rs b/aztec_macros/src/utils/parse_utils.rs index 4c6cbb10d9f..2226667de8a 100644 --- a/aztec_macros/src/utils/parse_utils.rs +++ b/aztec_macros/src/utils/parse_utils.rs @@ -50,7 +50,7 @@ fn empty_item(item: &mut Item) { empty_parsed_submodule(parsed_submodule); } ItemKind::ModuleDecl(module_declaration) => empty_module_declaration(module_declaration), - ItemKind::Import(use_tree) => empty_use_tree(use_tree), + ItemKind::Import(use_tree, _) => empty_use_tree(use_tree), ItemKind::Struct(noir_struct) => empty_noir_struct(noir_struct), ItemKind::TypeAlias(noir_type_alias) => empty_noir_type_alias(noir_type_alias), } diff --git a/compiler/noirc_frontend/src/ast/mod.rs b/compiler/noirc_frontend/src/ast/mod.rs index 6f6d5cbccdf..23dd9e1d670 100644 --- a/compiler/noirc_frontend/src/ast/mod.rs +++ b/compiler/noirc_frontend/src/ast/mod.rs @@ -460,6 +460,16 @@ pub enum ItemVisibility { PublicCrate, } +impl std::fmt::Display for ItemVisibility { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + ItemVisibility::Public => write!(f, "{}", "pub"), + ItemVisibility::Private => Ok(()), + ItemVisibility::PublicCrate => write!(f, "{}", "pub(crate)"), + } + } +} + #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, Serialize, Deserialize)] /// Represents whether the parameter is public or known only to the prover. pub enum Visibility { diff --git a/compiler/noirc_frontend/src/ast/statement.rs b/compiler/noirc_frontend/src/ast/statement.rs index edccf545a02..7150e676715 100644 --- a/compiler/noirc_frontend/src/ast/statement.rs +++ b/compiler/noirc_frontend/src/ast/statement.rs @@ -7,7 +7,7 @@ use iter_extended::vecmap; use noirc_errors::{Span, Spanned}; use super::{ - BlockExpression, Expression, ExpressionKind, GenericTypeArgs, IndexExpression, + BlockExpression, Expression, ExpressionKind, GenericTypeArgs, IndexExpression, ItemVisibility, MemberAccessExpression, MethodCallExpression, UnresolvedType, }; use crate::elaborator::types::SELF_TYPE_NAME; @@ -293,6 +293,7 @@ impl std::fmt::Display for ModuleDeclaration { #[derive(Debug, PartialEq, Eq, Clone)] pub struct ImportStatement { + pub visibility: ItemVisibility, pub path: Path, pub alias: Option, } @@ -341,7 +342,7 @@ pub enum UseTreeKind { } impl UseTree { - pub fn desugar(self, root: Option) -> Vec { + pub fn desugar(self, root: Option, visibility: ItemVisibility) -> Vec { let prefix = if let Some(mut root) = root { root.segments.extend(self.prefix.segments); root @@ -351,11 +352,12 @@ impl UseTree { match self.kind { UseTreeKind::Path(name, alias) => { - vec![ImportStatement { path: prefix.join(name), alias }] - } - UseTreeKind::List(trees) => { - trees.into_iter().flat_map(|tree| tree.desugar(Some(prefix.clone()))).collect() + vec![ImportStatement { visibility, path: prefix.join(name), alias }] } + UseTreeKind::List(trees) => trees + .into_iter() + .flat_map(|tree| tree.desugar(Some(prefix.clone()), visibility)) + .collect(), } } } diff --git a/compiler/noirc_frontend/src/elaborator/comptime.rs b/compiler/noirc_frontend/src/elaborator/comptime.rs index 01b4585640f..c49cef1f38d 100644 --- a/compiler/noirc_frontend/src/elaborator/comptime.rs +++ b/compiler/noirc_frontend/src/elaborator/comptime.rs @@ -332,7 +332,7 @@ impl<'context> Elaborator<'context> { TopLevelStatement::Error => (), TopLevelStatement::Module(_) - | TopLevelStatement::Import(_) + | TopLevelStatement::Import(..) | TopLevelStatement::Struct(_) | TopLevelStatement::Trait(_) | TopLevelStatement::Impl(_) diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index 30c91b42b2e..aa8b053ba6a 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -19,8 +19,8 @@ use crate::node_interner::{ }; use crate::ast::{ - ExpressionKind, GenericTypeArgs, Ident, LetStatement, Literal, NoirFunction, NoirStruct, - NoirTrait, NoirTypeAlias, Path, PathKind, PathSegment, UnresolvedGenerics, + ExpressionKind, GenericTypeArgs, Ident, ItemVisibility, LetStatement, Literal, NoirFunction, + NoirStruct, NoirTrait, NoirTypeAlias, Path, PathKind, PathSegment, UnresolvedGenerics, UnresolvedTraitConstraint, UnresolvedType, }; @@ -360,9 +360,11 @@ impl DefCollector { let current_def_map = context.def_maps.get_mut(&crate_id).unwrap(); let name = resolved_import.name; + let visibility = collected_import.visibility; + let is_prelude = resolved_import.is_prelude; for ns in resolved_import.resolved_namespace.iter_defs() { let result = current_def_map.modules[resolved_import.module_scope.0] - .import(name.clone(), ns, resolved_import.is_prelude); + .import(name.clone(), visibility, ns, is_prelude); let file_id = current_def_map.file_id(module_id); let last_segment = collected_import.path.last_ident(); @@ -494,6 +496,7 @@ fn inject_prelude( collected_imports.insert( 0, ImportDirective { + visibility: ItemVisibility::Private, module_id: crate_root, path: Path { segments, kind: PathKind::Plain, span: Span::default() }, alias: None, 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 459c4869379..590cdc541ce 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -80,6 +80,7 @@ pub fn collect_defs( // Then add the imports to defCollector to resolve once all modules in the hierarchy have been resolved for import in ast.imports { collector.def_collector.imports.push(ImportDirective { + visibility: import.visibility, module_id: collector.module_id, path: import.path, alias: import.alias, diff --git a/compiler/noirc_frontend/src/hir/def_map/module_data.rs b/compiler/noirc_frontend/src/hir/def_map/module_data.rs index 7b14db8be77..0e62c518895 100644 --- a/compiler/noirc_frontend/src/hir/def_map/module_data.rs +++ b/compiler/noirc_frontend/src/hir/def_map/module_data.rs @@ -123,15 +123,16 @@ impl ModuleData { pub fn import( &mut self, name: Ident, + visibility: ItemVisibility, id: ModuleDefId, is_prelude: bool, ) -> Result<(), (Ident, Ident)> { // Empty spans could come from implicitly injected imports, and we don't want to track those - if name.span().start() < name.span().end() { + if visibility == ItemVisibility::Private && name.span().start() < name.span().end() { self.unused_imports.insert(name.clone()); } - self.scope.add_item_to_namespace(name, ItemVisibility::Public, id, None, is_prelude) + self.scope.add_item_to_namespace(name, visibility, id, None, is_prelude) } pub fn find_name(&self, name: &Ident) -> PerNs { diff --git a/compiler/noirc_frontend/src/hir/resolution/import.rs b/compiler/noirc_frontend/src/hir/resolution/import.rs index b820e4664e3..4de9df149ce 100644 --- a/compiler/noirc_frontend/src/hir/resolution/import.rs +++ b/compiler/noirc_frontend/src/hir/resolution/import.rs @@ -13,6 +13,7 @@ use super::errors::ResolverError; #[derive(Debug, Clone)] pub struct ImportDirective { + pub visibility: ItemVisibility, pub module_id: LocalModuleId, pub path: Path, pub alias: Option, @@ -262,7 +263,9 @@ fn resolve_name_in_module( } let mut warning: Option = None; - for (last_segment, current_segment) in import_path.iter().zip(import_path.iter().skip(1)) { + for (index, (last_segment, current_segment)) in + import_path.iter().zip(import_path.iter().skip(1)).enumerate() + { let last_segment = &last_segment.ident; let current_segment = ¤t_segment.ident; @@ -298,13 +301,15 @@ fn resolve_name_in_module( }; warning = warning.or_else(|| { - if can_reference_module_id( - def_maps, - importing_crate, - starting_mod, - current_mod_id, - visibility, - ) { + if index == 0 + || can_reference_module_id( + def_maps, + importing_crate, + starting_mod, + current_mod_id, + visibility, + ) + { None } else { Some(PathResolutionError::Private(last_segment.clone())) @@ -365,6 +370,7 @@ fn resolve_external_dep( span: Span::default(), }; let dep_directive = ImportDirective { + visibility: ItemVisibility::Private, module_id: dep_module.local_id, path, alias: directive.alias.clone(), diff --git a/compiler/noirc_frontend/src/hir/resolution/path_resolver.rs b/compiler/noirc_frontend/src/hir/resolution/path_resolver.rs index 712951ad6cb..8d7dbfab4cd 100644 --- a/compiler/noirc_frontend/src/hir/resolution/path_resolver.rs +++ b/compiler/noirc_frontend/src/hir/resolution/path_resolver.rs @@ -1,5 +1,5 @@ use super::import::{resolve_import, ImportDirective, PathResolution, PathResolutionResult}; -use crate::ast::Path; +use crate::ast::{ItemVisibility, Path}; use crate::node_interner::ReferenceId; use std::collections::BTreeMap; @@ -62,8 +62,13 @@ pub fn resolve_path( path_references: &mut Option<&mut Vec>, ) -> PathResolutionResult { // lets package up the path into an ImportDirective and resolve it using that - let import = - ImportDirective { module_id: module_id.local_id, path, alias: None, is_prelude: false }; + let import = ImportDirective { + visibility: ItemVisibility::Private, + module_id: module_id.local_id, + path, + alias: None, + is_prelude: false, + }; let resolved_import = resolve_import(module_id.krate, &import, def_maps, path_references)?; let namespace = resolved_import.resolved_namespace; diff --git a/compiler/noirc_frontend/src/parser/mod.rs b/compiler/noirc_frontend/src/parser/mod.rs index f1972bcb9b5..eca37dc80b2 100644 --- a/compiler/noirc_frontend/src/parser/mod.rs +++ b/compiler/noirc_frontend/src/parser/mod.rs @@ -12,8 +12,9 @@ mod labels; mod parser; use crate::ast::{ - Expression, Ident, ImportStatement, LetStatement, ModuleDeclaration, NoirFunction, NoirStruct, - NoirTrait, NoirTraitImpl, NoirTypeAlias, Recoverable, StatementKind, TypeImpl, UseTree, + Expression, Ident, ImportStatement, ItemVisibility, LetStatement, ModuleDeclaration, + NoirFunction, NoirStruct, NoirTrait, NoirTraitImpl, NoirTypeAlias, Recoverable, StatementKind, + TypeImpl, UseTree, }; use crate::token::{Keyword, Token}; @@ -32,7 +33,7 @@ pub use parser::{ pub enum TopLevelStatement { Function(NoirFunction), Module(ModuleDeclaration), - Import(UseTree), + Import(UseTree, ItemVisibility), Struct(NoirStruct), Trait(NoirTrait), TraitImpl(NoirTraitImpl), @@ -48,7 +49,7 @@ impl TopLevelStatement { match self { TopLevelStatement::Function(f) => Some(ItemKind::Function(f)), TopLevelStatement::Module(m) => Some(ItemKind::ModuleDecl(m)), - TopLevelStatement::Import(i) => Some(ItemKind::Import(i)), + TopLevelStatement::Import(i, visibility) => Some(ItemKind::Import(i, visibility)), TopLevelStatement::Struct(s) => Some(ItemKind::Struct(s)), TopLevelStatement::Trait(t) => Some(ItemKind::Trait(t)), TopLevelStatement::TraitImpl(t) => Some(ItemKind::TraitImpl(t)), @@ -298,7 +299,7 @@ impl ParsedModule { for item in self.items { match item.kind { - ItemKind::Import(import) => module.push_import(import), + ItemKind::Import(import, visibility) => module.push_import(import, visibility), ItemKind::Function(func) => module.push_function(func), ItemKind::Struct(typ) => module.push_type(typ), ItemKind::Trait(noir_trait) => module.push_trait(noir_trait), @@ -323,7 +324,7 @@ pub struct Item { #[derive(Clone, Debug)] pub enum ItemKind { - Import(UseTree), + Import(UseTree, ItemVisibility), Function(NoirFunction), Struct(NoirStruct), Trait(NoirTrait), @@ -398,8 +399,8 @@ impl SortedModule { self.type_aliases.push(type_alias); } - fn push_import(&mut self, import_stmt: UseTree) { - self.imports.extend(import_stmt.desugar(None)); + fn push_import(&mut self, import_stmt: UseTree, visibility: ItemVisibility) { + self.imports.extend(import_stmt.desugar(None, visibility)); } fn push_module_decl(&mut self, mod_decl: ModuleDeclaration) { @@ -497,7 +498,13 @@ impl std::fmt::Display for TopLevelStatement { match self { TopLevelStatement::Function(fun) => fun.fmt(f), TopLevelStatement::Module(m) => m.fmt(f), - TopLevelStatement::Import(tree) => write!(f, "use {tree}"), + TopLevelStatement::Import(tree, visibility) => { + if visibility == &ItemVisibility::Private { + write!(f, "use {tree}") + } else { + write!(f, "{visibility} use {tree}") + } + } TopLevelStatement::Trait(t) => t.fmt(f), TopLevelStatement::TraitImpl(i) => i.fmt(f), TopLevelStatement::Struct(s) => s.fmt(f), diff --git a/compiler/noirc_frontend/src/parser/parser.rs b/compiler/noirc_frontend/src/parser/parser.rs index 56c80ee1ce0..5f92f55490b 100644 --- a/compiler/noirc_frontend/src/parser/parser.rs +++ b/compiler/noirc_frontend/src/parser/parser.rs @@ -27,6 +27,7 @@ use self::path::as_trait_path; use self::primitives::{keyword, macro_quote_marker, mutable_reference, variable}; use self::types::{generic_type_args, maybe_comp_time}; pub use types::parse_type; +use visibility::visibility_modifier; use super::{ foldl_with_span, labels::ParsingRuleLabel, parameter_name_recovery, parameter_recovery, @@ -64,6 +65,7 @@ mod primitives; mod structs; pub(super) mod traits; mod types; +mod visibility; // synthesized by LALRPOP lalrpop_mod!(pub noir_parser); @@ -93,7 +95,7 @@ pub fn parse_program(source_program: &str) -> (ParsedModule, Vec) { for parsed_item in &parsed_module.items { if lalrpop_parser_supports_kind(&parsed_item.kind) { match &parsed_item.kind { - ItemKind::Import(parsed_use_tree) => { + ItemKind::Import(parsed_use_tree, _visibility) => { prototype_parse_use_tree(Some(parsed_use_tree), source_program); } // other kinds prevented by lalrpop_parser_supports_kind @@ -137,7 +139,7 @@ fn prototype_parse_use_tree(expected_use_tree_opt: Option<&UseTree>, input: &str ); match calculated.unwrap() { - TopLevelStatement::Import(parsed_use_tree) => { + TopLevelStatement::Import(parsed_use_tree, _visibility) => { assert_eq!(expected_use_tree, &parsed_use_tree); } unexpected_calculated => { @@ -159,7 +161,7 @@ fn prototype_parse_use_tree(expected_use_tree_opt: Option<&UseTree>, input: &str } fn lalrpop_parser_supports_kind(kind: &ItemKind) -> bool { - matches!(kind, ItemKind::Import(_)) + matches!(kind, ItemKind::Import(..)) } /// program: module EOF @@ -436,7 +438,10 @@ fn module_declaration() -> impl NoirParser { } fn use_statement() -> impl NoirParser { - keyword(Keyword::Use).ignore_then(use_tree()).map(TopLevelStatement::Import) + visibility_modifier() + .then_ignore(keyword(Keyword::Use)) + .then(use_tree()) + .map(|(visibility, use_tree)| TopLevelStatement::Import(use_tree, visibility)) } fn rename() -> impl NoirParser> { @@ -1534,7 +1539,7 @@ mod test { parse_recover(&use_statement(), &use_statement_str); use_statement_str.push(';'); match result_opt.unwrap() { - TopLevelStatement::Import(expected_use_statement) => { + TopLevelStatement::Import(expected_use_statement, _visibility) => { Some(expected_use_statement) } _ => unreachable!(), diff --git a/compiler/noirc_frontend/src/parser/parser/function.rs b/compiler/noirc_frontend/src/parser/parser/function.rs index 56760898374..9328c882e54 100644 --- a/compiler/noirc_frontend/src/parser/parser/function.rs +++ b/compiler/noirc_frontend/src/parser/parser/function.rs @@ -3,7 +3,9 @@ use super::{ block, fresh_statement, ident, keyword, maybe_comp_time, nothing, optional_visibility, parameter_name_recovery, parameter_recovery, parenthesized, parse_type, pattern, primitives::token_kind, - self_parameter, where_clause, NoirParser, + self_parameter, + visibility::visibility_modifier, + where_clause, NoirParser, }; use crate::token::{Keyword, Token, TokenKind}; use crate::{ @@ -73,21 +75,6 @@ pub(super) fn function_definition(allow_self: bool) -> impl NoirParser impl NoirParser { - let is_pub_crate = (keyword(Keyword::Pub) - .then_ignore(just(Token::LeftParen)) - .then_ignore(keyword(Keyword::Crate)) - .then_ignore(just(Token::RightParen))) - .map(|_| ItemVisibility::PublicCrate); - - let is_pub = keyword(Keyword::Pub).map(|_| ItemVisibility::Public); - - let is_private = empty().map(|_| ItemVisibility::Private); - - choice((is_pub_crate, is_pub, is_private)) -} - /// function_modifiers: 'unconstrained'? (visibility)? /// /// returns (is_unconstrained, visibility) for whether each keyword was present diff --git a/compiler/noirc_frontend/src/parser/parser/visibility.rs b/compiler/noirc_frontend/src/parser/parser/visibility.rs new file mode 100644 index 00000000000..d9c1abf2123 --- /dev/null +++ b/compiler/noirc_frontend/src/parser/parser/visibility.rs @@ -0,0 +1,27 @@ +use chumsky::{ + prelude::{choice, empty, just}, + Parser, +}; + +use crate::{ + ast::ItemVisibility, + parser::NoirParser, + token::{Keyword, Token}, +}; + +use super::primitives::keyword; + +/// visibility_modifier: 'pub(crate)'? 'pub'? '' +pub(crate) fn visibility_modifier() -> impl NoirParser { + let is_pub_crate = (keyword(Keyword::Pub) + .then_ignore(just(Token::LeftParen)) + .then_ignore(keyword(Keyword::Crate)) + .then_ignore(just(Token::RightParen))) + .map(|_| ItemVisibility::PublicCrate); + + let is_pub = keyword(Keyword::Pub).map(|_| ItemVisibility::Public); + + let is_private = empty().map(|_| ItemVisibility::Private); + + choice((is_pub_crate, is_pub, is_private)) +} diff --git a/noir_stdlib/src/ops/mod.nr b/noir_stdlib/src/ops/mod.nr index 8b1903cff0b..6cf20432468 100644 --- a/noir_stdlib/src/ops/mod.nr +++ b/noir_stdlib/src/ops/mod.nr @@ -1,5 +1,5 @@ mod arith; mod bit; -use arith::{Add, Sub, Mul, Div, Rem, Neg}; -use bit::{Not, BitOr, BitAnd, BitXor, Shl, Shr}; +pub use arith::{Add, Sub, Mul, Div, Rem, Neg}; +pub use bit::{Not, BitOr, BitAnd, BitXor, Shl, Shr}; diff --git a/noir_stdlib/src/prelude.nr b/noir_stdlib/src/prelude.nr index 0d423e3556d..b14f38bdf55 100644 --- a/noir_stdlib/src/prelude.nr +++ b/noir_stdlib/src/prelude.nr @@ -1,9 +1,9 @@ -use crate::collections::vec::Vec; -use crate::collections::bounded_vec::BoundedVec; -use crate::option::Option; -use crate::{print, println, assert_constant}; -use crate::uint128::U128; -use crate::cmp::{Eq, Ord}; -use crate::default::Default; -use crate::convert::{From, Into}; -use crate::meta::{derive, derive_via}; +pub use crate::collections::vec::Vec; +pub use crate::collections::bounded_vec::BoundedVec; +pub use crate::option::Option; +pub use crate::{print, println, assert_constant}; +pub use crate::uint128::U128; +pub use crate::cmp::{Eq, Ord}; +pub use crate::default::Default; +pub use crate::convert::{From, Into}; +pub use crate::meta::{derive, derive_via}; diff --git a/tooling/lsp/src/requests/completion.rs b/tooling/lsp/src/requests/completion.rs index c61f92795ad..ac99fc2a322 100644 --- a/tooling/lsp/src/requests/completion.rs +++ b/tooling/lsp/src/requests/completion.rs @@ -193,7 +193,7 @@ impl<'a> NodeFinder<'a> { } match &item.kind { - ItemKind::Import(use_tree) => { + ItemKind::Import(use_tree, _) => { let mut prefixes = Vec::new(); self.find_in_use_tree(use_tree, &mut prefixes); } diff --git a/tooling/lsp/src/requests/inlay_hint.rs b/tooling/lsp/src/requests/inlay_hint.rs index a1e083187d3..20cd7833eb6 100644 --- a/tooling/lsp/src/requests/inlay_hint.rs +++ b/tooling/lsp/src/requests/inlay_hint.rs @@ -127,7 +127,7 @@ impl<'a> InlayHintCollector<'a> { }); } ItemKind::ModuleDecl(_) => (), - ItemKind::Import(_) => (), + ItemKind::Import(..) => (), ItemKind::Struct(_) => (), ItemKind::TypeAlias(_) => (), } diff --git a/tooling/nargo_fmt/src/rewrite/imports.rs b/tooling/nargo_fmt/src/rewrite/imports.rs index 025d354259e..6c63c551f7d 100644 --- a/tooling/nargo_fmt/src/rewrite/imports.rs +++ b/tooling/nargo_fmt/src/rewrite/imports.rs @@ -1,4 +1,4 @@ -use noirc_frontend::ast; +use noirc_frontend::ast::{self, ItemVisibility}; use crate::{ items::Item, @@ -96,8 +96,18 @@ impl UseTree { result } - pub(crate) fn rewrite_top_level(&self, visitor: &FmtVisitor, shape: Shape) -> String { - format!("use {};", self.rewrite(visitor, shape)) + pub(crate) fn rewrite_top_level( + &self, + visitor: &FmtVisitor, + shape: Shape, + visibility: ItemVisibility, + ) -> String { + let rewrite = self.rewrite(visitor, shape); + if visibility == ItemVisibility::Private { + format!("use {};", rewrite) + } else { + format!("{} use {};", visibility, rewrite) + } } fn rewrite(&self, visitor: &FmtVisitor, shape: Shape) -> String { diff --git a/tooling/nargo_fmt/src/visitor/item.rs b/tooling/nargo_fmt/src/visitor/item.rs index 94a32449ebe..c084d8b4458 100644 --- a/tooling/nargo_fmt/src/visitor/item.rs +++ b/tooling/nargo_fmt/src/visitor/item.rs @@ -216,9 +216,12 @@ impl super::FmtVisitor<'_> { self.last_position = span.end(); } } - ItemKind::Import(use_tree) => { - let use_tree = - UseTree::from_ast(use_tree).rewrite_top_level(self, self.shape()); + ItemKind::Import(use_tree, visibility) => { + let use_tree = UseTree::from_ast(use_tree).rewrite_top_level( + self, + self.shape(), + visibility, + ); self.push_rewrite(use_tree, span); self.last_position = span.end(); } From bdc2008ec9dd92a18b6d35661f84a346842d5f1d Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 28 Aug 2024 20:28:25 -0300 Subject: [PATCH 02/24] Only consider first segment public for plain paths --- compiler/noirc_frontend/src/hir/resolution/import.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/compiler/noirc_frontend/src/hir/resolution/import.rs b/compiler/noirc_frontend/src/hir/resolution/import.rs index 4de9df149ce..58a5eb62072 100644 --- a/compiler/noirc_frontend/src/hir/resolution/import.rs +++ b/compiler/noirc_frontend/src/hir/resolution/import.rs @@ -158,6 +158,7 @@ fn resolve_path_to_ns( import_path, import_directive.module_id, def_maps, + true, path_references, ); } @@ -183,6 +184,7 @@ fn resolve_path_to_ns( import_path, import_directive.module_id, def_maps, + true, path_references, ) } @@ -205,6 +207,7 @@ fn resolve_path_to_ns( import_path, parent_module_id, def_maps, + false, path_references, ) } else { @@ -230,6 +233,7 @@ fn resolve_path_from_crate_root( import_path, def_maps[&crate_id].root, def_maps, + false, path_references, ) } @@ -240,6 +244,7 @@ fn resolve_name_in_module( import_path: &[PathSegment], starting_mod: LocalModuleId, def_maps: &BTreeMap, + plain: bool, path_references: &mut Option<&mut Vec>, ) -> NamespaceResolutionResult { let def_map = &def_maps[&krate]; @@ -301,7 +306,9 @@ fn resolve_name_in_module( }; warning = warning.or_else(|| { - if index == 0 + // If the path is plain, the first segment will always refer to + // something that's visible from the current module. + if (plain && index == 0) || can_reference_module_id( def_maps, importing_crate, From c880ae03181975b81ef2dad43954410134140956 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 28 Aug 2024 20:30:33 -0300 Subject: [PATCH 03/24] Fix lalrpop --- compiler/noirc_frontend/src/noir_parser.lalrpop | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_frontend/src/noir_parser.lalrpop b/compiler/noirc_frontend/src/noir_parser.lalrpop index 1488a53183e..01b8be8f721 100644 --- a/compiler/noirc_frontend/src/noir_parser.lalrpop +++ b/compiler/noirc_frontend/src/noir_parser.lalrpop @@ -103,7 +103,7 @@ extern { pub(crate) TopLevelStatement: TopLevelStatement = { "use" r"[\t\r\n ]+" ";" EOF => { - TopLevelStatement::Import(use_tree) + TopLevelStatement::Import(use_tree, crate::ast::ItemVisibility::Private) } } From 6e3cb97e19fb420302b11146602d28640bb39547 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 28 Aug 2024 20:53:15 -0300 Subject: [PATCH 04/24] LSP completion: suggest pub use items --- .../src/hir/resolution/import.rs | 3 +- tooling/lsp/src/requests/completion.rs | 21 ++---------- tooling/lsp/src/requests/completion/kinds.rs | 12 ------- tooling/lsp/src/requests/completion/tests.rs | 32 +++++++++++++++---- 4 files changed, 29 insertions(+), 39 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/resolution/import.rs b/compiler/noirc_frontend/src/hir/resolution/import.rs index 58a5eb62072..814f649fe0a 100644 --- a/compiler/noirc_frontend/src/hir/resolution/import.rs +++ b/compiler/noirc_frontend/src/hir/resolution/import.rs @@ -227,11 +227,12 @@ fn resolve_path_from_crate_root( def_maps: &BTreeMap, path_references: &mut Option<&mut Vec>, ) -> NamespaceResolutionResult { + let starting_mod = def_maps[&crate_id].root; resolve_name_in_module( crate_id, importing_crate, import_path, - def_maps[&crate_id].root, + starting_mod, def_maps, false, path_references, diff --git a/tooling/lsp/src/requests/completion.rs b/tooling/lsp/src/requests/completion.rs index ac99fc2a322..d03ade80ad1 100644 --- a/tooling/lsp/src/requests/completion.rs +++ b/tooling/lsp/src/requests/completion.rs @@ -10,7 +10,7 @@ use completion_items::{ }; use convert_case::{Case, Casing}; use fm::{FileId, FileMap, PathString}; -use kinds::{FunctionCompletionKind, FunctionKind, ModuleCompletionKind, RequestedItems}; +use kinds::{FunctionCompletionKind, FunctionKind, RequestedItems}; use lsp_types::{CompletionItem, CompletionItemKind, CompletionParams, CompletionResponse}; use noirc_errors::{Location, Span}; use noirc_frontend::{ @@ -815,12 +815,6 @@ impl<'a> NodeFinder<'a> { let is_single_segment = !after_colons && idents.is_empty() && path.kind == PathKind::Plain; let module_id; - let module_completion_kind = if after_colons || !idents.is_empty() { - ModuleCompletionKind::DirectChildren - } else { - ModuleCompletionKind::AllVisibleItems - }; - // When completing in the middle of an ident, we don't want to complete // with function parameters because there might already be function parameters, // and in the middle of a path it leads to code that won't compile @@ -883,7 +877,6 @@ impl<'a> NodeFinder<'a> { &prefix, path.kind, at_root, - module_completion_kind, function_completion_kind, requested_items, ); @@ -984,7 +977,6 @@ impl<'a> NodeFinder<'a> { } } - let module_completion_kind = ModuleCompletionKind::DirectChildren; let function_completion_kind = FunctionCompletionKind::Name; let requested_items = RequestedItems::AnyItems; @@ -1000,7 +992,6 @@ impl<'a> NodeFinder<'a> { prefix, path_kind, at_root, - module_completion_kind, function_completion_kind, requested_items, ); @@ -1015,7 +1006,6 @@ impl<'a> NodeFinder<'a> { &prefix, path_kind, at_root, - module_completion_kind, function_completion_kind, requested_items, ); @@ -1026,7 +1016,6 @@ impl<'a> NodeFinder<'a> { &prefix, path_kind, at_root, - module_completion_kind, function_completion_kind, requested_items, ); @@ -1198,7 +1187,6 @@ impl<'a> NodeFinder<'a> { prefix: &str, path_kind: PathKind, at_root: bool, - module_completion_kind: ModuleCompletionKind, function_completion_kind: FunctionCompletionKind, requested_items: RequestedItems, ) { @@ -1231,12 +1219,7 @@ impl<'a> NodeFinder<'a> { let function_kind = FunctionKind::Any; - let items = match module_completion_kind { - ModuleCompletionKind::DirectChildren => module_data.definitions(), - ModuleCompletionKind::AllVisibleItems => module_data.scope(), - }; - - for ident in items.names() { + for ident in module_data.scope().names() { let name = &ident.0.contents; if name_matches(name, prefix) { diff --git a/tooling/lsp/src/requests/completion/kinds.rs b/tooling/lsp/src/requests/completion/kinds.rs index e01fcfc8c56..2fe039ba331 100644 --- a/tooling/lsp/src/requests/completion/kinds.rs +++ b/tooling/lsp/src/requests/completion/kinds.rs @@ -1,17 +1,5 @@ use noirc_frontend::Type; -/// When finding items in a module, whether to show only direct children or all visible items. -#[derive(Clone, Copy, PartialEq, Eq, Debug)] -pub(super) enum ModuleCompletionKind { - // Only show a module's direct children. This is used when completing a use statement - // or a path after the first segment. - DirectChildren, - // Show all of a module's visible items. This is used when completing a path outside - // of a use statement (in regular code) when the path is just a single segment: - // we want to find items exposed in the current module. - AllVisibleItems, -} - /// When suggest a function as a result of completion, whether to autocomplete its name or its name and parameters. #[derive(Clone, Copy, PartialEq, Eq, Debug)] pub(super) enum FunctionCompletionKind { diff --git a/tooling/lsp/src/requests/completion/tests.rs b/tooling/lsp/src/requests/completion/tests.rs index 59e007c5a70..c850ad8c941 100644 --- a/tooling/lsp/src/requests/completion/tests.rs +++ b/tooling/lsp/src/requests/completion/tests.rs @@ -119,14 +119,14 @@ mod completion_tests { #[test] async fn test_use_first_segment() { let src = r#" - mod foo {} + mod foobaz {} mod foobar {} - use f>|< + use foob>|< "#; assert_completion( src, - vec![module_completion_item("foo"), module_completion_item("foobar")], + vec![module_completion_item("foobaz"), module_completion_item("foobar")], ) .await; } @@ -218,7 +218,7 @@ mod completion_tests { #[test] async fn test_use_suggests_hardcoded_crate() { let src = r#" - use c>|< + use cr>|< "#; assert_completion( @@ -291,16 +291,16 @@ mod completion_tests { #[test] async fn test_use_after_super() { let src = r#" - mod foo {} + mod foobar {} mod bar { mod something {} - use super::f>|< + use super::foob>|< } "#; - assert_completion(src, vec![module_completion_item("foo")]).await; + assert_completion(src, vec![module_completion_item("foobar")]).await; } #[test] @@ -1791,4 +1791,22 @@ mod completion_tests { ) .await; } + + #[test] + async fn test_suggests_pub_use() { + let src = r#" + mod bar { + mod baz { + mod coco {} + } + + pub use baz::coco; + } + + fn main() { + bar::c>|< + } + "#; + assert_completion(src, vec![module_completion_item("coco")]).await; + } } From fc7aca499c08a0d659ec5fb4c64e280307532ab0 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 28 Aug 2024 21:30:10 -0300 Subject: [PATCH 05/24] use pub for some functions --- noir_stdlib/src/hash/mod.nr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/noir_stdlib/src/hash/mod.nr b/noir_stdlib/src/hash/mod.nr index 657e1cd8309..5c46aae25f8 100644 --- a/noir_stdlib/src/hash/mod.nr +++ b/noir_stdlib/src/hash/mod.nr @@ -12,7 +12,7 @@ use crate::embedded_curve_ops::{EmbeddedCurvePoint, EmbeddedCurveScalar, multi_s use crate::meta::derive_via; // Kept for backwards compatibility -use sha256::{digest, sha256, sha256_compression, sha256_var}; +pub use sha256::{digest, sha256, sha256_compression, sha256_var}; #[foreign(blake2s)] // docs:start:blake2s From d1d90e47db2f8893d37ad5579e363e3290276e02 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 28 Aug 2024 21:30:45 -0300 Subject: [PATCH 06/24] Let LSP auto-import work well with pub use --- .../src/hir/def_collector/dc_crate.rs | 13 +++++ compiler/noirc_frontend/src/locations.rs | 34 +++++++++--- compiler/noirc_frontend/src/node_interner.rs | 4 +- tooling/lsp/src/requests/completion.rs | 10 ++-- .../src/requests/completion/auto_import.rs | 19 +++++-- .../requests/completion/completion_items.rs | 47 ++++++---------- tooling/lsp/src/requests/completion/tests.rs | 54 +++++++++++++++++++ 7 files changed, 134 insertions(+), 47 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index aa8b053ba6a..a601e60f7cf 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -366,6 +366,19 @@ impl DefCollector { let result = current_def_map.modules[resolved_import.module_scope.0] .import(name.clone(), visibility, ns, is_prelude); + if visibility != ItemVisibility::Private { + let defining_module = ModuleId { + krate: crate_id, + local_id: resolved_import.module_scope, + }; + context.def_interner.register_name_for_auto_import( + name.to_string(), + ns, + visibility, + Some(defining_module), + ); + } + let file_id = current_def_map.file_id(module_id); let last_segment = collected_import.path.last_ident(); diff --git a/compiler/noirc_frontend/src/locations.rs b/compiler/noirc_frontend/src/locations.rs index 0ac13a58ecf..ab85a76372e 100644 --- a/compiler/noirc_frontend/src/locations.rs +++ b/compiler/noirc_frontend/src/locations.rs @@ -278,7 +278,12 @@ impl NodeInterner { } pub(crate) fn register_module(&mut self, id: ModuleId, name: String) { - self.register_name_for_auto_import(name, ModuleDefId::ModuleId(id), ItemVisibility::Public); + self.register_name_for_auto_import( + name, + ModuleDefId::ModuleId(id), + ItemVisibility::Public, + None, + ); } pub(crate) fn register_global( @@ -290,7 +295,7 @@ impl NodeInterner { self.add_definition_location(ReferenceId::Global(id), Some(parent_module_id)); let visibility = ItemVisibility::Public; - self.register_name_for_auto_import(name, ModuleDefId::GlobalId(id), visibility); + self.register_name_for_auto_import(name, ModuleDefId::GlobalId(id), visibility, None); } pub(crate) fn register_struct( @@ -302,13 +307,18 @@ impl NodeInterner { self.add_definition_location(ReferenceId::Struct(id), Some(parent_module_id)); let visibility = ItemVisibility::Public; - self.register_name_for_auto_import(name, ModuleDefId::TypeId(id), visibility); + self.register_name_for_auto_import(name, ModuleDefId::TypeId(id), visibility, None); } pub(crate) fn register_trait(&mut self, id: TraitId, name: String, parent_module_id: ModuleId) { self.add_definition_location(ReferenceId::Trait(id), Some(parent_module_id)); - self.register_name_for_auto_import(name, ModuleDefId::TraitId(id), ItemVisibility::Public); + self.register_name_for_auto_import( + name, + ModuleDefId::TraitId(id), + ItemVisibility::Public, + None, + ); } pub(crate) fn register_type_alias( @@ -320,7 +330,7 @@ impl NodeInterner { self.add_definition_location(ReferenceId::Alias(id), Some(parent_module_id)); let visibility = ItemVisibility::Public; - self.register_name_for_auto_import(name, ModuleDefId::TypeAliasId(id), visibility); + self.register_name_for_auto_import(name, ModuleDefId::TypeAliasId(id), visibility, None); } pub(crate) fn register_function(&mut self, id: FuncId, func_def: &FunctionDefinition) { @@ -328,23 +338,31 @@ impl NodeInterner { func_def.name.0.contents.clone(), ModuleDefId::FunctionId(id), func_def.visibility, + None, ); } - fn register_name_for_auto_import( + pub fn register_name_for_auto_import( &mut self, name: String, module_def_id: ModuleDefId, visibility: ItemVisibility, + defining_module: Option, ) { if !self.lsp_mode { return; } - self.auto_import_names.entry(name).or_default().push((module_def_id, visibility)); + self.auto_import_names.entry(name).or_default().push(( + module_def_id, + visibility, + defining_module, + )); } - pub fn get_auto_import_names(&self) -> &HashMap> { + pub fn get_auto_import_names( + &self, + ) -> &HashMap)>> { &self.auto_import_names } } diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index 4837028b80f..b8d018cb58d 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -240,9 +240,11 @@ pub struct NodeInterner { pub(crate) reference_modules: HashMap, // All names (and their definitions) that can be offered for auto_import. + // The third value in the tuple is the module where the definition is (only for pub use). // These include top-level functions, global variables and types, but excludes // impl and trait-impl methods. - pub(crate) auto_import_names: HashMap>, + pub(crate) auto_import_names: + HashMap)>>, /// Each value currently in scope in the comptime interpreter. /// Each element of the Vec represents a scope with every scope together making diff --git a/tooling/lsp/src/requests/completion.rs b/tooling/lsp/src/requests/completion.rs index d03ade80ad1..1018ff6f859 100644 --- a/tooling/lsp/src/requests/completion.rs +++ b/tooling/lsp/src/requests/completion.rs @@ -1130,6 +1130,7 @@ impl<'a> NodeFinder<'a> { for func_id in methods.iter() { if name_matches(name, prefix) { if let Some(completion_item) = self.function_completion_item( + name, func_id, function_completion_kind, function_kind, @@ -1151,9 +1152,12 @@ impl<'a> NodeFinder<'a> { ) { for (name, func_id) in &trait_.method_ids { if name_matches(name, prefix) { - if let Some(completion_item) = - self.function_completion_item(*func_id, function_completion_kind, function_kind) - { + if let Some(completion_item) = self.function_completion_item( + name, + *func_id, + function_completion_kind, + function_kind, + ) { self.completion_items.push(completion_item); self.suggested_module_def_ids.insert(ModuleDefId::FunctionId(*func_id)); } diff --git a/tooling/lsp/src/requests/completion/auto_import.rs b/tooling/lsp/src/requests/completion/auto_import.rs index 162e1616832..a14af21dd59 100644 --- a/tooling/lsp/src/requests/completion/auto_import.rs +++ b/tooling/lsp/src/requests/completion/auto_import.rs @@ -30,7 +30,7 @@ impl<'a> NodeFinder<'a> { continue; } - for (module_def_id, visibility) in entries { + for (module_def_id, visibility, defining_module) in entries { if self.suggested_module_def_ids.contains(module_def_id) { continue; } @@ -46,7 +46,14 @@ impl<'a> NodeFinder<'a> { }; let module_full_path; - if let ModuleDefId::ModuleId(module_id) = module_def_id { + if let Some(defining_module) = defining_module { + module_full_path = module_id_path( + *defining_module, + &self.module_id, + current_module_parent_id, + self.interner, + ); + } else if let ModuleDefId::ModuleId(module_id) = module_def_id { module_full_path = module_id_path( *module_id, &self.module_id, @@ -81,10 +88,12 @@ impl<'a> NodeFinder<'a> { ); } - let full_path = if let ModuleDefId::ModuleId(..) = module_def_id { - module_full_path - } else { + let full_path = if defining_module.is_some() + || !matches!(module_def_id, ModuleDefId::ModuleId(..)) + { format!("{}::{}", module_full_path, name) + } else { + module_full_path }; let mut label_details = completion_item.label_details.unwrap(); diff --git a/tooling/lsp/src/requests/completion/completion_items.rs b/tooling/lsp/src/requests/completion/completion_items.rs index d4190f5241c..21c3a607b18 100644 --- a/tooling/lsp/src/requests/completion/completion_items.rs +++ b/tooling/lsp/src/requests/completion/completion_items.rs @@ -3,8 +3,8 @@ use lsp_types::{ }; use noirc_frontend::{ hir_def::{function::FuncMeta, stmt::HirPattern}, - macros_api::{ModuleDefId, StructId}, - node_interner::{FuncId, GlobalId, TraitId, TypeAliasId}, + macros_api::ModuleDefId, + node_interner::{FuncId, GlobalId}, Type, }; @@ -38,45 +38,32 @@ impl<'a> NodeFinder<'a> { match module_def_id { ModuleDefId::ModuleId(_) => Some(module_completion_item(name)), - ModuleDefId::FunctionId(func_id) => { - self.function_completion_item(func_id, function_completion_kind, function_kind) - } - ModuleDefId::TypeId(struct_id) => Some(self.struct_completion_item(struct_id)), - ModuleDefId::TypeAliasId(type_alias_id) => { - Some(self.type_alias_completion_item(type_alias_id)) - } - ModuleDefId::TraitId(trait_id) => Some(self.trait_completion_item(trait_id)), - ModuleDefId::GlobalId(global_id) => Some(self.global_completion_item(global_id)), + ModuleDefId::FunctionId(func_id) => self.function_completion_item( + &name, + func_id, + function_completion_kind, + function_kind, + ), + ModuleDefId::TypeId(..) => Some(self.struct_completion_item(name)), + ModuleDefId::TypeAliasId(..) => Some(self.type_alias_completion_item(name)), + ModuleDefId::TraitId(..) => Some(self.trait_completion_item(name)), + ModuleDefId::GlobalId(global_id) => Some(self.global_completion_item(name, global_id)), } } - fn struct_completion_item(&self, struct_id: StructId) -> CompletionItem { - let struct_type = self.interner.get_struct(struct_id); - let struct_type = struct_type.borrow(); - let name = struct_type.name.to_string(); - + fn struct_completion_item(&self, name: String) -> CompletionItem { simple_completion_item(name.clone(), CompletionItemKind::STRUCT, Some(name)) } - fn type_alias_completion_item(&self, type_alias_id: TypeAliasId) -> CompletionItem { - let type_alias = self.interner.get_type_alias(type_alias_id); - let type_alias = type_alias.borrow(); - let name = type_alias.name.to_string(); - + fn type_alias_completion_item(&self, name: String) -> CompletionItem { simple_completion_item(name.clone(), CompletionItemKind::STRUCT, Some(name)) } - fn trait_completion_item(&self, trait_id: TraitId) -> CompletionItem { - let trait_ = self.interner.get_trait(trait_id); - let name = trait_.name.to_string(); - + fn trait_completion_item(&self, name: String) -> CompletionItem { simple_completion_item(name.clone(), CompletionItemKind::INTERFACE, Some(name)) } - fn global_completion_item(&self, global_id: GlobalId) -> CompletionItem { - let global_definition = self.interner.get_global_definition(global_id); - let name = global_definition.name.clone(); - + fn global_completion_item(&self, name: String, global_id: GlobalId) -> CompletionItem { let global = self.interner.get_global(global_id); let typ = self.interner.definition_type(global.definition_id); let description = typ.to_string(); @@ -86,12 +73,12 @@ impl<'a> NodeFinder<'a> { pub(super) fn function_completion_item( &self, + name: &String, func_id: FuncId, function_completion_kind: FunctionCompletionKind, function_kind: FunctionKind, ) -> Option { let func_meta = self.interner.function_meta(&func_id); - let name = &self.interner.function_name(&func_id).to_string(); let func_self_type = if let Some((pattern, typ, _)) = func_meta.parameters.0.first() { if self.hir_pattern_is_self_type(pattern) { diff --git a/tooling/lsp/src/requests/completion/tests.rs b/tooling/lsp/src/requests/completion/tests.rs index c850ad8c941..16fe153e9c9 100644 --- a/tooling/lsp/src/requests/completion/tests.rs +++ b/tooling/lsp/src/requests/completion/tests.rs @@ -1809,4 +1809,58 @@ mod completion_tests { "#; assert_completion(src, vec![module_completion_item("coco")]).await; } + + #[test] + async fn test_auto_import_suggests_pub_use_for_module() { + let src = r#" + mod bar { + mod baz { + mod coco {} + } + + pub use baz::coco as foobar; + } + + fn main() { + foob>|< + } + "#; + + let items = get_completions(src).await; + assert_eq!(items.len(), 1); + + let item = &items[0]; + assert_eq!(item.label, "foobar"); + assert_eq!( + item.label_details.as_ref().unwrap().detail, + Some("(use bar::foobar)".to_string()), + ); + } + + #[test] + async fn test_auto_import_suggests_pub_use_for_function() { + let src = r#" + mod bar { + mod baz { + pub fn coco() {} + } + + pub use baz::coco as foobar; + } + + fn main() { + foob>|< + } + "#; + + let items = get_completions(src).await; + assert_eq!(items.len(), 1); + + let item = &items[0]; + assert_eq!(item.label, "foobar()"); + assert_eq!( + item.label_details.as_ref().unwrap().detail, + Some("(use bar::foobar)".to_string()), + ); + } } From c9e288cc78eecc0e3875e4464775b9a5350cde6d Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 28 Aug 2024 21:44:40 -0300 Subject: [PATCH 07/24] Always warn on unused imports regardless of package type --- compiler/noirc_driver/src/lib.rs | 28 +++++-------------------- tooling/lsp/src/notifications/mod.rs | 8 ++----- tooling/nargo/src/package.rs | 7 ------- tooling/nargo_cli/src/cli/check_cmd.rs | 11 ++++------ tooling/nargo_cli/src/cli/export_cmd.rs | 6 ++---- tooling/nargo_cli/src/cli/test_cmd.rs | 11 +++------- 6 files changed, 16 insertions(+), 55 deletions(-) diff --git a/compiler/noirc_driver/src/lib.rs b/compiler/noirc_driver/src/lib.rs index f95c9de7c2c..8e3004b8b9b 100644 --- a/compiler/noirc_driver/src/lib.rs +++ b/compiler/noirc_driver/src/lib.rs @@ -131,18 +131,6 @@ pub struct CompileOptions { pub skip_underconstrained_check: bool, } -#[derive(Clone, Debug, Default)] -pub struct CheckOptions { - pub compile_options: CompileOptions, - pub error_on_unused_imports: bool, -} - -impl CheckOptions { - pub fn new(compile_options: &CompileOptions, error_on_unused_imports: bool) -> Self { - Self { compile_options: compile_options.clone(), error_on_unused_imports } - } -} - pub fn parse_expression_width(input: &str) -> Result { use std::io::{Error, ErrorKind}; let width = input @@ -290,20 +278,19 @@ pub fn add_dep( pub fn check_crate( context: &mut Context, crate_id: CrateId, - check_options: &CheckOptions, + options: &CompileOptions, ) -> CompilationResult<()> { - let options = &check_options.compile_options; - let macros: &[&dyn MacroProcessor] = if options.disable_macros { &[] } else { &[&aztec_macros::AztecMacro] }; let mut errors = vec![]; + let error_on_unused_imports = true; let diagnostics = CrateDefMap::collect_defs( crate_id, context, options.debug_comptime_in_file.as_deref(), options.arithmetic_generics, - check_options.error_on_unused_imports, + error_on_unused_imports, macros, ); errors.extend(diagnostics.into_iter().map(|(error, file_id)| { @@ -337,10 +324,7 @@ pub fn compile_main( options: &CompileOptions, cached_program: Option, ) -> CompilationResult { - let error_on_unused_imports = true; - let check_options = CheckOptions::new(options, error_on_unused_imports); - - let (_, mut warnings) = check_crate(context, crate_id, &check_options)?; + let (_, mut warnings) = check_crate(context, crate_id, &options)?; let main = context.get_main_function(&crate_id).ok_or_else(|| { // TODO(#2155): This error might be a better to exist in Nargo @@ -375,9 +359,7 @@ pub fn compile_contract( crate_id: CrateId, options: &CompileOptions, ) -> CompilationResult { - let error_on_unused_imports = true; - let check_options = CheckOptions::new(options, error_on_unused_imports); - let (_, warnings) = check_crate(context, crate_id, &check_options)?; + let (_, warnings) = check_crate(context, crate_id, &options)?; // TODO: We probably want to error if contracts is empty let contracts = context.get_all_contracts(&crate_id); diff --git a/tooling/lsp/src/notifications/mod.rs b/tooling/lsp/src/notifications/mod.rs index 8b030c9e0aa..4d2186badc3 100644 --- a/tooling/lsp/src/notifications/mod.rs +++ b/tooling/lsp/src/notifications/mod.rs @@ -2,7 +2,7 @@ use std::ops::ControlFlow; use crate::insert_all_files_for_workspace_into_file_manager; use async_lsp::{ErrorCode, LanguageClient, ResponseError}; -use noirc_driver::{check_crate, file_manager_with_stdlib, CheckOptions}; +use noirc_driver::{check_crate, file_manager_with_stdlib}; use noirc_errors::{DiagnosticKind, FileDiagnostic}; use crate::types::{ @@ -132,11 +132,7 @@ pub(crate) fn process_workspace_for_noir_document( let (mut context, crate_id) = crate::prepare_package(&workspace_file_manager, &parsed_files, package); - let options = CheckOptions { - error_on_unused_imports: package.error_on_unused_imports(), - ..Default::default() - }; - let file_diagnostics = match check_crate(&mut context, crate_id, &options) { + let file_diagnostics = match check_crate(&mut context, crate_id, &Default::default()) { Ok(((), warnings)) => warnings, Err(errors_and_warnings) => errors_and_warnings, }; diff --git a/tooling/nargo/src/package.rs b/tooling/nargo/src/package.rs index cde616a9e32..f55ca5550a3 100644 --- a/tooling/nargo/src/package.rs +++ b/tooling/nargo/src/package.rs @@ -73,11 +73,4 @@ impl Package { pub fn is_library(&self) -> bool { self.package_type == PackageType::Library } - - pub fn error_on_unused_imports(&self) -> bool { - match self.package_type { - PackageType::Library => false, - PackageType::Binary | PackageType::Contract => true, - } - } } diff --git a/tooling/nargo_cli/src/cli/check_cmd.rs b/tooling/nargo_cli/src/cli/check_cmd.rs index 1130a82fdfc..9fab86583fa 100644 --- a/tooling/nargo_cli/src/cli/check_cmd.rs +++ b/tooling/nargo_cli/src/cli/check_cmd.rs @@ -10,7 +10,7 @@ use nargo::{ use nargo_toml::{get_package_manifest, resolve_workspace_from_toml, PackageSelection}; use noirc_abi::{AbiParameter, AbiType, MAIN_RETURN_NAME}; use noirc_driver::{ - check_crate, compute_function_abi, file_manager_with_stdlib, CheckOptions, CompileOptions, + check_crate, compute_function_abi, file_manager_with_stdlib, CompileOptions, NOIR_ARTIFACT_VERSION_STRING, }; use noirc_frontend::{ @@ -81,9 +81,7 @@ fn check_package( allow_overwrite: bool, ) -> Result { let (mut context, crate_id) = prepare_package(file_manager, parsed_files, package); - let error_on_unused_imports = package.error_on_unused_imports(); - let check_options = CheckOptions::new(compile_options, error_on_unused_imports); - check_crate_and_report_errors(&mut context, crate_id, &check_options)?; + check_crate_and_report_errors(&mut context, crate_id, &compile_options)?; if package.is_library() || package.is_contract() { // Libraries do not have ABIs while contracts have many, so we cannot generate a `Prover.toml` file. @@ -152,10 +150,9 @@ fn create_input_toml_template( pub(crate) fn check_crate_and_report_errors( context: &mut Context, crate_id: CrateId, - check_options: &CheckOptions, + options: &CompileOptions, ) -> Result<(), CompileError> { - let options = &check_options.compile_options; - let result = check_crate(context, crate_id, check_options); + let result = check_crate(context, crate_id, options); report_errors(result, &context.file_manager, options.deny_warnings, options.silence_warnings) } diff --git a/tooling/nargo_cli/src/cli/export_cmd.rs b/tooling/nargo_cli/src/cli/export_cmd.rs index 5721dd33e27..ad20a052fd3 100644 --- a/tooling/nargo_cli/src/cli/export_cmd.rs +++ b/tooling/nargo_cli/src/cli/export_cmd.rs @@ -12,7 +12,7 @@ use nargo::workspace::Workspace; use nargo::{insert_all_files_for_workspace_into_file_manager, parse_all}; use nargo_toml::{get_package_manifest, resolve_workspace_from_toml, PackageSelection}; use noirc_driver::{ - compile_no_check, file_manager_with_stdlib, CheckOptions, CompileOptions, CompiledProgram, + compile_no_check, file_manager_with_stdlib, CompileOptions, CompiledProgram, NOIR_ARTIFACT_VERSION_STRING, }; @@ -83,9 +83,7 @@ fn compile_exported_functions( compile_options: &CompileOptions, ) -> Result<(), CliError> { let (mut context, crate_id) = prepare_package(file_manager, parsed_files, package); - let error_on_unused_imports = package.error_on_unused_imports(); - let check_options = CheckOptions::new(compile_options, error_on_unused_imports); - check_crate_and_report_errors(&mut context, crate_id, &check_options)?; + check_crate_and_report_errors(&mut context, crate_id, &compile_options)?; let exported_functions = context.get_all_exported_functions_in_crate(&crate_id); diff --git a/tooling/nargo_cli/src/cli/test_cmd.rs b/tooling/nargo_cli/src/cli/test_cmd.rs index 2b0c0fd58db..8fc03c8e62d 100644 --- a/tooling/nargo_cli/src/cli/test_cmd.rs +++ b/tooling/nargo_cli/src/cli/test_cmd.rs @@ -10,8 +10,7 @@ use nargo::{ }; use nargo_toml::{get_package_manifest, resolve_workspace_from_toml, PackageSelection}; use noirc_driver::{ - check_crate, file_manager_with_stdlib, CheckOptions, CompileOptions, - NOIR_ARTIFACT_VERSION_STRING, + check_crate, file_manager_with_stdlib, CompileOptions, NOIR_ARTIFACT_VERSION_STRING, }; use noirc_frontend::{ graph::CrateName, @@ -181,9 +180,7 @@ fn run_test + Default>( // We then need to construct a separate copy for each test. let (mut context, crate_id) = prepare_package(file_manager, parsed_files, package); - let error_on_unused_imports = package.error_on_unused_imports(); - let check_options = CheckOptions::new(compile_options, error_on_unused_imports); - check_crate(&mut context, crate_id, &check_options) + check_crate(&mut context, crate_id, &compile_options) .expect("Any errors should have occurred when collecting test functions"); let test_functions = context @@ -212,9 +209,7 @@ fn get_tests_in_package( options: &CompileOptions, ) -> Result, CliError> { let (mut context, crate_id) = prepare_package(file_manager, parsed_files, package); - let error_on_unused_imports = package.error_on_unused_imports(); - let check_options = CheckOptions::new(options, error_on_unused_imports); - check_crate_and_report_errors(&mut context, crate_id, &check_options)?; + check_crate_and_report_errors(&mut context, crate_id, &options)?; Ok(context .get_all_test_functions_in_crate_matching(&crate_id, fn_name) From f32b08d5de0611d51b1874a951f49bf2a116e149 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 28 Aug 2024 21:47:56 -0300 Subject: [PATCH 08/24] Clippy --- compiler/noirc_driver/src/lib.rs | 4 ++-- compiler/noirc_frontend/src/ast/mod.rs | 4 ++-- compiler/noirc_frontend/src/locations.rs | 1 + tooling/nargo_cli/src/cli/check_cmd.rs | 2 +- tooling/nargo_cli/src/cli/export_cmd.rs | 2 +- tooling/nargo_cli/src/cli/test_cmd.rs | 4 ++-- 6 files changed, 9 insertions(+), 8 deletions(-) diff --git a/compiler/noirc_driver/src/lib.rs b/compiler/noirc_driver/src/lib.rs index 8e3004b8b9b..159f210cfe5 100644 --- a/compiler/noirc_driver/src/lib.rs +++ b/compiler/noirc_driver/src/lib.rs @@ -324,7 +324,7 @@ pub fn compile_main( options: &CompileOptions, cached_program: Option, ) -> CompilationResult { - let (_, mut warnings) = check_crate(context, crate_id, &options)?; + let (_, mut warnings) = check_crate(context, crate_id, options)?; let main = context.get_main_function(&crate_id).ok_or_else(|| { // TODO(#2155): This error might be a better to exist in Nargo @@ -359,7 +359,7 @@ pub fn compile_contract( crate_id: CrateId, options: &CompileOptions, ) -> CompilationResult { - let (_, warnings) = check_crate(context, crate_id, &options)?; + let (_, warnings) = check_crate(context, crate_id, options)?; // TODO: We probably want to error if contracts is empty let contracts = context.get_all_contracts(&crate_id); diff --git a/compiler/noirc_frontend/src/ast/mod.rs b/compiler/noirc_frontend/src/ast/mod.rs index 23dd9e1d670..a4633f9432a 100644 --- a/compiler/noirc_frontend/src/ast/mod.rs +++ b/compiler/noirc_frontend/src/ast/mod.rs @@ -463,9 +463,9 @@ pub enum ItemVisibility { impl std::fmt::Display for ItemVisibility { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { - ItemVisibility::Public => write!(f, "{}", "pub"), + ItemVisibility::Public => write!(f, "pub"), ItemVisibility::Private => Ok(()), - ItemVisibility::PublicCrate => write!(f, "{}", "pub(crate)"), + ItemVisibility::PublicCrate => write!(f, "pub(crate)"), } } } diff --git a/compiler/noirc_frontend/src/locations.rs b/compiler/noirc_frontend/src/locations.rs index ab85a76372e..8897b75ed47 100644 --- a/compiler/noirc_frontend/src/locations.rs +++ b/compiler/noirc_frontend/src/locations.rs @@ -360,6 +360,7 @@ impl NodeInterner { )); } + #[allow(clippy::type_complexity)] pub fn get_auto_import_names( &self, ) -> &HashMap)>> { diff --git a/tooling/nargo_cli/src/cli/check_cmd.rs b/tooling/nargo_cli/src/cli/check_cmd.rs index 9fab86583fa..5239070b4d2 100644 --- a/tooling/nargo_cli/src/cli/check_cmd.rs +++ b/tooling/nargo_cli/src/cli/check_cmd.rs @@ -81,7 +81,7 @@ fn check_package( allow_overwrite: bool, ) -> Result { let (mut context, crate_id) = prepare_package(file_manager, parsed_files, package); - check_crate_and_report_errors(&mut context, crate_id, &compile_options)?; + check_crate_and_report_errors(&mut context, crate_id, compile_options)?; if package.is_library() || package.is_contract() { // Libraries do not have ABIs while contracts have many, so we cannot generate a `Prover.toml` file. diff --git a/tooling/nargo_cli/src/cli/export_cmd.rs b/tooling/nargo_cli/src/cli/export_cmd.rs index ad20a052fd3..19add7f30dc 100644 --- a/tooling/nargo_cli/src/cli/export_cmd.rs +++ b/tooling/nargo_cli/src/cli/export_cmd.rs @@ -83,7 +83,7 @@ fn compile_exported_functions( compile_options: &CompileOptions, ) -> Result<(), CliError> { let (mut context, crate_id) = prepare_package(file_manager, parsed_files, package); - check_crate_and_report_errors(&mut context, crate_id, &compile_options)?; + check_crate_and_report_errors(&mut context, crate_id, compile_options)?; let exported_functions = context.get_all_exported_functions_in_crate(&crate_id); diff --git a/tooling/nargo_cli/src/cli/test_cmd.rs b/tooling/nargo_cli/src/cli/test_cmd.rs index 8fc03c8e62d..4e65ee72795 100644 --- a/tooling/nargo_cli/src/cli/test_cmd.rs +++ b/tooling/nargo_cli/src/cli/test_cmd.rs @@ -180,7 +180,7 @@ fn run_test + Default>( // We then need to construct a separate copy for each test. let (mut context, crate_id) = prepare_package(file_manager, parsed_files, package); - check_crate(&mut context, crate_id, &compile_options) + check_crate(&mut context, crate_id, compile_options) .expect("Any errors should have occurred when collecting test functions"); let test_functions = context @@ -209,7 +209,7 @@ fn get_tests_in_package( options: &CompileOptions, ) -> Result, CliError> { let (mut context, crate_id) = prepare_package(file_manager, parsed_files, package); - check_crate_and_report_errors(&mut context, crate_id, &options)?; + check_crate_and_report_errors(&mut context, crate_id, options)?; Ok(context .get_all_test_functions_in_crate_matching(&crate_id, fn_name) From 18af4e46b9127f753487d0a5ff306e635d3b86fa Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 29 Aug 2024 07:16:00 -0300 Subject: [PATCH 09/24] Remove unused imports in the std, and add a couple of `pub use` --- noir_stdlib/src/array.nr | 1 - noir_stdlib/src/collections/map.nr | 1 - noir_stdlib/src/collections/umap.nr | 5 +---- noir_stdlib/src/ec/consts/te.nr | 1 - noir_stdlib/src/eddsa.nr | 3 +-- noir_stdlib/src/field/bn254.nr | 2 +- noir_stdlib/src/hash/poseidon/bn254.nr | 2 +- noir_stdlib/src/hash/poseidon/bn254/perm.nr | 1 - noir_stdlib/src/sha256.nr | 2 +- noir_stdlib/src/sha512.nr | 2 +- noir_stdlib/src/uint128.nr | 1 - 11 files changed, 6 insertions(+), 15 deletions(-) diff --git a/noir_stdlib/src/array.nr b/noir_stdlib/src/array.nr index 23683a54e45..68e134b56fa 100644 --- a/noir_stdlib/src/array.nr +++ b/noir_stdlib/src/array.nr @@ -1,5 +1,4 @@ use crate::cmp::Ord; -use crate::option::Option; use crate::convert::From; impl [T; N] { diff --git a/noir_stdlib/src/collections/map.nr b/noir_stdlib/src/collections/map.nr index 4607b06d667..27a7d0d3550 100644 --- a/noir_stdlib/src/collections/map.nr +++ b/noir_stdlib/src/collections/map.nr @@ -1,5 +1,4 @@ use crate::cmp::Eq; -use crate::collections::vec::Vec; use crate::option::Option; use crate::default::Default; use crate::hash::{Hash, Hasher, BuildHasher}; diff --git a/noir_stdlib/src/collections/umap.nr b/noir_stdlib/src/collections/umap.nr index c552c053a92..c71905e63b3 100644 --- a/noir_stdlib/src/collections/umap.nr +++ b/noir_stdlib/src/collections/umap.nr @@ -1,10 +1,7 @@ use crate::cmp::Eq; -use crate::collections::vec::Vec; use crate::option::Option; use crate::default::Default; -use crate::hash::{Hash, Hasher, BuildHasher, BuildHasherDefault}; -use crate::hash::poseidon2::Poseidon2; -use crate::collections::bounded_vec::BoundedVec; +use crate::hash::{Hash, Hasher, BuildHasher}; // An unconstrained hash table with open addressing and quadratic probing. // Note that "unconstrained" here means that almost all operations on this diff --git a/noir_stdlib/src/ec/consts/te.nr b/noir_stdlib/src/ec/consts/te.nr index e25f373593a..8cea7654e39 100644 --- a/noir_stdlib/src/ec/consts/te.nr +++ b/noir_stdlib/src/ec/consts/te.nr @@ -1,4 +1,3 @@ -use crate::compat; use crate::ec::tecurve::affine::Point as TEPoint; use crate::ec::tecurve::affine::Curve as TECurve; diff --git a/noir_stdlib/src/eddsa.nr b/noir_stdlib/src/eddsa.nr index 337969be90e..cfdbbe9c3d0 100644 --- a/noir_stdlib/src/eddsa.nr +++ b/noir_stdlib/src/eddsa.nr @@ -1,7 +1,6 @@ -use crate::hash::poseidon; use crate::ec::consts::te::baby_jubjub; use crate::ec::tecurve::affine::Point as TEPoint; -use crate::hash::{Hash, Hasher, BuildHasher, BuildHasherDefault}; +use crate::hash::Hasher; use crate::hash::poseidon::PoseidonHasher; use crate::default::Default; diff --git a/noir_stdlib/src/field/bn254.nr b/noir_stdlib/src/field/bn254.nr index ed0053694c7..9f9851a8604 100644 --- a/noir_stdlib/src/field/bn254.nr +++ b/noir_stdlib/src/field/bn254.nr @@ -142,7 +142,7 @@ pub fn lt(a: Field, b: Field) -> bool { mod tests { // TODO: Allow imports from "super" - use crate::field::bn254::{decompose_hint, decompose, compute_lt, assert_gt, gt, lt, TWO_POW_128, compute_lte, PLO, PHI}; + use crate::field::bn254::{decompose, compute_lt, assert_gt, gt, TWO_POW_128, compute_lte, PLO, PHI}; #[test] fn check_decompose() { diff --git a/noir_stdlib/src/hash/poseidon/bn254.nr b/noir_stdlib/src/hash/poseidon/bn254.nr index 103ab3d166a..848d561f755 100644 --- a/noir_stdlib/src/hash/poseidon/bn254.nr +++ b/noir_stdlib/src/hash/poseidon/bn254.nr @@ -2,7 +2,7 @@ mod perm; mod consts; -use crate::hash::poseidon::{PoseidonConfig, absorb}; +use crate::hash::poseidon::absorb; // Variable-length Poseidon-128 sponge as suggested in second bullet point of §3 of https://eprint.iacr.org/2019/458.pdf #[field(bn254)] diff --git a/noir_stdlib/src/hash/poseidon/bn254/perm.nr b/noir_stdlib/src/hash/poseidon/bn254/perm.nr index b7c022c5a00..b7dc05ef9de 100644 --- a/noir_stdlib/src/hash/poseidon/bn254/perm.nr +++ b/noir_stdlib/src/hash/poseidon/bn254/perm.nr @@ -1,7 +1,6 @@ // Instantiations of Poseidon permutation for the prime field of the same order as BN254 use crate::hash::poseidon::bn254::consts; use crate::hash::poseidon::permute; -use crate::hash::poseidon::PoseidonConfig; #[field(bn254)] pub fn x5_2(mut state: [Field; 2]) -> [Field; 2] { diff --git a/noir_stdlib/src/sha256.nr b/noir_stdlib/src/sha256.nr index c3e18b13e91..ce217f7a689 100644 --- a/noir_stdlib/src/sha256.nr +++ b/noir_stdlib/src/sha256.nr @@ -1,2 +1,2 @@ // This file is kept for backwards compatibility. -use crate::hash::sha256::{digest, sha256_var}; +pub use crate::hash::sha256::{digest, sha256_var}; diff --git a/noir_stdlib/src/sha512.nr b/noir_stdlib/src/sha512.nr index 1ddbf6e0ae1..b474e27b416 100644 --- a/noir_stdlib/src/sha512.nr +++ b/noir_stdlib/src/sha512.nr @@ -1,2 +1,2 @@ // This file is kept for backwards compatibility. -use crate::hash::sha512::digest; +pub use crate::hash::sha512::digest; diff --git a/noir_stdlib/src/uint128.nr b/noir_stdlib/src/uint128.nr index a6d980c7f33..e2d6d913287 100644 --- a/noir_stdlib/src/uint128.nr +++ b/noir_stdlib/src/uint128.nr @@ -1,6 +1,5 @@ use crate::ops::{Add, Sub, Mul, Div, Rem, Not, BitOr, BitAnd, BitXor, Shl, Shr}; use crate::cmp::{Eq, Ord, Ordering}; -use crate::println; global pow64 : Field = 18446744073709551616; //2^64; global pow63 : Field = 9223372036854775808; // 2^63; From 52111d737b056d99b1bd025b9f312d69b3d9c9fb Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 29 Aug 2024 07:43:32 -0300 Subject: [PATCH 10/24] Fix typo (unrelated to this PR, but why not) --- compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index a601e60f7cf..690a14ca04c 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -286,8 +286,8 @@ impl DefCollector { def_map.extern_prelude.insert(dep.as_name(), module_id); let location = dep_def_map[dep_def_root].location; - let attriutes = ModuleAttributes { name: dep.as_name(), location, parent: None }; - context.def_interner.add_module_attributes(module_id, attriutes); + let attributes = ModuleAttributes { name: dep.as_name(), location, parent: None }; + context.def_interner.add_module_attributes(module_id, attributes); } // At this point, all dependencies are resolved and type checked. From 531b7ebee9b5d7573b71ab7aa454b05e5a568584 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 29 Aug 2024 07:49:48 -0300 Subject: [PATCH 11/24] Try to reduce the number of lines --- compiler/noirc_frontend/src/ast/statement.rs | 8 ++--- .../src/hir/def_collector/dc_crate.rs | 6 ++-- compiler/noirc_frontend/src/locations.rs | 32 ++++++------------- tooling/nargo_fmt/src/visitor/item.rs | 7 ++-- 4 files changed, 17 insertions(+), 36 deletions(-) diff --git a/compiler/noirc_frontend/src/ast/statement.rs b/compiler/noirc_frontend/src/ast/statement.rs index 7150e676715..4a5a1d6c963 100644 --- a/compiler/noirc_frontend/src/ast/statement.rs +++ b/compiler/noirc_frontend/src/ast/statement.rs @@ -354,10 +354,10 @@ impl UseTree { UseTreeKind::Path(name, alias) => { vec![ImportStatement { visibility, path: prefix.join(name), alias }] } - UseTreeKind::List(trees) => trees - .into_iter() - .flat_map(|tree| tree.desugar(Some(prefix.clone()), visibility)) - .collect(), + UseTreeKind::List(trees) => { + let trees = trees.into_iter(); + trees.flat_map(|tree| tree.desugar(Some(prefix.clone()), visibility)).collect() + } } } } diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index 690a14ca04c..17c257113c8 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -367,10 +367,8 @@ impl DefCollector { .import(name.clone(), visibility, ns, is_prelude); if visibility != ItemVisibility::Private { - let defining_module = ModuleId { - krate: crate_id, - local_id: resolved_import.module_scope, - }; + let local_id = resolved_import.module_scope; + let defining_module = ModuleId { krate: crate_id, local_id }; context.def_interner.register_name_for_auto_import( name.to_string(), ns, diff --git a/compiler/noirc_frontend/src/locations.rs b/compiler/noirc_frontend/src/locations.rs index 8897b75ed47..58de235455c 100644 --- a/compiler/noirc_frontend/src/locations.rs +++ b/compiler/noirc_frontend/src/locations.rs @@ -278,12 +278,8 @@ impl NodeInterner { } pub(crate) fn register_module(&mut self, id: ModuleId, name: String) { - self.register_name_for_auto_import( - name, - ModuleDefId::ModuleId(id), - ItemVisibility::Public, - None, - ); + let visibility = ItemVisibility::Public; + self.register_name_for_auto_import(name, ModuleDefId::ModuleId(id), visibility, None); } pub(crate) fn register_global( @@ -313,12 +309,8 @@ impl NodeInterner { pub(crate) fn register_trait(&mut self, id: TraitId, name: String, parent_module_id: ModuleId) { self.add_definition_location(ReferenceId::Trait(id), Some(parent_module_id)); - self.register_name_for_auto_import( - name, - ModuleDefId::TraitId(id), - ItemVisibility::Public, - None, - ); + let visibility = ItemVisibility::Public; + self.register_name_for_auto_import(name, ModuleDefId::TraitId(id), visibility, None); } pub(crate) fn register_type_alias( @@ -334,12 +326,9 @@ impl NodeInterner { } pub(crate) fn register_function(&mut self, id: FuncId, func_def: &FunctionDefinition) { - self.register_name_for_auto_import( - func_def.name.0.contents.clone(), - ModuleDefId::FunctionId(id), - func_def.visibility, - None, - ); + let name = func_def.name.0.contents.clone(); + let id = ModuleDefId::FunctionId(id); + self.register_name_for_auto_import(name, id, func_def.visibility, None); } pub fn register_name_for_auto_import( @@ -353,11 +342,8 @@ impl NodeInterner { return; } - self.auto_import_names.entry(name).or_default().push(( - module_def_id, - visibility, - defining_module, - )); + let entry = self.auto_import_names.entry(name).or_default(); + entry.push((module_def_id, visibility, defining_module)); } #[allow(clippy::type_complexity)] diff --git a/tooling/nargo_fmt/src/visitor/item.rs b/tooling/nargo_fmt/src/visitor/item.rs index c084d8b4458..0e2d07f13d0 100644 --- a/tooling/nargo_fmt/src/visitor/item.rs +++ b/tooling/nargo_fmt/src/visitor/item.rs @@ -217,11 +217,8 @@ impl super::FmtVisitor<'_> { } } ItemKind::Import(use_tree, visibility) => { - let use_tree = UseTree::from_ast(use_tree).rewrite_top_level( - self, - self.shape(), - visibility, - ); + let use_tree = UseTree::from_ast(use_tree); + let use_tree = use_tree.rewrite_top_level(self, self.shape(), visibility); self.push_rewrite(use_tree, span); self.last_position = span.end(); } From af1184887fd72d519673d8810d76c534a0cf08e2 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 29 Aug 2024 11:07:57 -0300 Subject: [PATCH 12/24] Correctly report private errors where they happen Previously it seems we always reported them on the crate root, making it really hard to understand what was going on. --- .../src/hir/def_collector/dc_crate.rs | 10 ++-- .../src/hir/resolution/import.rs | 53 +++++++++++++------ compiler/noirc_frontend/src/tests.rs | 14 +++-- 3 files changed, 51 insertions(+), 26 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index 17c257113c8..6cd88b7ccda 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -350,10 +350,9 @@ impl DefCollector { match resolved_import { Ok(resolved_import) => { if let Some(error) = resolved_import.error { - errors.push(( - DefCollectorErrorKind::PathResolutionError(error).into(), - root_file_id, - )); + let file = error.file(); + errors + .push((DefCollectorErrorKind::PathResolutionError(error).into(), file)); } // Populate module namespaces according to the imports used @@ -396,8 +395,7 @@ impl DefCollector { } } Err(error) => { - let current_def_map = context.def_maps.get(&crate_id).unwrap(); - let file_id = current_def_map.file_id(collected_import.module_id); + let file_id = error.file(); let error = DefCollectorErrorKind::PathResolutionError(error); errors.push((error.into(), file_id)); } diff --git a/compiler/noirc_frontend/src/hir/resolution/import.rs b/compiler/noirc_frontend/src/hir/resolution/import.rs index 814f649fe0a..5883a36629c 100644 --- a/compiler/noirc_frontend/src/hir/resolution/import.rs +++ b/compiler/noirc_frontend/src/hir/resolution/import.rs @@ -1,3 +1,4 @@ +use fm::FileId; use noirc_errors::{CustomDiagnostic, Span}; use thiserror::Error; @@ -39,11 +40,21 @@ pub(crate) type PathResolutionResult = Result FileId { + match self { + PathResolutionError::Unresolved(_, file) => *file, + PathResolutionError::Private(_, file) => *file, + PathResolutionError::NoSuper(_, file) => *file, + } + } } #[derive(Debug)] @@ -67,16 +78,16 @@ impl From for CompilationError { impl<'a> From<&'a PathResolutionError> for CustomDiagnostic { fn from(error: &'a PathResolutionError) -> Self { match &error { - PathResolutionError::Unresolved(ident) => { + PathResolutionError::Unresolved(ident, _) => { CustomDiagnostic::simple_error(error.to_string(), String::new(), ident.span()) } // This will be upgraded to an error in future versions - PathResolutionError::Private(ident) => CustomDiagnostic::simple_warning( + PathResolutionError::Private(ident, _) => CustomDiagnostic::simple_warning( error.to_string(), format!("{ident} is private"), ident.span(), ), - PathResolutionError::NoSuper(span) => { + PathResolutionError::NoSuper(span, _) => { CustomDiagnostic::simple_error(error.to_string(), String::new(), *span) } } @@ -114,7 +125,8 @@ pub fn resolve_import( ) { None } else { - Some(PathResolutionError::Private(name.clone())) + let file = def_maps[&crate_id].modules()[import_directive.module_id.0].location.file; + Some(PathResolutionError::Private(name.clone(), file)) } }); @@ -198,9 +210,8 @@ fn resolve_path_to_ns( ), crate::ast::PathKind::Super => { - if let Some(parent_module_id) = - def_maps[&crate_id].modules[import_directive.module_id.0].parent - { + let current_mod = &def_map.modules[import_directive.module_id.0]; + if let Some(parent_module_id) = current_mod.parent { resolve_name_in_module( crate_id, importing_crate, @@ -213,7 +224,7 @@ fn resolve_path_to_ns( } 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)) + Err(PathResolutionError::NoSuper(span, current_mod.location.file)) } } } @@ -265,7 +276,10 @@ fn resolve_name_in_module( let first_segment = &import_path.first().expect("ice: could not fetch first segment").ident; let mut current_ns = current_mod.find_name(first_segment); if current_ns.is_none() { - return Err(PathResolutionError::Unresolved(first_segment.clone())); + return Err(PathResolutionError::Unresolved( + first_segment.clone(), + current_mod.location.file, + )); } let mut warning: Option = None; @@ -276,7 +290,10 @@ fn resolve_name_in_module( let current_segment = ¤t_segment.ident; let (typ, visibility) = match current_ns.types { - None => return Err(PathResolutionError::Unresolved(last_segment.clone())), + None => { + let file = current_mod.location.file; + return Err(PathResolutionError::Unresolved(last_segment.clone(), file)); + } Some((typ, visibility, _)) => (typ, visibility), }; @@ -320,7 +337,8 @@ fn resolve_name_in_module( { None } else { - Some(PathResolutionError::Private(last_segment.clone())) + let file = current_mod.location.file; + Some(PathResolutionError::Private(last_segment.clone(), file)) } }); @@ -330,7 +348,8 @@ fn resolve_name_in_module( let found_ns = current_mod.find_name(current_segment); if found_ns.is_none() { - return Err(PathResolutionError::Unresolved(current_segment.clone())); + let file = current_mod.location.file; + return Err(PathResolutionError::Unresolved(current_segment.clone(), file)); } current_ns = found_ns; @@ -355,13 +374,15 @@ fn resolve_external_dep( ) -> NamespaceResolutionResult { // Use extern_prelude to get the dep let path = &directive.path.segments; + let current_mod = &def_maps[&importing_crate].modules()[directive.module_id.0]; + let file = current_mod.location.file; // Fetch the root module from the prelude let crate_name = &path.first().unwrap().ident; let dep_module = current_def_map .extern_prelude .get(&crate_name.0.contents) - .ok_or_else(|| PathResolutionError::Unresolved(crate_name.to_owned()))?; + .ok_or_else(|| PathResolutionError::Unresolved(crate_name.to_owned(), file))?; // Create an import directive for the dependency crate // XXX: This will panic if the path is of the form `use std`. Ideal algorithm will not distinguish between crate and module diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index 870c781b89d..c1786a55e88 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -514,7 +514,7 @@ fn check_trait_wrong_parameter_type() { for (err, _file_id) in errors { match &err { CompilationError::ResolverError(ResolverError::PathResolutionError( - PathResolutionError::Unresolved(ident), + PathResolutionError::Unresolved(ident, _), )) => { assert_eq!(ident, "NotAType"); } @@ -961,7 +961,10 @@ fn unresolved_path() { match compilation_error { CompilationError::ResolverError(err) => { match err { - ResolverError::PathResolutionError(PathResolutionError::Unresolved(name)) => { + ResolverError::PathResolutionError(PathResolutionError::Unresolved( + name, + _, + )) => { assert_eq!(name.to_string(), "some"); } _ => unimplemented!("we should only have an unresolved function"), @@ -1009,7 +1012,10 @@ fn multiple_resolution_errors() { ResolverError::VariableNotDeclared { name, .. } => { assert_eq!(name, "a"); } - ResolverError::PathResolutionError(PathResolutionError::Unresolved(name)) => { + ResolverError::PathResolutionError(PathResolutionError::Unresolved( + name, + _, + )) => { assert_eq!(name.to_string(), "foo"); } _ => unimplemented!(), @@ -2460,7 +2466,7 @@ fn no_super() { assert_eq!(errors.len(), 1); let CompilationError::DefinitionError(DefCollectorErrorKind::PathResolutionError( - PathResolutionError::NoSuper(span), + PathResolutionError::NoSuper(span, _), )) = &errors[0].0 else { panic!("Expected a 'no super' error, got {:?}", errors[0].0); From 2ae38cb9c1beac0cc1637025aedd4b376d9e8eb9 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 29 Aug 2024 11:40:46 -0300 Subject: [PATCH 13/24] Warn if re-exporting item with less visibility --- compiler/noirc_frontend/src/ast/mod.rs | 4 +-- .../src/hir/def_collector/dc_crate.rs | 36 +++++++++++++++---- .../src/hir/def_collector/errors.rs | 10 +++++- 3 files changed, 41 insertions(+), 9 deletions(-) diff --git a/compiler/noirc_frontend/src/ast/mod.rs b/compiler/noirc_frontend/src/ast/mod.rs index a4633f9432a..7f811a52b69 100644 --- a/compiler/noirc_frontend/src/ast/mod.rs +++ b/compiler/noirc_frontend/src/ast/mod.rs @@ -452,12 +452,12 @@ impl UnresolvedTypeExpression { } } -#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] +#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, PartialOrd, Ord)] /// Represents whether the definition can be referenced outside its module/crate pub enum ItemVisibility { - Public, Private, PublicCrate, + Public, } impl std::fmt::Display for ItemVisibility { diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index 6cd88b7ccda..8b56053aa39 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -357,31 +357,55 @@ impl DefCollector { // Populate module namespaces according to the imports used let current_def_map = context.def_maps.get_mut(&crate_id).unwrap(); + let file_id = current_def_map.file_id(module_id); let name = resolved_import.name; let visibility = collected_import.visibility; let is_prelude = resolved_import.is_prelude; - for ns in resolved_import.resolved_namespace.iter_defs() { + for (module_def_id, item_visibility, _) in + resolved_import.resolved_namespace.iter_items() + { + if item_visibility < visibility { + errors.push(( + DefCollectorErrorKind::CannotReexportItemWithLessVisibility { + item_name: name.clone(), + desired_visibility: visibility, + } + .into(), + file_id, + )); + } + let visibility = visibility.min(item_visibility); + let result = current_def_map.modules[resolved_import.module_scope.0] - .import(name.clone(), visibility, ns, is_prelude); + .import(name.clone(), visibility, module_def_id, is_prelude); if visibility != ItemVisibility::Private { let local_id = resolved_import.module_scope; let defining_module = ModuleId { krate: crate_id, local_id }; context.def_interner.register_name_for_auto_import( name.to_string(), - ns, + module_def_id, visibility, Some(defining_module), ); } - let file_id = current_def_map.file_id(module_id); let last_segment = collected_import.path.last_ident(); - add_import_reference(ns, &last_segment, &mut context.def_interner, file_id); + add_import_reference( + module_def_id, + &last_segment, + &mut context.def_interner, + file_id, + ); if let Some(ref alias) = collected_import.alias { - add_import_reference(ns, alias, &mut context.def_interner, file_id); + add_import_reference( + module_def_id, + alias, + &mut context.def_interner, + file_id, + ); } if let Err((first_def, second_def)) = result { diff --git a/compiler/noirc_frontend/src/hir/def_collector/errors.rs b/compiler/noirc_frontend/src/hir/def_collector/errors.rs index e705d7b6fad..f931a7cdf41 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/errors.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/errors.rs @@ -1,4 +1,4 @@ -use crate::ast::{Ident, Path, UnresolvedTypeData}; +use crate::ast::{Ident, ItemVisibility, Path, UnresolvedTypeData}; use crate::hir::resolution::import::PathResolutionError; use crate::hir::type_check::generics::TraitGenerics; @@ -35,6 +35,8 @@ pub enum DefCollectorErrorKind { OverlappingModuleDecls { mod_name: Ident, expected_path: String, alternative_path: String }, #[error("path resolution error")] PathResolutionError(PathResolutionError), + #[error("cannot re-export {item_name} because it has less visibility than this use statement")] + CannotReexportItemWithLessVisibility { item_name: Ident, desired_visibility: ItemVisibility }, #[error("Non-struct type used in impl")] NonStructTypeInImpl { span: Span }, #[error("Cannot implement trait on a mutable reference type")] @@ -173,6 +175,12 @@ impl<'a> From<&'a DefCollectorErrorKind> for Diagnostic { ) } DefCollectorErrorKind::PathResolutionError(error) => error.into(), + DefCollectorErrorKind::CannotReexportItemWithLessVisibility{item_name, desired_visibility} => { + Diagnostic::simple_warning( + format!("cannot re-export {item_name} because it has less visibility than this use statement"), + format!("consider marking {item_name} as {desired_visibility}"), + item_name.span()) + } DefCollectorErrorKind::NonStructTypeInImpl { span } => Diagnostic::simple_error( "Non-struct type used in impl".into(), "Only struct types may have implementation methods".into(), From 691d5239402bd47c26ee769356db2720005c5384 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 29 Aug 2024 13:39:10 -0300 Subject: [PATCH 14/24] Revert "Correctly report private errors where they happen" This reverts commit af1184887fd72d519673d8810d76c534a0cf08e2. --- .../src/hir/def_collector/dc_crate.rs | 10 ++-- .../src/hir/resolution/import.rs | 53 ++++++------------- compiler/noirc_frontend/src/tests.rs | 14 ++--- 3 files changed, 26 insertions(+), 51 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index 8b56053aa39..1a8f8e07695 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -350,9 +350,10 @@ impl DefCollector { match resolved_import { Ok(resolved_import) => { if let Some(error) = resolved_import.error { - let file = error.file(); - errors - .push((DefCollectorErrorKind::PathResolutionError(error).into(), file)); + errors.push(( + DefCollectorErrorKind::PathResolutionError(error).into(), + root_file_id, + )); } // Populate module namespaces according to the imports used @@ -419,7 +420,8 @@ impl DefCollector { } } Err(error) => { - let file_id = error.file(); + let current_def_map = context.def_maps.get(&crate_id).unwrap(); + let file_id = current_def_map.file_id(collected_import.module_id); let error = DefCollectorErrorKind::PathResolutionError(error); errors.push((error.into(), file_id)); } diff --git a/compiler/noirc_frontend/src/hir/resolution/import.rs b/compiler/noirc_frontend/src/hir/resolution/import.rs index 5883a36629c..814f649fe0a 100644 --- a/compiler/noirc_frontend/src/hir/resolution/import.rs +++ b/compiler/noirc_frontend/src/hir/resolution/import.rs @@ -1,4 +1,3 @@ -use fm::FileId; use noirc_errors::{CustomDiagnostic, Span}; use thiserror::Error; @@ -40,21 +39,11 @@ pub(crate) type PathResolutionResult = Result FileId { - match self { - PathResolutionError::Unresolved(_, file) => *file, - PathResolutionError::Private(_, file) => *file, - PathResolutionError::NoSuper(_, file) => *file, - } - } + NoSuper(Span), } #[derive(Debug)] @@ -78,16 +67,16 @@ impl From for CompilationError { impl<'a> From<&'a PathResolutionError> for CustomDiagnostic { fn from(error: &'a PathResolutionError) -> Self { match &error { - PathResolutionError::Unresolved(ident, _) => { + PathResolutionError::Unresolved(ident) => { CustomDiagnostic::simple_error(error.to_string(), String::new(), ident.span()) } // This will be upgraded to an error in future versions - PathResolutionError::Private(ident, _) => CustomDiagnostic::simple_warning( + PathResolutionError::Private(ident) => CustomDiagnostic::simple_warning( error.to_string(), format!("{ident} is private"), ident.span(), ), - PathResolutionError::NoSuper(span, _) => { + PathResolutionError::NoSuper(span) => { CustomDiagnostic::simple_error(error.to_string(), String::new(), *span) } } @@ -125,8 +114,7 @@ pub fn resolve_import( ) { None } else { - let file = def_maps[&crate_id].modules()[import_directive.module_id.0].location.file; - Some(PathResolutionError::Private(name.clone(), file)) + Some(PathResolutionError::Private(name.clone())) } }); @@ -210,8 +198,9 @@ fn resolve_path_to_ns( ), crate::ast::PathKind::Super => { - let current_mod = &def_map.modules[import_directive.module_id.0]; - if let Some(parent_module_id) = current_mod.parent { + 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, @@ -224,7 +213,7 @@ fn resolve_path_to_ns( } 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, current_mod.location.file)) + Err(PathResolutionError::NoSuper(span)) } } } @@ -276,10 +265,7 @@ fn resolve_name_in_module( let first_segment = &import_path.first().expect("ice: could not fetch first segment").ident; let mut current_ns = current_mod.find_name(first_segment); if current_ns.is_none() { - return Err(PathResolutionError::Unresolved( - first_segment.clone(), - current_mod.location.file, - )); + return Err(PathResolutionError::Unresolved(first_segment.clone())); } let mut warning: Option = None; @@ -290,10 +276,7 @@ fn resolve_name_in_module( let current_segment = ¤t_segment.ident; let (typ, visibility) = match current_ns.types { - None => { - let file = current_mod.location.file; - return Err(PathResolutionError::Unresolved(last_segment.clone(), file)); - } + None => return Err(PathResolutionError::Unresolved(last_segment.clone())), Some((typ, visibility, _)) => (typ, visibility), }; @@ -337,8 +320,7 @@ fn resolve_name_in_module( { None } else { - let file = current_mod.location.file; - Some(PathResolutionError::Private(last_segment.clone(), file)) + Some(PathResolutionError::Private(last_segment.clone())) } }); @@ -348,8 +330,7 @@ fn resolve_name_in_module( let found_ns = current_mod.find_name(current_segment); if found_ns.is_none() { - let file = current_mod.location.file; - return Err(PathResolutionError::Unresolved(current_segment.clone(), file)); + return Err(PathResolutionError::Unresolved(current_segment.clone())); } current_ns = found_ns; @@ -374,15 +355,13 @@ fn resolve_external_dep( ) -> NamespaceResolutionResult { // Use extern_prelude to get the dep let path = &directive.path.segments; - let current_mod = &def_maps[&importing_crate].modules()[directive.module_id.0]; - let file = current_mod.location.file; // Fetch the root module from the prelude let crate_name = &path.first().unwrap().ident; let dep_module = current_def_map .extern_prelude .get(&crate_name.0.contents) - .ok_or_else(|| PathResolutionError::Unresolved(crate_name.to_owned(), file))?; + .ok_or_else(|| PathResolutionError::Unresolved(crate_name.to_owned()))?; // Create an import directive for the dependency crate // XXX: This will panic if the path is of the form `use std`. Ideal algorithm will not distinguish between crate and module diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index c1786a55e88..870c781b89d 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -514,7 +514,7 @@ fn check_trait_wrong_parameter_type() { for (err, _file_id) in errors { match &err { CompilationError::ResolverError(ResolverError::PathResolutionError( - PathResolutionError::Unresolved(ident, _), + PathResolutionError::Unresolved(ident), )) => { assert_eq!(ident, "NotAType"); } @@ -961,10 +961,7 @@ fn unresolved_path() { match compilation_error { CompilationError::ResolverError(err) => { match err { - ResolverError::PathResolutionError(PathResolutionError::Unresolved( - name, - _, - )) => { + ResolverError::PathResolutionError(PathResolutionError::Unresolved(name)) => { assert_eq!(name.to_string(), "some"); } _ => unimplemented!("we should only have an unresolved function"), @@ -1012,10 +1009,7 @@ fn multiple_resolution_errors() { ResolverError::VariableNotDeclared { name, .. } => { assert_eq!(name, "a"); } - ResolverError::PathResolutionError(PathResolutionError::Unresolved( - name, - _, - )) => { + ResolverError::PathResolutionError(PathResolutionError::Unresolved(name)) => { assert_eq!(name.to_string(), "foo"); } _ => unimplemented!(), @@ -2466,7 +2460,7 @@ fn no_super() { assert_eq!(errors.len(), 1); let CompilationError::DefinitionError(DefCollectorErrorKind::PathResolutionError( - PathResolutionError::NoSuper(span, _), + PathResolutionError::NoSuper(span), )) = &errors[0].0 else { panic!("Expected a 'no super' error, got {:?}", errors[0].0); From 2820949d5fdb309c43866f2729faebf78acc903b Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 29 Aug 2024 13:42:39 -0300 Subject: [PATCH 15/24] Error on correct file --- compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index 1a8f8e07695..11fd668a852 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -349,17 +349,17 @@ impl DefCollector { }; match resolved_import { Ok(resolved_import) => { + let current_def_map = context.def_maps.get_mut(&crate_id).unwrap(); + let file_id = current_def_map.file_id(module_id); + if let Some(error) = resolved_import.error { errors.push(( DefCollectorErrorKind::PathResolutionError(error).into(), - root_file_id, + file_id, )); } // Populate module namespaces according to the imports used - let current_def_map = context.def_maps.get_mut(&crate_id).unwrap(); - let file_id = current_def_map.file_id(module_id); - let name = resolved_import.name; let visibility = collected_import.visibility; let is_prelude = resolved_import.is_prelude; From 79b507f30a13ef509442c7d126989d26da7d7c9f Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 29 Aug 2024 14:04:34 -0300 Subject: [PATCH 16/24] Add a few tests --- compiler/noirc_frontend/src/tests.rs | 71 ++++++++++++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index 870c781b89d..ddd595d4c70 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -3231,3 +3231,74 @@ fn errors_on_unused_import() { assert_eq!(ident.to_string(), "bar"); } + +#[test] +fn warns_on_use_of_private_exported_item() { + let src = r#" + mod foo { + mod bar { + pub fn baz() {} + } + + use bar::baz; + } + + fn main() { + foo::baz(); + } + "#; + + let errors = get_program_errors(src); + assert_eq!(errors.len(), 2); // An existing bug causes this error to be duplicated + + assert!(matches!( + &errors[0].0, + CompilationError::ResolverError(ResolverError::PathResolutionError( + PathResolutionError::Private(..), + )) + )); +} + +#[test] +fn can_use_pub_use_item() { + let src = r#" + mod foo { + mod bar { + pub fn baz() {} + } + + pub use bar::baz; + } + + fn main() { + foo::baz(); + } + "#; + assert_no_errors(src); +} + +#[test] +fn warns_on_re_export_of_item_with_less_visibility() { + let src = r#" + mod foo { + mod bar { + pub(crate) fn baz() {} + } + + pub use bar::baz; + } + + fn main() { + } + "#; + + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + + assert!(matches!( + &errors[0].0, + CompilationError::DefinitionError( + DefCollectorErrorKind::CannotReexportItemWithLessVisibility { .. } + ) + )); +} From e922bd9a5c8fa8c445f7dda5161419df0cf4c937 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 29 Aug 2024 14:42:35 -0300 Subject: [PATCH 17/24] Fix test --- compiler/noirc_frontend/src/tests.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index ddd595d4c70..18d2845b761 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -3241,6 +3241,10 @@ fn warns_on_use_of_private_exported_item() { } use bar::baz; + + fn qux() { + baz(); + } } fn main() { From 1b4bb9ed60c23305ea62b5c8b3048e2c2c9aa110 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 2 Sep 2024 08:26:16 -0300 Subject: [PATCH 18/24] Also warn on unused pub(crate) use --- .../src/hir/def_map/module_data.rs | 2 +- compiler/noirc_frontend/src/tests.rs | 36 ++++++++++++++++++- 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/def_map/module_data.rs b/compiler/noirc_frontend/src/hir/def_map/module_data.rs index 0e62c518895..bf10447e9c8 100644 --- a/compiler/noirc_frontend/src/hir/def_map/module_data.rs +++ b/compiler/noirc_frontend/src/hir/def_map/module_data.rs @@ -128,7 +128,7 @@ impl ModuleData { is_prelude: bool, ) -> Result<(), (Ident, Ident)> { // Empty spans could come from implicitly injected imports, and we don't want to track those - if visibility == ItemVisibility::Private && name.span().start() < name.span().end() { + if visibility != ItemVisibility::Public && name.span().start() < name.span().end() { self.unused_imports.insert(name.clone()); } diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index 18d2845b761..c7dec8a33f8 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -3199,7 +3199,7 @@ fn as_trait_path_syntax_no_impl() { } #[test] -fn errors_on_unused_import() { +fn errors_on_unused_private_import() { let src = r#" mod foo { pub fn bar() {} @@ -3232,6 +3232,40 @@ fn errors_on_unused_import() { assert_eq!(ident.to_string(), "bar"); } +#[test] +fn errors_on_unused_pub_crate_import() { + let src = r#" + mod foo { + pub fn bar() {} + pub fn baz() {} + + trait Foo { + } + } + + pub(crate) use foo::bar; + use foo::baz; + use foo::Foo; + + impl Foo for Field { + } + + fn main() { + baz(); + } + "#; + + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + + let CompilationError::ResolverError(ResolverError::UnusedImport { ident }) = &errors[0].0 + else { + panic!("Expected an unused import error"); + }; + + assert_eq!(ident.to_string(), "bar"); +} + #[test] fn warns_on_use_of_private_exported_item() { let src = r#" From 13f6b75b594a935f814b55268de9e86419098169 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 2 Sep 2024 09:31:23 -0300 Subject: [PATCH 19/24] Update compiler/noirc_frontend/src/tests.rs Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> --- compiler/noirc_frontend/src/tests.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index c7dec8a33f8..a30907211a3 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -3327,6 +3327,7 @@ fn warns_on_re_export_of_item_with_less_visibility() { } fn main() { + foo::baz(); } "#; From 5be006b33facdae7bcafe101db1e22aa00ed21ed Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 2 Sep 2024 11:08:21 -0300 Subject: [PATCH 20/24] Mark used imports as we solve a path --- .../noirc_frontend/src/elaborator/scope.rs | 58 +++++++++---------- .../src/hir/def_collector/dc_crate.rs | 8 +-- .../src/hir/resolution/import.rs | 36 ++++++++---- .../src/hir/resolution/path_resolver.rs | 6 +- tooling/lsp/src/requests/completion.rs | 21 +------ 5 files changed, 62 insertions(+), 67 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/scope.rs b/compiler/noirc_frontend/src/elaborator/scope.rs index a51fd737f74..1f00d7aa457 100644 --- a/compiler/noirc_frontend/src/elaborator/scope.rs +++ b/compiler/noirc_frontend/src/elaborator/scope.rs @@ -60,12 +60,6 @@ impl<'context> Elaborator<'context> { let mut module_id = self.module_id(); let mut path = path; - if path.kind == PathKind::Plain { - let def_map = self.def_maps.get_mut(&self.crate_id).unwrap(); - let module_data = &mut def_map.modules[module_id.local_id.0]; - module_data.use_import(&path.segments[0].ident); - } - if path.kind == PathKind::Plain && path.first_name() == SELF_TYPE_NAME { if let Some(Type::Struct(struct_type, _)) = &self.self_type { let struct_type = struct_type.borrow(); @@ -90,34 +84,38 @@ impl<'context> Elaborator<'context> { fn resolve_path_in_module(&mut self, path: Path, module_id: ModuleId) -> PathResolutionResult { let resolver = StandardPathResolver::new(module_id); - let path_resolution; - - if self.interner.lsp_mode { - let last_segment = path.last_ident(); - let location = Location::new(last_segment.span(), self.file); - let is_self_type_name = last_segment.is_self_type_name(); - - let mut references: Vec<_> = Vec::new(); - path_resolution = - resolver.resolve(self.def_maps, path.clone(), &mut Some(&mut references))?; - - for (referenced, segment) in references.iter().zip(path.segments) { - self.interner.add_reference( - *referenced, - Location::new(segment.ident.span(), self.file), - segment.ident.is_self_type_name(), - ); - } - self.interner.add_module_def_id_reference( - path_resolution.module_def_id, - location, - is_self_type_name, + if !self.interner.lsp_mode { + return resolver.resolve(self.def_maps, path, &mut None); + } + + let last_segment = path.last_ident(); + let location = Location::new(last_segment.span(), self.file); + let is_self_type_name = last_segment.is_self_type_name(); + + let mut references: Vec<_> = Vec::new(); + let path_resolution = + resolver.resolve(self.def_maps, path.clone(), &mut Some(&mut references)); + + for (referenced, segment) in references.iter().zip(path.segments) { + self.interner.add_reference( + *referenced, + Location::new(segment.ident.span(), self.file), + segment.ident.is_self_type_name(), ); - } else { - path_resolution = resolver.resolve(self.def_maps, path, &mut None)?; } + let path_resolution = match path_resolution { + Ok(path_resolution) => path_resolution, + Err(err) => return Err(err), + }; + + self.interner.add_module_def_id_reference( + path_resolution.module_def_id, + location, + is_self_type_name, + ); + Ok(path_resolution) } diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index 11fd668a852..c4ad1846047 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -327,7 +327,7 @@ impl DefCollector { let resolved_import = resolve_import( crate_id, &collected_import, - &context.def_maps, + &mut context.def_maps, &mut Some(&mut references), ); @@ -345,7 +345,7 @@ impl DefCollector { resolved_import } else { - resolve_import(crate_id, &collected_import, &context.def_maps, &mut None) + resolve_import(crate_id, &collected_import, &mut context.def_maps, &mut None) }; match resolved_import { Ok(resolved_import) => { @@ -493,7 +493,7 @@ fn add_import_reference( fn inject_prelude( crate_id: CrateId, - context: &Context, + context: &mut Context, crate_root: LocalModuleId, collected_imports: &mut Vec, ) { @@ -515,7 +515,7 @@ fn inject_prelude( }; if let Ok(PathResolution { module_def_id, error }) = path_resolver::resolve_path( - &context.def_maps, + &mut context.def_maps, ModuleId { krate: crate_id, local_id: crate_root }, path, &mut None, diff --git a/compiler/noirc_frontend/src/hir/resolution/import.rs b/compiler/noirc_frontend/src/hir/resolution/import.rs index 814f649fe0a..350bb5c5815 100644 --- a/compiler/noirc_frontend/src/hir/resolution/import.rs +++ b/compiler/noirc_frontend/src/hir/resolution/import.rs @@ -86,7 +86,7 @@ impl<'a> From<&'a PathResolutionError> for CustomDiagnostic { pub fn resolve_import( crate_id: CrateId, import_directive: &ImportDirective, - def_maps: &BTreeMap, + def_maps: &mut BTreeMap, path_references: &mut Option<&mut Vec>, ) -> Result { let module_scope = import_directive.module_id; @@ -131,11 +131,10 @@ fn resolve_path_to_ns( import_directive: &ImportDirective, crate_id: CrateId, importing_crate: CrateId, - def_maps: &BTreeMap, + def_maps: &mut BTreeMap, path_references: &mut Option<&mut Vec>, ) -> NamespaceResolutionResult { let import_path = &import_directive.path.segments; - let def_map = &def_maps[&crate_id]; match import_directive.path.kind { crate::ast::PathKind::Crate => { @@ -163,6 +162,7 @@ fn resolve_path_to_ns( ); } + let def_map = &def_maps[&crate_id]; let current_mod_id = ModuleId { krate: crate_id, local_id: import_directive.module_id }; let current_mod = &def_map.modules[current_mod_id.local_id.0]; let first_segment = @@ -170,7 +170,8 @@ fn resolve_path_to_ns( if current_mod.find_name(first_segment).is_none() { // Resolve externally when first segment is unresolved return resolve_external_dep( - def_map, + crate_id, + // def_map, import_directive, def_maps, path_references, @@ -190,7 +191,7 @@ fn resolve_path_to_ns( } crate::ast::PathKind::Dep => resolve_external_dep( - def_map, + crate_id, import_directive, def_maps, path_references, @@ -224,7 +225,7 @@ fn resolve_path_from_crate_root( importing_crate: CrateId, import_path: &[PathSegment], - def_maps: &BTreeMap, + def_maps: &mut BTreeMap, path_references: &mut Option<&mut Vec>, ) -> NamespaceResolutionResult { let starting_mod = def_maps[&crate_id].root; @@ -244,13 +245,13 @@ fn resolve_name_in_module( importing_crate: CrateId, import_path: &[PathSegment], starting_mod: LocalModuleId, - def_maps: &BTreeMap, + def_maps: &mut BTreeMap, plain: bool, path_references: &mut Option<&mut Vec>, ) -> NamespaceResolutionResult { - let def_map = &def_maps[&krate]; + let def_map = def_maps.get_mut(&krate).unwrap(); let mut current_mod_id = ModuleId { krate, local_id: starting_mod }; - let mut current_mod = &def_map.modules[current_mod_id.local_id.0]; + let mut current_mod = def_map.modules.get_mut(current_mod_id.local_id.0).unwrap(); // There is a possibility that the import path is empty // In that case, early return @@ -268,6 +269,8 @@ fn resolve_name_in_module( return Err(PathResolutionError::Unresolved(first_segment.clone())); } + current_mod.use_import(first_segment); + let mut warning: Option = None; for (index, (last_segment, current_segment)) in import_path.iter().zip(import_path.iter().skip(1)).enumerate() @@ -324,7 +327,12 @@ fn resolve_name_in_module( } }); - current_mod = &def_maps[¤t_mod_id.krate].modules[current_mod_id.local_id.0]; + current_mod = def_maps + .get_mut(¤t_mod_id.krate) + .unwrap() + .modules + .get_mut(current_mod_id.local_id.0) + .unwrap(); // Check if namespace let found_ns = current_mod.find_name(current_segment); @@ -333,6 +341,8 @@ fn resolve_name_in_module( return Err(PathResolutionError::Unresolved(current_segment.clone())); } + current_mod.use_import(current_segment); + current_ns = found_ns; } @@ -347,15 +357,17 @@ fn resolve_path_name(import_directive: &ImportDirective) -> Ident { } fn resolve_external_dep( - current_def_map: &CrateDefMap, + crate_id: CrateId, directive: &ImportDirective, - def_maps: &BTreeMap, + def_maps: &mut BTreeMap, path_references: &mut Option<&mut Vec>, importing_crate: CrateId, ) -> NamespaceResolutionResult { // Use extern_prelude to get the dep let path = &directive.path.segments; + let current_def_map = &def_maps[&crate_id]; + // Fetch the root module from the prelude let crate_name = &path.first().unwrap().ident; let dep_module = current_def_map diff --git a/compiler/noirc_frontend/src/hir/resolution/path_resolver.rs b/compiler/noirc_frontend/src/hir/resolution/path_resolver.rs index 8d7dbfab4cd..396f269eab4 100644 --- a/compiler/noirc_frontend/src/hir/resolution/path_resolver.rs +++ b/compiler/noirc_frontend/src/hir/resolution/path_resolver.rs @@ -13,7 +13,7 @@ pub trait PathResolver { /// a module or type). fn resolve( &self, - def_maps: &BTreeMap, + def_maps: &mut BTreeMap, path: Path, path_references: &mut Option<&mut Vec>, ) -> PathResolutionResult; @@ -37,7 +37,7 @@ impl StandardPathResolver { impl PathResolver for StandardPathResolver { fn resolve( &self, - def_maps: &BTreeMap, + def_maps: &mut BTreeMap, path: Path, path_references: &mut Option<&mut Vec>, ) -> PathResolutionResult { @@ -56,7 +56,7 @@ impl PathResolver for StandardPathResolver { /// Resolve the given path to a function or a type. /// In the case of a conflict, functions are given priority pub fn resolve_path( - def_maps: &BTreeMap, + def_maps: &mut BTreeMap, module_id: ModuleId, path: Path, path_references: &mut Option<&mut Vec>, diff --git a/tooling/lsp/src/requests/completion.rs b/tooling/lsp/src/requests/completion.rs index f075599fe3b..987746d37ce 100644 --- a/tooling/lsp/src/requests/completion.rs +++ b/tooling/lsp/src/requests/completion.rs @@ -18,16 +18,13 @@ use noirc_frontend::{ AsTraitPath, BlockExpression, CallExpression, ConstructorExpression, Expression, ExpressionKind, ForLoopStatement, GenericTypeArgs, Ident, IfExpression, ItemVisibility, Lambda, LetStatement, MemberAccessExpression, MethodCallExpression, NoirFunction, - NoirStruct, NoirTraitImpl, Path, PathKind, PathSegment, Pattern, Statement, TypeImpl, - UnresolvedGeneric, UnresolvedGenerics, UnresolvedType, UseTree, UseTreeKind, Visitor, + NoirStruct, NoirTraitImpl, Path, PathKind, Pattern, Statement, TypeImpl, UnresolvedGeneric, + UnresolvedGenerics, UnresolvedType, UseTree, UseTreeKind, Visitor, }, graph::{CrateId, Dependency}, hir::{ def_map::{CrateDefMap, LocalModuleId, ModuleId}, - resolution::{ - import::can_reference_module_id, - path_resolver::{PathResolver, StandardPathResolver}, - }, + resolution::import::can_reference_module_id, }, hir_def::traits::Trait, macros_api::{ModuleDefId, NodeInterner}, @@ -92,8 +89,6 @@ struct NodeFinder<'a> { lines: Vec<&'a str>, byte_index: usize, byte: Option, - /// The module ID of the current file. - root_module_id: ModuleId, /// The module ID in scope. This might change as we traverse the AST /// if we are analyzing something inside an inline module declaration. module_id: ModuleId, @@ -131,7 +126,6 @@ impl<'a> NodeFinder<'a> { ) -> Self { // Find the module the current file belongs to let def_map = &def_maps[&krate]; - let root_module_id = ModuleId { krate, local_id: def_map.root() }; let local_id = if let Some((module_index, _)) = def_map.modules().iter().find(|(_, module_data)| module_data.location.file == file) { @@ -146,7 +140,6 @@ impl<'a> NodeFinder<'a> { lines: source.lines().collect(), byte_index, byte, - root_module_id, module_id, def_maps, dependencies, @@ -760,14 +753,6 @@ impl<'a> NodeFinder<'a> { fn resolve_path(&self, segments: Vec) -> Option { let last_segment = segments.last().unwrap().clone(); - let path_segments = segments.into_iter().map(PathSegment::from).collect(); - let path = Path { segments: path_segments, kind: PathKind::Plain, span: Span::default() }; - - let path_resolver = StandardPathResolver::new(self.root_module_id); - if let Ok(path_resolution) = path_resolver.resolve(self.def_maps, path, &mut None) { - return Some(path_resolution.module_def_id); - } - // If we can't resolve a path trough lookup, let's see if the last segment is bound to a type let location = Location::new(last_segment.span(), self.file); if let Some(reference_id) = self.interner.find_referenced(location) { From e336f568f5928332b12166b28861ea21a2a5fa15 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 2 Sep 2024 11:57:28 -0300 Subject: [PATCH 21/24] Track unused imports in NodeInterner --- .../noirc_frontend/src/elaborator/scope.rs | 15 ++++- .../src/hir/def_collector/dc_crate.rs | 54 +++++++++++++---- .../src/hir/def_map/module_data.rs | 22 +------ .../src/hir/resolution/import.rs | 59 +++++++++++++------ .../src/hir/resolution/path_resolver.rs | 19 +++--- compiler/noirc_frontend/src/node_interner.rs | 6 ++ 6 files changed, 116 insertions(+), 59 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/scope.rs b/compiler/noirc_frontend/src/elaborator/scope.rs index 1f00d7aa457..5128f8a7828 100644 --- a/compiler/noirc_frontend/src/elaborator/scope.rs +++ b/compiler/noirc_frontend/src/elaborator/scope.rs @@ -86,7 +86,12 @@ impl<'context> Elaborator<'context> { let resolver = StandardPathResolver::new(module_id); if !self.interner.lsp_mode { - return resolver.resolve(self.def_maps, path, &mut None); + return resolver.resolve( + self.def_maps, + path, + &mut self.interner.unused_imports, + &mut None, + ); } let last_segment = path.last_ident(); @@ -94,8 +99,12 @@ impl<'context> Elaborator<'context> { let is_self_type_name = last_segment.is_self_type_name(); let mut references: Vec<_> = Vec::new(); - let path_resolution = - resolver.resolve(self.def_maps, path.clone(), &mut Some(&mut references)); + let path_resolution = resolver.resolve( + self.def_maps, + path.clone(), + &mut self.interner.unused_imports, + &mut Some(&mut references), + ); for (referenced, segment) in references.iter().zip(path.segments) { self.interner.add_reference( diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index c4ad1846047..e2f0dc98998 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -327,7 +327,8 @@ impl DefCollector { let resolved_import = resolve_import( crate_id, &collected_import, - &mut context.def_maps, + &context.def_maps, + &mut context.def_interner.unused_imports, &mut Some(&mut references), ); @@ -345,7 +346,13 @@ impl DefCollector { resolved_import } else { - resolve_import(crate_id, &collected_import, &mut context.def_maps, &mut None) + resolve_import( + crate_id, + &collected_import, + &context.def_maps, + &mut context.def_interner.unused_imports, + &mut None, + ) }; match resolved_import { Ok(resolved_import) => { @@ -381,6 +388,23 @@ impl DefCollector { let result = current_def_map.modules[resolved_import.module_scope.0] .import(name.clone(), visibility, module_def_id, is_prelude); + // Empty spans could come from implicitly injected imports, and we don't want to track those + if visibility != ItemVisibility::Public + && name.span().start() < name.span().end() + { + let module_id = ModuleId { + krate: crate_id, + local_id: resolved_import.module_scope, + }; + + context + .def_interner + .unused_imports + .entry(module_id) + .or_default() + .insert(name.clone()); + } + if visibility != ItemVisibility::Private { let local_id = resolved_import.module_scope; let defining_module = ModuleId { krate: crate_id, local_id }; @@ -466,13 +490,22 @@ impl DefCollector { crate_id: CrateId, errors: &mut Vec<(CompilationError, FileId)>, ) { - errors.extend(context.def_maps[&crate_id].modules().iter().flat_map(|(_, module)| { - module.unused_imports().iter().map(|ident| { - let ident = ident.clone(); - let error = CompilationError::ResolverError(ResolverError::UnusedImport { ident }); - (error, module.location.file) - }) - })); + errors.extend( + context + .def_interner + .unused_imports + .iter() + .filter(|(module_id, _)| module_id.krate == crate_id) + .flat_map(|(module_id, unused_imports)| { + let module = &context.def_maps[&crate_id].modules()[module_id.local_id.0]; + unused_imports.iter().map(|ident| { + let ident = ident.clone(); + let error = + CompilationError::ResolverError(ResolverError::UnusedImport { ident }); + (error, module.location.file) + }) + }), + ); } } @@ -515,9 +548,10 @@ fn inject_prelude( }; if let Ok(PathResolution { module_def_id, error }) = path_resolver::resolve_path( - &mut context.def_maps, + &context.def_maps, ModuleId { krate: crate_id, local_id: crate_root }, path, + &mut context.def_interner.unused_imports, &mut None, ) { assert!(error.is_none(), "Tried to add private item to prelude"); diff --git a/compiler/noirc_frontend/src/hir/def_map/module_data.rs b/compiler/noirc_frontend/src/hir/def_map/module_data.rs index bf10447e9c8..f9542094be7 100644 --- a/compiler/noirc_frontend/src/hir/def_map/module_data.rs +++ b/compiler/noirc_frontend/src/hir/def_map/module_data.rs @@ -1,4 +1,4 @@ -use std::collections::{HashMap, HashSet}; +use std::collections::HashMap; use noirc_errors::Location; @@ -24,10 +24,6 @@ pub struct ModuleData { /// True if this module is a `contract Foo { ... }` module containing contract functions pub is_contract: bool, - - /// List of all unused imports. Each time something is imported into this module it's added - /// to this set. When it's used, it's removed. At the end of the program only unused imports remain. - unused_imports: HashSet, } impl ModuleData { @@ -39,7 +35,6 @@ impl ModuleData { definitions: ItemScope::default(), location, is_contract, - unused_imports: HashSet::new(), } } @@ -127,11 +122,6 @@ impl ModuleData { id: ModuleDefId, is_prelude: bool, ) -> Result<(), (Ident, Ident)> { - // Empty spans could come from implicitly injected imports, and we don't want to track those - if visibility != ItemVisibility::Public && name.span().start() < name.span().end() { - self.unused_imports.insert(name.clone()); - } - self.scope.add_item_to_namespace(name, visibility, id, None, is_prelude) } @@ -148,14 +138,4 @@ impl ModuleData { pub fn value_definitions(&self) -> impl Iterator + '_ { self.definitions.values().values().flat_map(|a| a.values().map(|(id, _, _)| *id)) } - - /// Marks an ident as being used by an import. - pub fn use_import(&mut self, ident: &Ident) { - self.unused_imports.remove(ident); - } - - /// Returns the list of all unused imports at this moment. - pub fn unused_imports(&self) -> &HashSet { - &self.unused_imports - } } diff --git a/compiler/noirc_frontend/src/hir/resolution/import.rs b/compiler/noirc_frontend/src/hir/resolution/import.rs index 350bb5c5815..2d48bc54a8b 100644 --- a/compiler/noirc_frontend/src/hir/resolution/import.rs +++ b/compiler/noirc_frontend/src/hir/resolution/import.rs @@ -4,7 +4,8 @@ use thiserror::Error; use crate::graph::CrateId; use crate::hir::def_collector::dc_crate::CompilationError; use crate::node_interner::ReferenceId; -use std::collections::BTreeMap; +use rustc_hash::FxHashMap as HashMap; +use std::collections::{BTreeMap, HashSet}; use crate::ast::{Ident, ItemVisibility, Path, PathKind, PathSegment}; use crate::hir::def_map::{CrateDefMap, LocalModuleId, ModuleDefId, ModuleId, PerNs}; @@ -86,7 +87,8 @@ impl<'a> From<&'a PathResolutionError> for CustomDiagnostic { pub fn resolve_import( crate_id: CrateId, import_directive: &ImportDirective, - def_maps: &mut BTreeMap, + def_maps: &BTreeMap, + unused_imports: &mut HashMap>, path_references: &mut Option<&mut Vec>, ) -> Result { let module_scope = import_directive.module_id; @@ -94,7 +96,14 @@ pub fn resolve_import( module_id: resolved_module, namespace: resolved_namespace, mut error, - } = resolve_path_to_ns(import_directive, crate_id, crate_id, def_maps, path_references)?; + } = resolve_path_to_ns( + import_directive, + crate_id, + crate_id, + def_maps, + unused_imports, + path_references, + )?; let name = resolve_path_name(import_directive); @@ -131,7 +140,8 @@ fn resolve_path_to_ns( import_directive: &ImportDirective, crate_id: CrateId, importing_crate: CrateId, - def_maps: &mut BTreeMap, + def_maps: &BTreeMap, + unused_imports: &mut HashMap>, path_references: &mut Option<&mut Vec>, ) -> NamespaceResolutionResult { let import_path = &import_directive.path.segments; @@ -144,6 +154,7 @@ fn resolve_path_to_ns( importing_crate, import_path, def_maps, + unused_imports, path_references, ) } @@ -158,6 +169,7 @@ fn resolve_path_to_ns( import_directive.module_id, def_maps, true, + unused_imports, path_references, ); } @@ -174,6 +186,7 @@ fn resolve_path_to_ns( // def_map, import_directive, def_maps, + unused_imports, path_references, importing_crate, ); @@ -186,6 +199,7 @@ fn resolve_path_to_ns( import_directive.module_id, def_maps, true, + unused_imports, path_references, ) } @@ -194,6 +208,7 @@ fn resolve_path_to_ns( crate_id, import_directive, def_maps, + unused_imports, path_references, importing_crate, ), @@ -209,6 +224,7 @@ fn resolve_path_to_ns( parent_module_id, def_maps, false, + unused_imports, path_references, ) } else { @@ -225,7 +241,8 @@ fn resolve_path_from_crate_root( importing_crate: CrateId, import_path: &[PathSegment], - def_maps: &mut BTreeMap, + def_maps: &BTreeMap, + unused_imports: &mut HashMap>, path_references: &mut Option<&mut Vec>, ) -> NamespaceResolutionResult { let starting_mod = def_maps[&crate_id].root; @@ -236,22 +253,25 @@ fn resolve_path_from_crate_root( starting_mod, def_maps, false, + unused_imports, path_references, ) } +#[allow(clippy::too_many_arguments)] fn resolve_name_in_module( krate: CrateId, importing_crate: CrateId, import_path: &[PathSegment], starting_mod: LocalModuleId, - def_maps: &mut BTreeMap, + def_maps: &BTreeMap, plain: bool, + unused_imports: &mut HashMap>, path_references: &mut Option<&mut Vec>, ) -> NamespaceResolutionResult { - let def_map = def_maps.get_mut(&krate).unwrap(); + let def_map = &def_maps[&krate]; let mut current_mod_id = ModuleId { krate, local_id: starting_mod }; - let mut current_mod = def_map.modules.get_mut(current_mod_id.local_id.0).unwrap(); + let mut current_mod = &def_map.modules[current_mod_id.local_id.0]; // There is a possibility that the import path is empty // In that case, early return @@ -269,7 +289,7 @@ fn resolve_name_in_module( return Err(PathResolutionError::Unresolved(first_segment.clone())); } - current_mod.use_import(first_segment); + unused_imports.entry(current_mod_id).or_default().remove(first_segment); let mut warning: Option = None; for (index, (last_segment, current_segment)) in @@ -327,12 +347,7 @@ fn resolve_name_in_module( } }); - current_mod = def_maps - .get_mut(¤t_mod_id.krate) - .unwrap() - .modules - .get_mut(current_mod_id.local_id.0) - .unwrap(); + current_mod = &def_maps[¤t_mod_id.krate].modules[current_mod_id.local_id.0]; // Check if namespace let found_ns = current_mod.find_name(current_segment); @@ -341,7 +356,7 @@ fn resolve_name_in_module( return Err(PathResolutionError::Unresolved(current_segment.clone())); } - current_mod.use_import(current_segment); + unused_imports.entry(current_mod_id).or_default().remove(current_segment); current_ns = found_ns; } @@ -359,7 +374,8 @@ fn resolve_path_name(import_directive: &ImportDirective) -> Ident { fn resolve_external_dep( crate_id: CrateId, directive: &ImportDirective, - def_maps: &mut BTreeMap, + def_maps: &BTreeMap, + unused_imports: &mut HashMap>, path_references: &mut Option<&mut Vec>, importing_crate: CrateId, ) -> NamespaceResolutionResult { @@ -397,7 +413,14 @@ fn resolve_external_dep( is_prelude: false, }; - resolve_path_to_ns(&dep_directive, dep_module.krate, importing_crate, def_maps, path_references) + resolve_path_to_ns( + &dep_directive, + dep_module.krate, + importing_crate, + def_maps, + unused_imports, + path_references, + ) } // Returns false if the given private function is being called from a non-child module, or diff --git a/compiler/noirc_frontend/src/hir/resolution/path_resolver.rs b/compiler/noirc_frontend/src/hir/resolution/path_resolver.rs index 396f269eab4..7464f002469 100644 --- a/compiler/noirc_frontend/src/hir/resolution/path_resolver.rs +++ b/compiler/noirc_frontend/src/hir/resolution/path_resolver.rs @@ -1,7 +1,8 @@ use super::import::{resolve_import, ImportDirective, PathResolution, PathResolutionResult}; -use crate::ast::{ItemVisibility, Path}; +use crate::ast::{Ident, ItemVisibility, Path}; use crate::node_interner::ReferenceId; -use std::collections::BTreeMap; +use rustc_hash::FxHashMap as HashMap; +use std::collections::{BTreeMap, HashSet}; use crate::graph::CrateId; use crate::hir::def_map::{CrateDefMap, LocalModuleId, ModuleId}; @@ -13,8 +14,9 @@ pub trait PathResolver { /// a module or type). fn resolve( &self, - def_maps: &mut BTreeMap, + def_maps: &BTreeMap, path: Path, + unused_imports: &mut HashMap>, path_references: &mut Option<&mut Vec>, ) -> PathResolutionResult; @@ -37,11 +39,12 @@ impl StandardPathResolver { impl PathResolver for StandardPathResolver { fn resolve( &self, - def_maps: &mut BTreeMap, + def_maps: &BTreeMap, path: Path, + unused_imports: &mut HashMap>, path_references: &mut Option<&mut Vec>, ) -> PathResolutionResult { - resolve_path(def_maps, self.module_id, path, path_references) + resolve_path(def_maps, self.module_id, path, unused_imports, path_references) } fn local_module_id(&self) -> LocalModuleId { @@ -56,9 +59,10 @@ impl PathResolver for StandardPathResolver { /// Resolve the given path to a function or a type. /// In the case of a conflict, functions are given priority pub fn resolve_path( - def_maps: &mut BTreeMap, + def_maps: &BTreeMap, module_id: ModuleId, path: Path, + unused_imports: &mut HashMap>, path_references: &mut Option<&mut Vec>, ) -> PathResolutionResult { // lets package up the path into an ImportDirective and resolve it using that @@ -69,7 +73,8 @@ pub fn resolve_path( alias: None, is_prelude: false, }; - let resolved_import = resolve_import(module_id.krate, &import, def_maps, path_references)?; + let resolved_import = + resolve_import(module_id.krate, &import, def_maps, unused_imports, path_references)?; let namespace = resolved_import.resolved_namespace; let id = diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index 8c55854333e..7cad4941571 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -1,4 +1,5 @@ use std::borrow::Cow; +use std::collections::HashSet; use std::fmt; use std::hash::Hash; use std::marker::Copy; @@ -266,6 +267,10 @@ pub struct NodeInterner { /// This is stored in the NodeInterner so that the Elaborator from each crate can /// share the same global values. pub(crate) comptime_scopes: Vec>, + + /// List of all unused imports in each module. Each time something is imported it's added + /// to the module's set. When it's used, it's removed. At the end of the program only unused imports remain. + pub unused_imports: HashMap>, } /// A dependency in the dependency graph may be a type or a definition. @@ -652,6 +657,7 @@ impl Default for NodeInterner { auto_import_names: HashMap::default(), comptime_scopes: vec![HashMap::default()], trait_impl_associated_types: HashMap::default(), + unused_imports: std::collections::HashMap::default(), } } } From 5abc88ed02d6fd214893a0735e8eec1dbbd6b194 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 2 Sep 2024 13:46:53 -0300 Subject: [PATCH 22/24] Introduce UsageTracker --- .../noirc_frontend/src/elaborator/scope.rs | 4 +- .../src/hir/def_collector/dc_crate.rs | 51 ++++++++----------- .../src/hir/resolution/import.rs | 36 ++++++------- .../src/hir/resolution/path_resolver.rs | 16 +++--- compiler/noirc_frontend/src/lib.rs | 1 + compiler/noirc_frontend/src/node_interner.rs | 8 ++- compiler/noirc_frontend/src/usage_tracker.rs | 26 ++++++++++ 7 files changed, 80 insertions(+), 62 deletions(-) create mode 100644 compiler/noirc_frontend/src/usage_tracker.rs diff --git a/compiler/noirc_frontend/src/elaborator/scope.rs b/compiler/noirc_frontend/src/elaborator/scope.rs index 5128f8a7828..7a98e1856b3 100644 --- a/compiler/noirc_frontend/src/elaborator/scope.rs +++ b/compiler/noirc_frontend/src/elaborator/scope.rs @@ -89,7 +89,7 @@ impl<'context> Elaborator<'context> { return resolver.resolve( self.def_maps, path, - &mut self.interner.unused_imports, + &mut self.interner.usage_tracker, &mut None, ); } @@ -102,7 +102,7 @@ impl<'context> Elaborator<'context> { let path_resolution = resolver.resolve( self.def_maps, path.clone(), - &mut self.interner.unused_imports, + &mut self.interner.usage_tracker, &mut Some(&mut references), ); diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index e2f0dc98998..6a6cabe593d 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -253,7 +253,7 @@ impl DefCollector { root_file_id: FileId, debug_comptime_in_file: Option<&str>, enable_arithmetic_generics: bool, - error_on_unused_imports: bool, + error_on_usage_tracker: bool, macro_processors: &[&dyn MacroProcessor], ) -> Vec<(CompilationError, FileId)> { let mut errors: Vec<(CompilationError, FileId)> = vec![]; @@ -267,13 +267,13 @@ impl DefCollector { let crate_graph = &context.crate_graph[crate_id]; for dep in crate_graph.dependencies.clone() { - let error_on_unused_imports = false; + let error_on_usage_tracker = false; errors.extend(CrateDefMap::collect_defs( dep.crate_id, context, debug_comptime_in_file, enable_arithmetic_generics, - error_on_unused_imports, + error_on_usage_tracker, macro_processors, )); @@ -328,7 +328,7 @@ impl DefCollector { crate_id, &collected_import, &context.def_maps, - &mut context.def_interner.unused_imports, + &mut context.def_interner.usage_tracker, &mut Some(&mut references), ); @@ -350,7 +350,7 @@ impl DefCollector { crate_id, &collected_import, &context.def_maps, - &mut context.def_interner.unused_imports, + &mut context.def_interner.usage_tracker, &mut None, ) }; @@ -399,10 +399,8 @@ impl DefCollector { context .def_interner - .unused_imports - .entry(module_id) - .or_default() - .insert(name.clone()); + .usage_tracker + .add_unused_import(module_id, name.clone()); } if visibility != ItemVisibility::Private { @@ -478,34 +476,29 @@ impl DefCollector { ); } - if error_on_unused_imports { - Self::check_unused_imports(context, crate_id, &mut errors); + if error_on_usage_tracker { + Self::check_usage_tracker(context, crate_id, &mut errors); } errors } - fn check_unused_imports( + fn check_usage_tracker( context: &Context, crate_id: CrateId, errors: &mut Vec<(CompilationError, FileId)>, ) { - errors.extend( - context - .def_interner - .unused_imports - .iter() - .filter(|(module_id, _)| module_id.krate == crate_id) - .flat_map(|(module_id, unused_imports)| { - let module = &context.def_maps[&crate_id].modules()[module_id.local_id.0]; - unused_imports.iter().map(|ident| { - let ident = ident.clone(); - let error = - CompilationError::ResolverError(ResolverError::UnusedImport { ident }); - (error, module.location.file) - }) - }), - ); + let unused_imports = context.def_interner.usage_tracker.unused_imports().iter(); + let unused_imports = unused_imports.filter(|(module_id, _)| module_id.krate == crate_id); + + errors.extend(unused_imports.flat_map(|(module_id, usage_tracker)| { + let module = &context.def_maps[&crate_id].modules()[module_id.local_id.0]; + usage_tracker.iter().map(|ident| { + let ident = ident.clone(); + let error = CompilationError::ResolverError(ResolverError::UnusedImport { ident }); + (error, module.location.file) + }) + })); } } @@ -551,7 +544,7 @@ fn inject_prelude( &context.def_maps, ModuleId { krate: crate_id, local_id: crate_root }, path, - &mut context.def_interner.unused_imports, + &mut context.def_interner.usage_tracker, &mut None, ) { assert!(error.is_none(), "Tried to add private item to prelude"); diff --git a/compiler/noirc_frontend/src/hir/resolution/import.rs b/compiler/noirc_frontend/src/hir/resolution/import.rs index 2d48bc54a8b..938da0a879f 100644 --- a/compiler/noirc_frontend/src/hir/resolution/import.rs +++ b/compiler/noirc_frontend/src/hir/resolution/import.rs @@ -4,8 +4,8 @@ use thiserror::Error; use crate::graph::CrateId; use crate::hir::def_collector::dc_crate::CompilationError; use crate::node_interner::ReferenceId; -use rustc_hash::FxHashMap as HashMap; -use std::collections::{BTreeMap, HashSet}; +use crate::usage_tracker::UsageTracker; +use std::collections::BTreeMap; use crate::ast::{Ident, ItemVisibility, Path, PathKind, PathSegment}; use crate::hir::def_map::{CrateDefMap, LocalModuleId, ModuleDefId, ModuleId, PerNs}; @@ -88,7 +88,7 @@ pub fn resolve_import( crate_id: CrateId, import_directive: &ImportDirective, def_maps: &BTreeMap, - unused_imports: &mut HashMap>, + usage_tracker: &mut UsageTracker, path_references: &mut Option<&mut Vec>, ) -> Result { let module_scope = import_directive.module_id; @@ -101,7 +101,7 @@ pub fn resolve_import( crate_id, crate_id, def_maps, - unused_imports, + usage_tracker, path_references, )?; @@ -141,7 +141,7 @@ fn resolve_path_to_ns( crate_id: CrateId, importing_crate: CrateId, def_maps: &BTreeMap, - unused_imports: &mut HashMap>, + usage_tracker: &mut UsageTracker, path_references: &mut Option<&mut Vec>, ) -> NamespaceResolutionResult { let import_path = &import_directive.path.segments; @@ -154,7 +154,7 @@ fn resolve_path_to_ns( importing_crate, import_path, def_maps, - unused_imports, + usage_tracker, path_references, ) } @@ -169,7 +169,7 @@ fn resolve_path_to_ns( import_directive.module_id, def_maps, true, - unused_imports, + usage_tracker, path_references, ); } @@ -186,7 +186,7 @@ fn resolve_path_to_ns( // def_map, import_directive, def_maps, - unused_imports, + usage_tracker, path_references, importing_crate, ); @@ -199,7 +199,7 @@ fn resolve_path_to_ns( import_directive.module_id, def_maps, true, - unused_imports, + usage_tracker, path_references, ) } @@ -208,7 +208,7 @@ fn resolve_path_to_ns( crate_id, import_directive, def_maps, - unused_imports, + usage_tracker, path_references, importing_crate, ), @@ -224,7 +224,7 @@ fn resolve_path_to_ns( parent_module_id, def_maps, false, - unused_imports, + usage_tracker, path_references, ) } else { @@ -242,7 +242,7 @@ fn resolve_path_from_crate_root( import_path: &[PathSegment], def_maps: &BTreeMap, - unused_imports: &mut HashMap>, + usage_tracker: &mut UsageTracker, path_references: &mut Option<&mut Vec>, ) -> NamespaceResolutionResult { let starting_mod = def_maps[&crate_id].root; @@ -253,7 +253,7 @@ fn resolve_path_from_crate_root( starting_mod, def_maps, false, - unused_imports, + usage_tracker, path_references, ) } @@ -266,7 +266,7 @@ fn resolve_name_in_module( starting_mod: LocalModuleId, def_maps: &BTreeMap, plain: bool, - unused_imports: &mut HashMap>, + usage_tracker: &mut UsageTracker, path_references: &mut Option<&mut Vec>, ) -> NamespaceResolutionResult { let def_map = &def_maps[&krate]; @@ -289,7 +289,7 @@ fn resolve_name_in_module( return Err(PathResolutionError::Unresolved(first_segment.clone())); } - unused_imports.entry(current_mod_id).or_default().remove(first_segment); + usage_tracker.mark_as_used(current_mod_id, first_segment); let mut warning: Option = None; for (index, (last_segment, current_segment)) in @@ -356,7 +356,7 @@ fn resolve_name_in_module( return Err(PathResolutionError::Unresolved(current_segment.clone())); } - unused_imports.entry(current_mod_id).or_default().remove(current_segment); + usage_tracker.mark_as_used(current_mod_id, current_segment); current_ns = found_ns; } @@ -375,7 +375,7 @@ fn resolve_external_dep( crate_id: CrateId, directive: &ImportDirective, def_maps: &BTreeMap, - unused_imports: &mut HashMap>, + usage_tracker: &mut UsageTracker, path_references: &mut Option<&mut Vec>, importing_crate: CrateId, ) -> NamespaceResolutionResult { @@ -418,7 +418,7 @@ fn resolve_external_dep( dep_module.krate, importing_crate, def_maps, - unused_imports, + usage_tracker, path_references, ) } diff --git a/compiler/noirc_frontend/src/hir/resolution/path_resolver.rs b/compiler/noirc_frontend/src/hir/resolution/path_resolver.rs index 7464f002469..50089d849ae 100644 --- a/compiler/noirc_frontend/src/hir/resolution/path_resolver.rs +++ b/compiler/noirc_frontend/src/hir/resolution/path_resolver.rs @@ -1,8 +1,8 @@ use super::import::{resolve_import, ImportDirective, PathResolution, PathResolutionResult}; -use crate::ast::{Ident, ItemVisibility, Path}; +use crate::ast::{ItemVisibility, Path}; use crate::node_interner::ReferenceId; -use rustc_hash::FxHashMap as HashMap; -use std::collections::{BTreeMap, HashSet}; +use crate::usage_tracker::UsageTracker; +use std::collections::BTreeMap; use crate::graph::CrateId; use crate::hir::def_map::{CrateDefMap, LocalModuleId, ModuleId}; @@ -16,7 +16,7 @@ pub trait PathResolver { &self, def_maps: &BTreeMap, path: Path, - unused_imports: &mut HashMap>, + usage_tracker: &mut UsageTracker, path_references: &mut Option<&mut Vec>, ) -> PathResolutionResult; @@ -41,10 +41,10 @@ impl PathResolver for StandardPathResolver { &self, def_maps: &BTreeMap, path: Path, - unused_imports: &mut HashMap>, + usage_tracker: &mut UsageTracker, path_references: &mut Option<&mut Vec>, ) -> PathResolutionResult { - resolve_path(def_maps, self.module_id, path, unused_imports, path_references) + resolve_path(def_maps, self.module_id, path, usage_tracker, path_references) } fn local_module_id(&self) -> LocalModuleId { @@ -62,7 +62,7 @@ pub fn resolve_path( def_maps: &BTreeMap, module_id: ModuleId, path: Path, - unused_imports: &mut HashMap>, + usage_tracker: &mut UsageTracker, path_references: &mut Option<&mut Vec>, ) -> PathResolutionResult { // lets package up the path into an ImportDirective and resolve it using that @@ -74,7 +74,7 @@ pub fn resolve_path( is_prelude: false, }; let resolved_import = - resolve_import(module_id.krate, &import, def_maps, unused_imports, path_references)?; + resolve_import(module_id.krate, &import, def_maps, usage_tracker, path_references)?; let namespace = resolved_import.resolved_namespace; let id = diff --git a/compiler/noirc_frontend/src/lib.rs b/compiler/noirc_frontend/src/lib.rs index b14f65a3e35..ec09f680bc2 100644 --- a/compiler/noirc_frontend/src/lib.rs +++ b/compiler/noirc_frontend/src/lib.rs @@ -20,6 +20,7 @@ pub mod monomorphization; pub mod node_interner; pub mod parser; pub mod resolve_locations; +pub mod usage_tracker; pub mod hir; pub mod hir_def; diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index 7cad4941571..4a73df6a15f 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -1,5 +1,4 @@ use std::borrow::Cow; -use std::collections::HashSet; use std::fmt; use std::hash::Hash; use std::marker::Copy; @@ -28,6 +27,7 @@ use crate::hir::type_check::generics::TraitGenerics; use crate::hir_def::traits::NamedType; use crate::macros_api::ModuleDefId; use crate::macros_api::UnaryOp; +use crate::usage_tracker::UsageTracker; use crate::QuotedType; use crate::ast::{BinaryOpKind, FunctionDefinition, ItemVisibility}; @@ -268,9 +268,7 @@ pub struct NodeInterner { /// share the same global values. pub(crate) comptime_scopes: Vec>, - /// List of all unused imports in each module. Each time something is imported it's added - /// to the module's set. When it's used, it's removed. At the end of the program only unused imports remain. - pub unused_imports: HashMap>, + pub(crate) usage_tracker: UsageTracker, } /// A dependency in the dependency graph may be a type or a definition. @@ -657,7 +655,7 @@ impl Default for NodeInterner { auto_import_names: HashMap::default(), comptime_scopes: vec![HashMap::default()], trait_impl_associated_types: HashMap::default(), - unused_imports: std::collections::HashMap::default(), + usage_tracker: UsageTracker::default(), } } } diff --git a/compiler/noirc_frontend/src/usage_tracker.rs b/compiler/noirc_frontend/src/usage_tracker.rs new file mode 100644 index 00000000000..d8b7b271734 --- /dev/null +++ b/compiler/noirc_frontend/src/usage_tracker.rs @@ -0,0 +1,26 @@ +use std::collections::HashSet; + +use rustc_hash::FxHashMap as HashMap; + +use crate::{ast::Ident, hir::def_map::ModuleId}; + +#[derive(Debug, Default)] +pub struct UsageTracker { + /// List of all unused imports in each module. Each time something is imported it's added + /// to the module's set. When it's used, it's removed. At the end of the program only unused imports remain. + unused_imports: HashMap>, +} + +impl UsageTracker { + pub(crate) fn add_unused_import(&mut self, module_id: ModuleId, name: Ident) { + self.unused_imports.entry(module_id).or_default().insert(name); + } + + pub(crate) fn mark_as_used(&mut self, current_mod_id: ModuleId, name: &Ident) { + self.unused_imports.entry(current_mod_id).or_default().remove(name); + } + + pub(crate) fn unused_imports(&self) -> &HashMap> { + &self.unused_imports + } +} From cf153e3a737fee8ab130fee8bfc376a22e128165 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 2 Sep 2024 13:53:35 -0300 Subject: [PATCH 23/24] Add docs --- .../noir/modules_packages_crates/modules.md | 28 ++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/docs/docs/noir/modules_packages_crates/modules.md b/docs/docs/noir/modules_packages_crates/modules.md index 16b6307d2fd..6f79c1a55db 100644 --- a/docs/docs/noir/modules_packages_crates/modules.md +++ b/docs/docs/noir/modules_packages_crates/modules.md @@ -182,4 +182,30 @@ 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 +``` + +### `use` visibility + +`use` declarations are private to the containing module, by default. However, like functions, +they can be marked as `pub` or `pub(crate)`. Such a use declaration serves to _re-export_ a name. +A public `use` declaration can therefore redirect some public name to a different target definition: +even a definition with a private canonical path, inside a different module. + +An example of re-exporting: + +```rust +mod quux { + pub use foo::{bar, baz}; + mod foo { + pub fn bar() {} + pub fn baz() {} + } +} + +fn main() { + quux::bar(); + quux::baz(); +} +``` + +In this example, the module `quux` re-exports two public names defined in `foo`. \ No newline at end of file From 5412ed9af93a0de52db3dd1a32ee9ecd25fde0b5 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 2 Sep 2024 14:31:56 -0300 Subject: [PATCH 24/24] Spellcheck --- aztec_macros/src/utils/parse_utils.rs | 4 ++-- docs/docs/noir/modules_packages_crates/modules.md | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/aztec_macros/src/utils/parse_utils.rs b/aztec_macros/src/utils/parse_utils.rs index 8de7e9aeb0c..6a2a876e682 100644 --- a/aztec_macros/src/utils/parse_utils.rs +++ b/aztec_macros/src/utils/parse_utils.rs @@ -404,9 +404,9 @@ fn empty_pattern(pattern: &mut Pattern) { } fn empty_unresolved_trait_constraints( - unresolved_trait_constriants: &mut [UnresolvedTraitConstraint], + unresolved_trait_constraints: &mut [UnresolvedTraitConstraint], ) { - for trait_constraint in unresolved_trait_constriants.iter_mut() { + for trait_constraint in unresolved_trait_constraints.iter_mut() { empty_unresolved_trait_constraint(trait_constraint); } } diff --git a/docs/docs/noir/modules_packages_crates/modules.md b/docs/docs/noir/modules_packages_crates/modules.md index 6f79c1a55db..d21b009be3b 100644 --- a/docs/docs/noir/modules_packages_crates/modules.md +++ b/docs/docs/noir/modules_packages_crates/modules.md @@ -194,7 +194,7 @@ even a definition with a private canonical path, inside a different module. An example of re-exporting: ```rust -mod quux { +mod some_module { pub use foo::{bar, baz}; mod foo { pub fn bar() {} @@ -203,9 +203,9 @@ mod quux { } fn main() { - quux::bar(); - quux::baz(); + some_module::bar(); + some_module::baz(); } ``` -In this example, the module `quux` re-exports two public names defined in `foo`. \ No newline at end of file +In this example, the module `some_module` re-exports two public names defined in `foo`. \ No newline at end of file