chore: refactor constrainedness checks on trait impls

compiler/noirc_frontend/src/elaborator/traits.rs

@@ -276,7 +276,6 @@ pub(crate) fn check_trait_impl_method_matches_declaration(
     noir_function: &NoirFunction,
 ) -> Vec<TypeCheckError> {
     let meta = interner.function_meta(&function);
-    let modifiers = interner.function_modifiers(&function);
     let method_name = interner.function_name(&function);
     let mut errors = Vec::new();
 
@@ -315,14 +314,6 @@ pub(crate) fn check_trait_impl_method_matches_declaration(
     // issue an error for it here.
     if let Some(trait_fn_id) = trait_info.method_ids.get(method_name) {
         let trait_fn_meta = interner.function_meta(trait_fn_id);
-        let trait_fn_modifiers = interner.function_modifiers(trait_fn_id);
-
-        if modifiers.is_unconstrained != trait_fn_modifiers.is_unconstrained {
-            let expected = trait_fn_modifiers.is_unconstrained;
-            let location = meta.name.location;
-            let item = method_name.to_string();
-            errors.push(TypeCheckError::UnconstrainedMismatch { item, expected, location });
-        }
 
         if trait_fn_meta.direct_generics.len() != meta.direct_generics.len() {
             let expected = trait_fn_meta.direct_generics.len();
@@ -374,15 +365,21 @@ fn check_function_type_matches_expected_type(
     errors: &mut Vec<TypeCheckError>,
 ) {
     let mut bindings = TypeBindings::new();
-    // Shouldn't need to unify envs, they should always be equal since they're both free functions
     if let (
-        Type::Function(params_a, ret_a, _env_a, _unconstrained_a),
-        Type::Function(params_b, ret_b, _env_b, _unconstrained_b),
+        Type::Function(params_a, ret_a, env_a, unconstrained_a),
+        Type::Function(params_b, ret_b, env_b, unconstrained_b),
     ) = (expected, actual)
     {
-        // TODO: we don't yet allow marking a trait function or a trait impl function as unconstrained,
-        // so both values will always be false here. Once we support that, we should check that both
-        // match (adding a test for it).
+        // Shouldn't need to unify envs, they should always be equal since they're both free functions
+        debug_assert_eq!(env_a, env_b, "envs should match as they're both free functions");
+
+        if unconstrained_a != unconstrained_b {
+            errors.push(TypeCheckError::UnconstrainedMismatch {
+                item: method_name.to_string(),
+                expected: *unconstrained_a,
+                location,
+            });
+        }
 
         if params_a.len() == params_b.len() {
             for (i, (a, b)) in params_a.iter().zip(params_b.iter()).enumerate() {

compiler/noirc_frontend/src/node_interner.rs

@@ -215,12 +215,12 @@
     interned_statement_kinds: noirc_arena::Arena<StatementKind>,
 
     // Interned `UnresolvedTypeData`s during comptime code.
     interned_unresolved_type_datas: noirc_arena::Arena<UnresolvedTypeData>,
 
     // Interned `Pattern`s during comptime code.
     interned_patterns: noirc_arena::Arena<Pattern>,
 
     /// Determins whether to run in LSP mode. In LSP mode references are tracked.
     pub(crate) lsp_mode: bool,
 
     /// Whether to avoid comptime println from producing output
@@ -703,7 +703,7 @@
             quoted_types: Default::default(),
             interned_expression_kinds: Default::default(),
             interned_statement_kinds: Default::default(),
             interned_unresolved_type_datas: Default::default(),
             interned_patterns: Default::default(),
             lsp_mode: false,
             disable_comptime_printing: false,
@@ -1576,16 +1576,24 @@
 
             let mut check_trait_generics =
                 |impl_generics: &[Type], impl_associated_types: &[NamedType]| {
-                    trait_generics.iter().zip(impl_generics).all(|(trait_generic, impl_generic)| {
-                        let impl_generic = impl_generic.force_substitute(&instantiation_bindings);
-                        trait_generic.try_unify(&impl_generic, &mut fresh_bindings).is_ok()
-                    }) && trait_associated_types.iter().zip(impl_associated_types).all(
+                    let generics_unify = trait_generics.iter().zip(impl_generics).all(
                         |(trait_generic, impl_generic)| {
+                            let impl_generic =
+                                impl_generic.force_substitute(&instantiation_bindings);
+                            trait_generic.try_unify(&impl_generic, &mut fresh_bindings).is_ok()
+                        },
+                    );
+
+                    let associated_types_unify = trait_associated_types
+                        .iter()
+                        .zip(impl_associated_types)
+                        .all(|(trait_generic, impl_generic)| {
                             let impl_generic2 =
                                 impl_generic.typ.force_substitute(&instantiation_bindings);
                             trait_generic.typ.try_unify(&impl_generic2, &mut fresh_bindings).is_ok()
-                        },
-                    )
+                        });
+
+                    generics_unify && associated_types_unify
                 };
 
             let trait_generics = match impl_kind {
@@ -2193,11 +2201,11 @@
         &mut self,
         typ: UnresolvedTypeData,
     ) -> InternedUnresolvedTypeData {
         InternedUnresolvedTypeData(self.interned_unresolved_type_datas.insert(typ))
     }
 
     pub fn get_unresolved_type_data(&self, id: InternedUnresolvedTypeData) -> &UnresolvedTypeData {
         &self.interned_unresolved_type_datas[id.0]
     }
 
     /// Returns the type of an operator (which is always a function), along with its return type.

compiler/noirc_frontend/src/tests/traits.rs

@@ -1494,3 +1494,39 @@ fn returns_self_in_trait_method_3() {
     ";
     assert_no_errors!(src);
 }
+
+#[named]
+#[test]
+fn errors_if_constrained_trait_definition_has_unconstrained_impl() {
+    let src = r#"
+    pub trait Foo {
+        fn foo() -> Field;
+    }
+
+    impl Foo for Field {
+        unconstrained fn foo() -> Field {
+                         ^^^ foo is not expected to be unconstrained
+            42
+        }
+    }
+    "#;
+    check_errors!(src);
+}
+
+#[named]
+#[test]
+fn errors_if_unconstrained_trait_definition_has_constrained_impl() {
+    let src = r#"
+    pub trait Foo {
+        unconstrained fn foo() -> Field;
+    }
+
+    impl Foo for Field {
+        fn foo() -> Field {
+           ^^^ foo is expected to be unconstrained
+            42
+        }
+    }
+    "#;
+    check_errors!(src);
+}

test_programs/compile_failure/noirc_frontend_tests_traits_errors_if_constrained_trait_definition_has_unconstrained_impl/Nargo.toml

@@ -0,0 +1,7 @@
+
+            [package]
+            name = "noirc_frontend_tests_traits_errors_if_constrained_trait_definition_has_unconstrained_impl"
+            type = "bin"
+            authors = [""]
+
+            [dependencies]

test_programs/compile_failure/noirc_frontend_tests_traits_errors_if_constrained_trait_definition_has_unconstrained_impl/src/main.nr

@@ -0,0 +1,11 @@
+
+    pub trait Foo {
+        fn foo() -> Field;
+    }
+
+    impl Foo for Field {
+        unconstrained fn foo() -> Field {
+            42
+        }
+    }
+

test_programs/compile_failure/noirc_frontend_tests_traits_errors_if_constrained_trait_definition_has_unconstrained_impl/src_hash.txt

@@ -0,0 +1 @@
+11746277096026948021

test_programs/compile_failure/noirc_frontend_tests_traits_errors_if_constrained_trait_definition_has_unconstrained_impl/stderr.txt

@@ -0,0 +1,8 @@
+error: foo is not expected to be unconstrained
+  ┌─ src/main.nr:7:26
+  │
+7 │         unconstrained fn foo() -> Field {
+  │                          ---
+  │
+
+Aborting due to 1 previous error

test_programs/compile_failure/noirc_frontend_tests_traits_errors_if_unconstrained_trait_definition_has_constrained_impl/Nargo.toml

@@ -0,0 +1,7 @@
+
+            [package]
+            name = "noirc_frontend_tests_traits_errors_if_unconstrained_trait_definition_has_constrained_impl"
+            type = "bin"
+            authors = [""]
+
+            [dependencies]

test_programs/compile_failure/noirc_frontend_tests_traits_errors_if_unconstrained_trait_definition_has_constrained_impl/src/main.nr

@@ -0,0 +1,11 @@
+
+    pub trait Foo {
+        unconstrained fn foo() -> Field;
+    }
+
+    impl Foo for Field {
+        fn foo() -> Field {
+            42
+        }
+    }
+

test_programs/compile_failure/noirc_frontend_tests_traits_errors_if_unconstrained_trait_definition_has_constrained_impl/src_hash.txt

@@ -0,0 +1 @@
+9121808244621508618

test_programs/compile_failure/noirc_frontend_tests_traits_errors_if_unconstrained_trait_definition_has_constrained_impl/stderr.txt

@@ -0,0 +1,8 @@
+error: foo is expected to be unconstrained
+  ┌─ src/main.nr:7:12
+  │
+7 │         fn foo() -> Field {
+  │            ---
+  │
+
+Aborting due to 1 previous error

test_programs/execution_panic/regression_8210/temp/Nargo.toml

@@ -0,0 +1,6 @@
+[package]
+name = "temp"
+type = "bin"
+authors = [""]
+
+[dependencies]

test_programs/execution_panic/regression_8210/temp/Prover.toml

@@ -0,0 +1 @@
+x = "-1"

test_programs/execution_panic/regression_8210/temp/src/main.nr

@@ -0,0 +1,3 @@
+fn main(x: i8) {
+   assert_eq((x as i16) as i8, -1);
+}