-
-
Notifications
You must be signed in to change notification settings - Fork 669
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
Warn for unreachable code after panic #3148
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lovely error messages! Looks so good!
One thing I don't like so much about this implementation is that it traverses the sub-tree for every single AST node, which is very wasteful. I think we could instead store whether the previously analysed expression is a panic on the ExprTyper in place of the bool that's used currently to prevent emitting multiple warnings. Then it's O(n) rather than O(n log n)
Let's remove the PanicKind too, saying the previous one panics is good enough I think.
I'm not entirely sure I see how that could be implemented, so I would have to change every |
I think just the |
Thanks for the help! I should have fixed it now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super nice!! I've left two small notes
4cb7d3e
to
c49ebd6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delightful!! Thank you
This PR closes #3023
I tried copying Rust's approach where it tries to understand if the previous expression will always panic and raise a warning accordingly. I'm not sure about the error message, let me know if you think it can be improved somehow!