python / mypy

Optional static typing for Python
https://www.mypy-lang.org/
Other
18.57k stars 2.84k forks source link

False positive "Statement is unreachable" when checking if typevar is None #18126

Closed Cnoor0171 closed 1 week ago

Cnoor0171 commented 3 weeks ago

Bug Report

Mypy doesn't seem to like it when you check if an argument with typevar type is checked to see if it is None. It incorrectly reports error: Statement is unreachable [unreachable]

To Reproduce

https://mypy-play.net/?mypy=latest&python=3.12&flags=strict%2Cwarn-unreachable&gist=ceb8193f19f8b53a55dea70fad802469

import typing as t

T = t.TypeVar("T")

def foo(arg: T) -> None:
    if arg is None:
        return None
    # Do something with arg
    return None

foo(None) ## Allowed
foo(123)

Run mypy with --warn-unreachable flag

Expected Behavior

No errors

Actual Behavior

$ mypy .vscode/scratch.py
.vscode/scratch.py: note: In function "foo":
.vscode/scratch.py:9: error: Statement is unreachable  [unreachable]
Found 1 error in 1 file (checked 1 source file)

The error seems to only happen when checking if arg is None, but not with other types. For example, I tired changing if arg is None: line to the following and these were the results.

  1. if arg is True: -> no errors
  2. if isinstance(arg, int): -> no errors
  3. if isinstance(arg, type(None)): -> unreachable error
  4. if arg == None: -> No error

Also, changing the type to T | None removes the error as well.

Your Environment

sterliakov commented 2 weeks ago

There's something wrong with this logic. When checking for narrowing in if clause by is None, we explicitly request to not consider bare TypeVar overlapping with None. The obvious decision is to stop asking for that, but... There's a test asserting this behaviour (introduced in #5476, and if clause is indeed unreachable - using a non-trivial body gives an [unreachable] diagnostic with --warn-unreachable):

https://github.com/python/mypy/blob/3b00002acdf098e7241df8f2e1843f8b8260b168/test-data/unit/check-isinstance.test#L2210-L2227

I don't think that this approach is sound: narrowing a lone TypeVar is a common problem, it shouldn't erroneously mark code as unreachable.

binder builds a Union of types after all non-terminating branches when exiting a frame, so here it produces None | T (None from the if clause since it's reachable, and T from a pass-through).

I tried to modify the binder to keep track of whether every type in frame was updated via an isinstance check or by assignment, and this seems to work as expected. I'm not sure, however, that this approach covers all corner cases. We already know that TypeVar + isinstance causes a lot of pain (#9424 and many others), so this probably won't make things marginally worse.