Skip to content

fix!: disallow casting numeric to bool#8703

Merged
TomAFrench merged 13 commits intomasterfrom
ab/no-cast-from-numeric-to-bool
Jun 5, 2025
Merged

fix!: disallow casting numeric to bool#8703
TomAFrench merged 13 commits intomasterfrom
ab/no-cast-from-numeric-to-bool

Conversation

@asterite
Copy link
Collaborator

@asterite asterite commented May 28, 2025

Description

Problem

Resolves #8157

Summary

This seems to be a big breaking change but I think we should still disallow it. Casting using as for numeric values means truncating or extending a value. When casting a u32 to u1 we only keep the least significant byte, so casting 1 to u1 gives 0, but casting 2 to u1 gives 0. But when casting u32 to bool it's not clear what should happen: is the rule that 0 is false and 1 is true, or is the least significant byte what's used (similar to u1). Or maybe 0 is false, 1 is true and everything else is...? That's my guess of why Rust disallows this and asks to compare with zero, because like that there's no room for confusion.

The other way around, that is, casting from bool to a numeric value, is allowed in Rust and is still allowed in Noir. Here we get 0 for false and 1 for true, but I think that's expected (you wouldn't expect any other values here).

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.

@asterite
Copy link
Collaborator Author

asterite commented May 28, 2025

Oh, no! It seems that before this PR casting a numeric value to bool actually truncates the number to one bit, then that bit is the one used for the bool. That means that casting 2 to bool gives false 😨 This is why AztecProtocol/aztec-packages#14593 is likely failing.

What I mean is that this program:

fn main(x: Field) -> pub bool {
    x as bool
}

is generating this SSA:

acir(inline) pure fn main f0 {
  b0(v0: Field):
    v1 = truncate v0 to 1 bits, max_bit_size: 254
    v2 = cast v1 as u1
    return v2
}

@vezenovm
Copy link
Contributor

vezenovm commented May 28, 2025

It seems that before this PR casting a numeric value to bool actually truncates the number to one bit, then that bit is the one used for the bool. That means that casting 2 to bool gives false 😨 This is why AztecProtocol/aztec-packages#14593 is likely failing.

Ahhh! 😨

Your intuition about the failure makes sense. This just reiterates to me that we should disallow casting to bool. as bool makes me think "anything but zero is true", which is more semantically correct by doing != 0. For 2 as u1 I do expect zero as the result.

github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request May 29, 2025
For noir-lang/noir#8703

In Noir, casting a number to bool is the same as casting that number to
`u1` first, then checking if that `u1` is 0 or 1. That might be a bit
unexpected, and actually in Rust you can't cast numbers to bool, so it's
going to be disallowed soon.

This PR changes `as bool` to `!= 0` in most cases, though in some
unpacking cases it seems the desired behavior was actually `(value as
u1) != 0`, so there are a couple of those (without these a couple of
tests fail).
github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request May 29, 2025
For noir-lang/noir#8703

In Noir, casting a number to bool is the same as casting that number to
`u1` first, then checking if that `u1` is 0 or 1. That might be a bit
unexpected, and actually in Rust you can't cast numbers to bool, so it's
going to be disallowed soon.

This PR changes `as bool` to `!= 0` in most cases, though in some
unpacking cases it seems the desired behavior was actually `(value as
u1) != 0`, so there are a couple of those (without these a couple of
tests fail).
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Compilation Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: cac462c Previous: af2e5c9 Ratio
private-kernel-reset 8.588 s 6.96 s 1.23

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

@asterite
Copy link
Collaborator Author

asterite commented May 29, 2025

The only thing remaining now is releasing a new version of noir-bignum (0.7.3), then using that in Aztec-Packages , noir_bigcurve and noir_rsa.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Test Suite Duration'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: 2ae02a2 Previous: d6e4ce9 Ratio
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_rollup-lib 246 s 190 s 1.29

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

@github-actions
Copy link
Contributor

github-actions bot commented May 30, 2025

Changes to circuit sizes

Generated at commit: 86d1d598c0f82aa8166d4f71d4700f12a10da706, compared to commit: ce2fc37fd6286094df139308b90177df4a95c608

🧾 Summary (10% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
brillig_not -16 ✅ -61.54% -2,799 ✅ -99.22%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
brillig_not 10 (-16) -61.54% 22 (-2,799) -99.22%

@github-actions
Copy link
Contributor

github-actions bot commented May 30, 2025

Changes to Brillig bytecode sizes

Generated at commit: 86d1d598c0f82aa8166d4f71d4700f12a10da706, compared to commit: ce2fc37fd6286094df139308b90177df4a95c608

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
brillig_not_inliner_max -4 ✅ -10.81%
brillig_not_inliner_min -4 ✅ -10.81%
brillig_not_inliner_zero -4 ✅ -10.81%

Full diff report 👇
Program Brillig opcodes (+/-) %
brillig_not_inliner_max 33 (-4) -10.81%
brillig_not_inliner_min 33 (-4) -10.81%
brillig_not_inliner_zero 33 (-4) -10.81%

@github-actions
Copy link
Contributor

github-actions bot commented May 30, 2025

Changes to number of Brillig opcodes executed

Generated at commit: 86d1d598c0f82aa8166d4f71d4700f12a10da706, compared to commit: ce2fc37fd6286094df139308b90177df4a95c608

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
brillig_not_inliner_max -4 ✅ -12.90%
brillig_not_inliner_min -4 ✅ -12.90%
brillig_not_inliner_zero -4 ✅ -12.90%

Full diff report 👇
Program Brillig opcodes (+/-) %
brillig_not_inliner_max 27 (-4) -12.90%
brillig_not_inliner_min 27 (-4) -12.90%
brillig_not_inliner_zero 27 (-4) -12.90%

@asterite
Copy link
Collaborator Author

The "performance improvements" are just because I had to remove some code from test programs that checked that casting numbers to bools worked well.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Execution Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: 8262724 Previous: ce2fc37 Ratio
rollup-merge 0.004 s 0.003 s 1.33

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

@asterite asterite requested a review from a team June 2, 2025 15:26
@asterite asterite added this pull request to the merge queue Jun 5, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 5, 2025
@asterite asterite added this pull request to the merge queue Jun 5, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 5, 2025
@TomAFrench TomAFrench added this pull request to the merge queue Jun 5, 2025
Merged via the queue into master with commit c2cedd4 Jun 5, 2025
166 of 167 checks passed
@TomAFrench TomAFrench deleted the ab/no-cast-from-numeric-to-bool branch June 5, 2025 19:15
github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Jun 6, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
fix: type unification tests, and try moving constants to the other side
(noir-lang/noir#8807)
fix!: disallow casting numeric to bool
(noir-lang/noir#8703)
fix: delay associated constants resolution
(noir-lang/noir#8744)
fix: right shift overflow to 0
(noir-lang/noir#8772)
fix: use value predicated by range checks
(noir-lang/noir#8778)
fix: unify infix expressions by isolating unbound type variables
(noir-lang/noir#8796)
feat: use `asm` feature flag in arkworks
(noir-lang/noir#8792)
chore: add a failing test for #8780
(noir-lang/noir#8785)
chore: Adjust the frequency of 'for' statements in ACIR fuzz generation
(noir-lang/noir#8788)
fix: Merge `replacement_type` and `is_function_type` in
`defunctionalization` (noir-lang/noir#8784)
chore(fuzz): Remove unreachable functions in the AST fuzzer
(noir-lang/noir#8782)
chore(docs): Copy attribute docs into versioned docs
(noir-lang/noir#8777)
fix(licm): Preserve semantic ordering of side-effectual instructions
when hoisting (noir-lang/noir#8724)
fix: Create SSA interpreter arguments from scratch for each invocation
(noir-lang/noir#8762)
chore: mark sha512 as non-critical
(noir-lang/noir#8776)
fix!: disallow specifying associated items via generics
(noir-lang/noir#8756)
fix: stop inserting instructions after break and continue
(noir-lang/noir#8712)
fix: Fix comptime casts of negative integer to field
(noir-lang/noir#8696)
chore(SSA): restrict `shr` and `shl` right-hand side to u8
(noir-lang/noir#8753)
chore: bump some JS packages
(noir-lang/noir#8771)
chore: document `allow(dead_code)` and reorganize attributes
(noir-lang/noir#8766)
fix: Add missing cases for finding function values in
`find_functions_as_values` (noir-lang/noir#8738)
fix: correct bitsize in signed division
(noir-lang/noir#8733)
chore: remove noir-lang/noir_rsa from external libraries
(noir-lang/noir#8752)
chore: bump external pinned commits
(noir-lang/noir#8747)
END_COMMIT_OVERRIDE

---------

Co-authored-by: AztecBot <tech@aztecprotocol.com>
Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
danielntmd pushed a commit to danielntmd/aztec-packages that referenced this pull request Jul 16, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
fix: type unification tests, and try moving constants to the other side
(noir-lang/noir#8807)
fix!: disallow casting numeric to bool
(noir-lang/noir#8703)
fix: delay associated constants resolution
(noir-lang/noir#8744)
fix: right shift overflow to 0
(noir-lang/noir#8772)
fix: use value predicated by range checks
(noir-lang/noir#8778)
fix: unify infix expressions by isolating unbound type variables
(noir-lang/noir#8796)
feat: use `asm` feature flag in arkworks
(noir-lang/noir#8792)
chore: add a failing test for AztecProtocol#8780
(noir-lang/noir#8785)
chore: Adjust the frequency of 'for' statements in ACIR fuzz generation
(noir-lang/noir#8788)
fix: Merge `replacement_type` and `is_function_type` in
`defunctionalization` (noir-lang/noir#8784)
chore(fuzz): Remove unreachable functions in the AST fuzzer
(noir-lang/noir#8782)
chore(docs): Copy attribute docs into versioned docs
(noir-lang/noir#8777)
fix(licm): Preserve semantic ordering of side-effectual instructions
when hoisting (noir-lang/noir#8724)
fix: Create SSA interpreter arguments from scratch for each invocation
(noir-lang/noir#8762)
chore: mark sha512 as non-critical
(noir-lang/noir#8776)
fix!: disallow specifying associated items via generics
(noir-lang/noir#8756)
fix: stop inserting instructions after break and continue
(noir-lang/noir#8712)
fix: Fix comptime casts of negative integer to field
(noir-lang/noir#8696)
chore(SSA): restrict `shr` and `shl` right-hand side to u8
(noir-lang/noir#8753)
chore: bump some JS packages
(noir-lang/noir#8771)
chore: document `allow(dead_code)` and reorganize attributes
(noir-lang/noir#8766)
fix: Add missing cases for finding function values in
`find_functions_as_values` (noir-lang/noir#8738)
fix: correct bitsize in signed division
(noir-lang/noir#8733)
chore: remove noir-lang/noir_rsa from external libraries
(noir-lang/noir#8752)
chore: bump external pinned commits
(noir-lang/noir#8747)
END_COMMIT_OVERRIDE

---------

Co-authored-by: AztecBot <tech@aztecprotocol.com>
Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
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.

Field as bool acir vs brillig

3 participants