From 3222875d68c13c3d4181ee3d16a56f2e05b881ff Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 26 Jul 2024 18:17:04 -0300 Subject: [PATCH 1/5] fix: error on unbound struct generic types --- .../src/monomorphization/mod.rs | 105 ++++++++++++++++++ .../Nargo.toml | 7 ++ .../src/main.nr | 12 ++ 3 files changed, 124 insertions(+) create mode 100644 test_programs/compile_failure/type_annotation_needed_on_struct_new/Nargo.toml create mode 100644 test_programs/compile_failure/type_annotation_needed_on_struct_new/src/main.nr diff --git a/compiler/noirc_frontend/src/monomorphization/mod.rs b/compiler/noirc_frontend/src/monomorphization/mod.rs index c63a6961da5..b59d1fae1d0 100644 --- a/compiler/noirc_frontend/src/monomorphization/mod.rs +++ b/compiler/noirc_frontend/src/monomorphization/mod.rs @@ -975,12 +975,37 @@ impl<'interner> Monomorphizer<'interner> { } HirType::Struct(def, args) => { + // Even though we later call `convert_type` on `fields`, it might be the types + // in `args` end up not being part of fields. For example: + // + // struct Foo {} + // + // fn main() { + // let _ = Foo {}; + // } + // + // In the above case args is `[N]` but fields is `[]`, so T will never be checked. + // However, we want the above program to not compile. + for arg in args { + if let Some(error) = Self::check_type(arg, location) { + return Err(error); + } + } + 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 { + if let Some(error) = Self::check_type(arg, location) { + return Err(error); + } + } + Self::convert_type(&def.borrow().get_type(args), location)? } @@ -1019,6 +1044,86 @@ 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) -> Option { + match typ { + HirType::FieldElement + | HirType::Integer(..) + | HirType::Bool + | HirType::String(..) + | HirType::Unit => None, + 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::TraitAsType(..) => { + unreachable!("All TraitAsType should be replaced before calling convert_type"); + } + HirType::NamedGeneric(binding, _, _) => { + if let TypeBinding::Bound(binding) = &*binding.borrow() { + return Self::check_type(binding, location); + } + + None + } + + 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 Some(MonomorphizationError::TypeAnnotationsNeeded { location }), + }; + + Self::check_type(&default, location) + } + + HirType::Struct(_def, args) => { + for arg in args { + Self::check_type(arg, location)?; + } + + None + } + + HirType::Alias(_def, args) => { + for arg in args { + Self::check_type(arg, location)?; + } + + None + } + + HirType::Tuple(fields) => { + for field in fields { + Self::check_type(field, location)?; + } + + None + } + + 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), + + HirType::Forall(_, _) | HirType::Constant(_) | HirType::Error => { + unreachable!("Unexpected type {} found", typ) + } + HirType::Quoted(_) => unreachable!("Tried to translate Code type into runtime code"), + } + } + fn is_function_closure(&self, t: ast::Type) -> bool { if self.is_function_closure_type(&t) { true diff --git a/test_programs/compile_failure/type_annotation_needed_on_struct_new/Nargo.toml b/test_programs/compile_failure/type_annotation_needed_on_struct_new/Nargo.toml new file mode 100644 index 00000000000..cb53d2924f4 --- /dev/null +++ b/test_programs/compile_failure/type_annotation_needed_on_struct_new/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "type_annotation_needed_on_struct_new" +type = "bin" +authors = [""] +compiler_version = ">=0.31.0" + +[dependencies] \ No newline at end of file diff --git a/test_programs/compile_failure/type_annotation_needed_on_struct_new/src/main.nr b/test_programs/compile_failure/type_annotation_needed_on_struct_new/src/main.nr new file mode 100644 index 00000000000..f740dfa6d37 --- /dev/null +++ b/test_programs/compile_failure/type_annotation_needed_on_struct_new/src/main.nr @@ -0,0 +1,12 @@ +struct Foo { +} + +impl Foo { + fn new() -> Foo { + Foo {} + } +} + +fn main() { + let foo = Foo::new(); +} From be4928250e549f48cd657dd37b0ba7d21aad8f15 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 26 Jul 2024 18:26:53 -0300 Subject: [PATCH 2/5] fix: error on constructor with unbound generics --- .../src/monomorphization/mod.rs | 28 ++++++++++++++----- .../Nargo.toml | 7 +++++ .../src/main.nr | 6 ++++ 3 files changed, 34 insertions(+), 7 deletions(-) create mode 100644 test_programs/compile_failure/type_annotation_needed_on_struct_constructor/Nargo.toml create mode 100644 test_programs/compile_failure/type_annotation_needed_on_struct_constructor/src/main.nr diff --git a/compiler/noirc_frontend/src/monomorphization/mod.rs b/compiler/noirc_frontend/src/monomorphization/mod.rs index b59d1fae1d0..f22548ac4c6 100644 --- a/compiler/noirc_frontend/src/monomorphization/mod.rs +++ b/compiler/noirc_frontend/src/monomorphization/mod.rs @@ -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 = @@ -663,8 +663,10 @@ impl<'interner> Monomorphizer<'interner> { constructor: HirConstructorExpression, id: node_interner::ExprId, ) -> Result { + 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()); @@ -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 = @@ -1858,9 +1860,21 @@ fn unwrap_tuple_type(typ: &HirType) -> Vec { } } -fn unwrap_struct_type(typ: &HirType) -> Vec<(String, HirType)> { +fn unwrap_struct_type( + typ: &HirType, + location: Location, +) -> Result, 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 { + if let Some(error) = Monomorphizer::check_type(arg, location) { + return Err(error); + } + } + + Ok(def.borrow().get_fields(&args)) + } other => unreachable!("unwrap_struct_type: expected struct, found {:?}", other), } } diff --git a/test_programs/compile_failure/type_annotation_needed_on_struct_constructor/Nargo.toml b/test_programs/compile_failure/type_annotation_needed_on_struct_constructor/Nargo.toml new file mode 100644 index 00000000000..ac7933fa250 --- /dev/null +++ b/test_programs/compile_failure/type_annotation_needed_on_struct_constructor/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "type_annotation_needed_on_struct_constructor" +type = "bin" +authors = [""] +compiler_version = ">=0.31.0" + +[dependencies] \ No newline at end of file diff --git a/test_programs/compile_failure/type_annotation_needed_on_struct_constructor/src/main.nr b/test_programs/compile_failure/type_annotation_needed_on_struct_constructor/src/main.nr new file mode 100644 index 00000000000..5207210dfbf --- /dev/null +++ b/test_programs/compile_failure/type_annotation_needed_on_struct_constructor/src/main.nr @@ -0,0 +1,6 @@ +struct Foo { +} + +fn main() { + let foo = Foo {}; +} From 9d76341c4b9992a8b65aa164ebab59d82e62143f Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 26 Jul 2024 18:50:29 -0300 Subject: [PATCH 3/5] No need to panic on unexpected types during check --- .../noirc_frontend/src/monomorphization/mod.rs | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/compiler/noirc_frontend/src/monomorphization/mod.rs b/compiler/noirc_frontend/src/monomorphization/mod.rs index f22548ac4c6..deb7e6f8f03 100644 --- a/compiler/noirc_frontend/src/monomorphization/mod.rs +++ b/compiler/noirc_frontend/src/monomorphization/mod.rs @@ -1053,13 +1053,15 @@ impl<'interner> Monomorphizer<'interner> { | HirType::Integer(..) | HirType::Bool | HirType::String(..) - | HirType::Unit => None, + | HirType::Unit + | HirType::TraitAsType(..) + | HirType::Forall(_, _) + | HirType::Constant(_) + | HirType::Error + | HirType::Quoted(_) => None, 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::TraitAsType(..) => { - unreachable!("All TraitAsType should be replaced before calling convert_type"); - } HirType::NamedGeneric(binding, _, _) => { if let TypeBinding::Bound(binding) = &*binding.borrow() { return Self::check_type(binding, location); @@ -1118,11 +1120,6 @@ impl<'interner> Monomorphizer<'interner> { } HirType::MutableReference(element) => Self::check_type(element, location), - - HirType::Forall(_, _) | HirType::Constant(_) | HirType::Error => { - unreachable!("Unexpected type {} found", typ) - } - HirType::Quoted(_) => unreachable!("Tried to translate Code type into runtime code"), } } From af359599e9f287b6bbe3df511948122dd010f12e Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 29 Jul 2024 15:07:19 -0300 Subject: [PATCH 4/5] Update compiler/noirc_frontend/src/monomorphization/mod.rs Co-authored-by: jfecher --- compiler/noirc_frontend/src/monomorphization/mod.rs | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/compiler/noirc_frontend/src/monomorphization/mod.rs b/compiler/noirc_frontend/src/monomorphization/mod.rs index deb7e6f8f03..5bddff56a05 100644 --- a/compiler/noirc_frontend/src/monomorphization/mod.rs +++ b/compiler/noirc_frontend/src/monomorphization/mod.rs @@ -977,17 +977,8 @@ impl<'interner> Monomorphizer<'interner> { } HirType::Struct(def, args) => { - // Even though we later call `convert_type` on `fields`, it might be the types - // in `args` end up not being part of fields. For example: - // - // struct Foo {} - // - // fn main() { - // let _ = Foo {}; - // } - // - // In the above case args is `[N]` but fields is `[]`, so T will never be checked. - // However, we want the above program to not compile. + // 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 { if let Some(error) = Self::check_type(arg, location) { return Err(error); From 9fbdfecb3321001dbab1d57e72e7a61aa77fc672 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 29 Jul 2024 15:11:31 -0300 Subject: [PATCH 5/5] Let `check_type` return a Result --- .../src/monomorphization/mod.rs | 26 +++++++------------ 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/compiler/noirc_frontend/src/monomorphization/mod.rs b/compiler/noirc_frontend/src/monomorphization/mod.rs index 5bddff56a05..e6506a5fde6 100644 --- a/compiler/noirc_frontend/src/monomorphization/mod.rs +++ b/compiler/noirc_frontend/src/monomorphization/mod.rs @@ -980,9 +980,7 @@ impl<'interner> Monomorphizer<'interner> { // 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 { - if let Some(error) = Self::check_type(arg, location) { - return Err(error); - } + Self::check_type(arg, location)?; } let fields = def.borrow().get_fields(args); @@ -994,9 +992,7 @@ impl<'interner> Monomorphizer<'interner> { // 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 { - if let Some(error) = Self::check_type(arg, location) { - return Err(error); - } + Self::check_type(arg, location)?; } Self::convert_type(&def.borrow().get_type(args), location)? @@ -1038,7 +1034,7 @@ 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) -> Option { + fn check_type(typ: &HirType, location: Location) -> Result<(), MonomorphizationError> { match typ { HirType::FieldElement | HirType::Integer(..) @@ -1049,7 +1045,7 @@ impl<'interner> Monomorphizer<'interner> { | HirType::Forall(_, _) | HirType::Constant(_) | HirType::Error - | HirType::Quoted(_) => None, + | 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), @@ -1058,7 +1054,7 @@ impl<'interner> Monomorphizer<'interner> { return Self::check_type(binding, location); } - None + Ok(()) } HirType::TypeVariable(binding, kind) => { @@ -1071,7 +1067,7 @@ impl<'interner> Monomorphizer<'interner> { // and within a larger generic type. let default = match kind.default_type() { Some(typ) => typ, - None => return Some(MonomorphizationError::TypeAnnotationsNeeded { location }), + None => return Err(MonomorphizationError::TypeAnnotationsNeeded { location }), }; Self::check_type(&default, location) @@ -1082,7 +1078,7 @@ impl<'interner> Monomorphizer<'interner> { Self::check_type(arg, location)?; } - None + Ok(()) } HirType::Alias(_def, args) => { @@ -1090,7 +1086,7 @@ impl<'interner> Monomorphizer<'interner> { Self::check_type(arg, location)?; } - None + Ok(()) } HirType::Tuple(fields) => { @@ -1098,7 +1094,7 @@ impl<'interner> Monomorphizer<'interner> { Self::check_type(field, location)?; } - None + Ok(()) } HirType::Function(args, ret, env) => { @@ -1856,9 +1852,7 @@ fn unwrap_struct_type( 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 { - if let Some(error) = Monomorphizer::check_type(arg, location) { - return Err(error); - } + Monomorphizer::check_type(arg, location)?; } Ok(def.borrow().get_fields(&args))