From 3e8e90c4392a3b284a6daca1a0ede010c2148d96 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Tue, 26 Mar 2024 12:58:38 -0500 Subject: [PATCH 1/9] Fix issue --- .../noirc_frontend/src/hir/type_check/expr.rs | 86 +++++++------------ compiler/noirc_frontend/src/hir_def/types.rs | 1 + .../regression_4635/Nargo.toml | 7 ++ .../regression_4635/src/main.nr | 59 +++++++++++++ 4 files changed, 99 insertions(+), 54 deletions(-) create mode 100644 test_programs/compile_success_empty/regression_4635/Nargo.toml create mode 100644 test_programs/compile_success_empty/regression_4635/src/main.nr diff --git a/compiler/noirc_frontend/src/hir/type_check/expr.rs b/compiler/noirc_frontend/src/hir/type_check/expr.rs index 10476b6caef..569d2a8185b 100644 --- a/compiler/noirc_frontend/src/hir/type_check/expr.rs +++ b/compiler/noirc_frontend/src/hir/type_check/expr.rs @@ -152,14 +152,16 @@ impl<'interner> TypeChecker<'interner> { Ok((typ, use_impl)) => { if use_impl { let id = infix_expr.trait_method_id; - // Assume operators have no trait generics - self.verify_trait_constraint( - &lhs_type, - id.trait_id, - &[], - *expr_id, - span, - ); + + // Delay checking the trait constraint until the end of the function. + // Checking it now could bind an unbound type variable to any type + // that implements the trait. + let constraint = crate::hir_def::traits::TraitConstraint { + typ: lhs_type.clone(), + trait_id: id.trait_id, + trait_generics: Vec::new(), + }; + self.trait_constraints.push((constraint, *expr_id)); self.typecheck_operator_method(*expr_id, id, &lhs_type, span); } typ @@ -836,6 +838,10 @@ impl<'interner> TypeChecker<'interner> { match (lhs_type, rhs_type) { // Avoid reporting errors multiple times (Error, _) | (_, Error) => Ok((Bool, false)), + (Alias(alias, args), other) | (other, Alias(alias, args)) => { + let alias = alias.borrow().get_type(args); + self.comparator_operand_type_rules(&alias, other, op, span) + } // Matches on TypeVariable must be first to follow any type // bindings. @@ -844,12 +850,13 @@ impl<'interner> TypeChecker<'interner> { return self.comparator_operand_type_rules(other, binding, op, span); } - self.bind_type_variables_for_infix(lhs_type, op, rhs_type, span); - Ok((Bool, false)) - } - (Alias(alias, args), other) | (other, Alias(alias, args)) => { - let alias = alias.borrow().get_type(args); - self.comparator_operand_type_rules(&alias, other, op, span) + self.unify(lhs_type, rhs_type, || TypeCheckError::TypeMismatchWithSource { + expected: lhs_type.clone(), + actual: rhs_type.clone(), + source: Source::Binary, + span, + }); + Ok((Bool, true)) } (Integer(sign_x, bit_width_x), Integer(sign_y, bit_width_y)) => { if sign_x != sign_y { @@ -1079,38 +1086,6 @@ impl<'interner> TypeChecker<'interner> { } } - fn bind_type_variables_for_infix( - &mut self, - lhs_type: &Type, - op: &HirBinaryOp, - rhs_type: &Type, - span: Span, - ) { - self.unify(lhs_type, rhs_type, || TypeCheckError::TypeMismatchWithSource { - expected: lhs_type.clone(), - actual: rhs_type.clone(), - source: Source::Binary, - span, - }); - - // In addition to unifying both types, we also have to bind either - // the lhs or rhs to an integer type variable. This ensures if both lhs - // and rhs are type variables, that they will have the correct integer - // type variable kind instead of TypeVariableKind::Normal. - let target = if op.kind.is_valid_for_field_type() { - Type::polymorphic_integer_or_field(self.interner) - } else { - Type::polymorphic_integer(self.interner) - }; - - self.unify(lhs_type, &target, || TypeCheckError::TypeMismatchWithSource { - expected: lhs_type.clone(), - actual: rhs_type.clone(), - source: Source::Binary, - span, - }); - } - // Given a binary operator and another type. This method will produce the output type // and a boolean indicating whether to use the trait impl corresponding to the operator // or not. A value of false indicates the caller to use a primitive operation for this @@ -1130,6 +1105,10 @@ impl<'interner> TypeChecker<'interner> { match (lhs_type, rhs_type) { // An error type on either side will always return an error (Error, _) | (_, Error) => Ok((Error, false)), + (Alias(alias, args), other) | (other, Alias(alias, args)) => { + let alias = alias.borrow().get_type(args); + self.infix_operand_type_rules(&alias, op, other, span) + } // Matches on TypeVariable must be first so that we follow any type // bindings. @@ -1138,14 +1117,13 @@ impl<'interner> TypeChecker<'interner> { return self.infix_operand_type_rules(binding, op, other, span); } - self.bind_type_variables_for_infix(lhs_type, op, rhs_type, span); - - // Both types are unified so the choice of which to return is arbitrary - Ok((other.clone(), false)) - } - (Alias(alias, args), other) | (other, Alias(alias, args)) => { - let alias = alias.borrow().get_type(args); - self.infix_operand_type_rules(&alias, op, other, span) + self.unify(lhs_type, rhs_type, || TypeCheckError::TypeMismatchWithSource { + expected: lhs_type.clone(), + actual: rhs_type.clone(), + source: Source::Binary, + span, + }); + Ok((other.clone(), true)) } (Integer(sign_x, bit_width_x), Integer(sign_y, bit_width_y)) => { if sign_x != sign_y { diff --git a/compiler/noirc_frontend/src/hir_def/types.rs b/compiler/noirc_frontend/src/hir_def/types.rs index 3c5627f739b..03487f1f1ba 100644 --- a/compiler/noirc_frontend/src/hir_def/types.rs +++ b/compiler/noirc_frontend/src/hir_def/types.rs @@ -589,6 +589,7 @@ impl Type { TypeBinding::Bound(binding) => binding.is_bindable(), TypeBinding::Unbound(_) => true, }, + Type::Alias(alias, args) => alias.borrow().get_type(args).is_bindable(), _ => false, } } diff --git a/test_programs/compile_success_empty/regression_4635/Nargo.toml b/test_programs/compile_success_empty/regression_4635/Nargo.toml new file mode 100644 index 00000000000..563e262410f --- /dev/null +++ b/test_programs/compile_success_empty/regression_4635/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "regression_4635" +type = "bin" +authors = [""] +compiler_version = ">=0.26.0" + +[dependencies] diff --git a/test_programs/compile_success_empty/regression_4635/src/main.nr b/test_programs/compile_success_empty/regression_4635/src/main.nr new file mode 100644 index 00000000000..23918e30785 --- /dev/null +++ b/test_programs/compile_success_empty/regression_4635/src/main.nr @@ -0,0 +1,59 @@ +trait FromField { + fn from_field(field: Field) -> Self; +} + +impl FromField for Field { + fn from_field(value: Field) -> Self { + value + } +} + +trait Deserialize { + fn deserialize(fields: [Field; N]) -> Self; +} + +global AZTEC_ADDRESS_LENGTH = 1; + +struct AztecAddress { + inner : Field +} + +impl FromField for AztecAddress { + fn from_field(value: Field) -> Self { + Self { inner: value } + } +} + +impl Deserialize for AztecAddress { + fn deserialize(fields: [Field; AZTEC_ADDRESS_LENGTH]) -> Self { + AztecAddress::from_field(fields[0]) + } +} + +impl Eq for AztecAddress { + fn eq(self, other: Self) -> bool { + self.inner == other.inner + } +} + +// Custom code + +struct MyStruct { + a: T +} + +impl Deserialize<1> for MyStruct { + fn deserialize(fields: [Field; 1]) -> Self where T: FromField { + Self{ a: FromField::from_field(fields[0]) } + } +} + +fn main() { + let fields = [5; 1]; + let foo = MyStruct::deserialize(fields); // Note I don't specify T here (the type of `foo.a`) + + let bar = AztecAddress { inner: 5 }; + + // Here `T` is apparently inferred to be `AztecAddress`, presumably because of the comparison. + assert(foo.a == bar); +} From f55006894bf7b9a4431d511a44e916d1f18ff41c Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Wed, 27 Mar 2024 09:19:27 -0500 Subject: [PATCH 2/9] Reorder stdlib impls --- noir_stdlib/src/cmp.nr | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/noir_stdlib/src/cmp.nr b/noir_stdlib/src/cmp.nr index 0eed50eb42b..dde29d7ee87 100644 --- a/noir_stdlib/src/cmp.nr +++ b/noir_stdlib/src/cmp.nr @@ -6,10 +6,10 @@ trait Eq { impl Eq for Field { fn eq(self, other: Field) -> bool { self == other } } -impl Eq for u1 { fn eq(self, other: u1) -> bool { self == other } } -impl Eq for u8 { fn eq(self, other: u8) -> bool { self == other } } -impl Eq for u32 { fn eq(self, other: u32) -> bool { self == other } } impl Eq for u64 { fn eq(self, other: u64) -> bool { self == other } } +impl Eq for u32 { fn eq(self, other: u32) -> bool { self == other } } +impl Eq for u8 { fn eq(self, other: u8) -> bool { self == other } } +impl Eq for u1 { fn eq(self, other: u1) -> bool { self == other } } impl Eq for i8 { fn eq(self, other: i8) -> bool { self == other } } impl Eq for i32 { fn eq(self, other: i32) -> bool { self == other } } @@ -107,8 +107,8 @@ trait Ord { // Note: Field deliberately does not implement Ord -impl Ord for u8 { - fn cmp(self, other: u8) -> Ordering { +impl Ord for u64 { + fn cmp(self, other: u64) -> Ordering { if self < other { Ordering::less() } else if self > other { @@ -131,8 +131,8 @@ impl Ord for u32 { } } -impl Ord for u64 { - fn cmp(self, other: u64) -> Ordering { +impl Ord for u8 { + fn cmp(self, other: u8) -> Ordering { if self < other { Ordering::less() } else if self > other { From 42ff186a07f1c4a08c106a1c35310575821b6bb0 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Wed, 27 Mar 2024 09:30:02 -0500 Subject: [PATCH 3/9] Fix signed bit shifts --- noir_stdlib/src/ops.nr | 14 ++++++-------- .../bit_shifts_runtime/src/main.nr | 2 +- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/noir_stdlib/src/ops.nr b/noir_stdlib/src/ops.nr index e561265629e..10f9048b485 100644 --- a/noir_stdlib/src/ops.nr +++ b/noir_stdlib/src/ops.nr @@ -134,10 +134,9 @@ impl Shl for u8 { fn shl(self, other: u8) -> u8 { self << other } } impl Shl for u32 { fn shl(self, other: u32) -> u32 { self << other } } impl Shl for u64 { fn shl(self, other: u64) -> u64 { self << other } } -// Bit shifting is not currently supported for signed integer types -// impl Shl for i8 { fn shl(self, other: i8) -> i8 { self << other } } -// impl Shl for i32 { fn shl(self, other: i32) -> i32 { self << other } } -// impl Shl for i64 { fn shl(self, other: i64) -> i64 { self << other } } +impl Shl for i8 { fn shl(self, other: i8) -> i8 { self << other } } +impl Shl for i32 { fn shl(self, other: i32) -> i32 { self << other } } +impl Shl for i64 { fn shl(self, other: i64) -> i64 { self << other } } // docs:start:shr-trait trait Shr { @@ -149,7 +148,6 @@ impl Shr for u8 { fn shr(self, other: u8) -> u8 { self >> other } } impl Shr for u32 { fn shr(self, other: u32) -> u32 { self >> other } } impl Shr for u64 { fn shr(self, other: u64) -> u64 { self >> other } } -// Bit shifting is not currently supported for signed integer types -// impl Shr for i8 { fn shr(self, other: i8) -> i8 { self >> other } } -// impl Shr for i32 { fn shr(self, other: i32) -> i32 { self >> other } } -// impl Shr for i64 { fn shr(self, other: i64) -> i64 { self >> other } } +impl Shr for i8 { fn shr(self, other: i8) -> i8 { self >> other } } +impl Shr for i32 { fn shr(self, other: i32) -> i32 { self >> other } } +impl Shr for i64 { fn shr(self, other: i64) -> i64 { self >> other } } diff --git a/test_programs/execution_success/bit_shifts_runtime/src/main.nr b/test_programs/execution_success/bit_shifts_runtime/src/main.nr index 28b3ef656c1..ff424c9fbf6 100644 --- a/test_programs/execution_success/bit_shifts_runtime/src/main.nr +++ b/test_programs/execution_success/bit_shifts_runtime/src/main.nr @@ -7,7 +7,7 @@ fn main(x: u64, y: u64) { assert(x >> y == 32); // Bit-shift with signed integers - let mut a :i8 = y as i8; + let mut a: i8 = y as i8; let mut b: i8 = x as i8; assert(b << 1 == -128); assert(b >> 2 == 16); From 21b6b72cd5e805d50f4dff5a030d853ef4d674a8 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Wed, 27 Mar 2024 10:03:30 -0500 Subject: [PATCH 4/9] Fix remaining tests --- .../noirc_frontend/src/hir/type_check/mod.rs | 50 +++++++++---------- noir_stdlib/src/ops.nr | 40 ++++++++------- .../hashmap_load_factor/Prover.toml | 23 +++++++++ 3 files changed, 68 insertions(+), 45 deletions(-) create mode 100644 test_programs/compile_failure/hashmap_load_factor/Prover.toml diff --git a/compiler/noirc_frontend/src/hir/type_check/mod.rs b/compiler/noirc_frontend/src/hir/type_check/mod.rs index 137608f8037..bb42ebf68fb 100644 --- a/compiler/noirc_frontend/src/hir/type_check/mod.rs +++ b/compiler/noirc_frontend/src/hir/type_check/mod.rs @@ -86,31 +86,13 @@ pub fn type_check_func(interner: &mut NodeInterner, func_id: FuncId) -> Vec Vec Vec Vec Field { self + other } } -impl Add for u8 { fn add(self, other: u8) -> u8 { self + other } } -impl Add for u32 { fn add(self, other: u32) -> u32 { self + other } } impl Add for u64 { fn add(self, other: u64) -> u64 { self + other } } +impl Add for u32 { fn add(self, other: u32) -> u32 { self + other } } +impl Add for u8 { fn add(self, other: u8) -> u8 { self + other } } impl Add for i8 { fn add(self, other: i8) -> i8 { self + other } } impl Add for i32 { fn add(self, other: i32) -> i32 { self + other } } @@ -22,9 +22,9 @@ trait Sub { impl Sub for Field { fn sub(self, other: Field) -> Field { self - other } } -impl Sub for u8 { fn sub(self, other: u8) -> u8 { self - other } } -impl Sub for u32 { fn sub(self, other: u32) -> u32 { self - other } } impl Sub for u64 { fn sub(self, other: u64) -> u64 { self - other } } +impl Sub for u32 { fn sub(self, other: u32) -> u32 { self - other } } +impl Sub for u8 { fn sub(self, other: u8) -> u8 { self - other } } impl Sub for i8 { fn sub(self, other: i8) -> i8 { self - other } } impl Sub for i32 { fn sub(self, other: i32) -> i32 { self - other } } @@ -38,9 +38,9 @@ trait Mul { impl Mul for Field { fn mul(self, other: Field) -> Field { self * other } } -impl Mul for u8 { fn mul(self, other: u8) -> u8 { self * other } } -impl Mul for u32 { fn mul(self, other: u32) -> u32 { self * other } } impl Mul for u64 { fn mul(self, other: u64) -> u64 { self * other } } +impl Mul for u32 { fn mul(self, other: u32) -> u32 { self * other } } +impl Mul for u8 { fn mul(self, other: u8) -> u8 { self * other } } impl Mul for i8 { fn mul(self, other: i8) -> i8 { self * other } } impl Mul for i32 { fn mul(self, other: i32) -> i32 { self * other } } @@ -54,9 +54,9 @@ trait Div { impl Div for Field { fn div(self, other: Field) -> Field { self / other } } -impl Div for u8 { fn div(self, other: u8) -> u8 { self / other } } -impl Div for u32 { fn div(self, other: u32) -> u32 { self / other } } impl Div for u64 { fn div(self, other: u64) -> u64 { self / other } } +impl Div for u32 { fn div(self, other: u32) -> u32 { self / other } } +impl Div for u8 { fn div(self, other: u8) -> u8 { self / other } } impl Div for i8 { fn div(self, other: i8) -> i8 { self / other } } impl Div for i32 { fn div(self, other: i32) -> i32 { self / other } } @@ -68,9 +68,9 @@ trait Rem{ } // docs:end:rem-trait -impl Rem for u8 { fn rem(self, other: u8) -> u8 { self % other } } -impl Rem for u32 { fn rem(self, other: u32) -> u32 { self % other } } impl Rem for u64 { fn rem(self, other: u64) -> u64 { self % other } } +impl Rem for u32 { fn rem(self, other: u32) -> u32 { self % other } } +impl Rem for u8 { fn rem(self, other: u8) -> u8 { self % other } } impl Rem for i8 { fn rem(self, other: i8) -> i8 { self % other } } impl Rem for i32 { fn rem(self, other: i32) -> i32 { self % other } } @@ -84,9 +84,9 @@ trait BitOr { impl BitOr for bool { fn bitor(self, other: bool) -> bool { self | other } } -impl BitOr for u8 { fn bitor(self, other: u8) -> u8 { self | other } } -impl BitOr for u32 { fn bitor(self, other: u32) -> u32 { self | other } } impl BitOr for u64 { fn bitor(self, other: u64) -> u64 { self | other } } +impl BitOr for u32 { fn bitor(self, other: u32) -> u32 { self | other } } +impl BitOr for u8 { fn bitor(self, other: u8) -> u8 { self | other } } impl BitOr for i8 { fn bitor(self, other: i8) -> i8 { self | other } } impl BitOr for i32 { fn bitor(self, other: i32) -> i32 { self | other } } @@ -100,9 +100,9 @@ trait BitAnd { impl BitAnd for bool { fn bitand(self, other: bool) -> bool { self & other } } -impl BitAnd for u8 { fn bitand(self, other: u8) -> u8 { self & other } } -impl BitAnd for u32 { fn bitand(self, other: u32) -> u32 { self & other } } impl BitAnd for u64 { fn bitand(self, other: u64) -> u64 { self & other } } +impl BitAnd for u32 { fn bitand(self, other: u32) -> u32 { self & other } } +impl BitAnd for u8 { fn bitand(self, other: u8) -> u8 { self & other } } impl BitAnd for i8 { fn bitand(self, other: i8) -> i8 { self & other } } impl BitAnd for i32 { fn bitand(self, other: i32) -> i32 { self & other } } @@ -116,9 +116,9 @@ trait BitXor { impl BitXor for bool { fn bitxor(self, other: bool) -> bool { self ^ other } } -impl BitXor for u8 { fn bitxor(self, other: u8) -> u8 { self ^ other } } -impl BitXor for u32 { fn bitxor(self, other: u32) -> u32 { self ^ other } } impl BitXor for u64 { fn bitxor(self, other: u64) -> u64 { self ^ other } } +impl BitXor for u32 { fn bitxor(self, other: u32) -> u32 { self ^ other } } +impl BitXor for u8 { fn bitxor(self, other: u8) -> u8 { self ^ other } } impl BitXor for i8 { fn bitxor(self, other: i8) -> i8 { self ^ other } } impl BitXor for i32 { fn bitxor(self, other: i32) -> i32 { self ^ other } } @@ -130,9 +130,10 @@ trait Shl { } // docs:end:shl-trait -impl Shl for u8 { fn shl(self, other: u8) -> u8 { self << other } } impl Shl for u32 { fn shl(self, other: u32) -> u32 { self << other } } impl Shl for u64 { fn shl(self, other: u64) -> u64 { self << other } } +impl Shl for u8 { fn shl(self, other: u8) -> u8 { self << other } } +impl Shl for u1 { fn shl(self, other: u1) -> u1 { self << other } } impl Shl for i8 { fn shl(self, other: i8) -> i8 { self << other } } impl Shl for i32 { fn shl(self, other: i32) -> i32 { self << other } } @@ -144,9 +145,10 @@ trait Shr { } // docs:end:shr-trait -impl Shr for u8 { fn shr(self, other: u8) -> u8 { self >> other } } -impl Shr for u32 { fn shr(self, other: u32) -> u32 { self >> other } } impl Shr for u64 { fn shr(self, other: u64) -> u64 { self >> other } } +impl Shr for u32 { fn shr(self, other: u32) -> u32 { self >> other } } +impl Shr for u8 { fn shr(self, other: u8) -> u8 { self >> other } } +impl Shr for u1 { fn shr(self, other: u1) -> u1 { self >> other } } impl Shr for i8 { fn shr(self, other: i8) -> i8 { self >> other } } impl Shr for i32 { fn shr(self, other: i32) -> i32 { self >> other } } diff --git a/test_programs/compile_failure/hashmap_load_factor/Prover.toml b/test_programs/compile_failure/hashmap_load_factor/Prover.toml new file mode 100644 index 00000000000..6d72cab47fa --- /dev/null +++ b/test_programs/compile_failure/hashmap_load_factor/Prover.toml @@ -0,0 +1,23 @@ +[[input]] +key = 1 +value = 0 + +[[input]] +key = 2 +value = 0 + +[[input]] +key = 3 +value = 0 + +[[input]] +key = 4 +value = 0 + +[[input]] +key = 5 +value = 0 + +[[input]] +key = 6 +value = 0 From f56db1d6c2ef1f398b4471ffb7e2c82249451038 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Wed, 27 Mar 2024 10:19:26 -0500 Subject: [PATCH 5/9] Move hashmap_load_factor to execution_failure --- .../hashmap_load_factor/Nargo.toml | 0 .../hashmap_load_factor/Prover.toml | 0 .../hashmap_load_factor/src/main.nr | 0 3 files changed, 0 insertions(+), 0 deletions(-) rename test_programs/{compile_failure => execution_failure}/hashmap_load_factor/Nargo.toml (100%) rename test_programs/{compile_failure => execution_failure}/hashmap_load_factor/Prover.toml (100%) rename test_programs/{compile_failure => execution_failure}/hashmap_load_factor/src/main.nr (100%) diff --git a/test_programs/compile_failure/hashmap_load_factor/Nargo.toml b/test_programs/execution_failure/hashmap_load_factor/Nargo.toml similarity index 100% rename from test_programs/compile_failure/hashmap_load_factor/Nargo.toml rename to test_programs/execution_failure/hashmap_load_factor/Nargo.toml diff --git a/test_programs/compile_failure/hashmap_load_factor/Prover.toml b/test_programs/execution_failure/hashmap_load_factor/Prover.toml similarity index 100% rename from test_programs/compile_failure/hashmap_load_factor/Prover.toml rename to test_programs/execution_failure/hashmap_load_factor/Prover.toml diff --git a/test_programs/compile_failure/hashmap_load_factor/src/main.nr b/test_programs/execution_failure/hashmap_load_factor/src/main.nr similarity index 100% rename from test_programs/compile_failure/hashmap_load_factor/src/main.nr rename to test_programs/execution_failure/hashmap_load_factor/src/main.nr From 4835609c73a31d1922c5319721df1e8634f5ba4f Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Wed, 27 Mar 2024 11:17:39 -0500 Subject: [PATCH 6/9] Use primitive ops more often; spam type annotations in frontend tests --- .../noirc_frontend/src/hir/type_check/expr.rs | 8 ++++-- compiler/noirc_frontend/src/hir_def/types.rs | 5 ++++ compiler/noirc_frontend/src/tests.rs | 27 ++++++++++--------- 3 files changed, 25 insertions(+), 15 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/type_check/expr.rs b/compiler/noirc_frontend/src/hir/type_check/expr.rs index 569d2a8185b..f62c4512df2 100644 --- a/compiler/noirc_frontend/src/hir/type_check/expr.rs +++ b/compiler/noirc_frontend/src/hir/type_check/expr.rs @@ -856,7 +856,9 @@ impl<'interner> TypeChecker<'interner> { source: Source::Binary, span, }); - Ok((Bool, true)) + + let use_primitive_op = lhs_type.is_numeric(); + Ok((Bool, !use_primitive_op)) } (Integer(sign_x, bit_width_x), Integer(sign_y, bit_width_y)) => { if sign_x != sign_y { @@ -1123,7 +1125,9 @@ impl<'interner> TypeChecker<'interner> { source: Source::Binary, span, }); - Ok((other.clone(), true)) + + let use_primitive_op = lhs_type.is_numeric(); + Ok((other.clone(), !use_primitive_op)) } (Integer(sign_x, bit_width_x), Integer(sign_y, bit_width_y)) => { if sign_x != sign_y { diff --git a/compiler/noirc_frontend/src/hir_def/types.rs b/compiler/noirc_frontend/src/hir_def/types.rs index 03487f1f1ba..6b3260d390c 100644 --- a/compiler/noirc_frontend/src/hir_def/types.rs +++ b/compiler/noirc_frontend/src/hir_def/types.rs @@ -606,6 +606,11 @@ impl Type { matches!(self.follow_bindings(), Type::Integer(Signedness::Unsigned, _)) } + pub fn is_numeric(&self) -> bool { + use Type::*; + matches!(self.follow_bindings(), FieldElement | Integer(..) | Bool) + } + fn contains_numeric_typevar(&self, target_id: TypeVariableId) -> bool { // True if the given type is a NamedGeneric with the target_id let named_generic_id_matches_target = |typ: &Type| { diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index 6f92cb52a88..76a59f0d25c 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -1033,22 +1033,22 @@ mod test { fn resolve_complex_closures() { let src = r#" fn main(x: Field) -> pub Field { - let closure_without_captures = |x| x + x; - let a = closure_without_captures(1); + let closure_without_captures = |x: Field| -> Field { x + x }; + let a: Field = closure_without_captures(1); - let closure_capturing_a_param = |y| y + x; - let b = closure_capturing_a_param(2); + let closure_capturing_a_param = |y: Field| -> Field { y + x }; + let b: Field = closure_capturing_a_param(2); - let closure_capturing_a_local_var = |y| y + b; - let c = closure_capturing_a_local_var(3); + let closure_capturing_a_local_var = |y: Field| -> Field { y + b }; + let c: Field = closure_capturing_a_local_var(3); - let closure_with_transitive_captures = |y| { - let d = 5; - let nested_closure = |z| { - let doubly_nested_closure = |w| w + x + b; + let closure_with_transitive_captures = |y: Field| -> Field { + let d: Field = 5; + let nested_closure = |z: Field| -> Field { + let doubly_nested_closure = |w: Field| -> Field { w + x + b }; a + z + y + d + x + doubly_nested_closure(4) + x + y }; - let res = nested_closure(5); + let res: Field = nested_closure(5); res }; @@ -1219,8 +1219,8 @@ fn lambda$f1(mut env$l1: (Field)) -> Field { #[test] fn operators_in_global_used_in_type() { let src = r#" - global ONE = 1; - global COUNT = ONE + 2; + global ONE: Field = 1; + global COUNT: Field = ONE + 2; fn main() { let _array: [Field; COUNT] = [1, 2, 3]; } @@ -1233,6 +1233,7 @@ fn lambda$f1(mut env$l1: (Field)) -> Field { let src = r#" fn main() { for i in 0 .. 10 { + let i: u64 = i; if i == 2 { continue; } From 2d130d63dcead9fcb33622a0f2fb9bad841beabf Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Wed, 27 Mar 2024 11:59:29 -0500 Subject: [PATCH 7/9] Refine use_impl some more --- .../noirc_frontend/src/hir/type_check/expr.rs | 60 +++++++++++++------ compiler/noirc_frontend/src/hir_def/types.rs | 10 +++- 2 files changed, 51 insertions(+), 19 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/type_check/expr.rs b/compiler/noirc_frontend/src/hir/type_check/expr.rs index f62c4512df2..e705cf41fa5 100644 --- a/compiler/noirc_frontend/src/hir/type_check/expr.rs +++ b/compiler/noirc_frontend/src/hir/type_check/expr.rs @@ -850,15 +850,8 @@ impl<'interner> TypeChecker<'interner> { return self.comparator_operand_type_rules(other, binding, op, span); } - self.unify(lhs_type, rhs_type, || TypeCheckError::TypeMismatchWithSource { - expected: lhs_type.clone(), - actual: rhs_type.clone(), - source: Source::Binary, - span, - }); - - let use_primitive_op = lhs_type.is_numeric(); - Ok((Bool, !use_primitive_op)) + let use_impl = self.bind_type_variables_for_infix(lhs_type, op, rhs_type, span); + Ok((Bool, use_impl)) } (Integer(sign_x, bit_width_x), Integer(sign_y, bit_width_y)) => { if sign_x != sign_y { @@ -1088,6 +1081,44 @@ impl<'interner> TypeChecker<'interner> { } } + /// Handles the TypeVariable case for checking binary operators. + /// Returns true if we should use the impl for the operator instead of the primitive + /// version of it. + fn bind_type_variables_for_infix( + &mut self, + lhs_type: &Type, + op: &HirBinaryOp, + rhs_type: &Type, + span: Span, + ) -> bool { + self.unify(lhs_type, rhs_type, || TypeCheckError::TypeMismatchWithSource { + expected: lhs_type.clone(), + actual: rhs_type.clone(), + source: Source::Binary, + span, + }); + + let use_impl = !lhs_type.is_numeric(); + + // if the type variable is an integer or field we have to narrow it to only an integer + if !op.kind.is_valid_for_field_type() && lhs_type.is_numeric() { + // In addition to unifying both types, we also have to bind either + // the lhs or rhs to an integer type variable. This ensures if both lhs + // and rhs are type variables, that they will have the correct integer + // type variable kind instead of TypeVariableKind::Normal. + let target = Type::polymorphic_integer(self.interner); + + self.unify(lhs_type, &target, || TypeCheckError::TypeMismatchWithSource { + expected: lhs_type.clone(), + actual: rhs_type.clone(), + source: Source::Binary, + span, + }); + } + + use_impl + } + // Given a binary operator and another type. This method will produce the output type // and a boolean indicating whether to use the trait impl corresponding to the operator // or not. A value of false indicates the caller to use a primitive operation for this @@ -1119,15 +1150,8 @@ impl<'interner> TypeChecker<'interner> { return self.infix_operand_type_rules(binding, op, other, span); } - self.unify(lhs_type, rhs_type, || TypeCheckError::TypeMismatchWithSource { - expected: lhs_type.clone(), - actual: rhs_type.clone(), - source: Source::Binary, - span, - }); - - let use_primitive_op = lhs_type.is_numeric(); - Ok((other.clone(), !use_primitive_op)) + let use_impl = self.bind_type_variables_for_infix(lhs_type, op, rhs_type, span); + Ok((other.clone(), use_impl)) } (Integer(sign_x, bit_width_x), Integer(sign_y, bit_width_y)) => { if sign_x != sign_y { diff --git a/compiler/noirc_frontend/src/hir_def/types.rs b/compiler/noirc_frontend/src/hir_def/types.rs index f533d942384..970ff0f8e3b 100644 --- a/compiler/noirc_frontend/src/hir_def/types.rs +++ b/compiler/noirc_frontend/src/hir_def/types.rs @@ -608,7 +608,15 @@ impl Type { pub fn is_numeric(&self) -> bool { use Type::*; - matches!(self.follow_bindings(), FieldElement | Integer(..) | Bool) + use TypeVariableKind as K; + matches!( + self.follow_bindings(), + FieldElement | Integer(..) | Bool | TypeVariable(_, K::Integer | K::IntegerOrField) + ) + } + + pub(crate) fn is_integer_or_field_typevar(&self) -> bool { + matches!(self.follow_bindings(), Type::TypeVariable(_, TypeVariableKind::IntegerOrField)) } fn contains_numeric_typevar(&self, target_id: TypeVariableId) -> bool { From 17ca35f89d18a7d34d7c672037e0250b65fd222f Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Wed, 27 Mar 2024 12:03:50 -0500 Subject: [PATCH 8/9] Undo most type annotations in tests --- compiler/noirc_frontend/src/hir_def/types.rs | 4 ---- compiler/noirc_frontend/src/tests.rs | 15 +++++++-------- 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/compiler/noirc_frontend/src/hir_def/types.rs b/compiler/noirc_frontend/src/hir_def/types.rs index 970ff0f8e3b..a2aee5e4716 100644 --- a/compiler/noirc_frontend/src/hir_def/types.rs +++ b/compiler/noirc_frontend/src/hir_def/types.rs @@ -615,10 +615,6 @@ impl Type { ) } - pub(crate) fn is_integer_or_field_typevar(&self) -> bool { - matches!(self.follow_bindings(), Type::TypeVariable(_, TypeVariableKind::IntegerOrField)) - } - fn contains_numeric_typevar(&self, target_id: TypeVariableId) -> bool { // True if the given type is a NamedGeneric with the target_id let named_generic_id_matches_target = |typ: &Type| { diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index 76a59f0d25c..c4f0a8d67ba 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -1034,21 +1034,21 @@ mod test { let src = r#" fn main(x: Field) -> pub Field { let closure_without_captures = |x: Field| -> Field { x + x }; - let a: Field = closure_without_captures(1); + let a = closure_without_captures(1); let closure_capturing_a_param = |y: Field| -> Field { y + x }; - let b: Field = closure_capturing_a_param(2); + let b = closure_capturing_a_param(2); let closure_capturing_a_local_var = |y: Field| -> Field { y + b }; - let c: Field = closure_capturing_a_local_var(3); + let c = closure_capturing_a_local_var(3); let closure_with_transitive_captures = |y: Field| -> Field { - let d: Field = 5; + let d = 5; let nested_closure = |z: Field| -> Field { let doubly_nested_closure = |w: Field| -> Field { w + x + b }; a + z + y + d + x + doubly_nested_closure(4) + x + y }; - let res: Field = nested_closure(5); + let res = nested_closure(5); res }; @@ -1219,8 +1219,8 @@ fn lambda$f1(mut env$l1: (Field)) -> Field { #[test] fn operators_in_global_used_in_type() { let src = r#" - global ONE: Field = 1; - global COUNT: Field = ONE + 2; + global ONE = 1; + global COUNT = ONE + 2; fn main() { let _array: [Field; COUNT] = [1, 2, 3]; } @@ -1233,7 +1233,6 @@ fn lambda$f1(mut env$l1: (Field)) -> Field { let src = r#" fn main() { for i in 0 .. 10 { - let i: u64 = i; if i == 2 { continue; } From cc8baff4be568f8a517bedcb539010a41ac93681 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 29 Mar 2024 09:26:42 -0500 Subject: [PATCH 9/9] Fix comment and improve error message --- .../src/hir/type_check/errors.rs | 7 ++---- .../noirc_frontend/src/hir/type_check/expr.rs | 23 ++++++++++--------- 2 files changed, 14 insertions(+), 16 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/type_check/errors.rs b/compiler/noirc_frontend/src/hir/type_check/errors.rs index 6beb6929ce1..fe9cc9ea2d6 100644 --- a/compiler/noirc_frontend/src/hir/type_check/errors.rs +++ b/compiler/noirc_frontend/src/hir/type_check/errors.rs @@ -65,8 +65,6 @@ pub enum TypeCheckError { VariableMustBeMutable { name: String, span: Span }, #[error("No method named '{method_name}' found for type '{object_type}'")] UnresolvedMethodCall { method_name: String, object_type: Type, span: Span }, - #[error("Comparisons are invalid on Field types. Try casting the operands to a sized integer type first")] - InvalidComparisonOnField { span: Span }, #[error("Integers must have the same signedness LHS is {sign_x:?}, RHS is {sign_y:?}")] IntegerSignedness { sign_x: Signedness, sign_y: Signedness, span: Span }, #[error("Integers must have the same bit width LHS is {bit_width_x}, RHS is {bit_width_y}")] @@ -76,7 +74,7 @@ pub enum TypeCheckError { #[error("{kind} cannot be used in a unary operation")] InvalidUnaryOp { kind: String, span: Span }, #[error("Bitwise operations are invalid on Field types. Try casting the operands to a sized integer type first.")] - InvalidBitwiseOperationOnField { span: Span }, + FieldBitwiseOp { span: Span }, #[error("Integer cannot be used with type {typ}")] IntegerTypeMismatch { typ: Type, span: Span }, #[error("Cannot use an integer and a Field in a binary operation, try converting the Field into an integer first")] @@ -224,12 +222,11 @@ impl From for Diagnostic { | TypeCheckError::TupleIndexOutOfBounds { span, .. } | TypeCheckError::VariableMustBeMutable { span, .. } | TypeCheckError::UnresolvedMethodCall { span, .. } - | TypeCheckError::InvalidComparisonOnField { span } | TypeCheckError::IntegerSignedness { span, .. } | TypeCheckError::IntegerBitWidth { span, .. } | TypeCheckError::InvalidInfixOp { span, .. } | TypeCheckError::InvalidUnaryOp { span, .. } - | TypeCheckError::InvalidBitwiseOperationOnField { span, .. } + | TypeCheckError::FieldBitwiseOp { span, .. } | TypeCheckError::IntegerTypeMismatch { span, .. } | TypeCheckError::FieldComparison { span, .. } | TypeCheckError::AmbiguousBitWidth { span, .. } diff --git a/compiler/noirc_frontend/src/hir/type_check/expr.rs b/compiler/noirc_frontend/src/hir/type_check/expr.rs index e705cf41fa5..fb66bdeae6e 100644 --- a/compiler/noirc_frontend/src/hir/type_check/expr.rs +++ b/compiler/noirc_frontend/src/hir/type_check/expr.rs @@ -1100,19 +1100,20 @@ impl<'interner> TypeChecker<'interner> { let use_impl = !lhs_type.is_numeric(); - // if the type variable is an integer or field we have to narrow it to only an integer + // If this operator isn't valid for fields we have to possibly narrow + // TypeVariableKind::IntegerOrField to TypeVariableKind::Integer. + // Doing so also ensures a type error if Field is used. + // The is_numeric check is to allow impls for custom types to bypass this. if !op.kind.is_valid_for_field_type() && lhs_type.is_numeric() { - // In addition to unifying both types, we also have to bind either - // the lhs or rhs to an integer type variable. This ensures if both lhs - // and rhs are type variables, that they will have the correct integer - // type variable kind instead of TypeVariableKind::Normal. let target = Type::polymorphic_integer(self.interner); - self.unify(lhs_type, &target, || TypeCheckError::TypeMismatchWithSource { - expected: lhs_type.clone(), - actual: rhs_type.clone(), - source: Source::Binary, - span, + use BinaryOpKind::*; + use TypeCheckError::*; + self.unify(lhs_type, &target, || match op.kind { + Less | LessEqual | Greater | GreaterEqual => FieldComparison { span }, + And | Or | Xor | ShiftRight | ShiftLeft => FieldBitwiseOp { span }, + Modulo => FieldModulo { span }, + other => unreachable!("Operator {other:?} should be valid for Field"), }); } @@ -1176,7 +1177,7 @@ impl<'interner> TypeChecker<'interner> { if op.kind == BinaryOpKind::Modulo { return Err(TypeCheckError::FieldModulo { span }); } else { - return Err(TypeCheckError::InvalidBitwiseOperationOnField { span }); + return Err(TypeCheckError::FieldBitwiseOp { span }); } } Ok((FieldElement, false))