chore(ssa): Implement ir errors
#2111
Conversation
|
I don't think we should convert all panics in the compiler into Result errors. The SSA passes in particular are never supposed to error (unfortunately they sometimes do - this is always a bug). If an error is truly expected to be unreachable then I think it is fine to keep it as an expect/panic. CC @TomAFrench, @phated for opinions here. |
yeah it is not easy for me to decide which error should be returned and which ones should be leave as |
This is my opinion on errors vs panics (and is general rust best practices afaik), handling |
|
A lot of these errors seem to be something that a user can't trigger by themselves but would require us to have an incorrect compiler implementation so I think we can remove most/all of these. |
| } | ||
|
|
||
| /// Returns a mutable reference to the terminator of this block. | ||
| /// | ||
| /// Once this block has finished construction, this is expected to always be Some. | ||
| pub(crate) fn unwrap_terminator_mut(&mut self) -> &mut TerminatorInstruction { | ||
| self.terminator.as_mut().expect("Expected block to have terminator instruction") | ||
| pub(crate) fn unwrap_terminator_mut( |
There was a problem hiding this comment.
"unwrap" implies we're not checking to make sure that the terminator exists so we can remove this error.
|
I grouped it in a way that all I don't have any preference on how errors are handled but I should probably ask what the team wants before I do anything further do internal (compiler implementation) errors need to be grouped together or leave as are there general heuristics I should try to follow on which kind of errors should leave as it is and should I continue working on this |
|
I left some |
|
should also mention that I only changed errors in |
Ethan-000
force-pushed
the
e/ssa_error_continue
branch
from
de2e78e
to
0081c7b
Compare
|
I think we should close this PR and commit to the "unrecoverable errors or core logic errors should be panics" ideology mentioned above. Since none of the SSA is meant to fail (yet at least) all of the errors in this PR would fall under this category and don't need to be changed to error enums.
I think the most important heuristic is "can users of Noir trigger this by writing normal Noir code?" E.g. triggering a type error, using an undefined symbol, or messing around with crates imports that don't exist should never crash the compiler. On the other hand, internal assumptions like an SSA block always having a terminator instruction (once the block is completed at least) should be panics if they're violated because it means our assumptions writing the code was wrong. There is also nothing we can do besides report it and halt if we were to bubble it up as an error enum.
I don't think so. I apologize for closing this PR when you've clearly put a lot of work into it but I think not trying to remove all panics from the code is healthier in the long term for the code base. That being said, if you see any panics which can be caused by user input, a PR to remove them would be greatly appreciated! If it would be a lot of work to remove and you're unsure if it is needed, you can always feel free to open an issue documenting it as well. |
Description
Problem*
towards #1473
Summary*
Additional Context
PR Checklist*
cargo fmton default settings.