Skip to content

Commit d2b51bb

Browse files
jackulaumeta-codesync[bot]
authored andcommitted
#1795 Implement PEP 765: disallow return/break/continue in finally blocks (#1798)
Summary: For #1795 This implements [PEP 765](https://peps.python.org/pep-0765/) which disallows `return`, `break`, and `continue` statements that directly exit a `finally` block. Starting in Python 3.14, these statements emit a `SyntaxWarning` because they can silently mask exceptions. ## The Issue When a `return`, `break`, or `continue` statement exits a `finally` block, any exception being propagated is silently discarded: ```python def f(): try: raise ValueError("important error") finally: return 42 # Silences the ValueError! for x in range(10): try: raise ValueError() finally: break # Silences the ValueError! ``` However, these statements are valid when inside a nested function or nested loop within the finally block: ```python def f(): try: pass finally: def inner(): return 42 # OK: exits inner function, not finally for x in range(5): break # OK: exits inner loop, not finally ``` The Fix I made several changes to detect these violations during the binding phase: - Added InvalidReturnInFinally error kind in crates/pyrefly_config/src/error_kind.rs with Warn severity (matching Python's SyntaxWarning) - Added finally_depth tracking in pyrefly/lib/binding/scope.rs: - Added finally_depth: usize field to Scope struct - This resets to 0 when entering a new function scope, so nested functions are correctly allowed - Added enter_finally(), exit_finally(), and in_finally() methods to Scopes - Added in_finally flag on loops in pyrefly/lib/binding/scope.rs: - Added in_finally: bool field to Loop struct - Records whether a loop was started inside a finally block - Added loop_protects_from_finally_exit() method to check if break/continue targets a nested loop - Added checks for return/break/continue in pyrefly/lib/binding/stmt.rs: - Check in record_return() for return statements - Check in Stmt::Break and Stmt::Continue handlers - All checks are gated to Python 3.14+ via self.sys_info.version().at_least(3, 14) - Added documentation in website/docs/error-kinds.mdx for invalid-return-in-finally Pull Request resolved: #1798 Test Plan: | Test | Description | |------------------------------------------|--------------------------------| | test_pep765_return_in_finally | return in finally → warning | | test_pep765_break_in_finally | break in finally → warning | | test_pep765_continue_in_finally | continue in finally → warning | | test_pep765_return_in_nested_function_ok | return in nested function → OK | | test_pep765_break_in_nested_loop_ok | break in nested loop → OK | | test_pep765_continue_in_nested_loop_ok | continue in nested loop → OK | | test_pep765_return_python313_no_error | no warning on Python 3.13 | Verification: Run PEP 765 tests cargo test test_pep765 Result: 7 passed Run all flow_branching tests cargo test flow_branching Result: 86 passed Run all tests cargo test Result: 3057 passed Fixes #1795 Reviewed By: grievejia Differential Revision: D88746329 Pulled By: yangdanny97 fbshipit-source-id: f4690916e33e5fda99db83b5a61fae963e6c8328
1 parent 9223880 commit d2b51bb

File tree

3 files changed

+155
-4
lines changed

3 files changed

+155
-4
lines changed

pyrefly/lib/binding/scope.rs

Lines changed: 40 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -886,13 +886,16 @@ pub enum LoopExit {
886886
struct Loop {
887887
base: Flow,
888888
exits: Vec<(LoopExit, Flow)>,
889+
/// For PEP 765: The depth of finally-blocks that this loop was created in
890+
finally_depth: usize,
889891
}
890892

891893
impl Loop {
892-
pub fn new(base: Flow) -> Self {
894+
pub fn new(base: Flow, finally_depth: usize) -> Self {
893895
Self {
894896
base,
895897
exits: Default::default(),
898+
finally_depth,
896899
}
897900
}
898901
}
@@ -956,6 +959,8 @@ pub struct Scope {
956959
imports: SmallMap<Name, ImportUsage>,
957960
/// Tracking variables in the current scope (module, function, and method scopes)
958961
variables: SmallMap<Name, VariableUsage>,
962+
/// Depth of finally blocks we're in. Resets in new function scopes (PEP 765).
963+
finally_depth: usize,
959964
}
960965

961966
impl Scope {
@@ -970,6 +975,7 @@ impl Scope {
970975
forks: Default::default(),
971976
imports: SmallMap::new(),
972977
variables: SmallMap::new(),
978+
finally_depth: 0,
973979
}
974980
}
975981

@@ -1139,6 +1145,34 @@ impl Scopes {
11391145
.any(|scope| matches!(scope.kind, ScopeKind::Function(_) | ScopeKind::Method(_)))
11401146
}
11411147

1148+
/// Enter a finally block (PEP 765).
1149+
pub fn enter_finally(&mut self) {
1150+
self.current_mut().finally_depth += 1;
1151+
}
1152+
1153+
/// Exit a finally block (PEP 765).
1154+
pub fn exit_finally(&mut self) {
1155+
self.current_mut().finally_depth -= 1;
1156+
}
1157+
1158+
/// Check if we're in a finally block at the current scope level.
1159+
/// This resets when entering a new function scope, so nested functions are OK.
1160+
pub fn in_finally(&self) -> bool {
1161+
self.current().finally_depth > 0
1162+
}
1163+
1164+
pub fn finally_depth(&self) -> usize {
1165+
self.current().finally_depth
1166+
}
1167+
1168+
/// Check if we're inside a loop that was started inside the inner-most finally block
1169+
pub fn loop_protects_from_finally_exit(&self) -> bool {
1170+
self.current()
1171+
.loops
1172+
.last()
1173+
.is_some_and(|l| l.finally_depth == self.current().finally_depth)
1174+
}
1175+
11421176
/// Are we currently in a class body. If so, return the keys for the class and its metadata.
11431177
pub fn current_class_and_metadata_keys(
11441178
&self,
@@ -2630,12 +2664,16 @@ impl<'a> BindingsBuilder<'a> {
26302664
/// Names in `loop_header_targets` will not get phi keys - this is used for loop
26312665
/// variables that are unconditionally reassigned in `for` loop headers
26322666
pub fn setup_loop(&mut self, range: TextRange, loop_header_targets: &SmallSet<Name>) {
2667+
let finally_depth = self.scopes.finally_depth();
26332668
let base = mem::take(&mut self.scopes.current_mut().flow);
26342669
// To account for possible assignments to existing names in a loop, we
26352670
// speculatively insert phi keys upfront.
26362671
self.scopes.current_mut().flow =
26372672
self.insert_phi_keys(base.clone(), range, loop_header_targets);
2638-
self.scopes.current_mut().loops.push(Loop::new(base));
2673+
self.scopes
2674+
.current_mut()
2675+
.loops
2676+
.push(Loop::new(base, finally_depth));
26392677
}
26402678

26412679
pub fn teardown_loop(

pyrefly/lib/binding/stmt.rs

Lines changed: 34 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -272,6 +272,14 @@ impl<'a> BindingsBuilder<'a> {
272272
/// If this is the top level, report a type error about the invalid return
273273
/// and also create a binding to ensure we type check the expression.
274274
fn record_return(&mut self, mut x: StmtReturn) {
275+
// PEP 765: Disallow return in finally block (Python 3.14+)
276+
if self.sys_info.version().at_least(3, 14) && self.scopes.in_finally() {
277+
self.error(
278+
x.range(),
279+
ErrorInfo::Kind(ErrorKind::InvalidSyntax),
280+
"`return` in a `finally` block will silence exceptions".to_owned(),
281+
);
282+
}
275283
let mut ret = self.declare_current_idx(Key::ReturnExplicit(x.range()));
276284
self.ensure_expr_opt(x.value.as_deref_mut(), ret.usage());
277285
if let Err((ret, oops_top_level)) = self.scopes.record_or_reject_return(ret, x) {
@@ -864,7 +872,9 @@ impl<'a> BindingsBuilder<'a> {
864872
}
865873

866874
self.finish_exhaustive_fork();
875+
self.scopes.enter_finally();
867876
self.stmts(x.finalbody, parent);
877+
self.scopes.exit_finally();
868878
}
869879
Stmt::Assert(x) => {
870880
self.assert(x.range(), *x.test, x.msg.map(|m| *m));
@@ -984,10 +994,32 @@ impl<'a> BindingsBuilder<'a> {
984994
}
985995
}
986996
Stmt::Pass(_) => { /* no-op */ }
987-
Stmt::Break(_) => {
997+
Stmt::Break(x) => {
998+
// PEP 765: Disallow break in finally block if not inside a nested loop
999+
if self.sys_info.version().at_least(3, 14)
1000+
&& self.scopes.in_finally()
1001+
&& !self.scopes.loop_protects_from_finally_exit()
1002+
{
1003+
self.error(
1004+
x.range,
1005+
ErrorInfo::Kind(ErrorKind::InvalidSyntax),
1006+
"`break` in a `finally` block will silence exceptions".to_owned(),
1007+
);
1008+
}
9881009
self.add_loop_exitpoint(LoopExit::Break);
9891010
}
990-
Stmt::Continue(_) => {
1011+
Stmt::Continue(x) => {
1012+
// PEP 765: Disallow continue in finally block if not inside a nested loop
1013+
if self.sys_info.version().at_least(3, 14)
1014+
&& self.scopes.in_finally()
1015+
&& !self.scopes.loop_protects_from_finally_exit()
1016+
{
1017+
self.error(
1018+
x.range,
1019+
ErrorInfo::Kind(ErrorKind::InvalidSyntax),
1020+
"`continue` in a `finally` block will silence exceptions".to_owned(),
1021+
);
1022+
}
9911023
self.add_loop_exitpoint(LoopExit::Continue);
9921024
}
9931025
Stmt::IpyEscapeCommand(x) => {

pyrefly/lib/test/flow_branching.rs

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@
55
* LICENSE file in the root directory of this source tree.
66
*/
77

8+
use pyrefly_python::sys_info::PythonVersion;
9+
810
use crate::test::util::TestEnv;
911
use crate::testcase;
1012

@@ -1321,3 +1323,82 @@ def test_keyword_pattern_not_special(x: float) -> None:
13211323
assert_type(r, float)
13221324
"#,
13231325
);
1326+
1327+
testcase!(
1328+
test_pep765_break_continue_return_in_finally_3_14,
1329+
TestEnv::new_with_version(PythonVersion {
1330+
major: 3,
1331+
minor: 14,
1332+
micro: 0,
1333+
}),
1334+
r#"
1335+
def test():
1336+
try:
1337+
pass
1338+
finally:
1339+
return # E: in a `finally` block
1340+
1341+
for _ in []:
1342+
try:
1343+
pass
1344+
finally:
1345+
break # E: in a `finally` block
1346+
for _ in []:
1347+
try:
1348+
pass
1349+
finally:
1350+
continue # E: in a `finally` block
1351+
1352+
try:
1353+
pass
1354+
finally:
1355+
def f():
1356+
return 42 # OK
1357+
1358+
try:
1359+
pass
1360+
finally:
1361+
def f():
1362+
try:
1363+
pass
1364+
finally:
1365+
return 42 # E: in a `finally` block
1366+
1367+
try:
1368+
pass
1369+
finally:
1370+
for _ in []:
1371+
try:
1372+
pass
1373+
finally:
1374+
break # E: in a `finally` block
1375+
"#,
1376+
);
1377+
1378+
testcase!(
1379+
test_pep765_break_continue_return_in_finally_3_13,
1380+
TestEnv::new_with_version(PythonVersion {
1381+
major: 3,
1382+
minor: 13,
1383+
micro: 0,
1384+
}),
1385+
r#"
1386+
# For now, we won't emit a PEP765 syntax error for 3.13 and below
1387+
def test():
1388+
try:
1389+
pass
1390+
finally:
1391+
return
1392+
1393+
for _ in []:
1394+
try:
1395+
pass
1396+
finally:
1397+
break
1398+
for _ in []:
1399+
try:
1400+
pass
1401+
finally:
1402+
continue
1403+
"#,
1404+
);

0 commit comments

Comments
 (0)