-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[partially defined] implement support for try statements #14114
Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, nested try statements seem to be potentially quite inefficient due to exponential behavior? I'll try to come up with a way to avoid analyzing the same nodes multiple times.
# Conflicts: # mypy/partially_defined.py
So right now we process the body for every except type. We could probably cache the result of processing the body and just reuse that. What do you think about that? Note that we will still need to process it twice (once with disabling jumps and once without). That one is potentially more difficult to fix. |
This comment has been minimized.
This comment has been minimized.
mypy/partially_defined.py
Outdated
self.tracker.next_branch() | ||
self.tracker.end_branch_statement() | ||
o.handlers[i].accept(self) | ||
for v in o.vars: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This handling is not correct. I think they should be defined conditionally for the handler.
# Conflicts: # mypy/partially_defined.py
This comment has been minimized.
This comment has been minimized.
Here's a quick idea about how we might be able to avoid exponential behavior. I haven't fully thought this out -- there maybe some obvious thing why this doesn't work. Anyway, I wonder if we could analyze a try/except like these statements (and just once): if ...:
try_body
if ...:
except 1
elif ...:
except 2
elif ...:
except 3
else:
else_block
finally_block The target try statement would look like this: try:
try_body
<except blocks 1..3>
else:
else_block
finally:
finally_block |
I implemented your proposal (which was pretty easy). Here's an example where this doesn't work: def f1(div: int) -> int:
try:
x = 100 // div
except ArithmeticError:
x = 0
return x # False positive: "x" is always defined but we get an error here. This is happening because the body of the try is conditional, independently of the except clause. Now, I also implemented the following: if ...:
try_body
else_block
elif ...: # Note the change from if to elif here.
except 1
elif ...:
except 2
finally_block However, in this case we don't detect undefined variables inside finally itself. def f0(div: int) -> int:
try:
x = 100 // div
except ArithmeticError:
return 0
finally:
# "x" is not defined in the except ArithmeticError branch, which returns.
# Because it returns, the PartiallyDefined check considers this branch "skipped"
# and doesn't think it's undefined.
y = x # False negative: this should be an error but mypy reports nothing.
return y So to implement it 100% correct, we need to disable branch skipping. I have updated this PR with a version of the check that goes through every try/except/finally 2 times (the previous implementation was doing some unnecessary work). I believe this is the best we can do, unless we're willing to trade off correctness. I've also found an unrelated bug in nested branch statements that get skipped. It's fixed and I added tests. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…kipped branch (#14221) The goal of partially-defined check is to detect variables that could be undefined but semantic analyzer doesn't detect them as undefined. In this case, a variable was defined in a branch that returns, so semantic analyzer considered it defined when it was not. I've discovered this when testing support for try/except statements (#14114).
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Apologies for the slow review.
[builtins fixtures/exception.pyi] | ||
|
||
[case testTryElse] | ||
# flags: --enable-error-code partially-defined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error codes need to be updated.
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
This adds support for try/except/finally/else check.
The implementation ended up pretty complicated because it had to handle jumps different for finally. It took me a few iterations to get to this solution and that's the cleanest one I could come up with.
Let me know if you have suggestions on how to structure the code differently.
Closes #13928.