From 0864e7c945089cc06f8cc9e5c7d933c465d8c892 Mon Sep 17 00:00:00 2001 From: jfecher Date: Thu, 19 Sep 2024 12:31:45 -0500 Subject: [PATCH] fix: Allow macros to change types on each iteration of a comptime loop (#6105) # Description ## Problem\* Resolves https://github.com/noir-lang/noir/pull/6086#issuecomment-2361085425 ## Summary\* Allows macros to have differing result types on different loop iterations again after it was banned by the unification in #6086. This is a difficult feature to use since users will still have to work around the type system only type checking loops once. The recommended technique found for this PR is to delay type checks by quoting the relevant function calls that need to be re-typechecked so that when they are unquoted by the interpreter they are forced to be typechecked every loop iteration. The downside of this is that it requires some knowledge of when to do this. If we go back to the naive approach of `comptime_change_type_each_loop_iteration` before the quoting: ```noir fn main() { comptime { for i in 9..11 { // Lengths are different on each iteration: "foo9", "foo10" let name = f"foo{i}".as_ctstring().as_quoted_str!(); let hash = from_signature(name); assert(hash > 3); } } } fn from_signature(_signature: str) -> u32 { N } ``` We get a confusing error now that the unification is gone: "Non-integer array length `_`" pointing to the body of `from_signature`. The fix for this is to quote & unquote the `from_signature` call: ```noir let hash = unquote!(quote { from_signature($name) }); ``` But there's nothing telling users this. Ideally we could warn users when a type changes like this and perhaps have them opt-in to it with a "I know what I'm doing" of some kind. ## Additional Context ## Documentation\* Check one: - [ ] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [ ] I have tested the changes locally. - [ ] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --- .../noirc_frontend/src/elaborator/types.rs | 18 ++++++++++++++- .../src/hir/comptime/interpreter.rs | 20 ++++++++-------- compiler/noirc_frontend/src/tests.rs | 23 +++++++++++++++++++ .../macro_result_type/Nargo.toml | 0 .../macro_result_type/src/main.nr | 0 .../Nargo.toml | 7 ++++++ .../src/main.nr | 19 +++++++++++++++ .../macro_result_type/t.rs | 12 ---------- 8 files changed, 76 insertions(+), 23 deletions(-) rename test_programs/{compile_success_empty => compile_failure}/macro_result_type/Nargo.toml (100%) rename test_programs/{compile_success_empty => compile_failure}/macro_result_type/src/main.nr (100%) create mode 100644 test_programs/compile_success_empty/comptime_change_type_each_iteration/Nargo.toml create mode 100644 test_programs/compile_success_empty/comptime_change_type_each_iteration/src/main.nr delete mode 100644 test_programs/compile_success_empty/macro_result_type/t.rs diff --git a/compiler/noirc_frontend/src/elaborator/types.rs b/compiler/noirc_frontend/src/elaborator/types.rs index 6be18df7b52..264b83956f8 100644 --- a/compiler/noirc_frontend/src/elaborator/types.rs +++ b/compiler/noirc_frontend/src/elaborator/types.rs @@ -633,7 +633,7 @@ impl<'context> Elaborator<'context> { 0 } - pub fn unify( + pub(super) fn unify( &mut self, actual: &Type, expected: &Type, @@ -644,6 +644,22 @@ impl<'context> Elaborator<'context> { } } + /// Do not apply type bindings even after a successful unification. + /// This function is used by the interpreter for some comptime code + /// which can change types e.g. on each iteration of a for loop. + pub fn unify_without_applying_bindings( + &mut self, + actual: &Type, + expected: &Type, + file: fm::FileId, + make_error: impl FnOnce() -> TypeCheckError, + ) { + let mut bindings = TypeBindings::new(); + if actual.try_unify(expected, &mut bindings).is_err() { + self.errors.push((make_error().into(), file)); + } + } + /// Wrapper of Type::unify_with_coercions using self.errors pub(super) fn unify_with_coercions( &mut self, diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter.rs index b5ed8126e33..e920073b453 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter.rs +++ b/compiler/noirc_frontend/src/hir/comptime/interpreter.rs @@ -1303,9 +1303,11 @@ impl<'local, 'interner> Interpreter<'local, 'interner> { // Macro calls are typed as type variables during type checking. // Now that we know the type we need to further unify it in case there // are inconsistencies or the type needs to be known. + // We don't commit any type bindings made this way in case the type of + // the macro result changes across loop iterations. let expected_type = self.elaborator.interner.id_type(id); let actual_type = result.get_type(); - self.unify(&actual_type, &expected_type, location); + self.unify_without_binding(&actual_type, &expected_type, location); } Ok(result) } @@ -1319,16 +1321,14 @@ impl<'local, 'interner> Interpreter<'local, 'interner> { } } - fn unify(&mut self, actual: &Type, expected: &Type, location: Location) { - // We need to swap out the elaborator's file since we may be - // in a different one currently, and it uses that for the error location. - let old_file = std::mem::replace(&mut self.elaborator.file, location.file); - self.elaborator.unify(actual, expected, || TypeCheckError::TypeMismatch { - expected_typ: expected.to_string(), - expr_typ: actual.to_string(), - expr_span: location.span, + fn unify_without_binding(&mut self, actual: &Type, expected: &Type, location: Location) { + self.elaborator.unify_without_applying_bindings(actual, expected, location.file, || { + TypeCheckError::TypeMismatch { + expected_typ: expected.to_string(), + expr_typ: actual.to_string(), + expr_span: location.span, + } }); - self.elaborator.file = old_file; } fn evaluate_method_call( diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index e21db0790d7..dc54fd624e4 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -3725,3 +3725,26 @@ fn use_numeric_generic_in_trait_method() { println!("{errors:?}"); assert_eq!(errors.len(), 0); } + +#[test] +fn macro_result_type_mismatch() { + let src = r#" + fn main() { + comptime { + let x = unquote!(quote { "test" }); + let _: Field = x; + } + } + + comptime fn unquote(q: Quoted) -> Quoted { + q + } + "#; + + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + assert!(matches!( + errors[0].0, + CompilationError::TypeError(TypeCheckError::TypeMismatch { .. }) + )); +} diff --git a/test_programs/compile_success_empty/macro_result_type/Nargo.toml b/test_programs/compile_failure/macro_result_type/Nargo.toml similarity index 100% rename from test_programs/compile_success_empty/macro_result_type/Nargo.toml rename to test_programs/compile_failure/macro_result_type/Nargo.toml diff --git a/test_programs/compile_success_empty/macro_result_type/src/main.nr b/test_programs/compile_failure/macro_result_type/src/main.nr similarity index 100% rename from test_programs/compile_success_empty/macro_result_type/src/main.nr rename to test_programs/compile_failure/macro_result_type/src/main.nr diff --git a/test_programs/compile_success_empty/comptime_change_type_each_iteration/Nargo.toml b/test_programs/compile_success_empty/comptime_change_type_each_iteration/Nargo.toml new file mode 100644 index 00000000000..38e72395bb5 --- /dev/null +++ b/test_programs/compile_success_empty/comptime_change_type_each_iteration/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "comptime_change_type_each_iteration" +type = "bin" +authors = [""] +compiler_version = ">=0.34.0" + +[dependencies] diff --git a/test_programs/compile_success_empty/comptime_change_type_each_iteration/src/main.nr b/test_programs/compile_success_empty/comptime_change_type_each_iteration/src/main.nr new file mode 100644 index 00000000000..7b34c112d4f --- /dev/null +++ b/test_programs/compile_success_empty/comptime_change_type_each_iteration/src/main.nr @@ -0,0 +1,19 @@ +fn main() { + comptime + { + for i in 9..11 { + // Lengths are different on each iteration: + // foo9, foo10 + let name = f"foo{i}".as_ctstring().as_quoted_str!(); + + // So to call `from_signature` we need to delay the type check + // by quoting the function call so that we re-typecheck on each iteration + let hash = std::meta::unquote!(quote { from_signature($name) }); + assert(hash > 3); + } + } +} + +fn from_signature(_signature: str) -> u32 { + N +} diff --git a/test_programs/compile_success_empty/macro_result_type/t.rs b/test_programs/compile_success_empty/macro_result_type/t.rs deleted file mode 100644 index bcd91d7bf5d..00000000000 --- a/test_programs/compile_success_empty/macro_result_type/t.rs +++ /dev/null @@ -1,12 +0,0 @@ - -trait Foo { - fn foo() {} -} - -impl Foo<3> for () { - fn foo() {} -} - -fn main() { - let _ = Foo::foo(); -}