Enable `warn_unreachable` for `mypy` self-check by sobolevn · Pull Request #18523 · python/mypy

sobolevn · 2025-01-24T09:21:08Z

This check prooved to be useful, since it find a lot of dead / incorrect code.

Closes#18079

for more information, see https://pre-commit.ci

hauntsaninja

Awesome. I think it could worth replacing some of these with asserts, just in case there's something mypy has got wrong

sobolevn · 2025-01-24T09:44:42Z

I think it could worth replacing some of these with asserts

Do you have some specific items in mind? I can surely add asserts where needed.

…ue-18079

A5rocks · 2025-01-24T19:37:21Z

I think it could worth replacing some of these with asserts
Do you have some specific items in mind? I can surely add asserts where needed.

I actually just did this too! I ended up with quite a few asserts so maybe that can be a second opinion. master...A5rocks:mypy:experiments/warn_unreachable (I also deferred on mypy_extensions.trait subclassing instead of updating the isinstance)

Though that branch also passes mypy's test suite, so maybe that's a sign that actually those asserts aren't necessary at all and mypy was correct.

A5rocks

I'm generally unsure about the isinstance(x, FuncBase) changes where x: Node (and similar mypy_extensions.trait workarounds) because it seems like that just uses tuples to work around reachability? I haven't tried some testcases locally so I'm not entirely sure though.

A5rocks · 2025-01-24T19:40:56Z

- assert sys.platform == "win32"
- if sys.platform == "win32":
+ assert sys.platform == "win32", "Running not on Windows"
+ if sys.platform == "win32": # needed to find win specific sys apis


You can un-indent this.

Nope, sadly I can't :(
Looks like that mypy does not narrow sys.platform after assert (this might be a bug).

But, I added assert False in the end instead, it is more readable now.

Nope, sadly I can't :( Looks like that mypy does not narrow sys.platform after assert (this might be a bug).
Huh I was able to. But I guess that might just be because I was testing on Windows.

Yes, you need to set platform = win32 or platform = linux manually in .ini file for that.

sobolevn · 2025-01-24T21:12:02Z

I'm generally unsure about the isinstance(x, FuncBase) changes where x: Node (and similar mypy_extensions.trait workarounds) because it seems like that just uses tuples to work around reachability?

Sorry, I don't understand your question. Can you please provide an example?

A5rocks · 2025-01-25T06:19:51Z

I'm generally unsure about the isinstance(x, FuncBase) changes where x: Node (and similar mypy_extensions.trait workarounds) because it seems like that just uses tuples to work around reachability?
Sorry, I don't understand your question. Can you please provide an example?

I was confused, actually. I've now read the docstring of FuncBase and I get why you switched out the isinstance checks... Except it refers to an issue that's closed! You're just doing #13607 (comment) (3), which will mean we have to revert a bunch of lines whenever we actually fix the issue (1).

IMO we should fix the issue at hand but continuing with (3) is OK as long as you update the comment around SYMBOL_FUNCBASE_TYPES (and maybe put a TODO or something there?).

sobolevn · 2025-01-26T08:03:56Z

I will leave this open for a couple of days to give an opportunity for second rounds of reviewes, and merge on Wen or so :)

Thanks, @A5rocks!

ammaraskar · 2025-01-26T20:56:57Z



 def normalize_path_separators(path: str) -> str:
- if sys.platform == "win32":


If #18539 or a similar fix is merged, then this change should no longer be necessary I believe.

sobolevn · 2025-02-13T15:50:47Z

Going to merge #18523 later tomorrow, if no one objects

github-actions · 2025-02-13T22:48:41Z

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

freqtrade (https://github.com/freqtrade/freqtrade): 1.95x slower (129.1s -> 252.2s in a single noisy sample)

This check prooved to be useful, since it find a lot of dead / incorrect code. Closespython#18079 --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

Enable warn_unreachable for mypy self-check
f45652d

sobolevn requested review from JukkaL, hauntsaninja and ilevkivskyi January 24, 2025 09:21

[pre-commit.ci] auto fixes from pre-commit.com hooks
867958e
for more information, see https://pre-commit.ci

This comment has been minimized.
Sign in to view

hauntsaninja approved these changes Jan 24, 2025
View reviewed changes

sobolevn added 3 commits January 24, 2025 12:44

Fix CI
792ba28

Merge branch 'issue-18079' ofhttps://github.com/python/mypyinto iss…
4607277
…ue-18079

Fix CI
b65e27a

This comment has been minimized.
Sign in to view

Fix CI
8a73b5d

This comment has been minimized.
Sign in to view

A5rocks reviewed Jan 24, 2025
View reviewed changes

Address review
bbb830f

ilevkivskyi reviewed Jan 24, 2025
View reviewed changes

Comment threadmypy/constraints.py

ilevkivskyi reviewed Jan 24, 2025
View reviewed changes

Comment threadmypy/nodes.py

This comment has been minimized.
Sign in to view

ilevkivskyi reviewed Jan 24, 2025
View reviewed changes

Comment threadmypy/server/mergecheck.py

Address reviews
768b54c

This comment has been minimized.
Sign in to view

A5rocks approved these changes Jan 26, 2025
View reviewed changes

ammaraskar reviewed Jan 26, 2025
View reviewed changes

sobolevn added 2 commits February 13, 2025 18:02

Merge branch 'master' into issue-18079
547a6d7

Update nodes.py
4859d91

This comment has been minimized.
Sign in to view

A5rocks reviewed Feb 13, 2025
View reviewed changes

Comment threadmypy/nodes.py Outdated

sobolevn added 2 commits February 14, 2025 01:15

Merge branch 'master' into issue-18079
bf56d55

Address review
c5e56cb

sobolevn merged commit 555bfae into masterFeb 14, 2025

sobolevn deleted the issue-18079 branch February 14, 2025 06:13



		def normalize_path_separators(path: str) -> str:
		if sys.platform == "win32":

Uh oh!

Uh oh!

Conversation

sobolevn commented Jan 24, 2025

Uh oh!

This comment has been minimized.

hauntsaninja left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sobolevn commented Jan 24, 2025

Uh oh!

This comment has been minimized.

This comment has been minimized.

A5rocks commented Jan 24, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

A5rocks left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

A5rocksJan 24, 2025

Choose a reason for hiding this comment

Uh oh!

sobolevnJan 24, 2025

Choose a reason for hiding this comment

Uh oh!

sobolevnJan 24, 2025

Choose a reason for hiding this comment

Uh oh!

A5rocksJan 25, 2025

Choose a reason for hiding this comment

Uh oh!

sobolevnJan 25, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sobolevn commented Jan 24, 2025

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

A5rocks commented Jan 25, 2025

Uh oh!

This comment has been minimized.

sobolevn commented Jan 26, 2025

Uh oh!

ammaraskarJan 26, 2025

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

sobolevn commented Feb 13, 2025

Uh oh!

Uh oh!

github-actionsBot commented Feb 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hauntsaninja left a comment •
edited
Loading

A5rocks commented Jan 24, 2025•
edited
Loading

A5rocks left a comment •
edited
Loading