CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[red-knot] Add control flow for try/except blocks (v3) #13729
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
Conversation
1779602
to
aacc4e7
Compare
|
crates/red_knot_python_semantic/src/semantic_index/builder/except_handlers.rs
Outdated
Show resolved
Hide resolved
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.
Wow, the code makes this seem easy
This looks good but I would prefer if we avoid using RefCell
in the context. I even think that regular borrowing will simplify the implementation by removing the need for the MutRef
type.
crates/red_knot_python_semantic/src/semantic_index/builder/except_handlers.rs
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/src/semantic_index/builder/except_handlers.rs
Outdated
Show resolved
Hide resolved
e4d1f72
to
21ab481
Compare
Thanks @MichaReiser! Managed to get rid of the RefCell 🎉 |
CodSpeed Performance ReportMerging #13729 will not alter performanceComparing Summary
|
I don't understand how the last commit (21ab481) I pushed could have caused this. Nothing I did in that commit should have had any negative impact on performance, I don't think. This doesn't make any sense to me. (Edit: I tried squashing commits and force-pushing to trigger a rerun of the benchmarks; no difference |
21ab481
to
7717eae
Compare
Ready for re-review again! |
e946b32
to
926df03
Compare
I'll wait before merging in case @carljm wants to take another look first. |
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.
Very happy with where this ended up, for now! Awesome work.
Just a few nits, all related to test/comment text, none of them blocking.
crates/red_knot_python_semantic/resources/mdtest/exception/control_flow.md
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/resources/mdtest/exception/control_flow.md
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/resources/mdtest/exception/control_flow.md
Show resolved
Hide resolved
// jumped from a `try`, `else` or `except` branch straight into the `finally` branch. | ||
// This requires some rethinking of some fundamental assumptions the `use-def` map makes. | ||
// For more details, see: | ||
// - https://astral-sh.notion.site/Exception-handler-control-flow-11348797e1ca80bb8ce1e9aedbbe439d |
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.
Again here, I'm not convinced we add value by linking this doc, again. I think the narrative in the test suite already provides an excellent overview of the finally
topic, with examples, and will be easier to maintain and keep correct over time than an external Notion document.
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 Notion document is more expansive and includes some examples and discussion that were not included in the test suite, either for brevity or because it's silly to add tests for things where all the relevant assertions would be TODOs. I agree with your point that there are too many links to it, but this one I'm inclined to keep.
Co-authored-by: Carl Meyer <carl@astral.sh>
Woohoo, congrats on getting this landed!! |
Summary
This is a third attempt to add some understanding of exception-handler control flow to red-knot. It follows the previous attempts #13338 (which was very naive about
finally
blocks) and #13633 (which was overly ambitious in how it attempted to handlefinally
blocks).The discussion in #13633 concluded that we aren't yet ready to add an accurate understanding of
finally
suites to red-knot, as it will require double-visiting offinally
suites in order to accurately model the semantics, and this violates some core assumptions made by the use-def map. As such, this PR rips out a lot of the infrastructure that #13633 added for specifically dealing withfinally
suites, and instead adds some copious TODO comments inbuilder.rs
and the tests.The rest of this summary section is copied from the summary section of #13633:
The semantics of
try
/except
blocks are very complicated! I've written up a long document outlining all the various jumps control flow could take, which can be found here. I won't try to summarise that document in this PR description. But I will give a brief description of some of the ways I've attempted to model these semantics in this PR:Abstractions for handling
try
/except
blocks have been added to a newbuilder
submodule,builder/exception_handlers.rs
:TryNodeContext
keeps track of state for a singletry
/except
/else
/finally
block. Exactly what state we need to keep track of varies according to whether the node has afinally
branch, and according to which branch of theStmtTry
node we're currently visiting.TryNodeContextStack
is a stack ofTryNodeContext
instances. For any given scope,try
blocks can be arbitrarily nested; this means that we must keep a stack ofTryNodeContext
s for each scope we visit.TryNodeContextStackManager
is a stack ofTryNodeContextStack
s. Whenever we enter a nested scope, a newTryNodeContextStack
is initialised by theTryNodeContextStackManager
and appended to the stack of stacks. Whenever we exit that scope, theTryNodeContextStack
is popped off the stack of stacks.The diff for this PR is quite large, but this is mostly tests. There aren't actually that many tests, but they unfortunately need to be quite verbose. This is because we may add a more sophisticated understanding of exception handlers in the future (where we would understand that e.g.
x = 1
can never raise an exception), and I wanted the tests to be robust to this so that they wouldn't have to be rewritten when that happens. (This also helps readability of the tests, since we obviously know thatx = 1
can never raise exceptions.) To address this, I made sure to use assignments to function calls for testing places where a raised exception could cause a jump in control flow. This will be robust to future improvements, since it will always be the case that we will consider a function call capable of raising arbitrary exceptions.Test Plan
Tests have been added using
mdtest
, and can be found incrates/red_knot_python_semantic/resources/mdtest/except_handler_control_flow.md
.