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.

Sign up for GitHub

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

Jump to bottom

Handle out-of-bound errors in CSE (#471) #673

Merged
merged 10 commits into from
Jan 31, 2023
Merged

Handle out-of-bound errors in CSE (#471) #673

merged 10 commits into from
Jan 31, 2023

Conversation

guipublic
Copy link
Contributor

Related issue(s)

Resolves #471

Description

Summary of changes

report an error when trying to access an array with index out-of-bounds during CSE

Checklist

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt with default settings.
  • I have linked this PR to the issue(s) that it resolves.
  • I have reviewed the changes on GitHub, line by line.
  • I have ensured all changes are covered in the description.

vezenovm
vezenovm requested changes Jan 20, 2023
View reviewed changes
Copy link
Contributor

@vezenovm vezenovm left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Could we thread location data into instructions (or rather NodeObj) so that we can display where the error has occurred as per the issue description?

Having the error

error: index out of bounds

is definitely better than a panic, but in a large circuit it might be hard to find this problem.

For some reason it looks like the secondary message of the diagnostic is not outputting for me. Is what I pasted above what you see or is your error more descriptive? I also did not get a secondary message for the diagnostic you added here: #672.

@guipublic
Copy link
Contributor Author

Could we thread location data into instructions (or rather NodeObj) so that we can display where the error has occurred as per the issue description?

Yes, we have already an issue open for this: #472

For some reason it looks like the secondary message of the diagnostic is not outputting for me. Is what I pasted above what you see or is your error more descriptive? I also did not get a secondary message for the diagnostic you added here: #672.

Yes I have the same, it is due to what I said in #672:

Note that the error is less user-friendly than the previous panic message, this is because errors with no location do not print the secondary message.

@vezenovm
Copy link
Contributor

vezenovm commented Jan 20, 2023 •
edited
Loading

Yes I have the same, it is due to what I said in #672:

Ah I missed the additional context. Ok in this case I am fine with these PRs then and a PR fixing #472 will alleviate my concerns. What do you think about moving the secondary messages to the main message until we have a location to associate with the errors?

kevaundray
kevaundray suggested changes Jan 21, 2023
View reviewed changes
Copy link
Contributor

@kevaundray kevaundray left a comment

Choose a reason for hiding this comment

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

Added a comment RE not embedding the Error variant in CseAction

@guipublic
Copy link
Contributor Author

What do you think about moving the secondary messages to the main message until we have a location to associate with the errors?

I was thinking the same, but it should be done when we report the error; if there is no location, we print the secondary message as part of the main message.

jfecher
jfecher reviewed Jan 23, 2023
View reviewed changes
Copy link
Contributor

@jfecher jfecher left a comment

Choose a reason for hiding this comment

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

Looks good, though I would like to see some Location handling as part of this PR though. Adding them to every instruction is out of scope but can we add them to only Index ast nodes and the Operation::Store and Operation::Loads that result from them?

crates/noirc_evaluator/src/ssa/anchor.rs Outdated Show resolved Hide resolved
crates/noirc_evaluator/src/ssa/anchor.rs Outdated Show resolved Hide resolved
jfecher
jfecher reviewed Jan 31, 2023
View reviewed changes
Copy link
Contributor

@jfecher jfecher left a comment

Choose a reason for hiding this comment

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

Looks good to me. We can merge once the clippy issues are fixed

@kevaundray kevaundray merged commit 4872e91 into master Jan 31, 2023
@kevaundray kevaundray deleted the gd/issue_471 branch January 31, 2023 19:07
TomAFrench added a commit that referenced this pull request Feb 3, 2023
@TomAFrench
Merge branch 'master' into kw/ref-nargo-maps
1bbf679 
* master:
  Rename methods that use `conditionalize` to be more descriptive (#739)
  feat(noir)!:  Returned values are no longer required by the prover (#731)
  chore: explicit versions for dependencies (#727)
  chore: readability improvements (#726)
  feat(nargo): include short git commit in cli version output (#721)
  Remove print to console for named proofs in `nargo prove` (#718)
  chore: clean up serde-related dependencies (#722)
  Handle out-of-bound errors in CSE (#471) (#673)
  Remove unused dependencies and only use workspace inheritance on shared deps (#671)
  feat(std_lib)!: modulus bits/bytes methods, and to_bits -> to_le_bits (#697)
  Implement numeric generics (#620)
  Review some TODO in SSA (#698)
  Replace `toml_map_to_field` and `toml_remap` with traits to map between `InputValue`s and `TomlTypes` (#677)
  Apply witness visibility on a parameter level rather than witness level (#712)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jfecher jfecher jfecher approved these changes

@kevaundray kevaundray Awaiting requested review from kevaundray

@vezenovm vezenovm Awaiting requested review from vezenovm

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Compiler panics on out-of-bounds array accesses
4 participants
@guipublic @vezenovm @jfecher @kevaundray