Skip to content
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.

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

Overflow checking bitwise operators removes almost all utility from them #3481

Closed
tsujp opened this issue Nov 12, 2023 · 2 comments · Fixed by #3518
Closed

Overflow checking bitwise operators removes almost all utility from them #3481

tsujp opened this issue Nov 12, 2023 · 2 comments · Fixed by #3518
Assignees
Labels
enhancement New feature or request

Comments

@tsujp
Copy link
Contributor

tsujp commented Nov 12, 2023

Problem

The recent addition for overflow checking is good. This kind of error (albeit not as explicit as demonstrated here) should be caught: let foo: u8 = 200 * 5;. Clearly the value 1000 (one-thousand) can not fit into 8-bits and so an overflow error is raised.

HOWEVER these checks are triggering on bitwise operations, notably bitshifts. This is very, very bad. For integers bitwise operations cannot overflow their operands by design.

let foo: u8 = 200;
let bar: u8 = 200 << 5; // INCORRECT OVERFLOW ERROR RAISED!

I don't know the exact reason within the compiler internals but if I had to guess I would say that the compiler is equating 200 << 5 to 200 ** 5 and checking if that value can be represented with 8-bits, here 320 000 000 000 cannot fit in 8-bits, except 200 << 5 is not 320 000 000 000 it's 0. Bits shift "off" the MSB side and bits shifted "on" the LSB side are 0. The value 0 can indeed be represented by an 8-bit integer.

Note, wrapping_mul is not appropriate here for bitshifts because the intention is not to multiply the value it's to move bits 5 places to the left with the "drop off" and "insert on" behaviour that every other programming language (and the design of bitwise operators) performs.

Happy Case

Revert the overflow checking on bitwise operations because by definition bitwise operations cannot overflow integer values.

Alternatives Considered

No response

Additional Context

No response

Would you like to submit a PR for this Issue?

No

Support Needs

No response

@tsujp tsujp added the enhancement New feature or request label Nov 12, 2023
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir Nov 12, 2023
@guipublic
Copy link
Contributor

bitwise operations can indeed overflow. For instance this code in rust does overflow:

2 | let bar: u8 = 200 << 8;
  |               ^^^^^^^^ attempt to shift left by `8_i32`, which would overflow

What you want is the wrapping version of shift left, which you can get from the stdlib:
std::wrapping_shift_left(200, 8);

Do not hesitate to clarify any point that I may be missing.

@guipublic
Copy link
Contributor

guipublic commented Nov 20, 2023

It turns out that your comment is still valid though:
let bar: u8 = 200 << 5; // INCORRECT OVERFLOW ERROR RAISED!

Rust overflows with <<8 but not with <<5 (for 8 bits unsigned), even if the result overflow the bit size.
I will fix this issue so that Noir behaviour is the same as Rust.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants