Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: detect unconstructed structs #6061

Merged
merged 14 commits into from
Sep 25, 2024
8 changes: 8 additions & 0 deletions compiler/noirc_frontend/src/elaborator/expressions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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<StructType>) {
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.
Expand Down
67 changes: 58 additions & 9 deletions compiler/noirc_frontend/src/elaborator/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,

Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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 {
Expand Down
10 changes: 1 addition & 9 deletions compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down Expand Up @@ -505,14 +504,7 @@ fn struct_def_module(
) -> IResult<Value> {
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))
}

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 @@ -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)
})
Expand Down
8 changes: 6 additions & 2 deletions compiler/noirc_frontend/src/hir/def_map/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<ModuleId> {
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<CrateId, CrateDefMap>;
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
6 changes: 6 additions & 0 deletions compiler/noirc_frontend/src/node_interner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)]
Expand Down
Loading
Loading