Fix Type Comparison Edge Cases

[George-Ogden]

Fixes #20275 and #20041
This code snippet fails to type-check before this PR:

class X:
    y: int

    def __eq__(self, other: object) -> bool:
        return type(other) is type(self) and other.y == self.y


idx: int | float = 0
if type(idx) == int == int:
    x = [100][idx]

But type-checks afterwards.
This is done by finding a least type as the upper bound when the equality succeeds.

[George-Ogden]

I don't quite understand what I should do with get_proper_type, but the mypy_primer diff seems to reduce the number of type errors.

[A5rocks]

I'm not familiar with this part of mypy (this is the first time I saw TypeRange!), so my comments may be wrong. Initial pass through though.

I'm not sure about the bigger picture here, it feels somewhat hacky to rely on type(x) == Y logic to handle type(x) == type(y). I don't have any better ideas though.

[tyralla]

Thanks for the PR!

Also, just a short remark/question without any deep knowledge of the matter: If I understand #20275 correctly, calling reveal_type already helps. Doesn't this mean the required narrowing logic is already available and only needs to be activated somehow for (normal) cases where reveal_type is not used?

[George-Ogden]

@tyralla:
before this PR, this worked best with direct comparison to types: type(x) == int => x: int (1)
this also includes y: type[int]; type(x) == y => x: int (2)
The previous logic looked at when type() was used to separate what was narrowed and what was constant
However, it fails when comparing type(x) == type(y) (3)
When reveal_type is used, it's equivalent to the second case.
after PR analyses the third case is analysed so that it looks more like
x: X; y: Y; type(x) == Y and type(y) == X (4)
But this 4th case is all-or-nothing, so either both variables have the type of the meet or they retain the originals.

Let me know if this makes it clear or if any of that doesn't make sense.

[A5rocks]

Huh, then couldn't we get rid of any special casing for looking for specifically type(x)? i.e. couldn't we just use case 1/2 always?

I imagine it's a bit strange (e.g. type(x) == int == str should like, be marked unreachable not be int & str) but maybe we just need to check for anything where the TypeRange.is_upper_bound is False and try using that, otherwise use the meet? I haven't really thought much about this so it's possible there's some flaw here. But this would mean e.g. for function f(x: T) -> T we would support f(type(x)) == int -- kind of silly, but maybe useful?

(I guess the flaw is actually that we need to know the relevant name to know what to narrow. Maybe that's why we don't do that)

[tyralla]

I never really thought about type narrowing with is (except for handling None, of course).

If I look at this example:

class X: x = 1
class Y: y = 1
class Z(X, Y): ...

z = Z()
x: X = z
y: Y = z
if x is y:
    x.y + y.x  # error: "X" has no attribute "y"
               # error: "Y" has no attribute "x

I ask myself if Mypy should not handle it like:

if isinstance(x, type(y)) and isinstance(y, type(x)):
    x.y + y.x

Which could eventually easily be implemented with the help of TypeChecker.intersect_instances or something similar?

[George-Ogden]

I never really thought about type narrowing with is (except for handling None, of course).

If I look at this example:

class X: x = 1
class Y: y = 1
class Z(X, Y): ...

z = Z()
x: X = z
y: Y = z
if x is y:
    x.y + y.x  # error: "X" has no attribute "y"
               # error: "Y" has no attribute "x

I ask myself if Mypy should not handle it like:

if isinstance(x, type(y)) and isinstance(y, type(x)):
    x.y + y.x

Which could eventually easily be implemented with the help of TypeChecker.intersect_instances or something similar?

I had a look at this and I've modified the implementation to allow this kind of checking. It just needs a bit of cleaning up.

[George-Ogden]

This cannot be directly handled here because == and is are handled together.
However, we can compare types instead

class X: x = 1
class Y: y = 1
class Z(X, Y): ...

z = Z()
x: X = z
y: Y = z
if type(x) is type(y): # instead of x is y
    x.y + y.x  

[George-Ogden]

The only part I'm not sure about is how to handle the error messages.

[George-Ogden]

I'm also not sure about the failures - should they be flagged?

[github-actions]

Diff from mypy_primer, showing the effect of this PR on open source code:

pandas (https://github.com/pandas-dev/pandas)
+ pandas/core/arrays/base.py:1502: error: Redundant cast to "ExtensionArray"  [redundant-cast]

ibis (https://github.com/ibis-project/ibis)
- ibis/common/patterns.py:658: error: "Pattern" has no attribute "type"  [attr-defined]
- ibis/common/patterns.py:934: error: "Pattern" has no attribute "patterns"  [attr-defined]

jax (https://github.com/google/jax)
+ jax/_src/interpreters/partial_eval.py:96: error: Unused "type: ignore" comment  [unused-ignore]
+ jax/_src/interpreters/partial_eval.py:99: error: No overload variant of "get" of "dict" matches argument types "int | DArray | Tracer | Var | DBIdx | InDBIdx | OutDBIdx", "int | DArray | Tracer | Var | DBIdx | InDBIdx | OutDBIdx"  [call-overload]
+ jax/_src/interpreters/partial_eval.py:99: note: Possible overload variants:
+ jax/_src/interpreters/partial_eval.py:99: note:     def get(self, Name, None = ..., /) -> DBIdx | None
+ jax/_src/interpreters/partial_eval.py:99: note:     def get(self, Name, DBIdx, /) -> DBIdx
+ jax/_src/interpreters/partial_eval.py:99: note:     def [_T] get(self, Name, _T, /) -> DBIdx | _T
+ jax/_src/interpreters/batching.py:231: error: Unused "type: ignore" comment  [unused-ignore]
+ jax/_src/interpreters/batching.py:232: error: Unused "type: ignore" comment  [unused-ignore]
+ jax/_src/interpreters/batching.py:234: error: No overload variant of "get" of "dict" matches argument types "int | DArray | Tracer | Var | DBIdx | InDBIdx | OutDBIdx", "int | DArray | Tracer | Var | DBIdx | InDBIdx | OutDBIdx"  [call-overload]
+ jax/_src/interpreters/batching.py:234: note: Possible overload variants:
+ jax/_src/interpreters/batching.py:234: note:     def get(self, Name, None = ..., /) -> DBIdx | None
+ jax/_src/interpreters/batching.py:234: note:     def get(self, Name, DBIdx, /) -> DBIdx
+ jax/_src/interpreters/batching.py:234: note:     def [_T] get(self, Name, _T, /) -> DBIdx | _T
+ jax/_src/interpreters/batching.py:258: error: Unused "type: ignore" comment  [unused-ignore]

rotki (https://github.com/rotki/rotki)
+ rotkehlchen/history/events/structures/base.py:161: error: Unused "type: ignore" comment  [unused-ignore]
+ rotkehlchen/history/events/structures/base.py:162: error: Unused "type: ignore" comment  [unused-ignore]
+ rotkehlchen/history/events/structures/base.py:163: error: Unused "type: ignore" comment  [unused-ignore]
+ rotkehlchen/history/events/structures/base.py:164: error: Unused "type: ignore" comment  [unused-ignore]
+ rotkehlchen/history/events/structures/base.py:165: error: Unused "type: ignore" comment  [unused-ignore]
+ rotkehlchen/history/events/structures/base.py:166: error: Unused "type: ignore" comment  [unused-ignore]
+ rotkehlchen/history/events/structures/base.py:167: error: Unused "type: ignore" comment  [unused-ignore]
+ rotkehlchen/history/events/structures/base.py:168: error: Unused "type: ignore" comment  [unused-ignore]
+ rotkehlchen/history/events/structures/base.py:169: error: Unused "type: ignore" comment  [unused-ignore]
+ rotkehlchen/history/events/structures/base.py:170: error: Unused "type: ignore" comment  [unused-ignore]
+ rotkehlchen/history/events/structures/base.py:171: error: Unused "type: ignore" comment  [unused-ignore]
+ rotkehlchen/history/events/structures/base.py:172: error: Unused "type: ignore" comment  [unused-ignore]

pip (https://github.com/pypa/pip)
+ src/pip/_internal/utils/misc.py:551: error: Redundant cast to "HiddenText"  [redundant-cast]

[hauntsaninja]

Thanks! I think all the test cases you'd talked about are handled on master.

Do let me know if there is something on master that isn't working as intended! :-)
(I do still have a few changes planned re: reachability, generics, promotions, final classes)