Skip to content

Commit

Permalink
fix: error on unbound generics in structs (#5619)
Browse files Browse the repository at this point in the history
# Description

## Problem

Resolves #5436

## Summary

The monomorphizer checks that no types exist that aren't unbound and
can't have a default type. This is also the case for structs, but this
check is done on field types after they are paired with generics. For
example:

```rust
struct Foo<T> {
  x: T
}

fn main() {
  let _ = Foo { x: 1 };
}
```

Here we pair `T` with the generic type of 1, then it's defaulted to
`Field`, no error.

However, it can happen that a generic parameter isn't mentioned at all
in a struct field. For example:

```rust
struct Foo<T> {
}

fn main() {
  let _ = Foo {};
}
```

Here `T` isn't paired with anything, so it's left unbound. However,
because we only check that the resulting field types aren't unbound, no
error happens.

The solution in this PR is to check that generic parameters aren't
unbound. This required a method like `convert_type`, called
`check_type`, that only checks for this errors. The reason is that I
wanted to avoid converting types twice. But let me know if that would be
fine (or if there's another solution for this).

## Additional Context

In Rust if a generic parameter isn't used in a field, an error about
that is made. However, no error happens if the generic parameter is a
`const` (similar to a `let N: u32` parameter in Noir) so here I chose to
error un unbound generics, not check if a generic is unused.

## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] 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.

---------

Co-authored-by: jfecher <[email protected]>
  • Loading branch information
asterite and jfecher authored Jul 31, 2024
1 parent b7e4f42 commit efef6b4
Show file tree
Hide file tree
Showing 5 changed files with 140 additions and 7 deletions.
115 changes: 108 additions & 7 deletions compiler/noirc_frontend/src/monomorphization/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -383,8 +383,8 @@ impl<'interner> Monomorphizer<'interner> {
self.parameter(field, &typ, new_params)?;
}
}
HirPattern::Struct(_, fields, _) => {
let struct_field_types = unwrap_struct_type(typ);
HirPattern::Struct(_, fields, location) => {
let struct_field_types = unwrap_struct_type(typ, *location)?;
assert_eq!(struct_field_types.len(), fields.len());

let mut fields =
Expand Down Expand Up @@ -663,8 +663,10 @@ impl<'interner> Monomorphizer<'interner> {
constructor: HirConstructorExpression,
id: node_interner::ExprId,
) -> Result<ast::Expression, MonomorphizationError> {
let location = self.interner.expr_location(&id);

let typ = self.interner.id_type(id);
let field_types = unwrap_struct_type(&typ);
let field_types = unwrap_struct_type(&typ, location)?;

let field_type_map = btree_map(&field_types, |x| x.clone());

Expand Down Expand Up @@ -740,8 +742,8 @@ impl<'interner> Monomorphizer<'interner> {
let fields = unwrap_tuple_type(typ);
self.unpack_tuple_pattern(value, patterns.into_iter().zip(fields))
}
HirPattern::Struct(_, patterns, _) => {
let fields = unwrap_struct_type(typ);
HirPattern::Struct(_, patterns, location) => {
let fields = unwrap_struct_type(typ, location)?;
assert_eq!(patterns.len(), fields.len());

let mut patterns =
Expand Down Expand Up @@ -975,12 +977,24 @@ impl<'interner> Monomorphizer<'interner> {
}

HirType::Struct(def, args) => {
// Not all generic arguments may be used in a struct's fields so we have to check
// the arguments as well as the fields in case any need to be defaulted or are unbound.
for arg in args {
Self::check_type(arg, location)?;
}

let fields = def.borrow().get_fields(args);
let fields = try_vecmap(fields, |(_, field)| Self::convert_type(&field, location))?;
ast::Type::Tuple(fields)
}

HirType::Alias(def, args) => {
// Similar to the struct case above: generics of an alias might not end up being
// used in the type that is aliased.
for arg in args {
Self::check_type(arg, location)?;
}

Self::convert_type(&def.borrow().get_type(args), location)?
}

Expand Down Expand Up @@ -1019,6 +1033,83 @@ impl<'interner> Monomorphizer<'interner> {
})
}

// Similar to `convert_type` but returns an error if any type variable can't be defaulted.
fn check_type(typ: &HirType, location: Location) -> Result<(), MonomorphizationError> {
match typ {
HirType::FieldElement
| HirType::Integer(..)
| HirType::Bool
| HirType::String(..)
| HirType::Unit
| HirType::TraitAsType(..)
| HirType::Forall(_, _)
| HirType::Constant(_)
| HirType::Error
| HirType::Quoted(_) => Ok(()),
HirType::FmtString(_size, fields) => Self::check_type(fields.as_ref(), location),
HirType::Array(_length, element) => Self::check_type(element.as_ref(), location),
HirType::Slice(element) => Self::check_type(element.as_ref(), location),
HirType::NamedGeneric(binding, _, _) => {
if let TypeBinding::Bound(binding) = &*binding.borrow() {
return Self::check_type(binding, location);
}

Ok(())
}

HirType::TypeVariable(binding, kind) => {
if let TypeBinding::Bound(binding) = &*binding.borrow() {
return Self::check_type(binding, location);
}

// Default any remaining unbound type variables.
// This should only happen if the variable in question is unused
// and within a larger generic type.
let default = match kind.default_type() {
Some(typ) => typ,
None => return Err(MonomorphizationError::TypeAnnotationsNeeded { location }),
};

Self::check_type(&default, location)
}

HirType::Struct(_def, args) => {
for arg in args {
Self::check_type(arg, location)?;
}

Ok(())
}

HirType::Alias(_def, args) => {
for arg in args {
Self::check_type(arg, location)?;
}

Ok(())
}

HirType::Tuple(fields) => {
for field in fields {
Self::check_type(field, location)?;
}

Ok(())
}

HirType::Function(args, ret, env) => {
for arg in args {
Self::check_type(arg, location)?;
}

Self::check_type(ret, location)?;
Self::check_type(env, location)
}

HirType::MutableReference(element) => Self::check_type(element, location),
}
}

fn is_function_closure(&self, t: ast::Type) -> bool {
if self.is_function_closure_type(&t) {
true
Expand Down Expand Up @@ -1753,9 +1844,19 @@ fn unwrap_tuple_type(typ: &HirType) -> Vec<HirType> {
}
}

fn unwrap_struct_type(typ: &HirType) -> Vec<(String, HirType)> {
fn unwrap_struct_type(
typ: &HirType,
location: Location,
) -> Result<Vec<(String, HirType)>, MonomorphizationError> {
match typ.follow_bindings() {
HirType::Struct(def, args) => def.borrow().get_fields(&args),
HirType::Struct(def, args) => {
// Some of args might not be mentioned in fields, so we need to check that they aren't unbound.
for arg in &args {
Monomorphizer::check_type(arg, location)?;
}

Ok(def.borrow().get_fields(&args))
}
other => unreachable!("unwrap_struct_type: expected struct, found {:?}", other),
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
name = "type_annotation_needed_on_struct_constructor"
type = "bin"
authors = [""]
compiler_version = ">=0.31.0"

[dependencies]
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
struct Foo<T> {
}

fn main() {
let foo = Foo {};
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
name = "type_annotation_needed_on_struct_new"
type = "bin"
authors = [""]
compiler_version = ">=0.31.0"

[dependencies]
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
struct Foo<T> {
}

impl <T> Foo<T> {
fn new() -> Foo<T> {
Foo {}
}
}

fn main() {
let foo = Foo::new();
}

0 comments on commit efef6b4

Please sign in to comment.