diff --git a/pyrefly/lib/binding/scope.rs b/pyrefly/lib/binding/scope.rs index 0c35aed9b4..7984c9122a 100644 --- a/pyrefly/lib/binding/scope.rs +++ b/pyrefly/lib/binding/scope.rs @@ -756,7 +756,8 @@ impl FlowStyle { MergeStyle::Loop | MergeStyle::LoopDefinitelyRuns | MergeStyle::Exclusive - | MergeStyle::Inclusive => FlowStyle::PossiblyUninitialized, + | MergeStyle::Inclusive + | MergeStyle::Finally => FlowStyle::PossiblyUninitialized, } } } @@ -2961,6 +2962,10 @@ enum MergeStyle { /// Distinct from [Branching] because we have to be more lax about /// uninitialized locals (see `FlowStyle::merge` for details). BoolOp, + /// This is the merge immediately before analyzing a `finally` block. + /// Terminating branches still contribute because `finally` executes before + /// their control-flow effect is observed outside the statement. + Finally, } impl MergeStyle { @@ -3313,17 +3318,20 @@ impl<'a> BindingsBuilder<'a> { return; } + let use_all_branches = matches!(merge_style, MergeStyle::Finally); // We normally only merge the live branches (where control flow is not // known to terminate), but if nothing is live we still need to fill in // the Phi keys and potentially analyze downstream code, so in that case // we'll use the terminated branches. - let (terminated_branches, live_branches): (Vec<_>, Vec<_>) = - branches.into_iter().partition(|flow| flow.has_terminated); - let has_terminated = live_branches.is_empty() && !merge_style.is_loop(); - let flows = if has_terminated { - terminated_branches + let n_live_branches = branches.iter().filter(|flow| !flow.has_terminated).count(); + let has_terminated = n_live_branches == 0 && !merge_style.is_loop(); + let flows = if use_all_branches || has_terminated { + branches } else { - live_branches + branches + .into_iter() + .filter(|flow| !flow.has_terminated) + .collect() }; // Determine reachability of the merged flow. // For Loop style with empty flows (all branches terminated), the loop body might @@ -3335,6 +3343,11 @@ impl<'a> BindingsBuilder<'a> { MergeStyle::Loop => base.is_definitely_unreachable, _ => true, } + } else if use_all_branches && n_live_branches > 0 { + flows + .iter() + .filter(|f| !f.has_terminated) + .all(|f| f.is_definitely_unreachable) } else { flows.iter().all(|f| f.is_definitely_unreachable) }; @@ -3639,6 +3652,18 @@ impl<'a> BindingsBuilder<'a> { self.finish_fork_impl(None, false, None) } + /// Finish an exhaustive fork for a `finally` block. Unlike a normal merge, + /// branches that have already terminated still contribute to the merged type + /// state because they will execute the `finally` body first. + pub fn finish_finally_fork(&mut self) { + let fork = self.scopes.current_mut().forks.pop().unwrap(); + assert!( + !fork.branch_started, + "A branch is started - did you forget to call `finish_branch`?" + ); + self.merge_flow(fork.base, fork.branches, fork.range, MergeStyle::Finally); + } + /// Finish a non-exhaustive fork in which the base flow is part of the merge. It negates /// the branch-choosing narrows by applying `negated_prev_ops` to base before merging, which /// is important so that we can preserve any cases where a termanating branch has permanently diff --git a/pyrefly/lib/binding/stmt.rs b/pyrefly/lib/binding/stmt.rs index 8177c2785f..73b49e0a8d 100644 --- a/pyrefly/lib/binding/stmt.rs +++ b/pyrefly/lib/binding/stmt.rs @@ -1190,11 +1190,10 @@ impl<'a> BindingsBuilder<'a> { // https://docs.python.org/3/reference/compound_stmts.html#except-clause self.scopes.mark_as_deleted(&name.id); } - self.finish_branch(); } - self.finish_exhaustive_fork(); + self.finish_finally_fork(); self.scopes.enter_finally(); self.stmts(x.finalbody, parent); self.scopes.exit_finally(); diff --git a/pyrefly/lib/test/flow_branching.rs b/pyrefly/lib/test/flow_branching.rs index 512a2c9df2..45357250c2 100644 --- a/pyrefly/lib/test/flow_branching.rs +++ b/pyrefly/lib/test/flow_branching.rs @@ -2085,6 +2085,33 @@ def main(resolve: bool) -> None: "#, ); +// Issue #2845: `finally` must see both the successful `try` state and terminating `except` state. +testcase!( + test_try_finally_preserves_pre_try_possibility_from_terminating_except, + r#" +from typing import assert_type + +def something_that_might_throw() -> None: + raise Exception() + +class Thing: + def something(self) -> None: + pass + +def blah() -> None: + x = None + try: + something_that_might_throw() + if x is None: + x = Thing() + except Exception: + raise + finally: + assert_type(x, Thing | None) + x.something() # E: Object of class `NoneType` has no attribute `something` +"#, +); + // for https://github.com/facebook/pyrefly/issues/1840 testcase!( test_exhaustive_flow_no_fall_through, diff --git a/pyrefly/lib/test/lsp/lsp_interaction/pytorch_benchmark.rs b/pyrefly/lib/test/lsp/lsp_interaction/pytorch_benchmark.rs index cf56f84412..62429ffd53 100644 --- a/pyrefly/lib/test/lsp/lsp_interaction/pytorch_benchmark.rs +++ b/pyrefly/lib/test/lsp/lsp_interaction/pytorch_benchmark.rs @@ -48,7 +48,7 @@ fn test_pytorch_error_propagation_latency() { }; // Use all available cores for realistic benchmarking let mut interaction = - LspInteraction::new_with_args(args, NoTelemetry, Some(ThreadCount::AllThreads)); + LspInteraction::new_with_args(args, NoTelemetry, Some(ThreadCount::AllThreads), None); interaction.set_root(pytorch_root.clone()); interaction