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

Change fatal error to error #327

Closed
philberty opened this issue Mar 31, 2021 · 6 comments · Fixed by #374
Closed

Change fatal error to error #327

philberty opened this issue Mar 31, 2021 · 6 comments · Fixed by #374
Labels
bug diagnostic diagnostic static analysis good-first-pr

Comments

@philberty
Copy link
Member

rust_fatal_error (stmt.get_locus (),

This fatal error is annoying because if we get a bad type error for example "expected u32 got f32" we get back a TyTy::Error which is correct. This stops the type resolver going through the rest of the crate and we wont get any typed HIR debug dump to help diagnose the error if it is a bug.

This change will likely break some of the expected failure test cases @dkm any opinion?

In general we should open a new issue to think about handling lots of errors in general.

@philberty philberty added bug good-first-pr diagnostic diagnostic static analysis labels Mar 31, 2021
@dkm
Copy link
Member

dkm commented Mar 31, 2021

It was already seen that some tests are a bit too verbose because of the current compiler state. For these tests, the solution is to tell dg that we are expecting some output from the compiler and it should not consider this as an error.

If your change simply adds some output, adding // { dg-excess-errors "Noisy error and debug" } should do the trick. There should still be some dg-error to match some expected error message, or the test could easily fail for something else and we won't easily see this regression. I see that generics4.rs has this problem, so I'll push something to fix it :)

@dkm
Copy link
Member

dkm commented Apr 2, 2021

I wanted to see how "bad" this change would break current tests.

                === rust Summary ===

# of expected passes            838
# of expected failures          26

It looks like its currently harmless.

@dkm
Copy link
Member

dkm commented Apr 3, 2021

gccrs/gcc/testsuite/rust.test/compile/let1.rs:3:5: error: expected [i8] got [i32]
gccrs/gcc/testsuite/rust.test/compile/let1.rs:3:5: fatal error: failure in setting up let stmt type

Becomes:

gccrs/gcc/testsuite/rust.test/compile/let1.rs:3:5: error: failure in setting up let stmt type
gccrs/gcc/testsuite/rust.test/compile/let1.rs:3:5: error: Failed to resolve IdentifierExpr type: ( y ([C: 0 Nid: 26 Hid: 26]))
gccrs/gcc/testsuite/rust.test/compile/let1.rs:4:5: error: failed to type resolve expression
gccrs/gcc/testsuite/rust.test/compile/let1.rs:1:1: error: expected [i32] got [<tyty::error>]

Is it what you expected from this :

fn foo() -> i32 {
    let x:i32 = 123;
    let y:i8 = x;
    y
}

@philberty
Copy link
Member Author

What if we just remove the error message completely? It doesn't really add any more information. What do you think?

@philberty
Copy link
Member Author

Some of those other error messages are also not helpful too, the goal here in this issue is the fatal_error blocks the output of the toy Typed HIR dump which blocks us from debugging stuff. So I think diagnostic errors improvements should be a separate issue.

@lrh2000
Copy link
Contributor

lrh2000 commented Apr 5, 2021

I suggest just doing this: lrh2000@9a6441d (or removing the rust_fatal_error/rust_error_at completely).

Then the error messages for the above program will become

1.rs:3:5: error: expected [i8] got [i32]
    3 |     let y:i8 = x;
      |     ^
1.rs:3:5: error: failure in setting up let stmt type
1.rs:1:1: error: expected [i32] got [i8]
    1 | fn foo() -> i32 {
      | ^

The result is exactly what I expected, and rustc also generates similar error messages:

error[E0308]: mismatched types
 --> 1.rs:3:16
  |
3 |     let y:i8 = x;
  |           --   ^ expected `i8`, found `i32`
  |           |
  |           expected due to this
  |
help: you can convert an `i32` to an `i8` and panic if the converted value doesn't fit
  |
3 |     let y:i8 = x.try_into().unwrap();
  |                ^^^^^^^^^^^^^^^^^^^^^

error[E0308]: mismatched types
 --> 1.rs:4:5
  |
1 | fn foo() -> i32 {
  |             --- expected `i32` because of return type
...
4 |     y
  |     ^
  |     |
  |     expected `i32`, found `i8`
  |     help: you can convert an `i8` to an `i32`: `y.into()`

It does not break any current tests. Maybe we should add new tests to check the last error message?

philberty added a commit that referenced this issue Apr 15, 2021
Adding rich location will improve our error message diagnostics, this is
an initial building block to keep a wrapper over the GCC stuff we call.

Addresses: #97
Fixes: #327
philberty added a commit that referenced this issue Apr 15, 2021
This fatal error stops the compiler in its tracks and will mean the Typed
HIR dump will not occur limiting how much we can debug errors/bugs in the
compiler.

Fixes #327
bors bot added a commit that referenced this issue Apr 17, 2021
374: Add basic wrapper over gcc rich_location r=philberty a=philberty

Adding rich location will improve our error message diagnostics, this is
an initial building block to keep a wrapper over the GCC stuff we call.

Addresses: #97 
Fixes: #327 

Co-authored-by: Philip Herron <philip.herron@embecosm.com>
bors bot added a commit that referenced this issue Apr 17, 2021
374: Add basic wrapper over gcc rich_location r=philberty a=philberty

Adding rich location will improve our error message diagnostics, this is
an initial building block to keep a wrapper over the GCC stuff we call.

Addresses: #97 
Fixes: #327 

Co-authored-by: Philip Herron <philip.herron@embecosm.com>
@bors bors bot closed this as completed in 01486c1 Apr 17, 2021
@bors bors bot closed this as completed in #374 Apr 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug diagnostic diagnostic static analysis good-first-pr
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants