Skip to content

fix(interpreter): Promote ConstantDoesNotFitInType to non-internal error#8866

Closed
vezenovm wants to merge 17 commits intomasterfrom
mv/promote-ConstantDoesNotFitInType-err
Closed

fix(interpreter): Promote ConstantDoesNotFitInType to non-internal error#8866
vezenovm wants to merge 17 commits intomasterfrom
mv/promote-ConstantDoesNotFitInType-err

Conversation

@vezenovm
Copy link
Contributor

@vezenovm vezenovm commented Jun 10, 2025

Description

Problem*

Builds upon #8862 to fix some of the fuzzing errors.

To reproduce take this example:

NOIR_ARBTEST_SEED=0x1b4496bb00100000 cargo test -p noir_ast_fuzzer_fuzz pass_vs_prev 

This will produce an AST that is expected to have a signed integer overflow.

Summary*

Leaving as a draft to see if it fixes the fuzzing failures. However, I'm not sure this is the best approach, as ConstantDoesNotFitInType does make sense as an internal error. We may want to fix how Cast's are interpreted instead.

We could also have both an internal ConstantDoesNotFitInType and an external ConstantDoesNotFitInType. For casting it would be an external error, but for things like converting an ir::value::Value::NumericConstant to an interpreter::Value we would trigger an internal error.

Another option could be to delay these internal errors in some way to see if we run into an expected external error.

Additional Context

A similar case has occurred here #8618.

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.

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: 32bc07f Previous: 9580a12 Ratio
test_report_zkpassport_noir_rsa_ 2 s 1 s 2

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

CC: @TomAFrench

@vezenovm vezenovm requested review from aakoshh and jfecher June 10, 2025 17:33
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: 32bc07f Previous: 9580a12 Ratio
rollup-merge 0.004 s 0.003 s 1.33

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

CC: @TomAFrench

Base automatically changed from mv/fix-signed-cast-simplification to master June 12, 2025 15:26
@vezenovm
Copy link
Contributor Author

Going to close this for now. As I am not a fan of this solution on its own, but we will have to think of something to avoid ConstantDoesNotFitInType errors on programs we expect to overflow.

@vezenovm vezenovm closed this Jun 12, 2025
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.

1 participant