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(ssa refactor): Ignore array out of bounds errors when enable_side_effects is false #1797

Merged
merged 4 commits into from
Jun 22, 2023

Conversation

jfecher
Copy link
Contributor

@jfecher jfecher commented Jun 22, 2023

Description

Problem*

When enable_side_effects is false, we are meant to be in a false branch of an if that was not taken, so we should ignore side effects causing the program to fail, such as array out of bounds errors. I've added a check for this to only panic for array index of out of bounds errors if enable side effects is not known to be false.

This fixes the bug in #1629 but means the issue will still appear if the if uses a witness as its condition, as it would not be known to be false. I've changed the logic to only panic if enable_side_effects is known to be true rather than not known to be false.

Resolves #1629

Summary*

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 jfecher requested a review from kevaundray June 22, 2023 17:58
@ludamad
Copy link
Collaborator

ludamad commented Jun 22, 2023

Hmm interesting problem, longer term we may have to ignore even more array out of bounds I guess you're implying? LGTM

ludamad
ludamad previously approved these changes Jun 22, 2023
@ludamad
Copy link
Collaborator

ludamad commented Jun 22, 2023

Cool that change makes sense to me

@ludamad
Copy link
Collaborator

ludamad commented Jun 22, 2023

Maybe a comment about the logic that led here? Probably surprising that it's usually ignored at first read

@jfecher
Copy link
Contributor Author

jfecher commented Jun 22, 2023

detecting if both sides would have an error, possibly a conservative setting that doesn't allow an error on either side but that I'm more speculative on

What do you mean by both sides here?

@kevaundray kevaundray added this pull request to the merge queue Jun 22, 2023
@ludamad
Copy link
Collaborator

ludamad commented Jun 22, 2023

Deleted that comment, thought this came up more generally

Merged via the queue into master with commit 7b7682a Jun 22, 2023
@kevaundray kevaundray deleted the jf/fix-1629 branch June 22, 2023 21:27
This was referenced Jun 22, 2023
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.

Test named sha2_byte crashes during compliation
3 participants