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

Warn for unreachable code after panic #3148

Merged
merged 9 commits into from
May 20, 2024
Merged

Conversation

giacomocavalieri
Copy link
Member

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!

Copy link
Member

@lpil lpil left a 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.

@giacomocavalieri
Copy link
Member Author

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.

I'm not entirely sure I see how that could be implemented, so I would have to change every infer_x function to update a field in the typer? For cases that wouldn't work right away because I have to check that all branches panic; I would have to type a branch, save the value of the field and go on with the others accumulating that result somewhere. Is this how it should work?

@lpil
Copy link
Member

lpil commented May 19, 2024

I think just the infer function and the ones for panic, anonymous functions, and case should be sufficient. In case I'd use a mutable variable as an accumulator to see if all clauses panic.

@giacomocavalieri
Copy link
Member Author

I think just the infer function and the ones for panic, anonymous functions, and case should be sufficient. In case I'd use a mutable variable as an accumulator to see if all clauses panic.

Thanks for the help! I should have fixed it now

Copy link
Member

@lpil lpil left a 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

compiler-core/src/type_/pipe.rs Outdated Show resolved Hide resolved
compiler-core/src/type_/expression.rs Show resolved Hide resolved
Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delightful!! Thank you

@lpil lpil merged commit 1313f88 into gleam-lang:main May 20, 2024
12 checks passed
@giacomocavalieri giacomocavalieri deleted the fix-3023 branch May 20, 2024 20:51
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

Successfully merging this pull request may close these issues.

Warn for unreachable code after todo and panic
2 participants