feat: Add `assert_eq` keyword #2137

TomAFrench · 2023-08-03T08:35:22Z

Description

Problem*

Resolves #2031
Resolves #1431

Summary*

This PR is an exploration of adding the built-in function assert_eq(lhs, rhs) which constains lhs == rhs. This is more efficient than assert(lhs == rhs) as laid out in #2031. It also allows proper usage of #1431 and can be modified in future to address #1995

There are a couple of blockers for this

assert() is still implemented as a keyword rather than another builtin function.
Instruction::Constrain should be replaced with a call to Intrinsic::AssertEq
assert_eq() needs to be explicitly imported from the stdlib before it can be used.

Documentation

This PR requires documentation updates when merged.
- I will submit a noir-lang/docs PR.
- I will request for and support Dev Rel's help in documenting this PR.

Additional Context

PR Checklist*

I have tested the changes locally.
I have formatted the changes with Prettier and/or cargo fmt on default settings.

jfecher · 2023-08-03T13:22:51Z

assert_eq() needs to be explicitly imported from the stdlib before it can be used.

Why not make assert_eq a keyword as well?

TomAFrench · 2023-08-03T13:30:20Z

I'm going off the conversation here where consensus seemed to change these to functions.

crates/noirc_evaluator/src/ssa/acir_gen/mod.rs

crates/nargo_cli/tests/execution_success/array_eq/src/main.nr

TomAFrench · 2023-08-16T22:59:40Z

~~This seems to be working now but a mutable reference to a closure which is performing acir-gen as it's being passed down recursively is pretty gross.~~

~~Failing constraints behind a if statement are still being applied~~ Fixed now.

crates/noirc_evaluator/src/ssa/opt/flatten_cfg.rs

crates/noirc_evaluator/src/ssa/acir_gen/mod.rs

crates/nargo_cli/tests/execution_success/array_dynamic/src/main.nr

TomAFrench · 2023-08-21T11:34:57Z

@jfecher I think this is at the point were we can start doing a proper review. I've got a separate branch which replaces Instruction::Constrain with a call to Intrinsic::AssertEq so lmk if you want that rolled into this PR.

jfecher · 2023-08-21T13:38:00Z

@TomAFrench Since it seems like it's going to be a little while before we have an actual Prelude I'm fine with adding assert_eq as a keyword for now and demoting it to a function later once we have a prelude. Otherwise this PR will be blocked for a little while longer.

TomAFrench · 2023-08-26T00:00:59Z

We could remove the assert_eq keyword after these changes now that assert(x == y) is equivalent and just have this PR affect SSA. I'm inclined to leave it in however.

* master: chore: Perform formatting changes to integration tests (#2448)

* master: chore: Run `cargo fmt` (#2455)

jfecher

👍

* master: 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)

* master: fix(ssa): Do not optimize for allocates in constant folding (#2466)

kevaundray

re-approving in Jake's stead

* 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) ...

Savio-Sou · 2023-09-14T07:21:45Z

~~Just to confirm: should this be documented (i.e. meant to be used by Noir devs)?~~

UPDATE: From my understanding this was baked into assert(... == ...) (instead of adding a new assert_eq keyword), hence no documentation changes are needed.

This was referenced Aug 3, 2023

Add an assert_eq function to remove unnecessary constraints from calls to assert #2031

Closed

Allow usage of some stdlib functions without an explicit import #2140

Closed

Support functions with optional arguments #2145

Closed

TomAFrench force-pushed the assert-eq-builtin branch 3 times, most recently from 7ed59f0 to 0f5392f Compare August 11, 2023 06:14

TomAFrench commented Aug 11, 2023

View reviewed changes

crates/noirc_evaluator/src/ssa/acir_gen/mod.rs Outdated Show resolved Hide resolved

TomAFrench commented Aug 11, 2023

View reviewed changes

crates/nargo_cli/tests/execution_success/array_eq/src/main.nr Outdated Show resolved Hide resolved

TomAFrench force-pushed the assert-eq-builtin branch from e8f2854 to 82ecbd9 Compare August 20, 2023 02:20

TomAFrench changed the base branch from master to update-artifacts August 20, 2023 02:44

TomAFrench force-pushed the assert-eq-builtin branch 2 times, most recently from 8a50ca6 to 3e50b54 Compare August 20, 2023 03:33

TomAFrench changed the title ~~feat: add assert_eq builtin function [do not merge]~~ feat: Add assert_eq builtin function Aug 20, 2023

TomAFrench mentioned this pull request Aug 20, 2023

feat: Use equivalence information from equality assertions to simplify circuit #2378

Merged

5 tasks

TomAFrench commented Aug 20, 2023

View reviewed changes

crates/noirc_evaluator/src/ssa/opt/flatten_cfg.rs Show resolved Hide resolved

crates/noirc_evaluator/src/ssa/acir_gen/mod.rs Outdated Show resolved Hide resolved

crates/nargo_cli/tests/execution_success/array_dynamic/src/main.nr Outdated Show resolved Hide resolved

Base automatically changed from update-artifacts to master August 21, 2023 09:31

TomAFrench added 8 commits August 21, 2023 10:39

feat: add assert_eq builtin function

1520b6b

chore: add integration test for array equality

504c997

chore: update test cases to use assert_eq

870f5e0

chore: run rebuild.sh script

f4f088f

chore: move test cases to compile_success_empty

511f0dd

feat: support calls to std::assert_eq from brillig

2e7ffff

chore: revert changes to array_dynamic test

b00058e

chore: remove usage of expect in assert_eq

c18e6c1

TomAFrench force-pushed the assert-eq-builtin branch from 3f6e4ed to c18e6c1 Compare August 21, 2023 09:39

chore: revert brillig optimization

8e08755

TomAFrench added 8 commits August 26, 2023 01:04

chore: run rebuild.sh script

b7aba4d

Merge branch 'master' into assert-eq-builtin

1434982

* master: chore: Perform formatting changes to integration tests (#2448)

chore: add integration test for using assert_eq on structs

955d513

chore: small changes

b89897c

chore: break up creation of statement in assertion_eq

b2b9124

chore: remove unnecessary equality in create_apply_function

148efa8

chore: remove redundant check in Constrain simplification

1978036

Merge branch 'master' into assert-eq-builtin

4dfff98

* master: chore: Run `cargo fmt` (#2455)

jfecher previously approved these changes Aug 28, 2023

View reviewed changes

jfecher enabled auto-merge August 28, 2023 16:37

jfecher added this pull request to the merge queue Aug 28, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 28, 2023

TomAFrench added this pull request to the merge queue Aug 28, 2023

github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Aug 28, 2023

TomAFrench dismissed jfecher’s stale review via 78a57fc August 28, 2023 17:52

TomAFrench requested a review from jfecher August 28, 2023 18:03

Merge branch 'master' into assert-eq-builtin

1c7883d

* master: fix(ssa): Do not optimize for allocates in constant folding (#2466)

kevaundray enabled auto-merge August 28, 2023 22:41

kevaundray added this pull request to the merge queue Aug 28, 2023

kevaundray approved these changes Aug 28, 2023

View reviewed changes

Merged via the queue into master with commit b467a2d Aug 28, 2023
16 checks passed

kevaundray deleted the assert-eq-builtin branch August 28, 2023 23:02

kevaundray mentioned this pull request Aug 28, 2023

chore(noir): Release 0.10.4 #2354

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add `assert_eq` keyword #2137

feat: Add `assert_eq` keyword #2137

TomAFrench commented Aug 3, 2023 •

edited

Loading

jfecher commented Aug 3, 2023

TomAFrench commented Aug 3, 2023

TomAFrench commented Aug 16, 2023 •

edited

Loading

TomAFrench commented Aug 21, 2023

jfecher commented Aug 21, 2023 •

edited

Loading

TomAFrench commented Aug 26, 2023

jfecher left a comment

kevaundray left a comment

Savio-Sou commented Sep 14, 2023 •

edited

Loading

feat: Add assert_eq keyword #2137

feat: Add assert_eq keyword #2137

Conversation

TomAFrench commented Aug 3, 2023 • edited Loading

Description

Problem*

Summary*

Documentation

Additional Context

PR Checklist*

jfecher commented Aug 3, 2023

TomAFrench commented Aug 3, 2023

TomAFrench commented Aug 16, 2023 • edited Loading

TomAFrench commented Aug 21, 2023

jfecher commented Aug 21, 2023 • edited Loading

TomAFrench commented Aug 26, 2023

jfecher left a comment

Choose a reason for hiding this comment

kevaundray left a comment

Choose a reason for hiding this comment

Savio-Sou commented Sep 14, 2023 • edited Loading

feat: Add `assert_eq` keyword #2137

feat: Add `assert_eq` keyword #2137

TomAFrench commented Aug 3, 2023 •

edited

Loading

TomAFrench commented Aug 16, 2023 •

edited

Loading

jfecher commented Aug 21, 2023 •

edited

Loading

Savio-Sou commented Sep 14, 2023 •

edited

Loading