Skip to content
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

Consistently avoid type-checking unreachable code #15386

Merged
merged 2 commits into from
Jul 13, 2023

Conversation

ikonst
Copy link
Contributor

@ikonst ikonst commented Jun 7, 2023

  • 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 ikonst force-pushed the 06-06-Warn_unreachable_once_per_block branch from 0c9f506 to 1747d75 Compare June 7, 2023 04:26
@ikonst ikonst force-pushed the 06-06-Warn_unreachable_once_per_block branch from 1747d75 to 517ff28 Compare June 7, 2023 05:23
@AlexWaygood AlexWaygood added the topic-reachability Detecting unreachable code label Jun 7, 2023
@ikonst ikonst changed the title Decouple warning and skipping unreachable code Consistently avoid type-checking unreachable code Jun 7, 2023
@ikonst
Copy link
Contributor Author

ikonst commented Jun 9, 2023

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

@ikonst
Copy link
Contributor Author

ikonst commented Jun 12, 2023

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
Copy link
Contributor

KotlinIsland commented Jun 12, 2023

@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
Copy link
Contributor Author

ikonst commented Jun 12, 2023

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

@ikonst
Copy link
Contributor Author

ikonst commented Jun 12, 2023

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).

try:
# try accessing those globals that were never properly initialized
import native
native.test()
Copy link
Contributor Author

@ikonst ikonst Jun 22, 2023

Choose a reason for hiding this comment

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

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().

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

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
Copy link
Collaborator

JukkaL commented Jul 13, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic-reachability Detecting unreachable code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants