chore: disabling display location section

[ghost]

Description

disabling display location section on release build

Problem

Resolves #2104

Summary

• I have tested the changes locally.
• I have formatted the changes with Prettier and/or cargo fmt on default settings.

[jfecher]

I'd like for this to be disabled on debug builds as well. Instead of changing the panic hook, I think we shouldn't panic at all when issuing compiler errors.

[TomAFrench]

Yeah, I'm getting a panic on compilation errors in Noir with this. As this is a situation where the issue is bad user input, it shouldn't panic.

[ghost]

as far as I understand, we are not panicking now, but returning an error:

[crates/nargo_cli/src/main.rs:16] start_cli() = Err(
    ReportedErrors(
        ReportedErrors {
            error_count: 1,
        },
    ),
)

the HookBuilder has the capability to handle both regular errors and panic situations, as demonstrated by the following code snippet:

https://github.com/yaahc/color-eyre/blob/4a7b4d6988c6b0da5e04e29c9d6e10595b5dc302/src/config.rs#L703-L708

In the event of utilizing RUST_BACKTRACE=1, a backtrace is displayed; nevertheless, I am unable to identify any instances of panic during function calls.

[TomAFrench]

If I compile a bad program such as

fn main(x: bool) {
    assert(foo);
}

I now get the output

$ nargo prove
error: cannot find `foo` in this scope 
  ┌─ /home/tom/Programming/noir/tmep/src/main.nr:2:12
  │
2 │     assert(foo);
  │            --- not found in this scope

warning: unused variable x
  ┌─ /home/tom/Programming/noir/tmep/src/main.nr:1:9
  │
1 │ fn main(x: bool) {
  │         - unused variable 

Error: 
   0: Aborting due to 1 previous error

  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ BACKTRACE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
                                ⋮ 2 frames hidden ⋮                               
   3: nargo_cli::cli::start_cli::ha2a4506e189093d5
      at <unknown source file>:<unknown line>
   4: std::sys_common::backtrace::__rust_begin_short_backtrace::h0afdd9622daa4648
      at <unknown source file>:<unknown line>
   5: std::rt::lang_start::{{closure}}::h7a1a28d32aaaecbb
      at <unknown source file>:<unknown line>
   6: core::ops::function::impls::<impl core::ops::function::FnOnce<A> for &F>::call_once::h072eb4cd8da964ba
      at /rustc/69f9c33d71c871fc16ac445211281c6e7a340943/library/core/src/ops/function.rs:286
   7: std::panicking::try::do_call::h8eca204fe9266946
      at /rustc/69f9c33d71c871fc16ac445211281c6e7a340943/library/std/src/panicking.rs:483
   8: std::panicking::try::h12574e1b7b2cbacb
      at /rustc/69f9c33d71c871fc16ac445211281c6e7a340943/library/std/src/panicking.rs:447
   9: std::panic::catch_unwind::hf71522d4448329d6
      at /rustc/69f9c33d71c871fc16ac445211281c6e7a340943/library/std/src/panic.rs:137
  10: std::rt::lang_start_internal::{{closure}}::h65b66ac9bff580f8
      at /rustc/69f9c33d71c871fc16ac445211281c6e7a340943/library/std/src/rt.rs:148
  11: std::panicking::try::do_call::hfff61e33ca3db9f1
      at /rustc/69f9c33d71c871fc16ac445211281c6e7a340943/library/std/src/panicking.rs:483
  12: std::panicking::try::he48c8ecead279cad
      at /rustc/69f9c33d71c871fc16ac445211281c6e7a340943/library/std/src/panicking.rs:447
  13: std::panic::catch_unwind::hd510a26bfc950ccc
      at /rustc/69f9c33d71c871fc16ac445211281c6e7a340943/library/std/src/panic.rs:137
  14: std::rt::lang_start_internal::hc680b25eab888da9
      at /rustc/69f9c33d71c871fc16ac445211281c6e7a340943/library/std/src/rt.rs:148
  15: main<unknown>
      at <unknown source file>:<unknown line>
  16: __libc_start_call_main<unknown>
      at <unknown source file>:<unknown line>
  17: __libc_start_main_alias_1<unknown>
      at <unknown source file>:<unknown line>
  18: _start<unknown>
      at <unknown source file>:<unknown line>

I don't see how this is an improvement over the existing behaviour.

[ghost]

this output will be only with "RUST_BACKTRACE=1"

(https://github.com/yaahc/color-eyre/blob/4a7b4d6988c6b0da5e04e29c9d6e10595b5dc302/src/config.rs#L1208-L1214)

> RUST_BACKTRACE=0 nargo check
warning: unused variable y
  ┌─ /home/work/subject/oliver/noir/h/src/main.nr:1:20
  │
1 │ fn main(x : Field, y : pub Field) {
  │                    - unused variable 

error: Expected type (), found type Field
  ┌─ /home/work/subject/oliver/noir/h/src/main.nr:1:4
  │
1 │ fn main(x : Field, y : pub Field) {
  │    ----


Error: 
   0: Aborting due to 1 previous error

[ghost]

eyre-rs/color-eyre#129

[jfecher]

Is this happening because we're installing to the eyre-hook as well now? We should not print stack traces when issuing compiler errors under any circumstances, lets remove it. Many users may have RUST_BACKTRACE=1 in their environment from working on other rust programs, they should not get a backtrace from the noir compiler as well.

[ghost]

locations in this case should be disabled specifically for EyreHook, the workaround is to write your own report handler without printing the location

[jfecher]

Can we revisit this PR? The Location section is still showing up in error messages. Can we disable it entirely for debug and release builds without getting any RUST_BACKTRACE noise as well?

[kevaundray]

Possibly relevant is the disabling section in the readme: https://github.com/eyre-rs/color-eyre#disabling-tracing-support

[ghost]

I've tried disabling the feature, but it seems that because acvm-backend-barretenberg is enabled, it affects our crate

[jfecher]

Still getting some odd error formatting with this PR:

error: 
  ┌─ /.../noir/short/src/main.nr:3:12
  │
3 │     assert(false);
  │            ----- Failed constraint

Error: 
   0: Aborting due to 1 previous error
exit 1

Looks like the Error:\n 0: is from eyre still.

If this is difficult to remove, wasn't there an alternative library to lyre? We could try swapping to that instead to see if it is easier to disable this Location information.

[ghost]

there is a miette, but there is the same problem: zkat/miette#269

miette:

❯ ../target/debug/nargo check
warning: unused variable x
  ┌─ /app/src/main.nr:1:9
  │
1 │ fn main(x : Field, y : pub Field) {
  │         - unused variable 

warning: unused variable y
  ┌─ /app/main.nr:1:20
  │
1 │ fn main(x : Field, y : pub Field) {
  │                    - unused variable 

error: Expected type (), found type Field
  ┌─ /app/main.nr:1:4
  │
1 │ fn main(x : Field, y : pub Field) {
  │    ----

Error:   × Aborting due to 1 previous error

perhaps it would be easier to fork color_eyre?

[jfecher]

Looks like it may be printed by rust itself because we return a Result from main. Perhaps we should consider refactoring so that we do not do that?

If we print out the errors directly, then set the exit code with std::process::exit, would this still work on wasm builds? CC @TomAFrench

[TomAFrench]

If we're doing this in the nargo_cli crate then it's fine as that never gets compiled into wasm.

[jfecher]

Thanks for the exploration here! I think this is a good approach we've settled on. Let's just revert back to color-eyre if possible.

[jfecher]

Pending re-review from @TomAFrench

[TomAFrench]

Will handle on Monday

manually checked and it works