Skip to content

Commit

Permalink
feat: detect unconstructed structs (#6061)
Browse files Browse the repository at this point in the history
# 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.
  • Loading branch information
asterite authored Sep 25, 2024
1 parent c679bc6 commit bcb438b
Show file tree
Hide file tree
Showing 16 changed files with 272 additions and 85 deletions.
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

0 comments on commit bcb438b

Please sign in to comment.