diff --git a/pyrefly/lib/binding/scope.rs b/pyrefly/lib/binding/scope.rs index 82ee4da9ce..19ef8b6e2e 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, } } } @@ -2945,6 +2946,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 { @@ -3297,17 +3302,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 @@ -3319,6 +3327,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) }; @@ -3623,6 +3636,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 93f7449a63..01775d4733 100644 --- a/pyrefly/lib/binding/stmt.rs +++ b/pyrefly/lib/binding/stmt.rs @@ -1184,11 +1184,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 14d8cd1226..2861e54839 100644 --- a/pyrefly/lib/test/flow_branching.rs +++ b/pyrefly/lib/test/flow_branching.rs @@ -2036,6 +2036,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,