From ecf61d49fa3fdfacb542c300d740c059b2cd8c00 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 21 Jun 2023 12:53:58 -0400 Subject: [PATCH] Restore existing bindings when unbinding caught exceptions (#5256) ## Summary In the latest release, we made some improvements to the semantic model, but our modifications to exception-unbinding are causing some false-positives. For example: ```py try: v = 3 except ImportError as v: print(v) else: print(v) ``` In the latest release, we started unbinding `v` after the `except` handler. (We used to restore the existing binding, the `v = 3`, but this was quite complicated.) Because we don't have full branch analysis, we can't then know that `v` is still bound in the `else` branch. The solution here modifies `resolve_read` to skip-lookup when hitting unbound exceptions. So when store the "unbind" for `except ImportError as v`, we save the binding that it shadowed `v = 3`, and skip to that. Closes #5249. Closes #5250. --- crates/ruff/src/checkers/ast/mod.rs | 20 +-- crates/ruff/src/renamer.rs | 2 +- crates/ruff/src/rules/pyflakes/mod.rs | 125 +++++++++++++++++- ...er_multiple_unbinds_from_module_scope.snap | 44 ++++++ ...s__load_after_unbind_from_class_scope.snap | 32 +++++ ...__load_after_unbind_from_module_scope.snap | 24 ++++ ...after_unbind_from_nested_module_scope.snap | 44 ++++++ ...in_body_after_double_shadowing_except.snap | 45 +++++++ ...print_in_body_after_shadowing_except.snap} | 10 +- ...int_in_if_else_after_shadowing_except.snap | 4 + ...nt_in_try_else_after_shadowing_except.snap | 4 + crates/ruff_python_semantic/src/binding.rs | 8 +- crates/ruff_python_semantic/src/model.rs | 92 ++++++++++++- 13 files changed, 429 insertions(+), 25 deletions(-) create mode 100644 crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__load_after_multiple_unbinds_from_module_scope.snap create mode 100644 crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__load_after_unbind_from_class_scope.snap create mode 100644 crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__load_after_unbind_from_module_scope.snap create mode 100644 crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__load_after_unbind_from_nested_module_scope.snap create mode 100644 crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__print_in_body_after_double_shadowing_except.snap rename crates/ruff/src/rules/pyflakes/snapshots/{ruff__rules__pyflakes__tests__print_after_shadowing_except.snap => ruff__rules__pyflakes__tests__print_in_body_after_shadowing_except.snap} (76%) create mode 100644 crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__print_in_if_else_after_shadowing_except.snap create mode 100644 crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__print_in_try_else_after_shadowing_except.snap diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index 33e59c3841..ecd0b18a7a 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -3852,6 +3852,9 @@ where ); } + // Store the existing binding, if any. + let existing_id = self.semantic.lookup(name); + // Add the bound exception name to the scope. let binding_id = self.add_binding( name, @@ -3862,14 +3865,6 @@ where walk_except_handler(self, except_handler); - // Remove it from the scope immediately after. - self.add_binding( - name, - range, - BindingKind::UnboundException, - BindingFlags::empty(), - ); - // If the exception name wasn't used in the scope, emit a diagnostic. if !self.semantic.is_used(binding_id) { if self.enabled(Rule::UnusedVariable) { @@ -3889,6 +3884,13 @@ where self.diagnostics.push(diagnostic); } } + + self.add_binding( + name, + range, + BindingKind::UnboundException(existing_id), + BindingFlags::empty(), + ); } None => walk_except_handler(self, except_handler), } @@ -4236,7 +4238,7 @@ impl<'a> Checker<'a> { let shadowed = &self.semantic.bindings[shadowed_id]; if !matches!( shadowed.kind, - BindingKind::Builtin | BindingKind::Deletion | BindingKind::UnboundException, + BindingKind::Builtin | BindingKind::Deletion | BindingKind::UnboundException(_), ) { let references = shadowed.references.clone(); let is_global = shadowed.is_global(); diff --git a/crates/ruff/src/renamer.rs b/crates/ruff/src/renamer.rs index fe31104198..a7beaafc5e 100644 --- a/crates/ruff/src/renamer.rs +++ b/crates/ruff/src/renamer.rs @@ -251,7 +251,7 @@ impl Renamer { | BindingKind::ClassDefinition | BindingKind::FunctionDefinition | BindingKind::Deletion - | BindingKind::UnboundException => { + | BindingKind::UnboundException(_) => { Some(Edit::range_replacement(target.to_string(), binding.range)) } } diff --git a/crates/ruff/src/rules/pyflakes/mod.rs b/crates/ruff/src/rules/pyflakes/mod.rs index 87876e96d6..243be429ce 100644 --- a/crates/ruff/src/rules/pyflakes/mod.rs +++ b/crates/ruff/src/rules/pyflakes/mod.rs @@ -353,9 +353,59 @@ mod tests { except Exception as x: pass + # No error here, though it should arguably be an F821 error. `x` will + # be unbound after the `except` block (assuming an exception is raised + # and caught). print(x) "#, - "print_after_shadowing_except" + "print_in_body_after_shadowing_except" + )] + #[test_case( + r#" + def f(): + x = 1 + + try: + 1 / 0 + except ValueError as x: + pass + except ImportError as x: + pass + + # No error here, though it should arguably be an F821 error. `x` will + # be unbound after the `except` block (assuming an exception is raised + # and caught). + print(x) + "#, + "print_in_body_after_double_shadowing_except" + )] + #[test_case( + r#" + def f(): + try: + x = 3 + except ImportError as x: + print(x) + else: + print(x) + "#, + "print_in_try_else_after_shadowing_except" + )] + #[test_case( + r#" + def f(): + list = [1, 2, 3] + + for e in list: + if e % 2 == 0: + try: + pass + except Exception as e: + print(e) + else: + print(e) + "#, + "print_in_if_else_after_shadowing_except" )] #[test_case( r#" @@ -366,6 +416,79 @@ mod tests { "#, "double_del" )] + #[test_case( + r#" + x = 1 + + def f(): + try: + pass + except ValueError as x: + pass + + # This should resolve to the `x` in `x = 1`. + print(x) + "#, + "load_after_unbind_from_module_scope" + )] + #[test_case( + r#" + x = 1 + + def f(): + try: + pass + except ValueError as x: + pass + + try: + pass + except ValueError as x: + pass + + # This should resolve to the `x` in `x = 1`. + print(x) + "#, + "load_after_multiple_unbinds_from_module_scope" + )] + #[test_case( + r#" + x = 1 + + def f(): + try: + pass + except ValueError as x: + pass + + def g(): + try: + pass + except ValueError as x: + pass + + # This should resolve to the `x` in `x = 1`. + print(x) + "#, + "load_after_unbind_from_nested_module_scope" + )] + #[test_case( + r#" + class C: + x = 1 + + def f(): + try: + pass + except ValueError as x: + pass + + # This should raise an F821 error, rather than resolving to the + # `x` in `x = 1`. + print(x) + "#, + "load_after_unbind_from_class_scope" + )] fn contents(contents: &str, snapshot: &str) { let diagnostics = test_snippet(contents, &Settings::for_rules(&Linter::Pyflakes)); assert_messages!(snapshot, diagnostics); diff --git a/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__load_after_multiple_unbinds_from_module_scope.snap b/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__load_after_multiple_unbinds_from_module_scope.snap new file mode 100644 index 0000000000..32d31a0137 --- /dev/null +++ b/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__load_after_multiple_unbinds_from_module_scope.snap @@ -0,0 +1,44 @@ +--- +source: crates/ruff/src/rules/pyflakes/mod.rs +--- +:7:26: F841 [*] Local variable `x` is assigned to but never used + | +5 | try: +6 | pass +7 | except ValueError as x: + | ^ F841 +8 | pass + | + = help: Remove assignment to unused variable `x` + +ℹ Fix +4 4 | def f(): +5 5 | try: +6 6 | pass +7 |- except ValueError as x: + 7 |+ except ValueError: +8 8 | pass +9 9 | +10 10 | try: + +:12:26: F841 [*] Local variable `x` is assigned to but never used + | +10 | try: +11 | pass +12 | except ValueError as x: + | ^ F841 +13 | pass + | + = help: Remove assignment to unused variable `x` + +ℹ Fix +9 9 | +10 10 | try: +11 11 | pass +12 |- except ValueError as x: + 12 |+ except ValueError: +13 13 | pass +14 14 | +15 15 | # This should resolve to the `x` in `x = 1`. + + diff --git a/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__load_after_unbind_from_class_scope.snap b/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__load_after_unbind_from_class_scope.snap new file mode 100644 index 0000000000..058a92ae6f --- /dev/null +++ b/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__load_after_unbind_from_class_scope.snap @@ -0,0 +1,32 @@ +--- +source: crates/ruff/src/rules/pyflakes/mod.rs +--- +:8:30: F841 [*] Local variable `x` is assigned to but never used + | +6 | try: +7 | pass +8 | except ValueError as x: + | ^ F841 +9 | pass + | + = help: Remove assignment to unused variable `x` + +ℹ Fix +5 5 | def f(): +6 6 | try: +7 7 | pass +8 |- except ValueError as x: + 8 |+ except ValueError: +9 9 | pass +10 10 | +11 11 | # This should raise an F821 error, rather than resolving to the + +:13:15: F821 Undefined name `x` + | +11 | # This should raise an F821 error, rather than resolving to the +12 | # `x` in `x = 1`. +13 | print(x) + | ^ F821 + | + + diff --git a/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__load_after_unbind_from_module_scope.snap b/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__load_after_unbind_from_module_scope.snap new file mode 100644 index 0000000000..2f25f19aeb --- /dev/null +++ b/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__load_after_unbind_from_module_scope.snap @@ -0,0 +1,24 @@ +--- +source: crates/ruff/src/rules/pyflakes/mod.rs +--- +:7:26: F841 [*] Local variable `x` is assigned to but never used + | +5 | try: +6 | pass +7 | except ValueError as x: + | ^ F841 +8 | pass + | + = help: Remove assignment to unused variable `x` + +ℹ Fix +4 4 | def f(): +5 5 | try: +6 6 | pass +7 |- except ValueError as x: + 7 |+ except ValueError: +8 8 | pass +9 9 | +10 10 | # This should resolve to the `x` in `x = 1`. + + diff --git a/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__load_after_unbind_from_nested_module_scope.snap b/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__load_after_unbind_from_nested_module_scope.snap new file mode 100644 index 0000000000..7ab30bf910 --- /dev/null +++ b/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__load_after_unbind_from_nested_module_scope.snap @@ -0,0 +1,44 @@ +--- +source: crates/ruff/src/rules/pyflakes/mod.rs +--- +:7:26: F841 [*] Local variable `x` is assigned to but never used + | +5 | try: +6 | pass +7 | except ValueError as x: + | ^ F841 +8 | pass + | + = help: Remove assignment to unused variable `x` + +ℹ Fix +4 4 | def f(): +5 5 | try: +6 6 | pass +7 |- except ValueError as x: + 7 |+ except ValueError: +8 8 | pass +9 9 | +10 10 | def g(): + +:13:30: F841 [*] Local variable `x` is assigned to but never used + | +11 | try: +12 | pass +13 | except ValueError as x: + | ^ F841 +14 | pass + | + = help: Remove assignment to unused variable `x` + +ℹ Fix +10 10 | def g(): +11 11 | try: +12 12 | pass +13 |- except ValueError as x: + 13 |+ except ValueError: +14 14 | pass +15 15 | +16 16 | # This should resolve to the `x` in `x = 1`. + + diff --git a/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__print_in_body_after_double_shadowing_except.snap b/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__print_in_body_after_double_shadowing_except.snap new file mode 100644 index 0000000000..616577852c --- /dev/null +++ b/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__print_in_body_after_double_shadowing_except.snap @@ -0,0 +1,45 @@ +--- +source: crates/ruff/src/rules/pyflakes/mod.rs +--- +:7:26: F841 [*] Local variable `x` is assigned to but never used + | +5 | try: +6 | 1 / 0 +7 | except ValueError as x: + | ^ F841 +8 | pass +9 | except ImportError as x: + | + = help: Remove assignment to unused variable `x` + +ℹ Fix +4 4 | +5 5 | try: +6 6 | 1 / 0 +7 |- except ValueError as x: + 7 |+ except ValueError: +8 8 | pass +9 9 | except ImportError as x: +10 10 | pass + +:9:27: F841 [*] Local variable `x` is assigned to but never used + | + 7 | except ValueError as x: + 8 | pass + 9 | except ImportError as x: + | ^ F841 +10 | pass + | + = help: Remove assignment to unused variable `x` + +ℹ Fix +6 6 | 1 / 0 +7 7 | except ValueError as x: +8 8 | pass +9 |- except ImportError as x: + 9 |+ except ImportError: +10 10 | pass +11 11 | +12 12 | # No error here, though it should arguably be an F821 error. `x` will + + diff --git a/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__print_after_shadowing_except.snap b/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__print_in_body_after_shadowing_except.snap similarity index 76% rename from crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__print_after_shadowing_except.snap rename to crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__print_in_body_after_shadowing_except.snap index 1bc5062f45..085cb7c453 100644 --- a/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__print_after_shadowing_except.snap +++ b/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__print_in_body_after_shadowing_except.snap @@ -19,14 +19,6 @@ source: crates/ruff/src/rules/pyflakes/mod.rs 7 |+ except Exception: 8 8 | pass 9 9 | -10 10 | print(x) - -:10:11: F821 Undefined name `x` - | - 8 | pass - 9 | -10 | print(x) - | ^ F821 - | +10 10 | # No error here, though it should arguably be an F821 error. `x` will diff --git a/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__print_in_if_else_after_shadowing_except.snap b/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__print_in_if_else_after_shadowing_except.snap new file mode 100644 index 0000000000..1976c4331d --- /dev/null +++ b/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__print_in_if_else_after_shadowing_except.snap @@ -0,0 +1,4 @@ +--- +source: crates/ruff/src/rules/pyflakes/mod.rs +--- + diff --git a/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__print_in_try_else_after_shadowing_except.snap b/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__print_in_try_else_after_shadowing_except.snap new file mode 100644 index 0000000000..1976c4331d --- /dev/null +++ b/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__print_in_try_else_after_shadowing_except.snap @@ -0,0 +1,4 @@ +--- +source: crates/ruff/src/rules/pyflakes/mod.rs +--- + diff --git a/crates/ruff_python_semantic/src/binding.rs b/crates/ruff_python_semantic/src/binding.rs index 833ad4effe..00d138494f 100644 --- a/crates/ruff_python_semantic/src/binding.rs +++ b/crates/ruff_python_semantic/src/binding.rs @@ -75,7 +75,7 @@ impl<'a> Binding<'a> { pub const fn is_unbound(&self) -> bool { matches!( self.kind, - BindingKind::Annotation | BindingKind::Deletion | BindingKind::UnboundException + BindingKind::Annotation | BindingKind::Deletion | BindingKind::UnboundException(_) ) } @@ -427,7 +427,11 @@ pub enum BindingKind<'a> { /// /// After the `except` block, `x` is unbound, despite the lack /// of an explicit `del` statement. - UnboundException, + /// + /// + /// Stores the ID of the binding that was shadowed in the enclosing + /// scope, if any. + UnboundException(Option), } bitflags! { diff --git a/crates/ruff_python_semantic/src/model.rs b/crates/ruff_python_semantic/src/model.rs index 3ea95b2dee..d39d36def7 100644 --- a/crates/ruff_python_semantic/src/model.rs +++ b/crates/ruff_python_semantic/src/model.rs @@ -327,14 +327,56 @@ impl<'a> SemanticModel<'a> { // ``` // // The `x` in `print(x)` should be treated as unresolved. - BindingKind::Deletion | BindingKind::UnboundException => { + // + // Similarly, given: + // + // ```python + // try: + // pass + // except ValueError as x: + // pass + // + // print(x) + // + // The `x` in `print(x)` should be treated as unresolved. + BindingKind::Deletion | BindingKind::UnboundException(None) => { return ResolvedRead::UnboundLocal(binding_id) } - // Otherwise, treat it as resolved. - _ => { + // If we hit an unbound exception that shadowed a bound name, resole to the + // bound name. For example, given: + // + // ```python + // x = 1 + // + // try: + // pass + // except ValueError as x: + // pass + // + // print(x) + // ``` + // + // The `x` in `print(x)` should resolve to the `x` in `x = 1`. + BindingKind::UnboundException(Some(binding_id)) => { + // Mark the binding as used. + let context = self.execution_context(); + let reference_id = self.references.push(self.scope_id, range, context); + self.bindings[binding_id].references.push(reference_id); + + // Mark any submodule aliases as used. + if let Some(binding_id) = + self.resolve_submodule(symbol, scope_id, binding_id) + { + let reference_id = self.references.push(self.scope_id, range, context); + self.bindings[binding_id].references.push(reference_id); + } + return ResolvedRead::Resolved(binding_id); } + + // Otherwise, treat it as resolved. + _ => return ResolvedRead::Resolved(binding_id), } } @@ -370,6 +412,50 @@ impl<'a> SemanticModel<'a> { } } + /// Lookup a symbol in the current scope. This is a carbon copy of [`Self::resolve_read`], but + /// doesn't add any read references to the resolved symbol. + pub fn lookup(&mut self, symbol: &str) -> Option { + if self.in_forward_reference() { + if let Some(binding_id) = self.scopes.global().get(symbol) { + if !self.bindings[binding_id].is_unbound() { + return Some(binding_id); + } + } + } + + let mut seen_function = false; + for (index, scope_id) in self.scopes.ancestor_ids(self.scope_id).enumerate() { + let scope = &self.scopes[scope_id]; + if scope.kind.is_class() { + if seen_function && matches!(symbol, "__class__") { + return None; + } + if index > 0 { + continue; + } + } + + if let Some(binding_id) = scope.get(symbol) { + match self.bindings[binding_id].kind { + BindingKind::Annotation => continue, + BindingKind::Deletion | BindingKind::UnboundException(None) => return None, + BindingKind::UnboundException(Some(binding_id)) => return Some(binding_id), + _ => return Some(binding_id), + } + } + + if index == 0 && scope.kind.is_class() { + if matches!(symbol, "__module__" | "__qualname__") { + return None; + } + } + + seen_function |= scope.kind.is_any_function(); + } + + None + } + /// Given a `BindingId`, return the `BindingId` of the submodule import that it aliases. fn resolve_submodule( &self,