Skip to content

Typecheck unreachable code #18707

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 46 commits into
base: master
Choose a base branch
from

Conversation

A5rocks
Copy link
Collaborator

@A5rocks A5rocks commented Feb 19, 2025

Mypy doesn't typecheck unreachable code and this is surprising to many. This is an attempt at fixing that. It's a draft until:

  • I list out the issues fixed (done)
  • I fix the bugs raised by the test cases (done)
  • I fix the bugs raised by mypy primer (done)
  • I get feedback that this is fine (skipped for now)
  • I put this behind a flag (we can bundle this in --strict maybe and hopefully enable it by default in 2.0) (skipped for now, I will add after reviews)

Problem areas based on test cases:

  • currently we use None for unreachable type maps but we want to know what is unreachable to mark it as Never (fixed what was relevant in tests. narrowing is not guaranteed by mypy and we can fix things in the future -- I'd rather have context for any fixes)
    • it doesn't seem too bad to mark unreachability based on if a variable has a Never type
  • analyzing a function multiple times for type var values (fixed)
    • I think I can disable this new behavior based on if we're in a function like that?
  • reporting unreachability multiple times (block in a block) (fixed)
    • blocks could store unreachability report status
  • exports could change (see check-incremental.test's changes)
    • I think this is fine?
  • Never doesn't support operations (e.g. <Never>.f(), <Never> > 5) (fixed)
    • it should return Never for these, I think?

Also maybe reveal_type(<Never>) should be non-trivial. (fixed)

Generally I tried enabling --warn-unreachable in a test case if it looked like people were relying on un-checked unreachable code as a test. Also obviously many of the test case changes are wrong I just did not have the energy to fix all the bugs (and honestly maybe should think through the two things above -- the first especially was a source of issues).

I also have no clue what mypyc does so the test change there could be awful.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@A5rocks
Copy link
Collaborator Author

A5rocks commented Mar 6, 2025

I am happy with how the test changes are now. I'll update code a bit based on mypy primer results but I think this is mostly final.

@A5rocks A5rocks requested a review from JukkaL March 6, 2025 09:43

This comment has been minimized.

I don't know how to test this.
@A5rocks
Copy link
Collaborator Author

A5rocks commented Mar 13, 2025

Alright, I went through mypy-primer. ... well, skimmed over at the end, but whatever. The biggest thing that feels wrong but actually isn't is something like this:

x: int
if isinstance(x, int):
    raise ValueError()

reveal_type(x)  # <- this is not Never

This is because the end of both branches are unreachable. It's impossible to get to that reveal_type through the if branch (an error is unconditionally raised) and the same for the else branch (an int is always a subclass of an int). Therefore something must be lying to mypy and importantly mypy does not know what. So it can't narrow. (this is a justification -- I'm not sure if the reason this actually happens is a is None check or something. But it works just fine and probably should remain. In fact, I'll add a test for this)

This comment has been minimized.

@A5rocks A5rocks marked this pull request as ready for review March 13, 2025 10:42
Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! Not a full review yet, just a few comments. I probably won't have time to review this in the next few days at least, so if anybody else has bandwidth, please have a look.

@JukkaL
Copy link
Collaborator

JukkaL commented Mar 17, 2025

Also there's now a merge conflict.

This comment has been minimized.

@A5rocks A5rocks requested a review from JukkaL March 31, 2025 14:42

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

github-actions bot commented Apr 9, 2025

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

beartype (https://github.com/beartype/beartype)
+ beartype/_util/cache/utilcachecall.py:575: error: Incompatible return value type (got "Callable[..., Any]", expected "CallableT")  [return-value]

manticore (https://github.com/trailofbits/manticore)
+ manticore/wasm/types.py:56: error: Unused "type: ignore" comment  [unused-ignore]

prefect (https://github.com/PrefectHQ/prefect)
- src/prefect/server/models/deployments.py:744: error: "Never" has no attribute "model_dump"  [attr-defined]

pandas (https://github.com/pandas-dev/pandas)
- pandas/io/formats/style.py:1903: error: "Never" has no attribute "shape"  [attr-defined]
- pandas/io/formats/style.py:1906: error: "Never" has no attribute "shape"  [attr-defined]

spark (https://github.com/apache/spark)
+ python/pyspark/core/context.py:818: error: Unused "type: ignore" comment  [unused-ignore]
+ python/pyspark/core/context.py:819: error: Unused "type: ignore" comment  [unused-ignore]

xarray (https://github.com/pydata/xarray)
+ xarray/core/variable.py:900: error: Unused "type: ignore" comment  [unused-ignore]
+ xarray/core/variable.py:902: error: Unused "type: ignore" comment  [unused-ignore]
+ xarray/core/variable.py:2786: error: Unused "type: ignore" comment  [unused-ignore]
+ xarray/core/variable.py:2788: error: Unused "type: ignore" comment  [unused-ignore]
- xarray/computation/apply_ufunc.py: note: In function "apply_variable_ufunc":
- xarray/computation/apply_ufunc.py:845: error: "Never" has no attribute "ndim"  [attr-defined]
- xarray/computation/apply_ufunc.py:848: error: "Never" has no attribute "ndim"  [attr-defined]

jinja (https://github.com/pallets/jinja)
+ src/jinja2/runtime.py:903: error: Statement is unreachable  [unreachable]

websockets (https://github.com/aaugustin/websockets)
+ src/websockets/legacy/protocol.py:1077: error: Argument 1 to "join" of "str" has incompatible type "list[str | bytes]"; expected "Iterable[str]"  [arg-type]
+ src/websockets/legacy/protocol.py:1077: error: Argument 1 to "join" of "bytes" has incompatible type "list[str | bytes]"; expected "Iterable[Buffer]"  [arg-type]

Tanjun (https://github.com/FasterSpeeding/Tanjun)
- tanjun/parsing.py:588: error: "Never" has no attribute "__iter__" (not iterable)  [attr-defined]
- tanjun/parsing.py:1632: error: Statement is unreachable  [unreachable]
- tanjun/parsing.py:1642: error: Statement is unreachable  [unreachable]
- tanjun/dependencies/limiters.py:851: error: "Never" has no attribute "unlock"  [attr-defined]
+ tanjun/dependencies/limiters.py:852: error: Statement is unreachable  [unreachable]
- tanjun/commands/slash.py:820: error: Statement is unreachable  [unreachable]
- tanjun/commands/slash.py:1787: error: Statement is unreachable  [unreachable]
- tanjun/clients.py:2294: error: "Never" has no attribute "__iter__" (not iterable)  [attr-defined]
+ tanjun/clients.py:2295: error: Statement is unreachable  [unreachable]

ibis (https://github.com/ibis-project/ibis)
- ibis/common/bases.py:231: error: Need type annotation for "field"  [var-annotated]

jax (https://github.com/google/jax)
+ jax/_src/interpreters/partial_eval.py:97: error: Unused "type: ignore" comment  [unused-ignore]
+ jax/_src/interpreters/partial_eval.py:100: 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:100: note: Possible overload variants:
+ jax/_src/interpreters/partial_eval.py:100: note:     def get(self, Name, None = ..., /) -> DBIdx | None
+ jax/_src/interpreters/partial_eval.py:100: note:     def get(self, Name, DBIdx, /) -> DBIdx
+ jax/_src/interpreters/partial_eval.py:100: note:     def [_T] get(self, Name, _T, /) -> DBIdx | _T
+ jax/_src/interpreters/batching.py:235: error: Unused "type: ignore" comment  [unused-ignore]
+ jax/_src/interpreters/batching.py:237: 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:237: note: Possible overload variants:
+ jax/_src/interpreters/batching.py:237: note:     def get(self, Name, None = ..., /) -> DBIdx | None
+ jax/_src/interpreters/batching.py:237: note:     def get(self, Name, DBIdx, /) -> DBIdx
+ jax/_src/interpreters/batching.py:237: note:     def [_T] get(self, Name, _T, /) -> DBIdx | _T
+ jax/_src/interpreters/batching.py:261: error: Unused "type: ignore" comment  [unused-ignore]

core (https://github.com/home-assistant/core)
+ homeassistant/components/rfxtrx/__init__.py:455: error: Statement is unreachable  [unreachable]

graphql-core (https://github.com/graphql-python/graphql-core)
+ tests/execution/test_subscribe.py:212: error: Unused "type: ignore" comment  [unused-ignore]

pytest (https://github.com/pytest-dev/pytest)
+ testing/code/test_source.py:238: error: Unused "type: ignore" comment  [unused-ignore]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants