Fix caught exception leaking through except scope
#715
Conversation
fangyi-zhou
force-pushed
the
fix-exception-leaking
branch
from
718cf0a to
7207921
Compare
fangyi-zhou
changed the title
fangyi-zhou
changed the title
except scope
pyrefly/lib/binding/stmt.rs
Outdated
| ); | ||
| } | ||
|
|
||
| if let Some(ref name) = last_caught_exception_name { |
There was a problem hiding this comment.
I'm confused why this block & the one on 816 are necessary - doesn't each exception get marked as uninitialized on 799?
There was a problem hiding this comment.
I'll try to give a more detailed answer later. In short, this has something to do with how we error on uninit variable (cf. PR #666).
If we only error at the end of the except scope, then the uninit does nothing in the next scope since silent lookup failures won't error.
So we inject an uninit to make it error.
On the other hand, we get slightly confusing and error messages of "maybe uninit" if we only inject uninit in the next scope due to how flow merging works.
I concede this is a somewhat hacky solution, let me know if you have a better idea. Sorry for the concise explanation.
Edit: Please disregard what I said previously. I agree with you that this is unnecessary
As reported in facebook#154, if a caught exception is bound to a name `e` in a `try`/`except` block, the caught name `e` should be out of scope after the current `except` scope. This is reflected in the spec as follows https://docs.python.org/3/reference/compound_stmts.html#except-clause: > When an exception has been assigned using as target, it is cleared at the end of the except clause. This PR aims to model this effect by marking the caught exception name as uninitialised at the end of the `except` clause.
fangyi-zhou
force-pushed
the
fix-exception-leaking
branch
from
7207921 to
d4cfbaa
Compare
|
Sorry for the earlier confusion. I was under the impression that adding another uninit in the next scope was necessary, but I seem to got confused by myself earlier during debugging. I've validated my first commit and concluded that adding an uninit in the end of the scope should be sufficient. It looks like I was chasing a non-existent bug earlier; but on the positive side, the solution to the issue is straightforward. |
|
@yangdanny97 has imported this pull request. If you are a Meta employee, you can view this in D78898967. |
|
@yangdanny97 merged this pull request in b0586d9. |
4 participants
As reported in #154, if a caught exception is bound to a name
ein aexceptblock, the caught nameeshould be out of scope after the correspondingexceptscope.This is reflected in the spec as follows
https://docs.python.org/3/reference/compound_stmts.html#except-clause:
This PR aims to model this effect by marking the caught exception name as uninitialised at the end of the current
exceptblock.Closes #154.