From b38d6a834616bad66f3d261fcfd84d29084f2182 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 17 Sep 2024 13:11:54 -0300 Subject: [PATCH] feat: detect unconstructed structs --- .../src/elaborator/expressions.rs | 12 +++++++ .../src/hir/def_collector/dc_crate.rs | 2 +- .../src/hir/resolution/errors.rs | 33 ++++++++++++++----- .../src/hir/resolution/import.rs | 4 +-- compiler/noirc_frontend/src/tests.rs | 32 ++++++++---------- compiler/noirc_frontend/src/usage_tracker.rs | 25 ++++++++++++-- noir_stdlib/src/meta/mod.nr | 10 ++++-- 7 files changed, 83 insertions(+), 35 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/expressions.rs b/compiler/noirc_frontend/src/elaborator/expressions.rs index 311c8f401be..d55210e3c99 100644 --- a/compiler/noirc_frontend/src/elaborator/expressions.rs +++ b/compiler/noirc_frontend/src/elaborator/expressions.rs @@ -10,6 +10,7 @@ use crate::{ }, hir::{ comptime::{self, InterpreterError}, + def_map::ModuleId, resolution::errors::ResolverError, type_check::{generics::TraitGenerics, TypeCheckError}, }, @@ -532,6 +533,8 @@ impl<'context> Elaborator<'context> { } }; + self.mark_struct_as_constructed(r#type.clone(), &last_segment.ident); + let turbofish_span = last_segment.turbofish_span(); let struct_generics = self.resolve_struct_turbofish_generics( @@ -561,6 +564,15 @@ impl<'context> Elaborator<'context> { (expr, Type::Struct(struct_type, generics)) } + fn mark_struct_as_constructed(&mut self, struct_type: Shared, name: &Ident) { + let struct_module_id = struct_type.borrow().id.module_id(); + 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 }; + self.interner.usage_tracker.mark_as_used(parent_module_id, 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/hir/def_collector/dc_crate.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index 6265d0e65f2..007bc71c0d2 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -508,7 +508,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/resolution/errors.rs b/compiler/noirc_frontend/src/hir/resolution/errors.rs index 08f57ae562a..1091204ef04 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,26 @@ 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/tests.rs b/compiler/noirc_frontend/src/tests.rs index 320adb2bf3b..3f90ada98b8 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -3241,14 +3241,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] @@ -3277,14 +3276,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] @@ -3413,14 +3411,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] @@ -3429,6 +3426,8 @@ fn errors_on_unused_struct() { struct Foo {} struct Bar {} + pub fn foo(_: Foo) {} + fn main() { let _ = Bar {}; } @@ -3437,14 +3436,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] @@ -3465,14 +3463,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] @@ -3489,14 +3486,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 23c97370b46..56c367cccf1 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), @@ -51,8 +51,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 2f244e66a62..df4b3ef5746 100644 --- a/noir_stdlib/src/meta/mod.nr +++ b/noir_stdlib/src/meta/mod.nr @@ -238,11 +238,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();