PLC-lang / rusty

Structured Text Parser and LLVM Frontend
GNU Lesser General Public License v3.0
181 stars 47 forks source link

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

Open ghaith opened 1 month ago

ghaith commented 1 month ago

1168 happens because an error was ignored, which led the codegen into an unreachable state.

This brings up 2 issues:

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 commented 1 month ago

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

ghaith commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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)