Fix false positive unreachable with Any in equality comparison #20538
Conversation
| # Skip Any types - they could be None, so comparing with them | ||
| # shouldn't narrow away None from other operands. | ||
| if isinstance(get_proper_type(typ), AnyType): | ||
| continue |
There was a problem hiding this comment.
-snip-
Shouldn't is_overlapping_none(Any) conceptually work? What's the fallout of making that work? (and Any in a union, of course)
There was a problem hiding this comment.
Yeah, that's good point, updated to fix is_overlapping_none() instead
This comment has been minimized.
This comment has been minimized.
calm329
force-pushed
the
fix-false-unreachable-any-equality-narrowing
branch
from
9a6676b to
a298d67
Compare
A5rocks
left a comment
A5rocks
left a comment
There was a problem hiding this comment.
Assuming all tests pass/no bad mypy primer fallout, this makes sense to me.
| t = get_proper_type(t) | ||
| return isinstance(t, NoneType) or ( | ||
| return isinstance(t, (NoneType, AnyType)) or ( | ||
| isinstance(t, UnionType) and any(isinstance(get_proper_type(e), NoneType) for e in t.items) |
There was a problem hiding this comment.
I think we can have Any in a union if there's some reason we didn't simplify it... I think we should do this just to make sure:
| isinstance(t, UnionType) and any(isinstance(get_proper_type(e), NoneType) for e in t.items) | |
| isinstance(t, UnionType) and any(isinstance(get_proper_type(e), (NoneType, AnyType)) for e in t.items) |
| from typing import Any, Optional | ||
| def main(contents: Any, commit: Optional[str]) -> None: | ||
| if contents.get("commit") == commit and (commit is not None or 1): | ||
| pass |
There was a problem hiding this comment.
Could you add a reveal_type(commit) here? On master that will reveal str, whereas I think it should be str | None now?
This comment has been minimized.
This comment has been minimized.
|
Huh, mypy primer didn't like that. I wonder why... |
Hmm... what about my original solution then? |
|
IMO I think this is a better solution and I think any errors now are a sign of deeper problems. But I haven't really looked at it and I'm also not really a maintainer so :P Up to you really. |
|
Yeah I agree the broader fix is conceptually cleaner, but it caused errors in several projects. So I think it'd be better to go with targeted fix to keep things safe. |
This comment has been minimized.
This comment has been minimized.
|
Diff from mypy_primer, showing the effect of this PR on open source code: cloud-init (https://github.com/canonical/cloud-init)
+ tests/unittests/sources/azure/test_kvp.py:101: error: Item "None" of "HyperVKvpReportingHandler | None" has no attribute "vm_id" [union-attr]
+ tests/unittests/sources/azure/test_kvp.py:127: error: Item "None" of "HyperVKvpReportingHandler | None" has no attribute "vm_id" [union-attr]
|
hauntsaninja
left a comment
hauntsaninja
left a comment
There was a problem hiding this comment.
Nice, thanks for the quick fix(es)!
I have a larger reworking of type narrowing that I'm trying to land (first PR in that series is #20492 ).
I'm probably going to hold off on merging this until that one is in, since there is a chance it might make some of the intermediate versions of this PR you had that were more principled possible
3 participants
Fixes #20532
When comparing a value of type
Anywith an optional type using==,!=, oris/is not, mypy was incorrectly narrowing the optional type by removingNone. This caused false positive "unreachable" warnings with--warn-unreachable.For example:
Previously reported: Right operand of "or" is never evaluated [unreachable]
The issue was in refine_away_none_in_comparison(): when building the list of "non-optional types" to use for narrowing, Any was included because is_overlapping_none(Any) returns False. However, Any could itself be None, so it shouldn't be used as evidence that the other operand can't be None.
The fix skips Any types when collecting non-optional types for narrowing.