In principle I think these tests would be better if they asserted the inferred type of the name inside the except block, rather than at end of scope.

But that's quite a major pain to do in our current test framework, so I would just leave the tests as-is for now and wait for the new test framework with reveal_type :)

I think a test that is missing here is something verifying that within the else block, names bound in the try block are definitely bound. (This is along the same lines as the above comment; we're generally weak on tests demonstrating inferred types inside a particular block, rather than at end of scope.)

But as mentioned above, this is difficult to do with current test infra. (You can assign the name to another name, but then that other name is still potentially unbound, so the test isn't super clear.) But it would be nice to have a TODO to add this test, once we have the new test framework.

This comment looks a bit outdated relative to the implementation?

It could be relevant to perf. Snapshotting and restoring flow states is sufficiently expensive (if there are lots of symbols) that it might be worth explicitly checking for empty else and finally, if that would allow us to skip any snapshots or restores/merges. And it looks like we could skip a fair amount of those, if there is no finally body and no parent try block?

Do we also need to merge the pre-try-block state here? (We could jump straight from before completing any statement in the try block, to executing the finally block and/or into an outer try block.)

I find it slightly confusing that within the same try block visit, above we refer to an outer try block as parent_try_block and here we refer to the same thing as current_try_block. Let's be consistent about what is "current" and what is "parent"?

I think x can be Unbound here; it's never set in an exhaustive except, or in a finally.

I think this might be the issue I pointed out above, where we don't include the pre-try-block state as a pre-finally possibility.

I don't see how x could actually be unbound in this scenario, but I'm also not sure if it's worth fixing. We assume that we could exit the outer try with an exception before executing the inner try at all, but that's not actually true in this example; nothing happens in the outer try before we enter the inner try, so in actual fact we are guaranteed to either complete the assignment x = 1 in the inner try, or have an exception caught by the exhaustive inner handler, which also binds x.

I think we would handle this correctly if, instead of saving a pre-try state, and then pushing a state post each definition, we instead pushed a state pre each definition, and then saved a post-try state.

I think this comment is wrong; given that we don't currently try to distinguish non-raising statements, we have to assume that x = 2 in the inner exhaustive except could also raise, so it is correct for x to be possibly unbound.

"otherwise-exhausive except/else branches in the inner block" doesn't seem right -- those aren't exhaustive

How is this assertion different from if the outer try/except weren't there?

How is this assertion different from if the outer try/except weren't there?

I recommend moving this comment next to the relevant errors. It increases the likelihood that the comment gets deleted when the errors are resolved (I would miss the comment up there)

The method name here seems misleading to me because it is called after we visit the try body. It seems it's about visiting the except handlers, finaly , and or else blocks of a try.

Related: What's the reason that we don't record definitions in a nested try? E.g. what if

try:
    try:
        x =  10:
        maybe_panic()
    finally:
        y = panic()
finally:
    y

shouldn't the use of y be an error?

but

try:
    try:
        x =  10:
        maybe_panic()
    finally:
        y = 10
finally:
    y

should be okay?

I commented elsewhere that it seems we currently don't have any tests that fail if we stop paying attention to visiting_nested_try_stmt entirely.

And it seems to me that Micha is right here -- outside the inner try block body itself, it does seem like intermediate states in the except/else/finally should all get registered with the outer try block, because we could have an exception happen in any of those places. (And such an exception would not cause us to still enter an inner finally block, which only applies to the inner try body; it would jump straight to being handled by the outer try block.) So it does seem to me like visiting_nested_try_stmt might be inherently semantically not right?

(Cases to consider for tests here are e.g. x = 1; x = 2 inside an except, else, or finally block in a nested try statement -- the x = 1 possibility should be visible to the outer try block.

(And such an exception would not cause us to still enter an inner finally block, which only applies to the inner try body; it would jump straight to being handled by the outer try block.)

What do you mean? The inner finally block is indeed always executed, even if the exception is caught by the outer try block:

>>> try:
...     try:
...         raise TypeError()
...     except NameError:
...         print(1)
...     finally:
...         print(2)
... except TypeError:
...     print(3)
... finally:
...     print(4)
... 
2
3
4

What I mean is that if an exception is raised inside an except/else/finally block in the nested try (not inside the try body), in that case the inner finally is not executed, the exception unwinds directly to the outer try statement handling.

... except this is just wrong 😆 apparently finally does still apply, even if the except is raised in an except handler or else block! So never mind.

I guess the one thing to consider still is the case where an exception is raised within the finally block itself? I.e. is it important that we register those intermediate states within the nested finally block with the outer try statement?

I still can't find any cases where the inner finally is not executed, though it is of course true that if an exception is raised in an except handler then we might switch to the inner finally block before the except handler has finished, and indeed if an exception is raised in the inner finally block then that finally block might not run to completion. I might not handle that properly right now...

>>> try:
...     try:
...         raise TypeError
...     except TypeError:
...         raise RuntimeError
...         print(1)
...     finally:
...         print(2)
... except RuntimeError:
...     print(3)
... finally:
...     print(4)
... 
2
3
4

Can't we call self.try_block_contexts_mut() here to avoid the RefCell in the contexts (and all the borrow_mut stuff)?

If I remove this check in the current version of this PR, all tests pass. So either we don't need this, or we need more tests.

A test fails if you make this change:

diff --git a/crates/red_knot_python_semantic/src/semantic_index/builder.rs b/crates/red_knot_python_semantic/src/semantic_index/builder.rs
index c7e8a6428..74cbd6b2d 100644
--- a/crates/red_knot_python_semantic/src/semantic_index/builder.rs
+++ b/crates/red_knot_python_semantic/src/semantic_index/builder.rs
@@ -873,16 +873,6 @@ where
                 }
 
                 self.visit_body(finalbody);
-
-                // Account for the fact that we could be visiting a nested `try` block,
-                // in which case the outer `try` block must be kept informed about all the possible
-                // between-definition states we could have encountered in the inner `try` block(!)
-                if let Some(current_try_block) =
-                    self.try_block_contexts().borrow_mut().current_try_block()
-                {
-                    current_try_block.record_exiting_nested_try_stmt();
-                    current_try_block.record_definition(self);
-                }
             }
             _ => {
                 walk_stmt(self, stmt);
diff --git a/crates/red_knot_python_semantic/src/semantic_index/except_handlers.rs b/crates/red_knot_python_semantic/src/semantic_index/except_handlers.rs
index 2eded0089..ce4e76ca0 100644
--- a/crates/red_knot_python_semantic/src/semantic_index/except_handlers.rs
+++ b/crates/red_knot_python_semantic/src/semantic_index/except_handlers.rs
@@ -30,9 +30,7 @@ pub(super) struct TryBlockContext {
 
 impl TryBlockContext {
     pub(super) fn record_definition(&mut self, builder: &SemanticIndexBuilder) {
-        if !self.visiting_nested_try_stmt {
-            self.snapshots.push(builder.flow_snapshot());
-        }
+        self.snapshots.push(builder.flow_snapshot());
     }

The if !self.visiting_nested_try_stmt check isn't necessary for correctness. But it's unnecessary to push all those snapshots to be recorded by the outer TryBlockContexts, since it's more efficient -- and more correct -- to simply record the state once at the completion of the inner try/except/else/finally block. I.e., the if !self.visiting_nested_try_stmt check is simply an optimisation to avoid unnecessary snapshotting.

I see that at the moment this is quite unclear; I'll add some more comments.