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

Fix #419 by disallowing bitwise operations on fields during type checking #687

Merged
merged 7 commits into from
Jan 27, 2023

Conversation

jfecher
Copy link
Contributor

@jfecher jfecher commented Jan 24, 2023

Related issue(s)

Resolves #419

Description

Summary of changes

This PR makes it a type error to use any of &, |, ^, <<, or >> on a Field type. The error message suggests users cast to a sized integer type beforehand.

In the case the operand types for a bitwise operator are still unknown, this PR introduces a delayed type error that is checked when the function as a whole is finished. This allows for cases like let x: u8 = 2 << 3; to be valid as the types of 2 and 3 are later resolved to u8, while still keeping ambiguous cases invalid. If the types are still ambiguous when the function is finished type checking, an error is issues explaining type annotations are needed to know how many bits to perform the bitwise operations on.

Test additions / changes

None

Checklist

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt with default settings.
  • I have linked this PR to the issue(s) that it resolves.
  • I have reviewed the changes on GitHub, line by line.
  • I have ensured all changes are covered in the description.

Additional context

The delayed type error mechanism added by this PR is a workaround over splitting PolymorphicInt into PolymorphicInt and PolymorphicIntOrField which turned out to be more work.

@jfecher
Copy link
Contributor Author

jfecher commented Jan 24, 2023

Example of what these errors look like:

error: Bitwise operations are invalid on Field types. Try casting the operands to a sized integer type first.
  ┌─ /.../src/main.nr:4:20
  │
4 │     let b: Field = a << 4;
  │                    ------

error: The number of bits to use for this bitwise operation is ambiguous. The type of at least one operand should be specified before this point
  ┌─ /.../src/main.nr:6:14
  │
6 │     let _c = 2 << 4;
  │              ------

error: aborting due to 2 previous errors
exit 1

@jfecher jfecher requested review from vezenovm and guipublic January 26, 2023 20:09
vezenovm
vezenovm previously approved these changes Jan 26, 2023
Copy link
Contributor

@vezenovm vezenovm left a comment

Choose a reason for hiding this comment

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

I think PR is good. I left one question but it is non-blocking. When CI passes we can merge

crates/noirc_frontend/src/node_interner.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@vezenovm vezenovm left a comment

Choose a reason for hiding this comment

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

This looks good to me now

@jfecher jfecher merged commit 35bef8d into master Jan 27, 2023
@jfecher jfecher deleted the jf/fix-419 branch January 27, 2023 18:40
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.

Proof creation involving bit shift results in runtime error
2 participants