feat: use visibility (noir-lang#5856)

# Description

## Problem

Part of noir-lang#4515

## Summary

We recently added a warning for unused imports... but only for bin and
contract packages. We didn't enable it for lib packages because a `use`
could also be used as `pub use`, something we don't have yet. I thought
it would be really nice if we had `pub use`, and warned on unused
imports in libs too. I checked the code and we already track visibility
for any item, in general, it's just that for things that don't allow a
visibility modifier we just consider it's public. So I tried to see how
difficult it would be to implement it, and it turned out it wasn't that
hard or time-consuming.

That said, visibility for `use` involves some more logic, particularly
for autocompletion, because now `pub use` should be suggested, but the
"parent" module of that item isn't the actual parent (it's the module
where the `pub use` is defined) but that was relatively striaght-forward
to implement too.

## Additional Context

If we decide to go forward with this, any existing `use` that was used
as `pub use` will likely start producing a warning for libs (a lot of
them in Aztec-Packages), but now that can be silenced by changing them
to `pub use`.

Where should this new feature be documented? I'm not sure if it should
go in `dependencies.md` or `modules.md`.

## Documentation\*

Check one:
- [ ] No documentation needed.
- [x] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist

- [x] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.

---------

Co-authored-by: Tom French <[email protected]>

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),
     }
@@ -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);
     }
 }

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<ExpressionWidth, std::io::Error> {
     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<CompiledProgram>,
 ) -> CompilationResult<CompiledProgram> {
-    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<CompiledContract> {
-    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);

compiler/noirc_frontend/src/ast/mod.rs

@@ -460,12 +460,22 @@ 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 {
+    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)]

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;
@@ -302,6 +302,7 @@ impl std::fmt::Display for ModuleDeclaration {
 
 #[derive(Debug, PartialEq, Eq, Clone)]
 pub struct ImportStatement {
+    pub visibility: ItemVisibility,
     pub path: Path,
     pub alias: Option<Ident>,
 }
@@ -350,7 +351,7 @@ pub enum UseTreeKind {
 }
 
 impl UseTree {
-    pub fn desugar(self, root: Option<Path>) -> Vec<ImportStatement> {
+    pub fn desugar(self, root: Option<Path>, visibility: ItemVisibility) -> Vec<ImportStatement> {
         let prefix = if let Some(mut root) = root {
             root.segments.extend(self.prefix.segments);
             root
@@ -360,10 +361,11 @@ impl UseTree {
 
         match self.kind {
             UseTreeKind::Path(name, alias) => {
-                vec![ImportStatement { path: prefix.join(name), alias }]
+                vec![ImportStatement { visibility, path: prefix.join(name), alias }]
             }
             UseTreeKind::List(trees) => {
-                trees.into_iter().flat_map(|tree| tree.desugar(Some(prefix.clone()))).collect()
+                let trees = trees.into_iter();
+                trees.flat_map(|tree| tree.desugar(Some(prefix.clone()), visibility)).collect()
             }
         }
     }

compiler/noirc_frontend/src/ast/visitor.rs

@@ -21,8 +21,9 @@ use crate::{
 };
 
 use super::{
-    FunctionReturnType, GenericTypeArgs, IntegerBitSize, Pattern, Signedness, UnresolvedGenerics,
-    UnresolvedTraitConstraint, UnresolvedType, UnresolvedTypeData, UnresolvedTypeExpression,
+    FunctionReturnType, GenericTypeArgs, IntegerBitSize, ItemVisibility, Pattern, Signedness,
+    UnresolvedGenerics, UnresolvedTraitConstraint, UnresolvedType, UnresolvedTypeData,
+    UnresolvedTypeExpression,
 };
 
 /// Implements the [Visitor pattern](https://en.wikipedia.org/wiki/Visitor_pattern) for Noir's AST.
@@ -252,7 +253,7 @@ pub trait Visitor {
         true
     }
 
-    fn visit_import(&mut self, _: &UseTree) -> bool {
+    fn visit_import(&mut self, _: &UseTree, _visibility: ItemVisibility) -> bool {
         true
     }
 
@@ -470,8 +471,8 @@ impl Item {
                 }
             }
             ItemKind::Trait(noir_trait) => noir_trait.accept(self.span, visitor),
-            ItemKind::Import(use_tree) => {
-                if visitor.visit_import(use_tree) {
+            ItemKind::Import(use_tree, visibility) => {
+                if visitor.visit_import(use_tree, *visibility) {
                     use_tree.accept(visitor);
                 }
             }

compiler/noirc_frontend/src/elaborator/comptime.rs

@@ -333,7 +333,7 @@ impl<'context> Elaborator<'context> {
             TopLevelStatement::Error => (),
 
             TopLevelStatement::Module(_)
-            | TopLevelStatement::Import(_)
+            | TopLevelStatement::Import(..)
             | TopLevelStatement::Struct(_)
             | TopLevelStatement::Trait(_)
             | TopLevelStatement::Impl(_)

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,47 @@ 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 self.interner.usage_tracker,
+                &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 self.interner.usage_tracker,
+            &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)
     }