Skip to content

Commit

Permalink
feat: detect unconstructed structs
Browse files Browse the repository at this point in the history
  • Loading branch information
asterite committed Sep 17, 2024
1 parent c5697da commit 9d944b7
Show file tree
Hide file tree
Showing 7 changed files with 81 additions and 35 deletions.
12 changes: 12 additions & 0 deletions compiler/noirc_frontend/src/elaborator/expressions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use crate::{
},
hir::{
comptime::{self, InterpreterError},
def_map::ModuleId,
resolution::errors::ResolverError,
type_check::{generics::TraitGenerics, TypeCheckError},
},
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -561,6 +564,15 @@ impl<'context> Elaborator<'context> {
(expr, Type::Struct(struct_type, generics))
}

fn mark_struct_as_constructed(&mut self, struct_type: Shared<StructType>, 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.
Expand Down
2 changes: 1 addition & 1 deletion compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
})
Expand Down
31 changes: 22 additions & 9 deletions compiler/noirc_frontend/src/hir/resolution/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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")]
Expand Down Expand Up @@ -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
}
Expand Down
4 changes: 2 additions & 2 deletions compiler/noirc_frontend/src/hir/resolution/import.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<PathResolutionError> = None;
for (index, (last_segment, current_segment)) in
Expand Down Expand Up @@ -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;
}
Expand Down
32 changes: 14 additions & 18 deletions compiler/noirc_frontend/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down Expand Up @@ -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]
Expand Down Expand Up @@ -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]
Expand All @@ -3429,6 +3426,8 @@ fn errors_on_unused_struct() {
struct Foo {}
struct Bar {}
pub fn foo(_: Foo) {}
fn main() {
let _ = Bar {};
}
Expand All @@ -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]
Expand All @@ -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]
Expand All @@ -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]
Expand Down
25 changes: 23 additions & 2 deletions compiler/noirc_frontend/src/usage_tracker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down Expand Up @@ -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(&current_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(&current_mod_id) {
items.remove(name);
};
}

pub(crate) fn unused_items(&self) -> &HashMap<ModuleId, HashMap<Ident, UnusedItem>> {
Expand Down
10 changes: 7 additions & 3 deletions noir_stdlib/src/meta/mod.nr
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down

0 comments on commit 9d944b7

Please sign in to comment.