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

Add support for jump statements in partially defined vars check #13632

Merged
merged 6 commits into from
Sep 12, 2022

Conversation

ilinum
Copy link
Collaborator

@ilinum ilinum commented Sep 8, 2022

Description

This builds on #13601 to add support for statements like continue, break, return, raise in partially defined variables check. The simplest example is:

def f1() -> int:
    if int():
        x = 1
    else:
        return 0
    return x

Previously, mypy would generate a false positive on the last line of example. See test cases for more details.

Adding this support was relatively simple, given all the already existing code.

Things that aren't supported yet: match, with, and detecting unreachable blocks.

After this PR, when enabling this check on mypy itself, it generates 18 errors, all of them are potential bugs.

Test Plan

Tests added.

@github-actions

This comment has been minimized.

@ilinum ilinum marked this pull request as ready for review September 8, 2022 19:23
@ilinum ilinum requested a review from JukkaL September 8, 2022 19:23
Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Awesome feature!

mypy/build.py Outdated Show resolved Hide resolved
@@ -137,3 +144,158 @@ else:
z = 2

a = z + y # E: Name "y" may be undefined
[case testReturn]
Copy link
Member

@sobolevn sobolevn Sep 8, 2022

Choose a reason for hiding this comment

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

Question: will it work well together with rechability check? For example:

if int():
   raise ValueError  # or `return` or whatever
   x = 1
else:
   x = 2

a = x  # ?

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The example you provided will work. But here's an example where it doesn't work very well yet:

if False:
    y = 1  # E: statement is unreachable
else:
    x = 2

a = x  # E: variable may be undefined

The plan is to check if a block is reachable, which I think should make the sample I posted return have only one error (the unreachable one).

mypy/partially_defined.py Outdated Show resolved Hide resolved
@github-actions

This comment has been minimized.

2 similar comments
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

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.

Looks great! Left a few minor comments.

I also noticed that this case still doesn't work as expected:

def f() -> None:
    while True:
        x = 1
        if int():
            break
    print(x)  # should not be an error

If the while condition is always true, the body is always executed at least once. It's okay to fix this in a follow-up PR if the fix is non-trivial.

test-data/unit/check-partially-defined.test Outdated Show resolved Hide resolved
test-data/unit/check-partially-defined.test Show resolved Hide resolved
x = 1
else:
break
y = x # No error -- x is always defined.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Test accessing variables defined in the body of a loop that uses break outside the loop?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not quite sure what you mean. Are you referring to the example you gave in the other message?

def f() -> None:
    while True:
        x = 1
        if int():
            break
    print(x)  # should not be an error

@ilinum
Copy link
Collaborator Author

ilinum commented Sep 10, 2022

I also noticed that this case still doesn't work as expected:

def f() -> None:
    while True:
        x = 1
        if int():
            break
    print(x)  # should not be an error

Added support for this. One question is that I have a check for if is_true_literal(expr) here, which means that something like while not False will not work the same way.

I noticed that there's a function called infer_condition_value, which would be exactly what I want but it, unfortunately, doesn't support bool literals. Is this something that we would want to fix in that function? Or is support for literals intentionally omitted there?

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

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 updates!

@JukkaL JukkaL merged commit 216a45b into python:master Sep 12, 2022
@JukkaL
Copy link
Collaborator

JukkaL commented Sep 12, 2022

I noticed that there's a function called infer_condition_value, which would be exactly what I want but it, unfortunately, doesn't support bool literals. Is this something that we would want to fix in that function? Or is support for literals intentionally omitted there?

Hmm, I don't remember why it behaves like this. I think that using while True: / while 1: is a common idiom, and anything else that is always true is probably pretty marginal and not essential, at least initially. What about creating a follow-up issue about this, and we may get it to it later once we have tried this with more real-world code?

@ilinum ilinum deleted the partially-defined/support-jumps branch September 12, 2022 16:50
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.

3 participants