From b7e4f424f3fc461122447c30989a3ce52ddea09d Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 31 Jul 2024 09:25:51 -0300 Subject: [PATCH 1/3] fix: allow using Self for function calls (#5629) # Description ## Problem Resolves #1791 ## Summary When resolving a path (any path), we now check if the first segment is `Self` (a similar check is done in another places of our codebase). If so, resolve the next path segments relative to that type. Only struct types are supported here... in theory a trait type can also show up, but that case is handled as a special case in the compiler. I'll create a follow-up PR removing that special case and seeing if it works. ## Additional Context None. ## 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. --- .../noirc_frontend/src/elaborator/scope.rs | 31 ++++++- compiler/noirc_frontend/src/tests.rs | 80 +++++++++++++++++++ 2 files changed, 109 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/scope.rs b/compiler/noirc_frontend/src/elaborator/scope.rs index 73aed9bf06c..258c32d4427 100644 --- a/compiler/noirc_frontend/src/elaborator/scope.rs +++ b/compiler/noirc_frontend/src/elaborator/scope.rs @@ -1,6 +1,6 @@ use noirc_errors::{Location, Spanned}; -use crate::ast::ERROR_IDENT; +use crate::ast::{PathKind, ERROR_IDENT}; use crate::hir::def_map::{LocalModuleId, ModuleId}; use crate::hir::resolution::path_resolver::{PathResolver, StandardPathResolver}; use crate::hir::scope::{Scope as GenericScope, ScopeTree as GenericScopeTree}; @@ -43,7 +43,34 @@ impl<'context> Elaborator<'context> { } pub(super) fn resolve_path(&mut self, path: Path) -> Result { - let resolver = StandardPathResolver::new(self.module_id()); + let mut module_id = self.module_id(); + let mut path = path; + + if path.kind == PathKind::Plain && path.first_name() == SELF_TYPE_NAME { + if let Some(Type::Struct(struct_type, _)) = &self.self_type { + let struct_type = struct_type.borrow(); + if path.segments.len() == 1 { + return Ok(ModuleDefId::TypeId(struct_type.id)); + } + + module_id = struct_type.id.module_id(); + path = Path { + segments: path.segments[1..].to_vec(), + kind: PathKind::Plain, + span: path.span(), + }; + } + } + + self.resolve_path_in_module(path, module_id) + } + + fn resolve_path_in_module( + &mut self, + path: Path, + module_id: ModuleId, + ) -> Result { + let resolver = StandardPathResolver::new(module_id); let path_resolution; if self.interner.track_references { diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index a21259a4f0d..91644ff4470 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -2729,3 +2729,83 @@ fn incorrect_generic_count_on_type_alias() { assert_eq!(actual, 1); assert_eq!(expected, 0); } + +#[test] +fn uses_self_type_for_struct_function_call() { + let src = r#" + struct S { } + + impl S { + fn one() -> Field { + 1 + } + + fn two() -> Field { + Self::one() + Self::one() + } + } + + fn main() {} + "#; + assert_no_errors(src); +} + +#[test] +fn uses_self_type_inside_trait() { + let src = r#" + trait Foo { + fn foo() -> Self { + Self::bar() + } + + fn bar() -> Self; + } + + impl Foo for Field { + fn bar() -> Self { + 1 + } + } + + fn main() { + let _: Field = Foo::foo(); + } + "#; + assert_no_errors(src); +} + +#[test] +fn uses_self_type_in_trait_where_clause() { + let src = r#" + trait Trait { + fn trait_func() -> bool; + } + + trait Foo where Self: Trait { + fn foo(self) -> bool { + self.trait_func() + } + } + + struct Bar { + + } + + impl Foo for Bar { + + } + + fn main() {} + "#; + + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + + let CompilationError::TypeError(TypeCheckError::UnresolvedMethodCall { method_name, .. }) = + &errors[0].0 + else { + panic!("Expected an unresolved method call error, got {:?}", errors[0].0); + }; + + assert_eq!(method_name, "trait_func"); +} From efef6b4c9f2ff4bce7e83bed004eb05332ac349f Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 31 Jul 2024 12:37:37 -0300 Subject: [PATCH 2/3] fix: error on unbound generics in structs (#5619) # 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 { 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 { } 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 --- .../src/monomorphization/mod.rs | 115 ++++++++++++++++-- .../Nargo.toml | 7 ++ .../src/main.nr | 6 + .../Nargo.toml | 7 ++ .../src/main.nr | 12 ++ 5 files changed, 140 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 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..e6506a5fde6 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 = @@ -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)? } @@ -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 @@ -1753,9 +1844,19 @@ 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 { + Monomorphizer::check_type(arg, location)?; + } + + 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 {}; +} 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 0ca5d9d7b389d24cf48fa62a29cb437b1855438a Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 31 Jul 2024 12:38:53 -0300 Subject: [PATCH 3/3] feat: don't eagerly error on cast expressions (#5635) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Description ## Problem Resolves #4994 ## Summary Before this PR, if you had something like: ```rust x as Field ``` and x's type was an unbound type variable, the compiler would immediately produce an error. In this PR that check is delayed. In order to do that, the cast can only succeed if `x` is an integer, a field... or a bool. We don't have a polymorphic type that's one of those, so this PR introduces that. (note: I think what I'm doing here is correct, but let me know if it's not!) Then, because we now have three different polymorphic types, I put them all into an enum, mainly because some of this polymorphic handling was sometimes duplicated, or almost the same with slight changes. ## Additional Context I'm not sure I covered all the scenarios where `IntegerOrFieldOrBool` should be handled. I'm not even sure I handled correctly the cases I needed to handle. This code now works fine: ```rust fn main() { let a: [u8; 10] = [1; 10]; let b: [Field; 10] = a.map(|x| x as Field); } ``` Also this one: ```rust fn main() { let a: [u8; 10] = [true; 10]; let b: [Field; 10] = a.map(|x| x as Field); } ``` However, this code correctly fails to compile, but the error is a bit strange: ```rust fn main() { let a = ["a"; 10]; let b: [Field; 10] = a.map(|x| x as Field); } ``` The error is: ``` error: Expected type fn(str<1>) -> _ with env _, found type fn(Integer | Field | bool) -> Field ┌─ src/main.nr:3:32 │ 3 │ let b: [Field; 10] = a.map(|x| x as Field); │ -------------- ``` but maybe that's expected? (I also don't know what type we could show here instead of `Integer | Field | bool`) ## 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 --- .../src/elaborator/expressions.rs | 2 +- .../noirc_frontend/src/elaborator/types.rs | 11 ++++-- compiler/noirc_frontend/src/tests.rs | 37 +++++++++++++++++++ 3 files changed, 46 insertions(+), 4 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/expressions.rs b/compiler/noirc_frontend/src/elaborator/expressions.rs index 295297cc738..6e2756f0301 100644 --- a/compiler/noirc_frontend/src/elaborator/expressions.rs +++ b/compiler/noirc_frontend/src/elaborator/expressions.rs @@ -553,7 +553,7 @@ impl<'context> Elaborator<'context> { fn elaborate_cast(&mut self, cast: CastExpression, span: Span) -> (HirExpression, Type) { let (lhs, lhs_type) = self.elaborate_expression(cast.lhs); let r#type = self.resolve_type(cast.r#type); - let result = self.check_cast(lhs_type, &r#type, span); + let result = self.check_cast(&lhs_type, &r#type, span); let expr = HirExpression::Cast(HirCastExpression { lhs, r#type }); (expr, result) } diff --git a/compiler/noirc_frontend/src/elaborator/types.rs b/compiler/noirc_frontend/src/elaborator/types.rs index c134820811e..0973e592c1e 100644 --- a/compiler/noirc_frontend/src/elaborator/types.rs +++ b/compiler/noirc_frontend/src/elaborator/types.rs @@ -779,7 +779,7 @@ impl<'context> Elaborator<'context> { } } - pub(super) fn check_cast(&mut self, from: Type, to: &Type, span: Span) -> Type { + pub(super) fn check_cast(&mut self, from: &Type, to: &Type, span: Span) -> Type { match from.follow_bindings() { Type::Integer(..) | Type::FieldElement @@ -788,8 +788,13 @@ impl<'context> Elaborator<'context> { | Type::Bool => (), Type::TypeVariable(_, _) => { - self.push_err(TypeCheckError::TypeAnnotationsNeeded { span }); - return Type::Error; + // NOTE: in reality the expected type can also include bool, but for the compiler's simplicity + // we only allow integer types. If a bool is in `from` it will need an explicit type annotation. + let expected = Type::polymorphic_integer_or_field(self.interner); + self.unify(from, &expected, || TypeCheckError::InvalidCast { + from: from.clone(), + span, + }); } Type::Error => return Type::Error, from => { diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index 91644ff4470..8ce430b6e48 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -2809,3 +2809,40 @@ fn uses_self_type_in_trait_where_clause() { assert_eq!(method_name, "trait_func"); } + +#[test] +fn do_not_eagerly_error_on_cast_on_type_variable() { + let src = r#" + pub fn foo(x: T, f: fn(T) -> U) -> U { + f(x) + } + + fn main() { + let x: u8 = 1; + let _: Field = foo(x, |x| x as Field); + } + "#; + assert_no_errors(src); +} + +#[test] +fn error_on_cast_over_type_variable() { + let src = r#" + pub fn foo(x: T, f: fn(T) -> U) -> U { + f(x) + } + + fn main() { + let x = "a"; + let _: Field = foo(x, |x| x as Field); + } + "#; + + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + + assert!(matches!( + errors[0].0, + CompilationError::TypeError(TypeCheckError::TypeMismatch { .. }) + )); +}