0.990 regression with implicit complex/int promotion

[JelleZijlstra]

Given this code:

from typing import reveal_type
x: complex = 1
reveal_type(x)
if isinstance(x, int):
    reveal_type(x)

mypy 0.990:

% mypy --strict --warn-unreachable ~/py/tmp/compl.py
/Users/jelle/py/tmp/compl.py:3: note: Revealed type is "builtins.complex"
/Users/jelle/py/tmp/compl.py:4: error: Subclass of "complex" and "int" cannot exist: would have incompatible method signatures  [unreachable]
/Users/jelle/py/tmp/compl.py:5: error: Statement is unreachable  [unreachable]
Found 2 errors in 1 file (checked 1 source file)

mypy 0.982:

% mypy --strict --warn-unreachable ~/py/tmp/compl.py
/Users/jelle/py/tmp/compl.py:3: note: Revealed type is "builtins.complex"
/Users/jelle/py/tmp/compl.py:5: note: Revealed type is "builtins.int"
Success: no issues found in 1 source file

The 0.982 behavior is correct, since int is a subclass of complex in the static type system.

[AlexWaygood]

This bug introduces 8 false positives when mypy checks the flake8-pyi codebase (we have --warn-unreachable enabled in our pyproject.toml file):

pyi.py:1126: error: Subclass of "complex" and "int" cannot exist: would
have incompatible method signatures  [unreachable]
                    if type(literal.n) is int:
                            ^~~~~~~~~
pyi.py:1275: error: Subclass of "complex" and "int" cannot exist: would
have incompatible method signatures  [unreachable]
                    if isinstance(slc.n, int) and slc.n == 0:
                                  ^~~~~
pyi.py:1275: error: Right operand of "and" is never evaluated
[unreachable]
                    if isinstance(slc.n, int) and slc.n == 0:
                                                  ^~~~~~~~~~
pyi.py:1276: error: Statement is unreachable  [unreachable]
                        must_be_single = True
                        ^~~~~~~~~~~~~~~~~~~~~
pyi.py:1287: error: Subclass of "complex" and "int" cannot exist: would
have incompatible method signatures  [unreachable]
                        and isinstance(slc.upper.n, int)
                                       ^~~~~~~~~~~
pyi.py:1288: error: Right operand of "and" is never evaluated
[unreachable]
                        and slc.upper.n in (1, 2)
                            ^~~~~~~~~~~~~~~~~~~~~
pyi.py:1290: error: Statement is unreachable  [unreachable]
                        can_have_strict_equals = slc.upper.n
                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
pyi.py:1313: error: Subclass of "complex" and "int" cannot exist: would
have incompatible method signatures  [unreachable]
    ...nstance(comparator, ast.Num) or not isinstance(comparator.n, int):
                                                      ^~~~~~~~~~~~
Found 8 errors in 1 file (checked 1 source file)

[hauntsaninja]

cc @ilevkivskyi this is from #13781

[ilevkivskyi]

FWIW this is intentional, isinstance() checks have always been determined by runtime behavior, not static subtyping. Previously promotions sometimes leaked, and caused various inconsistencies.

There is really no middle ground, if we enable promotions, there will be other issues, see, e.g. #6060. I would say current behavior is better because:

• It is more consistent with how isinstance() check behaves in other situations
• It may cause "false positives" with a strictness flag, while opposite causes false positives with default options.

[ilevkivskyi]

Also fix should be trivial, use a more precise type, like Union[int, complex].

[JelleZijlstra]

Without strictness flags, the current behavior leads code to be silently skipped as unreachable, which is arguably worse. I agree that there are few good options here though.

[hauntsaninja]

Maybe there's some hack, so we keep the better narrowing behaviour, but it doesn't affect reachability

[ilevkivskyi]

@hauntsaninja Actually, if you like hacks, I think there may be a (relatively) easy one: ignore the impossible intersection check in case if one type can be promoted to another one.

[ilevkivskyi]

Well, we should probably still prohibit (consider unreachable) something like:

if isinstance(x, int) and isinstance(x, float):
    ...

so it may require some work to implement.

[A5rocks]

Just making sure you all are aware of the idea in #11516

Specifically: in the if statement, split types up into this union and rely on already-existing narrowing.

Y'all are probably already aware, but just making sure this "solution" is considered. (I don't see any reasoning on that issue itself that would disqualify it)

[ilevkivskyi]

@hauntsaninja Actually after thinking some more, it seems to me we shouldn't change this, otherwise we may not flag legitimately unreachable code like

x = 1j
if isinstance(x, int):
    ...

that may potentially also hide bugs.

In general, most of the original motivation for type promotions was that practically all functions originally intended for float will also (accidentally) work for int. So to avoid forcing people to annotate all their argument types as e.g. Union[int, float], this decision was made. On the opposite, if a function actually differentiates between int and float, it should be annotated as taking Union[int, float] to emphasize this:

def weighted(a: float, b: float) -> float:
    return (a + 2 * b) / 3
weighted(1, 2)  # This is fine

def num_type(x: float) -> str:  # Bad, should be Union[int, float]
    if isinstance(x, int):
        return "Integer"
    return "Floating"

Anyway, it is still gray area and likely there will still be some people who likes (or used to) the old behavior. So we should clearly document this.

[JelleZijlstra]

I agree with Ivan's proposed approach. Some thoughts on what we'd need to change:

• In mypy, the new behavior can lead mypy to silently skip checking code. To make that a less likely problem, we should move forward with including --warn-unreachable in --strict (Add --warn-unreachable to strict mode #11223).
• In flake8-pyi we should drop the check against int | float unions (Y041).
• In typeshed we should document when to use int | float and make changes where appropriate, including in the type for ast.Num.n.

[AlexWaygood]

Some thoughts from me:

• I find it pretty confusing for us to say "int is an implicit subtype of float, but only sometimes. Sometimes you can just annotate an argument with float and it'll mean float | int, but other times it'll cause surprising errors." Rereading PEP 484, the spec is vaguer than I remembered, so I guess this isn't going against the spec. It feels inherently confusing and pretty user-unfriendly, though, and I'm not sure any amount of documentation is going to solve that :(
• I'm not exactly a fan of implicit promotions; in an ideal world, I'd love it if we could have a more principled solution to this problem. But deprecating float/int subtyping doesn't feel like a realistic proposal at this point in time.
• I agree that we should definitely change flake8-pyi's Y041 check if mypy is sticking with this behaviour. I'm not sure if we should drop it completely or change it so that it only applies to parameter annotations in functions.
• Making --warn-unreachable the default if you have --strict enabled would be great, but I don't think it's doable at the moment. It currently has loads of false positives if you're dealing with code that has platform-specific branches. You can see this if you try to enable --warn-unreachable for mypy's selfcheck.

[JukkaL]

I think that it would be least confusing if we managed to preserve the isinstance behavior of 0.982 while still ignoring promotions when simplifying unions generally.

I think that it's okay to allow users to be explicit and annotate things as float | int and so on, but this feels like a pretty advanced feature and most users should need to know about it.

... that may potentially also hide bugs.

I don't think that is a significant issue. This is also possible with real subclasses and I don't think anybody has complained about code like this not generating errors:

class Base: pass
class Sub(Base): pass

o = Base()
if isinstance(o, Sub): 
    <something>

[ilevkivskyi]

I think that it would be least confusing if we managed to preserve the isinstance behavior of 0.982 while still ignoring promotions when simplifying unions generally.

isinstance() behavior in 0.982 is just wrong, and we had issues about it, examples #6060, #12824

[ilevkivskyi]

This is also possible with real subclasses and I don't think anybody has complained about code like this not generating errors

This is different. With complex vs int that branch is always unreachable, and if someone asks for --warn-unreachable we should warn. Not doing so is a bug.

[ilevkivskyi]

I find it pretty confusing for us to say "int is an implicit subtype of float, but only sometimes...

It was always like this, since both extremes are clearly bad, we need to find a reasonable/clear midpoint. FWIW the current one seems more consistent to me than in 0.982

[ilevkivskyi]

Also in general I am in favor of adding --warn-unreachable to --strict, but as it was mentioned above, this will require some work.

[JukkaL]

isinstance() behavior in 0.982 is just wrong, and we had issues about it, examples #6060, #12824

I don't want to resurrect those bugs. But I'd specifically like this one to not generate unreachable code:

from typing import reveal_type
x: complex = 1
reveal_type(x)
if isinstance(x, int):
    reveal_type(x)

Requiring x: complex | int = 1 here (but not generally) seems hard to explain, and having the body of the if statement unreachable could result in buggy code getting accepted if --warn-unreachable is not used. I may be okay with some special casing for this use case. Uses may also consider the 0.990 behavior a regression from 0.982.

This is different. With complex vs int that branch is always unreachable, and if someone asks for --warn-unreachable we should warn. Not doing so is a bug.

I think that sometimes not detecting unreachable code is less of a problem than code that is silently not being type checked because mypy things that it's unreachable but it really is reachable. I'd expect that the vast majority of users aren't using --warn-unreachable.

[ilevkivskyi]

I don't want to resurrect those bugs. But I'd specifically like this one to not generate unreachable code

OK, as I said above it is possible with some special casing. The less easy part is how to make the special-casing consistent. e.g. should these two be flagged as unreachable?

x: object  # Or a union
if isinstance(x, complex) and isinstance(x, int):
    ...

if isinsatnce(x, complex):
    if isinstance(x, int):
        ...

If we continue to flag them as unreachable, then it will be inconsistent with

x: complex
if isinsatnce(x, int):
    ...

If we will also make those two reachable, what type will we give to x inside? An impossible intersection, or make it depend on order int vs complex?

[ilevkivskyi]

Actually, maybe saying all of those are reachable, and x having int type (even with flipped order) is an option.

[JukkaL]

Actually, maybe saying all of those are reachable, and x having int type (even with flipped order) is an option.

It seems reasonable to have all of the examples reachable. This could be either way (we can go with whatever is straightforward to implement):

x: object  # Or a union
if isinsatnce(x, int):
    if isinstance(x, complex):
        ...  # Unreachable?