Skip to content

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

Closed
vezenovm wants to merge 1 commit intomasterfrom
mv/wrapping-signed-ops-comptime-interpreter
Closed

fix(comptime): Do not overflow on signed checked ops#8814
vezenovm wants to merge 1 commit intomasterfrom
mv/wrapping-signed-ops-comptime-interpreter

Conversation

@vezenovm
Copy link
Contributor

@vezenovm vezenovm commented Jun 6, 2025

Description

Problem*

Resolves #8806 (comment)

Summary*

Update the comptime interpreter to match the side effect (overflow) semantics of binary operations in the SSA.

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 and/or cargo fmt on default settings.

}
";
let result = interpret(src);
assert_eq!(result, Value::I8(126));
Copy link
Contributor

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
Contributor

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
Contributor

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
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants