feat: warn on unused imports (#5847)

# Description

## Problem

Resolves #4762

## Summary

The compiler now produces a warning on unused imports.

## Additional Context

I don't know if this is the correct approach. I added a new HashSet to
track imported names, then those are removed as we import things. I
thought about tracking this in `ItemScope` but it's tricky because the
way things are they include imported and self things.

There's another thing: eventually it would be nice to warn on unused
types, like in Rust. For that maybe it would make sense to track this in
`ItemScope`... but given that we currently don't have visibility
modifiers for types, it can't be done right now. So maybe doing it just
for imports with specific code is fine.

I had to refactor a bit `resolve_path` because some paths were looked up
not using that method and so some things weren't marked as used... but
now `StandardPathResolver::new` is used in exactly one place in the
compiler.

## Documentation

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

# PR Checklist

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

compiler/noirc_driver/src/lib.rs

@@ -131,6 +131,18 @@ 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
@@ -278,8 +290,10 @@ pub fn add_dep(
 pub fn check_crate(
     context: &mut Context,
     crate_id: CrateId,
-    options: &CompileOptions,
+    check_options: &CheckOptions,
 ) -> CompilationResult<()> {
+    let options = &check_options.compile_options;
+
     let macros: &[&dyn MacroProcessor] =
         if options.disable_macros { &[] } else { &[&aztec_macros::AztecMacro] };
 
@@ -289,6 +303,7 @@ pub fn check_crate(
         context,
         options.debug_comptime_in_file.as_deref(),
         options.arithmetic_generics,
+        check_options.error_on_unused_imports,
         macros,
     );
     errors.extend(diagnostics.into_iter().map(|(error, file_id)| {
@@ -322,7 +337,10 @@ pub fn compile_main(
     options: &CompileOptions,
     cached_program: Option<CompiledProgram>,
 ) -> CompilationResult<CompiledProgram> {
-    let (_, mut warnings) = check_crate(context, crate_id, options)?;
+    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 main = context.get_main_function(&crate_id).ok_or_else(|| {
         // TODO(#2155): This error might be a better to exist in Nargo
@@ -357,7 +375,9 @@ pub fn compile_contract(
     crate_id: CrateId,
     options: &CompileOptions,
 ) -> CompilationResult<CompiledContract> {
-    let (_, warnings) = check_crate(context, crate_id, options)?;
+    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)?;
 
     // TODO: We probably want to error if contracts is empty
     let contracts = context.get_all_contracts(&crate_id);

compiler/noirc_frontend/src/elaborator/mod.rs

@@ -11,7 +11,7 @@ use crate::{
             UnresolvedTypeAlias,
         },
         def_map::DefMaps,
-        resolution::{errors::ResolverError, path_resolver::PathResolver},
+        resolution::errors::ResolverError,
         scope::ScopeForest as GenericScopeForest,
         type_check::{generics::TraitGenerics, TypeCheckError},
     },
@@ -36,7 +36,7 @@ use crate::{
     hir::{
         def_collector::{dc_crate::CollectedItems, errors::DefCollectorErrorKind},
         def_map::{LocalModuleId, ModuleDefId, ModuleId, MAIN_FUNCTION},
-        resolution::{import::PathResolution, path_resolver::StandardPathResolver},
+        resolution::import::PathResolution,
         Context,
     },
     hir_def::function::{FuncMeta, HirFunction},
@@ -630,10 +630,8 @@ impl<'context> Elaborator<'context> {
         }
     }
 
-    pub fn resolve_module_by_path(&self, path: Path) -> Option<ModuleId> {
-        let path_resolver = StandardPathResolver::new(self.module_id());
-
-        match path_resolver.resolve(self.def_maps, path.clone(), &mut None) {
+    pub fn resolve_module_by_path(&mut self, path: Path) -> Option<ModuleId> {
+        match self.resolve_path(path.clone()) {
             Ok(PathResolution { module_def_id: ModuleDefId::ModuleId(module_id), error }) => {
                 if error.is_some() {
                     None
@@ -646,9 +644,7 @@ impl<'context> Elaborator<'context> {
     }
 
     fn resolve_trait_by_path(&mut self, path: Path) -> Option<TraitId> {
-        let path_resolver = StandardPathResolver::new(self.module_id());
-
-        let error = match path_resolver.resolve(self.def_maps, path.clone(), &mut None) {
+        let error = match self.resolve_path(path.clone()) {
             Ok(PathResolution { module_def_id: ModuleDefId::TraitId(trait_id), error }) => {
                 if let Some(error) = error {
                     self.push_err(error);

compiler/noirc_frontend/src/elaborator/scope.rs

@@ -2,6 +2,7 @@ use noirc_errors::{Location, Spanned};
 
 use crate::ast::{PathKind, ERROR_IDENT};
 use crate::hir::def_map::{LocalModuleId, ModuleId};
+use crate::hir::resolution::import::{PathResolution, PathResolutionResult};
 use crate::hir::resolution::path_resolver::{PathResolver, StandardPathResolver};
 use crate::hir::scope::{Scope as GenericScope, ScopeTree as GenericScopeTree};
 use crate::macros_api::Ident;
@@ -29,7 +30,7 @@ type ScopeTree = GenericScopeTree<String, ResolverMeta>;
 impl<'context> Elaborator<'context> {
     pub(super) fn lookup<T: TryFromModuleDefId>(&mut self, path: Path) -> Result<T, ResolverError> {
         let span = path.span();
-        let id = self.resolve_path(path)?;
+        let id = self.resolve_path_or_error(path)?;
         T::try_from(id).ok_or_else(|| ResolverError::Expected {
             expected: T::description(),
             got: id.as_str().to_owned(),
@@ -42,15 +43,37 @@ impl<'context> Elaborator<'context> {
         ModuleId { krate: self.crate_id, local_id: self.local_module }
     }
 
-    pub(super) fn resolve_path(&mut self, path: Path) -> Result<ModuleDefId, ResolverError> {
+    pub(super) fn resolve_path_or_error(
+        &mut self,
+        path: Path,
+    ) -> Result<ModuleDefId, ResolverError> {
+        let path_resolution = self.resolve_path(path)?;
+
+        if let Some(error) = path_resolution.error {
+            self.push_err(error);
+        }
+
+        Ok(path_resolution.module_def_id)
+    }
+
+    pub(super) fn resolve_path(&mut self, path: Path) -> PathResolutionResult {
         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();
                 if path.segments.len() == 1 {
-                    return Ok(ModuleDefId::TypeId(struct_type.id));
+                    return Ok(PathResolution {
+                        module_def_id: ModuleDefId::TypeId(struct_type.id),
+                        error: None,
+                    });
                 }
 
                 module_id = struct_type.id.module_id();
@@ -65,11 +88,7 @@ impl<'context> Elaborator<'context> {
         self.resolve_path_in_module(path, module_id)
     }
 
-    fn resolve_path_in_module(
-        &mut self,
-        path: Path,
-        module_id: ModuleId,
-    ) -> Result<ModuleDefId, ResolverError> {
+    fn resolve_path_in_module(&mut self, path: Path, module_id: ModuleId) -> PathResolutionResult {
         let resolver = StandardPathResolver::new(module_id);
         let path_resolution;
 
@@ -99,11 +118,7 @@ impl<'context> Elaborator<'context> {
             path_resolution = resolver.resolve(self.def_maps, path, &mut None)?;
         }
 
-        if let Some(error) = path_resolution.error {
-            self.push_err(error);
-        }
-
-        Ok(path_resolution.module_def_id)
+        Ok(path_resolution)
     }
 
     pub(super) fn get_struct(&self, type_id: StructId) -> Shared<StructType> {
@@ -150,7 +165,7 @@ impl<'context> Elaborator<'context> {
 
     pub(super) fn lookup_global(&mut self, path: Path) -> Result<DefinitionId, ResolverError> {
         let span = path.span();
-        let id = self.resolve_path(path)?;
+        let id = self.resolve_path_or_error(path)?;
 
         if let Some(function) = TryFromModuleDefId::try_from(id) {
             return Ok(self.interner.function_definition_id(function));

compiler/noirc_frontend/src/elaborator/types.rs

@@ -421,7 +421,7 @@ impl<'context> Elaborator<'context> {
         }
 
         // If we cannot find a local generic of the same name, try to look up a global
-        match self.resolve_path(path.clone()) {
+        match self.resolve_path_or_error(path.clone()) {
             Ok(ModuleDefId::GlobalId(id)) => {
                 if let Some(current_item) = self.current_item {
                     self.interner.add_global_dependency(current_item, id);

compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs

@@ -245,13 +245,15 @@ impl DefCollector {
     /// Collect all of the definitions in a given crate into a CrateDefMap
     /// Modules which are not a part of the module hierarchy starting with
     /// the root module, will be ignored.
+    #[allow(clippy::too_many_arguments)]
     pub fn collect_crate_and_dependencies(
         mut def_map: CrateDefMap,
         context: &mut Context,
         ast: SortedModule,
         root_file_id: FileId,
         debug_comptime_in_file: Option<&str>,
         enable_arithmetic_generics: bool,
+        error_on_unused_imports: bool,
         macro_processors: &[&dyn MacroProcessor],
     ) -> Vec<(CompilationError, FileId)> {
         let mut errors: Vec<(CompilationError, FileId)> = vec![];
@@ -265,11 +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;
             errors.extend(CrateDefMap::collect_defs(
                 dep.crate_id,
                 context,
                 debug_comptime_in_file,
                 enable_arithmetic_generics,
+                error_on_unused_imports,
                 macro_processors,
             ));
 
@@ -413,8 +417,26 @@ impl DefCollector {
             );
         }
 
+        if error_on_unused_imports {
+            Self::check_unused_imports(context, crate_id, &mut errors);
+        }
+
         errors
     }
+
+    fn check_unused_imports(
+        context: &Context,
+        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)
+            })
+        }));
+    }
 }
 
 fn add_import_reference(

compiler/noirc_frontend/src/hir/def_map/mod.rs

@@ -77,6 +77,7 @@ impl CrateDefMap {
         context: &mut Context,
         debug_comptime_in_file: Option<&str>,
         enable_arithmetic_generics: bool,
+        error_on_unused_imports: bool,
         macro_processors: &[&dyn MacroProcessor],
     ) -> Vec<(CompilationError, FileId)> {
         // Check if this Crate has already been compiled
@@ -127,12 +128,14 @@ impl CrateDefMap {
             root_file_id,
             debug_comptime_in_file,
             enable_arithmetic_generics,
+            error_on_unused_imports,
             macro_processors,
         ));
 
         errors.extend(
             parsing_errors.iter().map(|e| (e.clone().into(), root_file_id)).collect::<Vec<_>>(),
         );
+
         errors
     }
 

compiler/noirc_frontend/src/hir/def_map/module_data.rs

@@ -1,4 +1,4 @@
-use std::collections::HashMap;
+use std::collections::{HashMap, HashSet};
 
 use noirc_errors::Location;
 
@@ -24,6 +24,10 @@ 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<Ident>,
 }
 
 impl ModuleData {
@@ -35,6 +39,7 @@ impl ModuleData {
             definitions: ItemScope::default(),
             location,
             is_contract,
+            unused_imports: HashSet::new(),
         }
     }
 
@@ -121,6 +126,11 @@ 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 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)
     }
 
@@ -137,4 +147,14 @@ impl ModuleData {
     pub fn value_definitions(&self) -> impl Iterator<Item = ModuleDefId> + '_ {
         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<Ident> {
+        &self.unused_imports
+    }
 }

compiler/noirc_frontend/src/hir/resolution/errors.rs

@@ -20,6 +20,8 @@ pub enum ResolverError {
     DuplicateDefinition { name: String, first_span: Span, second_span: Span },
     #[error("Unused variable")]
     UnusedVariable { ident: Ident },
+    #[error("Unused import")]
+    UnusedImport { ident: Ident },
     #[error("Could not find variable in this scope")]
     VariableNotDeclared { name: String, span: Span },
     #[error("path is not an identifier")]
@@ -152,6 +154,15 @@ impl<'a> From<&'a ResolverError> for Diagnostic {
                     ident.span(),
                 )
             }
+            ResolverError::UnusedImport { ident } => {
+                let name = &ident.0.contents;
+
+                Diagnostic::simple_warning(
+                    format!("unused import {name}"),
+                    "unused import ".to_string(),
+                    ident.span(),
+                )
+            }
             ResolverError::VariableNotDeclared { name, span } => Diagnostic::simple_error(
                 format!("cannot find `{name}` in this scope "),
                 "not found in this scope".to_string(),