Fix false positive unreachable with Any in equality comparison

[calm329]

Fixes #20532

When comparing a value of type Any with an optional type using ==, !=, or is/is not, mypy was incorrectly narrowing the optional type by removing None. This caused false positive "unreachable" warnings with --warn-unreachable.

For example:

from typing import Any

def main(contents: Any, commit: str | None) -> None:
    if (
        contents.get("commit") == commit
        and (commit is not None or print("can_be_reached"))
    ):
        pass

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.

[A5rocks]

-snip-

Shouldn't is_overlapping_none(Any) conceptually work? What's the fallout of making that work? (and Any in a union, of course)

[calm329]

Yeah, that's good point, updated to fix is_overlapping_none() instead

[A5rocks]

Assuming all tests pass/no bad mypy primer fallout, this makes sense to me.

[A5rocks]

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:

Suggested change

	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)

[A5rocks]

Could you add a reveal_type(commit) here? On master that will reveal str, whereas I think it should be str | None now?

[A5rocks]

Huh, mypy primer didn't like that. I wonder why...

[calm329]

Huh, mypy primer didn't like that. I wonder why...

Hmm... what about my original solution then?

[A5rocks]

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.

[calm329]

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.

[github-actions]

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]

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