From bcb438b0816fbe08344535545612d32b4730af79 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 25 Sep 2024 17:34:52 -0300 Subject: [PATCH] feat: detect unconstructed structs (#6061) # Description ## Problem Fixes #6055 ## Summary This is similar to how it works in Rust. ## Additional Context ## 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. --- .../src/elaborator/expressions.rs | 8 ++ compiler/noirc_frontend/src/elaborator/mod.rs | 67 +++++++-- .../src/hir/comptime/interpreter/builtin.rs | 10 +- .../src/hir/def_collector/dc_crate.rs | 2 +- .../noirc_frontend/src/hir/def_map/mod.rs | 8 +- .../src/hir/resolution/errors.rs | 31 ++-- .../src/hir/resolution/import.rs | 4 +- compiler/noirc_frontend/src/node_interner.rs | 6 + compiler/noirc_frontend/src/tests.rs | 132 ++++++++++++++++-- .../noirc_frontend/src/tests/unused_items.rs | 30 ++-- compiler/noirc_frontend/src/usage_tracker.rs | 25 +++- noir_stdlib/src/meta/mod.nr | 10 +- tooling/lsp/src/modules.rs | 9 -- .../requests/code_action/import_or_qualify.rs | 4 +- .../src/requests/completion/auto_import.rs | 4 +- .../src/trait_impl_method_stub_generator.rs | 7 +- 16 files changed, 272 insertions(+), 85 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/expressions.rs b/compiler/noirc_frontend/src/elaborator/expressions.rs index 9a74e77215a..46a22bb232f 100644 --- a/compiler/noirc_frontend/src/elaborator/expressions.rs +++ b/compiler/noirc_frontend/src/elaborator/expressions.rs @@ -535,6 +535,8 @@ impl<'context> Elaborator<'context> { } }; + self.mark_struct_as_constructed(r#type.clone()); + let turbofish_span = last_segment.turbofish_span(); let struct_generics = self.resolve_struct_turbofish_generics( @@ -564,6 +566,12 @@ impl<'context> Elaborator<'context> { (expr, Type::Struct(struct_type, generics)) } + pub(super) fn mark_struct_as_constructed(&mut self, struct_type: Shared) { + let struct_type = struct_type.borrow(); + let parent_module_id = struct_type.id.parent_module_id(self.def_maps); + self.interner.usage_tracker.mark_as_used(parent_module_id, &struct_type.name); + } + /// Resolve all the fields of a struct constructor expression. /// Ensures all fields are present, none are repeated, and all /// are part of the struct. diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index 9dd76885d61..c5e457c405b 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -95,7 +95,7 @@ pub struct Elaborator<'context> { pub(crate) interner: &'context mut NodeInterner, - def_maps: &'context mut DefMaps, + pub(crate) def_maps: &'context mut DefMaps, pub(crate) file: FileId, @@ -759,6 +759,10 @@ impl<'context> Elaborator<'context> { type_span, ); + if is_entry_point { + self.mark_parameter_type_as_used(&typ); + } + let pattern = self.elaborate_pattern_and_store_ids( pattern, typ.clone(), @@ -837,6 +841,57 @@ impl<'context> Elaborator<'context> { self.current_item = None; } + fn mark_parameter_type_as_used(&mut self, typ: &Type) { + match typ { + Type::Array(_n, typ) => self.mark_parameter_type_as_used(typ), + Type::Slice(typ) => self.mark_parameter_type_as_used(typ), + Type::Tuple(types) => { + for typ in types { + self.mark_parameter_type_as_used(typ); + } + } + Type::Struct(struct_type, generics) => { + self.mark_struct_as_constructed(struct_type.clone()); + for generic in generics { + self.mark_parameter_type_as_used(generic); + } + } + Type::Alias(alias_type, generics) => { + self.mark_parameter_type_as_used(&alias_type.borrow().get_type(generics)); + } + Type::MutableReference(typ) => { + self.mark_parameter_type_as_used(typ); + } + Type::InfixExpr(left, _op, right) => { + self.mark_parameter_type_as_used(left); + self.mark_parameter_type_as_used(right); + } + Type::FieldElement + | Type::Integer(..) + | Type::Bool + | Type::String(_) + | Type::FmtString(_, _) + | Type::Unit + | Type::Quoted(..) + | Type::Constant(..) + | Type::TraitAsType(..) + | Type::TypeVariable(..) + | Type::NamedGeneric(..) + | Type::Function(..) + | Type::Forall(..) + | Type::Error => (), + } + + if let Type::Alias(alias_type, generics) = typ { + self.mark_parameter_type_as_used(&alias_type.borrow().get_type(generics)); + return; + } + + if let Type::Struct(struct_type, _generics) = typ { + self.mark_struct_as_constructed(struct_type.clone()); + } + } + fn run_function_lints(&mut self, func: &FuncMeta, modifiers: &FunctionModifiers) { self.run_lint(|_| lints::inlining_attributes(func, modifiers).map(Into::into)); self.run_lint(|_| lints::missing_pub(func, modifiers).map(Into::into)); @@ -1163,15 +1218,9 @@ impl<'context> Elaborator<'context> { // then it's either accessible (all good) or it's not, in which case a different // error will happen somewhere else, but no need to error again here. if struct_module_id.krate == self.crate_id { - // Go to the struct's parent module - let module_data = self.get_module(struct_module_id); - let parent_local_id = - module_data.parent.expect("Expected struct module parent to exist"); - let parent_module_id = - ModuleId { krate: struct_module_id.krate, local_id: parent_local_id }; - let parent_module_data = self.get_module(parent_module_id); - // Find the struct in the parent module so we can know its visibility + let parent_module_id = struct_type.id.parent_module_id(self.def_maps); + let parent_module_data = self.get_module(parent_module_id); let per_ns = parent_module_data.find_name(&struct_type.name); if let Some((_, aliased_visibility, _)) = per_ns.types { if aliased_visibility < visibility { diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs index c21ca353e07..59fc186173a 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs +++ b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs @@ -30,7 +30,6 @@ use crate::{ InterpreterError, Value, }, def_collector::dc_crate::CollectedItems, - def_map::ModuleId, }, hir_def::function::FunctionBody, macros_api::{HirExpression, HirLiteral, Ident, ModuleDefId, NodeInterner, Signedness}, @@ -505,14 +504,7 @@ fn struct_def_module( ) -> IResult { let self_argument = check_one_argument(arguments, location)?; let struct_id = get_struct(self_argument)?; - let struct_module_id = struct_id.module_id(); - - // A struct's module is its own module. To get the module where its defined we need - // to look for its parent. - let module_data = interpreter.elaborator.get_module(struct_module_id); - let parent_local_id = module_data.parent.expect("Expected struct module parent to exist"); - let parent = ModuleId { krate: struct_module_id.krate, local_id: parent_local_id }; - + let parent = struct_id.parent_module_id(interpreter.elaborator.def_maps); Ok(Value::ModuleDefinition(parent)) } 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 d365e5807c2..faf72e86fb4 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -498,7 +498,7 @@ impl DefCollector { let ident = ident.clone(); let error = CompilationError::ResolverError(ResolverError::UnusedItem { ident, - item_type: unused_item.item_type(), + item: *unused_item, }); (error, module.location.file) }) diff --git a/compiler/noirc_frontend/src/hir/def_map/mod.rs b/compiler/noirc_frontend/src/hir/def_map/mod.rs index d810e95218c..9afe897a167 100644 --- a/compiler/noirc_frontend/src/hir/def_map/mod.rs +++ b/compiler/noirc_frontend/src/hir/def_map/mod.rs @@ -42,12 +42,16 @@ impl ModuleId { pub fn dummy_id() -> ModuleId { ModuleId { krate: CrateId::dummy_id(), local_id: LocalModuleId::dummy_id() } } -} -impl ModuleId { pub fn module(self, def_maps: &DefMaps) -> &ModuleData { &def_maps[&self.krate].modules()[self.local_id.0] } + + /// Returns this module's parent, if there's any. + pub fn parent(self, def_maps: &DefMaps) -> Option { + let module_data = &def_maps[&self.krate].modules()[self.local_id.0]; + module_data.parent.map(|local_id| ModuleId { krate: self.krate, local_id }) + } } pub type DefMaps = BTreeMap; diff --git a/compiler/noirc_frontend/src/hir/resolution/errors.rs b/compiler/noirc_frontend/src/hir/resolution/errors.rs index cd3acbdc0f1..f3c61a7fbe2 100644 --- a/compiler/noirc_frontend/src/hir/resolution/errors.rs +++ b/compiler/noirc_frontend/src/hir/resolution/errors.rs @@ -2,7 +2,10 @@ pub use noirc_errors::Span; use noirc_errors::{CustomDiagnostic as Diagnostic, FileDiagnostic}; use thiserror::Error; -use crate::{ast::Ident, hir::comptime::InterpreterError, parser::ParserError, Type}; +use crate::{ + ast::Ident, hir::comptime::InterpreterError, parser::ParserError, usage_tracker::UnusedItem, + Type, +}; use super::import::PathResolutionError; @@ -20,8 +23,8 @@ pub enum ResolverError { DuplicateDefinition { name: String, first_span: Span, second_span: Span }, #[error("Unused variable")] UnusedVariable { ident: Ident }, - #[error("Unused {item_type}")] - UnusedItem { ident: Ident, item_type: &'static str }, + #[error("Unused {}", item.item_type())] + UnusedItem { ident: Ident, item: UnusedItem }, #[error("Could not find variable in this scope")] VariableNotDeclared { name: String, span: Span }, #[error("path is not an identifier")] @@ -166,14 +169,24 @@ impl<'a> From<&'a ResolverError> for Diagnostic { diagnostic.unnecessary = true; diagnostic } - ResolverError::UnusedItem { ident, item_type } => { + ResolverError::UnusedItem { ident, item} => { let name = &ident.0.contents; + let item_type = item.item_type(); - let mut diagnostic = Diagnostic::simple_warning( - format!("unused {item_type} {name}"), - format!("unused {item_type}"), - ident.span(), - ); + let mut diagnostic = + if let UnusedItem::Struct(..) = item { + Diagnostic::simple_warning( + format!("{item_type} `{name}` is never constructed"), + format!("{item_type} is never constructed"), + ident.span(), + ) + } else { + Diagnostic::simple_warning( + format!("unused {item_type} {name}"), + format!("unused {item_type}"), + ident.span(), + ) + }; diagnostic.unnecessary = true; diagnostic } diff --git a/compiler/noirc_frontend/src/hir/resolution/import.rs b/compiler/noirc_frontend/src/hir/resolution/import.rs index 938da0a879f..73a1b1ccb7c 100644 --- a/compiler/noirc_frontend/src/hir/resolution/import.rs +++ b/compiler/noirc_frontend/src/hir/resolution/import.rs @@ -289,7 +289,7 @@ fn resolve_name_in_module( return Err(PathResolutionError::Unresolved(first_segment.clone())); } - usage_tracker.mark_as_used(current_mod_id, first_segment); + usage_tracker.mark_as_referenced(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())); } - usage_tracker.mark_as_used(current_mod_id, current_segment); + usage_tracker.mark_as_referenced(current_mod_id, current_segment); current_ns = found_ns; } diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index 23ca3387f09..a95282a1ec9 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -23,6 +23,7 @@ use crate::graph::CrateId; use crate::hir::comptime; use crate::hir::def_collector::dc_crate::CompilationError; use crate::hir::def_collector::dc_crate::{UnresolvedStruct, UnresolvedTrait, UnresolvedTypeAlias}; +use crate::hir::def_map::DefMaps; use crate::hir::def_map::{LocalModuleId, ModuleId}; use crate::hir::type_check::generics::TraitGenerics; use crate::hir_def::traits::NamedType; @@ -489,6 +490,11 @@ impl StructId { pub fn local_module_id(self) -> LocalModuleId { self.0.local_id } + + /// Returns the module where this struct is defined. + pub fn parent_module_id(self, def_maps: &DefMaps) -> ModuleId { + self.module_id().parent(def_maps).expect("Expected struct module parent to exist") + } } #[derive(Debug, Eq, PartialEq, Hash, Copy, Clone, PartialOrd, Ord)] diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index c8e94d2c3e0..4c91c90bc31 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -195,8 +195,10 @@ fn check_trait_implementation_duplicate_method() { x + 2 * y } } - - fn main() {}"; + + fn main() { + let _ = Foo { bar: 1, array: [2, 3] }; // silence Foo never constructed warning + }"; let errors = get_program_errors(src); assert!(!has_parser_error(&errors)); @@ -237,6 +239,7 @@ fn check_trait_wrong_method_return_type() { } fn main() { + let _ = Foo {}; // silence Foo never constructed warning } "; let errors = get_program_errors(src); @@ -279,6 +282,7 @@ fn check_trait_wrong_method_return_type2() { } fn main() { + let _ = Foo { bar: 1, array: [2, 3] }; // silence Foo never constructed warning }"; let errors = get_program_errors(src); assert!(!has_parser_error(&errors)); @@ -401,6 +405,7 @@ fn check_trait_wrong_method_name() { } fn main() { + let _ = Foo { bar: 1, array: [2, 3] }; // silence Foo never constructed warning }"; let compilation_errors = get_program_errors(src); assert!(!has_parser_error(&compilation_errors)); @@ -639,8 +644,10 @@ fn check_impl_struct_not_trait() { Self { bar: x, array: [x,y] } } } - - fn main() {} + + fn main() { + let _ = Default { x: 1, z: 1 }; // silence Default never constructed warning + } "; let errors = get_program_errors(src); assert!(!has_parser_error(&errors)); @@ -719,6 +726,7 @@ fn check_trait_duplicate_implementation() { impl Default for Foo { } fn main() { + let _ = Foo { bar: 1 }; // silence Foo never constructed warning } "; let errors = get_program_errors(src); @@ -757,6 +765,7 @@ fn check_trait_duplicate_implementation_with_alias() { } fn main() { + let _ = MyStruct {}; // silence MyStruct never constructed warning } "; let errors = get_program_errors(src); @@ -1549,7 +1558,9 @@ fn struct_numeric_generic_in_function() { inner: u64 } - pub fn bar() { } + pub fn bar() { + let _ = Foo { inner: 1 }; // silence Foo never constructed warning + } "#; let errors = get_program_errors(src); assert_eq!(errors.len(), 1); @@ -1724,7 +1735,7 @@ fn numeric_generic_used_in_nested_type_fails() { a: Field, b: Bar, } - struct Bar { + pub struct Bar { inner: N } "#; @@ -1879,6 +1890,10 @@ fn constant_used_with_numeric_generic() { [self.value] } } + + fn main() { + let _ = ValueNote { value: 1 }; // silence ValueNote never constructed warning + } "#; assert_no_errors(src); } @@ -1985,6 +2000,10 @@ fn numeric_generics_value_kind_mismatch_u32_u64() { self.len += 1; } } + + fn main() { + let _ = BoundedVec { storage: [1], len: 1 }; // silence never constructed warning + } "#; let errors = get_program_errors(src); assert_eq!(errors.len(), 1); @@ -2060,6 +2079,10 @@ fn impl_stricter_than_trait_no_trait_method_constraints() { process_array(serialize_thing(self)) } } + + fn main() { + let _ = MyType { a: 1, b: 1 }; // silence MyType never constructed warning + } "#; let errors = get_program_errors(src); @@ -2150,6 +2173,10 @@ fn impl_stricter_than_trait_different_object_generics() { fn tuple_bad() where (Option, Option): MyTrait { } } + + fn main() { + let _ = OtherOption { inner: Option { inner: 1 } }; // silence unused warnings + } "#; let errors = get_program_errors(src); @@ -2211,6 +2238,10 @@ fn impl_stricter_than_trait_different_trait() { // types are the same. fn bar() where Option: OtherDefault {} } + + fn main() { + let _ = Option { inner: 1 }; // silence Option never constructed warning + } "#; let errors = get_program_errors(src); @@ -2248,6 +2279,10 @@ fn trait_impl_where_clause_stricter_pass() { fn bad_foo() where A: OtherTrait { } } + + fn main() { + let _ = Option { inner: 1 }; // silence Option never constructed warning + } "#; let errors = get_program_errors(src); @@ -2333,6 +2368,10 @@ fn impl_not_found_for_inner_impl() { process_array(serialize_thing(self)) } } + + fn main() { + let _ = MyType { a: 1, b: 1 }; // silence MyType never constructed warning + } "#; let errors = get_program_errors(src); @@ -2639,7 +2678,9 @@ fn incorrect_generic_count_on_struct_impl() { let src = r#" struct Foo {} impl Foo {} - fn main() {} + fn main() { + let _ = Foo {}; // silence Foo never constructed warning + } "#; let errors = get_program_errors(src); @@ -2661,7 +2702,9 @@ fn incorrect_generic_count_on_type_alias() { let src = r#" pub struct Foo {} pub type Bar = Foo; - fn main() {} + fn main() { + let _ = Foo {}; // silence Foo never constructed warning + } "#; let errors = get_program_errors(src); @@ -2693,7 +2736,9 @@ fn uses_self_type_for_struct_function_call() { } } - fn main() {} + fn main() { + let _ = S {}; // silence S never constructed warning + } "#; assert_no_errors(src); } @@ -2743,7 +2788,9 @@ fn uses_self_type_in_trait_where_clause() { } - fn main() {} + fn main() { + let _ = Bar {}; // silence Bar never constructed warning + } "#; let errors = get_program_errors(src); @@ -2903,6 +2950,8 @@ fn as_trait_path_syntax_resolves_outside_impl() { // AsTraitPath syntax is a bit silly when associated types // are explicitly specified let _: i64 = 1 as >::Assoc; + + let _ = Bar {}; // silence Bar never constructed warning } "#; @@ -2934,6 +2983,8 @@ fn as_trait_path_syntax_no_impl() { fn main() { let _: i64 = 1 as >::Assoc; + + let _ = Bar {}; // silence Bar never constructed warning } "#; @@ -3047,7 +3098,9 @@ fn errors_once_on_unused_import_that_is_not_accessible() { struct Foo {} } use moo::Foo; - fn main() {} + fn main() { + let _ = Foo {}; + } "#; let errors = get_program_errors(src); @@ -3088,3 +3141,60 @@ fn trait_unconstrained_methods_typechecked_correctly() { println!("{errors:?}"); assert_eq!(errors.len(), 0); } + +#[test] +fn errors_if_type_alias_aliases_more_private_type() { + let src = r#" + struct Foo {} + pub type Bar = Foo; + + pub fn no_unused_warnings(_b: Bar) { + let _ = Foo {}; + } + + fn main() {} + "#; + + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + + let CompilationError::ResolverError(ResolverError::TypeIsMorePrivateThenItem { + typ, item, .. + }) = &errors[0].0 + else { + panic!("Expected an unused item error"); + }; + + assert_eq!(typ, "Foo"); + assert_eq!(item, "Bar"); +} + +#[test] +fn errors_if_type_alias_aliases_more_private_type_in_generic() { + let src = r#" + pub struct Generic { value: T } + + struct Foo {} + pub type Bar = Generic; + + pub fn no_unused_warnings(_b: Bar) { + let _ = Foo {}; + let _ = Generic { value: 1 }; + } + + fn main() {} + "#; + + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + + let CompilationError::ResolverError(ResolverError::TypeIsMorePrivateThenItem { + typ, item, .. + }) = &errors[0].0 + else { + panic!("Expected an unused item error"); + }; + + assert_eq!(typ, "Foo"); + assert_eq!(item, "Bar"); +} diff --git a/compiler/noirc_frontend/src/tests/unused_items.rs b/compiler/noirc_frontend/src/tests/unused_items.rs index 13787dd6955..be3b2996e5a 100644 --- a/compiler/noirc_frontend/src/tests/unused_items.rs +++ b/compiler/noirc_frontend/src/tests/unused_items.rs @@ -31,14 +31,13 @@ fn errors_on_unused_private_import() { let errors = get_program_errors(src); assert_eq!(errors.len(), 1); - let CompilationError::ResolverError(ResolverError::UnusedItem { ident, item_type }) = - &errors[0].0 + let CompilationError::ResolverError(ResolverError::UnusedItem { ident, item }) = &errors[0].0 else { panic!("Expected an unused item error"); }; assert_eq!(ident.to_string(), "bar"); - assert_eq!(*item_type, "import"); + assert_eq!(item.item_type(), "import"); } #[test] @@ -67,14 +66,13 @@ fn errors_on_unused_pub_crate_import() { let errors = get_program_errors(src); assert_eq!(errors.len(), 1); - let CompilationError::ResolverError(ResolverError::UnusedItem { ident, item_type }) = - &errors[0].0 + let CompilationError::ResolverError(ResolverError::UnusedItem { ident, item }) = &errors[0].0 else { panic!("Expected an unused item error"); }; assert_eq!(ident.to_string(), "bar"); - assert_eq!(*item_type, "import"); + assert_eq!(item.item_type(), "import"); } #[test] @@ -99,14 +97,13 @@ fn errors_on_unused_function() { let errors = get_program_errors(src); assert_eq!(errors.len(), 1); - let CompilationError::ResolverError(ResolverError::UnusedItem { ident, item_type }) = - &errors[0].0 + let CompilationError::ResolverError(ResolverError::UnusedItem { ident, item }) = &errors[0].0 else { panic!("Expected an unused item error"); }; assert_eq!(ident.to_string(), "foo"); - assert_eq!(*item_type, "function"); + assert_eq!(item.item_type(), "function"); } #[test] @@ -123,14 +120,13 @@ fn errors_on_unused_struct() { let errors = get_program_errors(src); assert_eq!(errors.len(), 1); - let CompilationError::ResolverError(ResolverError::UnusedItem { ident, item_type }) = - &errors[0].0 + let CompilationError::ResolverError(ResolverError::UnusedItem { ident, item }) = &errors[0].0 else { panic!("Expected an unused item error"); }; assert_eq!(ident.to_string(), "Foo"); - assert_eq!(*item_type, "struct"); + assert_eq!(item.item_type(), "struct"); } #[test] @@ -151,14 +147,13 @@ fn errors_on_unused_trait() { let errors = get_program_errors(src); assert_eq!(errors.len(), 1); - let CompilationError::ResolverError(ResolverError::UnusedItem { ident, item_type }) = - &errors[0].0 + let CompilationError::ResolverError(ResolverError::UnusedItem { ident, item }) = &errors[0].0 else { panic!("Expected an unused item error"); }; assert_eq!(ident.to_string(), "Foo"); - assert_eq!(*item_type, "trait"); + assert_eq!(item.item_type(), "trait"); } #[test] @@ -184,14 +179,13 @@ fn errors_on_unused_type_alias() { let errors = get_program_errors(src); assert_eq!(errors.len(), 1); - let CompilationError::ResolverError(ResolverError::UnusedItem { ident, item_type }) = - &errors[0].0 + let CompilationError::ResolverError(ResolverError::UnusedItem { ident, item }) = &errors[0].0 else { panic!("Expected an unused item error"); }; assert_eq!(ident.to_string(), "Foo"); - assert_eq!(*item_type, "type alias"); + assert_eq!(item.item_type(), "type alias"); } #[test] diff --git a/compiler/noirc_frontend/src/usage_tracker.rs b/compiler/noirc_frontend/src/usage_tracker.rs index 4224ba62df9..cd1041cc36f 100644 --- a/compiler/noirc_frontend/src/usage_tracker.rs +++ b/compiler/noirc_frontend/src/usage_tracker.rs @@ -7,7 +7,7 @@ use crate::{ node_interner::{FuncId, TraitId, TypeAliasId}, }; -#[derive(Debug)] +#[derive(Debug, Copy, Clone, PartialEq, Eq)] pub enum UnusedItem { Import, Function(FuncId), @@ -47,8 +47,29 @@ impl UsageTracker { } } + /// Marks an item as being referenced. This doesn't always makes the item as used. For example + /// if a struct is referenced it will still be considered unused unless it's constructed somewhere. + pub(crate) fn mark_as_referenced(&mut self, current_mod_id: ModuleId, name: &Ident) { + let Some(items) = self.unused_items.get_mut(¤t_mod_id) else { + return; + }; + + let Some(unused_item) = items.get(name) else { + return; + }; + + if let UnusedItem::Struct(_) = unused_item { + return; + } + + items.remove(name); + } + + /// Marks an item as being used. pub(crate) fn mark_as_used(&mut self, current_mod_id: ModuleId, name: &Ident) { - self.unused_items.entry(current_mod_id).or_default().remove(name); + if let Some(items) = self.unused_items.get_mut(¤t_mod_id) { + items.remove(name); + }; } pub(crate) fn unused_items(&self) -> &HashMap> { diff --git a/noir_stdlib/src/meta/mod.nr b/noir_stdlib/src/meta/mod.nr index 21d80e76bfc..0dc2e5fa714 100644 --- a/noir_stdlib/src/meta/mod.nr +++ b/noir_stdlib/src/meta/mod.nr @@ -243,11 +243,15 @@ mod tests { } // docs:end:big-derive-usage-example + impl DoNothing for Bar { + fn do_nothing(_: Self) {} + } + // This function is just to remove unused warnings fn remove_unused_warnings() { - let _: Bar = crate::panic::panic(f""); - let _: MyStruct = crate::panic::panic(f""); - let _: MyOtherStruct = crate::panic::panic(f""); + let _: Bar = Bar { x: 1, y: [2, 3] }; + let _: MyStruct = MyStruct { my_field: 1 }; + let _: MyOtherStruct = MyOtherStruct { my_other_field: 2 }; let _ = derive_do_nothing(crate::panic::panic(f"")); let _ = derive_do_nothing_alt(crate::panic::panic(f"")); remove_unused_warnings(); diff --git a/tooling/lsp/src/modules.rs b/tooling/lsp/src/modules.rs index c8f7430c997..f1eff3b5a7d 100644 --- a/tooling/lsp/src/modules.rs +++ b/tooling/lsp/src/modules.rs @@ -18,15 +18,6 @@ pub(crate) fn get_parent_module( interner.reference_module(reference_id).copied() } -pub(crate) fn get_parent_module_id( - def_maps: &BTreeMap, - module_id: ModuleId, -) -> Option { - let crate_def_map = &def_maps[&module_id.krate]; - let module_data = &crate_def_map.modules()[module_id.local_id.0]; - module_data.parent.map(|parent| ModuleId { krate: module_id.krate, local_id: parent }) -} - pub(crate) fn module_def_id_to_reference_id(module_def_id: ModuleDefId) -> ReferenceId { match module_def_id { ModuleDefId::ModuleId(id) => ReferenceId::Module(id), diff --git a/tooling/lsp/src/requests/code_action/import_or_qualify.rs b/tooling/lsp/src/requests/code_action/import_or_qualify.rs index 03a953bde85..0d97ccde2ed 100644 --- a/tooling/lsp/src/requests/code_action/import_or_qualify.rs +++ b/tooling/lsp/src/requests/code_action/import_or_qualify.rs @@ -7,7 +7,7 @@ use noirc_frontend::{ use crate::{ byte_span_to_range, - modules::{get_parent_module_id, relative_module_full_path, relative_module_id_path}, + modules::{relative_module_full_path, relative_module_id_path}, }; use super::CodeActionFinder; @@ -28,7 +28,7 @@ impl<'a> CodeActionFinder<'a> { return; } - let current_module_parent_id = get_parent_module_id(self.def_maps, self.module_id); + let current_module_parent_id = self.module_id.parent(self.def_maps); // The Path doesn't resolve to anything so it means it's an error and maybe we // can suggest an import or to fully-qualify the path. diff --git a/tooling/lsp/src/requests/completion/auto_import.rs b/tooling/lsp/src/requests/completion/auto_import.rs index 2713ae252bf..20b126a248d 100644 --- a/tooling/lsp/src/requests/completion/auto_import.rs +++ b/tooling/lsp/src/requests/completion/auto_import.rs @@ -1,7 +1,7 @@ use lsp_types::{Position, Range, TextEdit}; use noirc_frontend::macros_api::ModuleDefId; -use crate::modules::{get_parent_module_id, relative_module_full_path, relative_module_id_path}; +use crate::modules::{relative_module_full_path, relative_module_id_path}; use super::{ kinds::{FunctionCompletionKind, FunctionKind, RequestedItems}, @@ -17,7 +17,7 @@ impl<'a> NodeFinder<'a> { requested_items: RequestedItems, function_completion_kind: FunctionCompletionKind, ) { - let current_module_parent_id = get_parent_module_id(self.def_maps, self.module_id); + let current_module_parent_id = self.module_id.parent(self.def_maps); for (name, entries) in self.interner.get_auto_import_names() { if !name_matches(name, prefix) { diff --git a/tooling/lsp/src/trait_impl_method_stub_generator.rs b/tooling/lsp/src/trait_impl_method_stub_generator.rs index ae12bac4c06..56d2e5e1ea1 100644 --- a/tooling/lsp/src/trait_impl_method_stub_generator.rs +++ b/tooling/lsp/src/trait_impl_method_stub_generator.rs @@ -197,12 +197,7 @@ impl<'a> TraitImplMethodStubGenerator<'a> { } } - let module_id = struct_type.id.module_id(); - let module_data = &self.def_maps[&module_id.krate].modules()[module_id.local_id.0]; - let parent_module_local_id = module_data.parent.unwrap(); - let parent_module_id = - ModuleId { krate: module_id.krate, local_id: parent_module_local_id }; - + let parent_module_id = struct_type.id.parent_module_id(self.def_maps); let current_module_parent_id = current_module_data .parent .map(|parent| ModuleId { krate: self.module_id.krate, local_id: parent });