Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add flag to warn about unreachable branches and exprs #7050
Add flag to warn about unreachable branches and exprs #7050
Changes from 1 commit
a5b3737
615f3b2
ce8c357
74663e8
d16dd8e
0ccb367
373c6ec
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
What do you think about renaming this to
unreachable_statement
? Would it be clearer (and correct)?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.
Good idea! I think it'd be more correct, especially since we're also reporting these errors after things like asserts and returns. (I also added a test case for the latter).
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.
What about something like
DEBUG: Final = False
followed byif DEBUG: print('foo')
? Is there a test for this use case?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.
Our reachability checks currently don't special-case Literal bools (or Finals that are bools) at all -- we'd actually just end up checking both branches in the
if DEBUG: ... else: ...
block.I'm not sure exactly what behavior we want though, which is why I refrained from adding a test case.
(On one hand, it'd be nice to make mypy smarter about reachability in general; on the other improving mypy's understanding would lead to actual regressions in type inference if users don't opt-in to using this new flag.)
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.
The current behavior seems reasonable to me.
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.
Idea for message:
Statement is unreachable
Even better, maybe we can somehow detect that this is caused by an always-true
isinstance
check and generate an error on the line with theisinstance
check.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.
I experimented a bit with this idea. I think switching to reporting an error on the check might be a bit messy -- it'd actually be much easier to report both error message.
Do you think that's ok? If so, I can work on submitting a separate PR with this change after this one is landed. (I'll probably need to do some mild refactoring so we can reuse the "should we report a warning with this line?" logic, and I'd prefer to do that in a separate branch.)
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.
Random thought (no need to address in this PR): What do you think about providing a magic utility function such as
unreachable()
for easily silencing errors about unreachable code?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.
That could be handy, though possibly annoying to use for people with linters (since they don't understand mypy magic functions). We could work around this by adding the function to
mypy_extensions
? Or maybe that's overkill?Another idea: once the "tag all error messages with a code" PR is landed, the user could maybe add a
# type: ignore
for just the "Statement is unreachable" warning?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.
Or another idea -- what if we instead modified mypy so that we mandate that any calls to
unreachable()
are actually unreachable?We could also modify this function so that optionally take in any number of arguments: if the checker runs into this function, it'll run
reveal_type(...)
on the provided args before reporting an error that the statement was actually reachable. This would also let us supplant this "assert never" pattern (and providing a runtime implementation of the same logic inmypy_extensions
or something would feel less like overkill).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.
Yeah, that would be how I imagined it to work. Not sure about the arguments though. My idea would be to do something like this:
Maybe we could automatically recognize that pattern and print out the inferred type of
x
if we hit an error.