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

We should try not to panic on error but always exist with a reasonable error #1174

Open
ghaith opened this issue Mar 22, 2024 · 4 comments
Open

Comments

@ghaith
Copy link
Collaborator

ghaith commented Mar 22, 2024

#1168 happens because an error was ignored, which led the codegen into an unreachable state.
This brings up 2 issues:

  • Some of the errors cannot be ignored
  • Codegen should not panic if the errors are ignored

I would create a separate issue for the error configuration.
This issue is to handle codegen panics. We have a lot of "unreachable" blocks that are not really unreachable. We should try to fail gracefully here, we could return the same error code in that stage to explain that ignoring the original error caused this.
I would also do similar things to the expect keywords we have, or change them to the anyhow equivalent.

@riederm
Copy link
Collaborator

riederm commented Mar 22, 2024

what would you suggest to do instead of the panics?
I dont think it is a good idea to just skip some things?!

@ghaith
Copy link
Collaborator Author

ghaith commented Mar 22, 2024

Skipping diagnostics is indeed not a good idea, so I'm still open for something like the critical level we had before. But I still think that instead of a panic, we could return a result and exit with a meaningful error.
This is possible on places where we expect a result. On places where we are just not returning or returning a non result I' m guessing it would be more work. But yes in general wherever we thought we need an unreachable! block, we just return an Err which ends up being displayed as a meaningful message on exit. Something like this is a follow up error from an ignored diagnostics (E...)

@mhasel
Copy link
Member

mhasel commented Mar 22, 2024

To me this sounds more like an either/or situation - if we introduce critical diagnostics with a fixed severity, then we no longer need to change the unreachable statements in codegen into results.

We have a lot of "unreachable" blocks that are not really unreachable

Isn't that only due to the changes to the configurable diagnostics? We initially decided to introduce unreachable!() in places because we wanted to make sure to validate against cases where generating valid code is impossible. Maybe I'm just partial to failing spectacularly on programming errors. I did a quick search in the rustc repo and found ~1200 uses of unreachable!(), so I feel like this approach isn't entirely unjustified.

On the flipside, If we decide to keep the diagnostics as they are now, codegen needs to fail gracefully - but then we wouldn't need to worry about introducing a critical/immutable severity anymore.

Of course doing both is also an option, to me it just feels kind of redundant.

@riederm
Copy link
Collaborator

riederm commented Mar 25, 2024

Would we adding a diagnostic here that says: "Cannot do this & that. Please configure err123 as cirtical?". I even wonder if it is clear which ignore-rule lead to this situation?
What would be a valid usecase to turn such a rule into a warning when it cannot be compiled? (I understand the promotion warning->error, but error->warning is less clear)

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

No branches or pull requests

3 participants