fix: Divide by zero should fail to satisfy constraints for Field and ints#2475
Merged
fix: Divide by zero should fail to satisfy constraints for Field and ints#2475
Field and ints#2475Conversation
vezenovm
commented
Aug 29, 2023
vezenovm
changed the title
FieldField and ints
kevaundray
reviewed
Aug 29, 2023
kevaundray
approved these changes
Aug 29, 2023
TomAFrench
requested changes
Aug 29, 2023
Member
TomAFrench
left a comment
TomAFrench
left a comment
There was a problem hiding this comment.
Blocking this temporarily as this is a breaking change
Contributor
I don't think anyone was relying on their program panicking before |
Contributor
Author
Or a Field division by |
TomAFrench
approved these changes
Aug 29, 2023
This was referenced Aug 29, 2023
TomAFrench
added a commit
that referenced
this pull request
Aug 30, 2023
* master: (42 commits) fix(ssa): Handle right shift with constants (#2481) chore(noir): Release 0.10.4 (#2354) fix: Divide by zero should fail to satisfy constraints for `Field` and ints (#2475) fix(ssa): Remove padding from ToRadix call with constant inputs (#2479) fix: Implement handling of array aliasing in the mem2reg optimization pass (#2463) chore: resolve `Instruction` inputs fully before checking against cache (#2472) chore: Move independent `run_test` function into nargo core (#2468) feat: Standard library functions can now be called with closure args (#2471) feat(frontend): aztec syntactic sugar (feature flagged) (#2403) chore(ci): enforce compliance with `cargo fmt` (#2467) chore(ci): Allow releases to have additional feature flags (#2405) feat: Add `assert_eq` keyword (#2137) fix(ssa): Do not optimize for allocates in constant folding (#2466) feat(ssa): Reuse existing results for duplicated instructions with no side-effects (#2460) fix: Closure lvalue capture bugfix (#2457) feat: Syntax for environment types now works with generics (#2383) fix(parser): fixes for the parsing of 'where' clauses (#2430) fix: Run `wasm` nodejs tests with no fails (#2387) chore: Run `cargo fmt` (#2455) chore: Perform formatting changes to integration tests (#2448) ...
TomAFrench
added a commit
that referenced
this pull request
Aug 30, 2023
* master: (42 commits) fix(ssa): Handle right shift with constants (#2481) chore(noir): Release 0.10.4 (#2354) fix: Divide by zero should fail to satisfy constraints for `Field` and ints (#2475) fix(ssa): Remove padding from ToRadix call with constant inputs (#2479) fix: Implement handling of array aliasing in the mem2reg optimization pass (#2463) chore: resolve `Instruction` inputs fully before checking against cache (#2472) chore: Move independent `run_test` function into nargo core (#2468) feat: Standard library functions can now be called with closure args (#2471) feat(frontend): aztec syntactic sugar (feature flagged) (#2403) chore(ci): enforce compliance with `cargo fmt` (#2467) chore(ci): Allow releases to have additional feature flags (#2405) feat: Add `assert_eq` keyword (#2137) fix(ssa): Do not optimize for allocates in constant folding (#2466) feat(ssa): Reuse existing results for duplicated instructions with no side-effects (#2460) fix: Closure lvalue capture bugfix (#2457) feat: Syntax for environment types now works with generics (#2383) fix(parser): fixes for the parsing of 'where' clauses (#2430) fix: Run `wasm` nodejs tests with no fails (#2387) chore: Run `cargo fmt` (#2455) chore: Perform formatting changes to integration tests (#2448) ...
This was referenced Aug 30, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Problem*
Resolves #2149
Summary*
The issue was posted using nargo 0.9.0 and there has since been some changes, although the bug still exists.
let a: Field = 3 / 0;still unexpectedly prints zero whilelet a: u8 = 3 / 0;actually now panics during instruction simplification for attempting to divide by zero.let a: i8 = 3 / 0;fails in the same cases asu8.When the dividend comes from inputs to main,
Fieldbehaves the same way, whileu8panics inside of execution of the Brillig VM. The Brillig VM should return an error for division by zero rather than panic'ing but this is a separate issue.The following changes were made:
let a: u8 = 3 / 0we will get a constraint failure that points to the span3 / 0.All variants of division by zero (whether by constant or witness) now trigger a failed constraint.
Documentation
This PR requires documentation updates when merged.
Additional Context
PR Checklist*
cargo fmton default settings.