noir-lang / noir

Noir is a domain specific language for zero knowledge proofs
https://noir-lang.org
Apache License 2.0
821 stars 177 forks source link

fix: Avoid issuing duplicate errors during interpreting #5341

Closed jfecher closed 3 days ago

jfecher commented 4 days ago

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:

PR Checklist*

jfecher commented 4 days ago

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.