FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Consistently avoid type-checking unreachable code by ikonst · Pull Request #15386 · python/mypy · GitHub

  1. python
  2. mypy

Repository navigation

Consistently avoid type-checking unreachable code - #15386

Merged
JukkaL merged 2 commits into
python:masterfrom
ikonst:06-06-Warn_unreachable_once_per_block
Jul 13, 2023
ConversationCommits (2)ChecksFiles changed
Merged

Consistently avoid type-checking unreachable code#15386
JukkaL merged 2 commits into
python:masterfrom
ikonst:06-06-Warn_unreachable_once_per_block

Conversation

ikonst commented Jun 7, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor
  • On module-level, now we'll skip remaining statements once unreachable. This brings the behavior in line with function-level behavior.
  • For module and function code, if --warn-unreachable is enabled, we'll emit an error, just once, on the first unreachable statement that's not a no-op statement. Previously a no-op statement would not have the "Unreachable statement" error, but the subsequent statements did not have the error either, e.g. 
     raise Exception
 assert False  # no error since it's a "no-op statement"
-foo = 42
+foo = 42  # E: Unreachable statement
 spam = "ham"  # no error since we warn just once

ikonst force-pushed the 06-06-Warn_unreachable_once_per_block branch from 0c9f506 to 1747d75 Compare June 7, 2023 04:26
ikonst force-pushed the 06-06-Warn_unreachable_once_per_block branch from 1747d75 to 517ff28 Compare June 7, 2023 05:23
AlexWaygood added the topic-reachability Detecting unreachable code label Jun 7, 2023
ikonst changed the title Decouple warning and skipping unreachable code Consistently avoid type-checking unreachable code Jun 7, 2023
ikonst mentioned this pull request Jun 7, 2023
Closed

ikonst commented Jun 9, 2023

Copy link
Copy Markdown
Contributor Author

👋 @KotlinIsland and @sobolevn (re: #11361)

ikonst commented Jun 12, 2023

Copy link
Copy Markdown
Contributor Author

Thinking about it more, it's strange that we avoid type checking those blocks. Having --warn-unreachable isn't much helper since (a) it's not enabled by default, and (b) even if it was, it doesn't allude to type-checking being suppressed.

In this PR I'm making function and global level consistent, but how can we know it's the consistency we want?

(From quickly testing in pyright, it narrows the expression to Never but does not suppress type checking.)

KotlinIsland commented Jun 12, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

@ikonst

(a) it's not enabled by default

It's enabled by default in basedmypy.

I'm sorry, I'm struggling a little to see all the differences in behavior made here. You are stating that it fixes #12409, but the output is identical.

The only difference I could find is your example in the OP, although within a function, which now matches the behavior observed in master for module level. I think that is a great change.

ikonst commented Jun 12, 2023

Copy link
Copy Markdown
Contributor Author

@KotlinIsland Sorry, you are right. I was looking for pre-existing issues and mistook #12409 to be it :) Removing the link.

ikonst commented Jun 12, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

It's enabled by default in basedmypy.

Not disagreeing with that, mind you. Rather, questioning why it should suppress type-checking: IMO that's unexpected and comes with no warning.

Even if the branch is truly unreachable, suppressing all type checks until you fix reachability effectively makes it a 'blocking error', which we are trying not to have (judging by other 'blocking error' issues).

ikonst commented Jun 22, 2023
View reviewed changes
Comment thread mypyc/test-data/run-misc.test
try:
# try accessing those globals that were never properly initialized
import native
native.test()

ikonst Jun 22, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I don't think native.test() is effective in this test.

The first import native fails, and so the second import native also gets executed, and also fails, and we never reach native.test().

hauntsaninja approved these changes Jul 13, 2023
View reviewed changes

hauntsaninja left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This looks good to me!

We could explore doing things the other way round and see what the mypy_primer fallout looks like. But I'm not super optimistic, I think currently this papers over some bugs in the binder

JukkaL commented Jul 13, 2023

Copy link
Copy Markdown
Collaborator

I think it's a good idea to explore changes to this behavior, as it's not very intuitive.

JukkaL merged commit 3bf8521 into python:master Jul 13, 2023
sobolevn mentioned this pull request Jul 13, 2023
Closed
ikonst deleted the 06-06-Warn_unreachable_once_per_block branch July 13, 2023 14:32
kreathon mentioned this pull request Jul 13, 2023
Merged
ikonst mentioned this pull request Jul 13, 2023
Merged
ikonst mentioned this pull request Aug 12, 2023
Closed
A5rocks mentioned this pull request Aug 26, 2023
Merged
hauntsaninja mentioned this pull request Dec 24, 2024
Open
coderabbitai Bot mentioned this pull request Jun 1, 2026
Merged
3 of 4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

hauntsaninja hauntsaninja approved these changes

Assignees

No one assigned

Labels

topic-reachability Detecting unreachable code

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL