Fix partially defined in the case of missing type maps #15995
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Thanks @AlexWaygood for sending me on this adventure. This took me a while to debug! It turns out when we don't need to warn about unreachable code, we don't end up calling `self.is_noop_for_reachability(s)` (which is meant to tell us whether the code should be warned about or is `raise AssertionError` or `typing.assert_never(never)` or something. https://github.com/python/mypy/blob/6f650cff9ab21f81069e0ae30c92eae94219ea63/mypy/checker.py#L2748 This innocuous check has a side effect that turns out to be important for the partially undefined checks. These checks work by reaching into the type map populated by the checker. But if we never actually ended up analysing the code, we never populate the type map. This therefore changes things to assume that if we couldn't find the expression in the type map, it's probably because it was unreachable.
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
Thanks so much for digging into this! :D |
AlexWaygood
reviewed
Aug 30, 2023
Comment on lines
+1030
to
+1045
| [case testUnreachableCausingMissingTypeMap] | ||
| # flags: --enable-error-code possibly-undefined --enable-error-code used-before-def --no-warn-unreachable | ||
| # Regression test for https://github.com/python/mypy/issues/15958 | ||
| from typing import Union, NoReturn | ||
|
|
||
| def assert_never(__x: NoReturn) -> NoReturn: ... | ||
|
|
||
| def foo(x: Union[int, str]) -> None: | ||
| if isinstance(x, str): | ||
| f = "foo" | ||
| elif isinstance(x, int): | ||
| f = "bar" | ||
| else: | ||
| assert_never(x) | ||
| f # OK | ||
| [builtins fixtures/tuple.pyi] |
There was a problem hiding this comment.
Would it be worth adding a match/case test as well, or would that just be unnecessarily duplicative?
There was a problem hiding this comment.
Good q, I think just duplicative. (fwiw I'd changed to if since that runs on all Python versions)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Thanks @AlexWaygood for sending me on this adventure. This took me a while for me to debug!
When we don't need to warn about unreachable code, we don't end up calling
self.is_noop_for_reachability(s)(which is meant to tell us whether the code should be warned about or israise AssertionErrorortyping.assert_never(never)or something).mypy/mypy/checker.py
Line 2748 in 6f650cf
This innocuous check has a side effect that turns out to be important for the partially undefined checks. These checks work by reaching into the type map populated by the checker. But if we never actually ended up analysing the code, we never populate the type map.
This therefore changes things to assume that if we couldn't find the expression in the type map, it's probably because it was unreachable.