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

Checks module bodies to be reachable #11361

Merged
merged 6 commits into from
Oct 31, 2021
Merged

Checks module bodies to be reachable #11361

merged 6 commits into from
Oct 31, 2021

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Oct 19, 2021

This is a very simple change, but now module bodies are also checked to be reachable.
Let's see what effect it will have.

Closes #11331

@@ -307,6 +307,10 @@ def check_first_pass(self) -> None:
with self.tscope.module_scope(self.tree.fullname):
with self.enter_partial_types(), self.binder.top_frame_context():
for d in self.tree.defs:
if (self.binder.is_unreachable()
Copy link
Member Author

Choose a reason for hiding this comment

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

self.accept(Block(self.tree.defs)) didn't work. Let's try explicit condition.

@sobolevn sobolevn closed this Oct 19, 2021
@sobolevn sobolevn reopened this Oct 19, 2021
@sobolevn sobolevn changed the title Checks module bodies with to be reachable Checks module bodies to be reachable Oct 20, 2021
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.

Sounds like a good idea! If a user asks for warnings about unreachable code, this seems like what would be expected.

@@ -768,13 +768,13 @@ while isinstance(b, int):
else:
reveal_type(b) # E: Statement is unreachable

def foo(c: int) -> None:
def foo(c: int) -> None: # E: Statement is unreachable
Copy link
Collaborator

Choose a reason for hiding this comment

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

The rest of this test case should probably be moved to a new test case, since this is no longer testing what was the original intent, as everything is unreachable beyond this point.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this resolved? Afaict the if False check should be in its own test case right?

mypy/checker.py Show resolved Hide resolved
@sobolevn
Copy link
Member Author

@JukkaL done! Now it is in sync with pylance 👍

@@ -768,13 +768,13 @@ while isinstance(b, int):
else:
reveal_type(b) # E: Statement is unreachable

def foo(c: int) -> None:
def foo(c: int) -> None: # E: Statement is unreachable
reveal_type(c) # N: Revealed type is "builtins.int"
assert not isinstance(c, int)
Copy link

Choose a reason for hiding this comment

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

Could be a clearer statement of intent to explicitly raise an exception here. This will not make the code below unreachable if run with assertions disabled, which you can do in Python.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, because we already have tests for explicit raise. Here we test assert False

Copy link

Choose a reason for hiding this comment

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

Ah, I see. I guess my point is that assert False isn't a safe way to block off code. But whether mypy recognizes that or not is a more high level design decision, and not a subject for this PR.

@@ -768,13 +768,13 @@ while isinstance(b, int):
else:
reveal_type(b) # E: Statement is unreachable

def foo(c: int) -> None:
def foo(c: int) -> None: # E: Statement is unreachable
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this resolved? Afaict the if False check should be in its own test case right?

@sobolevn
Copy link
Member Author

@hauntsaninja I forgot to run git push 🤦

» git log
commit 2d51fb85261a23242f79e9ffaa4dc95cb7c0c5d0 (HEAD -> issue-11331)
Author: sobolevn <[email protected]>
Date:   Fri Oct 22 18:28:03 2021 +0300

    Addresses review

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.

I kind of suspected that :-) Thanks, looks great

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

Successfully merging this pull request may close these issues.

The [unreachable] check fails for module scope code and allows unreachable raising of Exceptions
4 participants