Skip to content

fix(comptime): Do not overflow on signed checked ops #8814

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
vezenovm wants to merge 1 commit into from
Closed

fix(comptime): Do not overflow on signed checked ops #8814

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions compiler/noirc_frontend/src/hir/comptime/interpreter/infix.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,14 +45,14 @@ pub(super) fn evaluate_infix(

/// Generate matches for arithmetic operations on `Field` and integers.
macro_rules! match_arithmetic {
(($lhs_value:ident as $lhs:ident $op:literal $rhs_value:ident as $rhs:ident) { field: $field_expr:expr, int: $int_expr:expr, }) => {
(($lhs_value:ident as $lhs:ident $op:literal $rhs_value:ident as $rhs:ident) { field: $field_expr:expr, int: $int_expr:expr, signed_int: $signed_int_expr:expr, }) => {
match_values! {
($lhs_value as $lhs $op $rhs_value as $rhs) {
(Field, Field) to Field => Some($field_expr),
(I8, I8) to I8 => $int_expr,
(I16, I16) to I16 => $int_expr,
(I32, I32) to I32 => $int_expr,
(I64, I64) to I64 => $int_expr,
(I8, I8) to I8 => $signed_int_expr,
(I16, I16) to I16 => $signed_int_expr,
(I32, I32) to I32 => $signed_int_expr,
(I64, I64) to I64 => $signed_int_expr,
(U8, U8) to U8 => $int_expr,
(U16, U16) to U16 => $int_expr,
(U32, U32) to U32 => $int_expr,
Expand Down Expand Up @@ -148,24 +148,28 @@ pub(super) fn evaluate_infix(
(lhs_value as lhs "+" rhs_value as rhs) {
field: lhs + rhs,
int: lhs.checked_add(rhs),
signed_int: Some(lhs.wrapping_add(rhs)),
}
},
BinaryOpKind::Subtract => match_arithmetic! {
(lhs_value as lhs "-" rhs_value as rhs) {
field: lhs - rhs,
int: lhs.checked_sub(rhs),
signed_int: Some(lhs.wrapping_sub(rhs)),
}
},
BinaryOpKind::Multiply => match_arithmetic! {
(lhs_value as lhs "*" rhs_value as rhs) {
field: lhs * rhs,
int: lhs.checked_mul(rhs),
signed_int: Some(lhs.wrapping_mul(rhs)),
}
},
BinaryOpKind::Divide => match_arithmetic! {
(lhs_value as lhs "/" rhs_value as rhs) {
field: lhs / rhs,
int: lhs.checked_div(rhs),
signed_int: lhs.checked_div(rhs),
}
},
BinaryOpKind::Equal => match_cmp! {
Expand Down
130 changes: 130 additions & 0 deletions compiler/noirc_frontend/src/hir/comptime/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,3 +314,133 @@ fn generic_functions() {
let result = interpret(program);
assert_eq!(result, Value::U8(2));
}

#[test]
fn add_unsigned() {
let src = "
comptime fn main() -> pub u32 {
2 + 100
}
";
let result = interpret(src);
assert_eq!(result, Value::U32(102));
}

#[test]
fn add_signed() {
let src = "
comptime fn main() -> pub i32 {
2 + 100
}
";
let result = interpret(src);
assert_eq!(result, Value::I32(102));
}

#[test]
fn add_overflow_unsigned() {
let src = "
comptime fn main() -> pub u8 {
200 + 100
}
";
let result = interpret_expect_error(src);
let InterpreterError::MathError { operator, .. } = result else {
panic!("Expected MathError");
};
assert_eq!(operator, "+");
}

#[test]
fn sub_unsigned() {
let src = "
comptime fn main() -> pub u32 {
10101 - 101
}
";
let result = interpret(src);
assert_eq!(result, Value::U32(10000));
}

#[test]
fn sub_signed() {
let src = "
comptime fn main() -> pub i32 {
10101 - 10102
}
";
let result = interpret(src);
assert_eq!(result, Value::I32(-1));
}

#[test]
fn sub_underflow_unsigned() {
let src = "
comptime fn main() -> pub u32 {
0 - 10
}
";
let result = interpret_expect_error(src);
let InterpreterError::MathError { operator, .. } = result else {
panic!("Expected MathError");
};
assert_eq!(operator, "-");
}

#[test]
fn sub_underflow_signed() {
let src = "
comptime fn main() -> pub i8 {
-120 - 10
}
";
let result = interpret(src);
assert_eq!(result, Value::I8(126));
Copy link
Copy Markdown
Contributor

@aakoshh aakoshh Jun 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding of #8806 was that the SSA interpreter should not error out on overflows because there will be an SSA operation following up that will check whether the overflow happened, and handle it by returning its own error. So an overflow will cause failure, it's just deferred.

However, in the comptime interpreter there are no follow up instructions. By allowing overflows for signed integers, aren't we deviating from what ACIR and Brillig do?

For example:

fn main() -> pub i8 {
    -120 - 10
}
❯ cargo run -q -p nargo_cli -- execute
error: Assertion failed: attempt to subtract with overflow
  ┌─ src/main.nr:2:5
  │
2 │     -120 - 10
  │     ---------
  │
  = Call stack:
    1. src/main.nr:2:5

Failed assertion

I'm surprised the AST fuzzer doesn't fail.

What am I missing?

Copy link
Copy Markdown
Contributor

@aakoshh aakoshh Jun 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another demonstration:

unconstrained fn main() {
    let a: i8 = comptime { foo() };
    let b = bar();
    assert_eq(a, b)
}

comptime fn foo() -> i8 {
    -120 - 10
}

fn bar() -> i8 {
    -120 - 10
}

Fails with the same assertion.

Copy link
Copy Markdown
Contributor

@aakoshh aakoshh Jun 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, the reason it comptime_vs_brillig did not fail is because we disabled the generation of overflowing operations, because they generated too many known bugs.

If I change avoid_overflow and avoid_negative_int_literals then it does fail, but often just because an overflow fails to compile the entire program. I do get some instances where only Brillig fails, though. @rkarabut maybe we can change comptime_vs_brillig to sometimes allow these things, but in the comparison do not fail if the program failed to compile due to an overflow, only if the comptime worked, but Brillig did not.

Copy link
Copy Markdown
Contributor Author

@vezenovm vezenovm Jun 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding of #8806 was that the SSA interpreter should not error out on overflows because there will be an SSA operation following up that will check whether the overflow happened, and handle it by returning its own error. So an overflow will cause failure, it's just deferred

Ah yes, good point.

However, in the comptime interpreter there are no follow up instructions. By allowing overflows for signed integers, aren't we deviating from what ACIR and Brillig do?

Good catch yes we would be deviating from ACIR and Brillig. cc @jfecher I think I'm going to close this PR then and we just need to update the interpreter as in #8806

}

#[test]
fn mul_unsigned() {
let src = "
comptime fn main() -> pub u64 {
2 * 100
}
";
let result = interpret(src);
assert_eq!(result, Value::U64(200));
}

#[test]
fn mul_signed() {
let src = "
comptime fn main() -> pub i64 {
2 * -100
}
";
let result = interpret(src);
assert_eq!(result, Value::I64(-200));
}

#[test]
fn mul_overflow_unsigned() {
let src = "
comptime fn main() -> pub u8 {
128 * 2
}
";
let result = interpret_expect_error(src);
let InterpreterError::MathError { operator, .. } = result else {
panic!("Expected MathError");
};
assert_eq!(operator, "*");
}

#[test]
fn mul_overflow_signed() {
let src = "
comptime fn main() -> pub i8 {
127 * 2
}
";
let result = interpret(src);
assert_eq!(result, Value::I8(-2));
}
Loading