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: Avoid issuing duplicate errors during interpreting #5341

Closed
wants to merge 3 commits into from

Conversation

jfecher
Copy link
Contributor

@jfecher jfecher commented Jun 26, 2024

Description

Problem*

Summary*

The interpreter can issue duplicate errors if there were e.g. type errors that occurred within the comptime block being run.

For example:

fn main() {
    comptime {
        let _x = 1 + "two";
    }
}

Will give:

error: Types in a binary operation should match, but found Field and str<3>
  ┌─ /.../short/src/main.nr:3:18
  │
3 │         let _x = 1 + "two";
  │                  ---------
  │

error: No implementation for `Field` + `str<3>`
  ┌─ /.../short/src/main.nr:3:18
  │
3 │         let _x = 1 + "two";
  │                  ---------
  │

The first of which is a type error, and the second an interpreter error. Similarly, if we have a type error in some builtin methods like slice_push_back, these will lead to panics in the interpreter which assume errors will already be caught during type checking (they are caught, but the interpreter is still run on this code afterwards anyway).

To fix this, I've added a SilentFail error variant to avoid issuing these errors that are expected to be caught by the type system already.

Additional Context

The interpreter is still run even on code that previously errored since it is difficult to track for a given block if any code reachable from it has errored.

One alternative could be to only run the interpreter if there has been zero errors in the whole program so far. I'm open to discussion here.

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.

@jfecher jfecher requested a review from a team June 26, 2024 14:58
Copy link
Member

@TomAFrench TomAFrench left a comment

Choose a reason for hiding this comment

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

Spoke with Jake on this and we agreed to kick this can down the road a bit. We're going to fix the panics in builtins in a separate PR in the short term.

@TomAFrench TomAFrench marked this pull request as draft June 26, 2024 15:59
@jfecher
Copy link
Contributor Author

jfecher commented Jun 26, 2024

we agreed to kick this can down the road a bit

To elaborate on this: our reasoning was that it feels a bit bad to throw away errors / information like this by replacing these existing errors with the SilentFail error kind. If we ever wanted a feature in the future which did need this dynamic checking we'd have to reimplement these errors any way. For now users will just have to live with a duplicate error if the comptime code they're interpreting also has a type error in it.

@jfecher jfecher closed this Jun 27, 2024
github-merge-queue bot pushed a commit that referenced this pull request Jul 2, 2024
…eck (#5382)

# Description

## Problem\*

Resolves comment:
#5341 (review)

## Summary\*

Before this PR, we could panic if a builtin within a comptime block
fails to type check, and is later executed by the interpreter.

## Additional Context

## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist\*

- [x] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
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.

2 participants