language-servers/ruff
1
0
Fork 0
mirror of https://github.com/astral-sh/ruff.git synced 2025-11-02 04:48:07 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 2

Restore existing bindings when unbinding caught exceptions (#5256)

Browse source 
## 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.
This commit is contained in:
Charlie Marsh 2023-06-21 12:53:58 -04:00 • committed by GitHub
parent d99b3bf661
commit ecf61d49fa
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
13 changed files with 429 additions and 25 deletions
Download patch file Download diff file Expand all files Collapse all files

20
crates/ruff/src/checkers/ast/mod.rs
View file

@ -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();

2
crates/ruff/src/renamer.rs
View file

@ -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))
}
}

125
crates/ruff/src/rules/pyflakes/mod.rs
View file

@ -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);

44
crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__load_after_multiple_unbinds_from_module_scope.snap Normal file
View file

@ -0,0 +1,44 @@
---
source: crates/ruff/src/rules/pyflakes/mod.rs
---
<filename>: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:
<filename>: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`.

32
crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__load_after_unbind_from_class_scope.snap Normal file
View file

@ -0,0 +1,32 @@
---
source: crates/ruff/src/rules/pyflakes/mod.rs
---
<filename>: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
<filename>: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
|

24
crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__load_after_unbind_from_module_scope.snap Normal file
View file

@ -0,0 +1,24 @@
---
source: crates/ruff/src/rules/pyflakes/mod.rs
---
<filename>: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`.

44
crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__load_after_unbind_from_nested_module_scope.snap Normal file
View file

@ -0,0 +1,44 @@
---
source: crates/ruff/src/rules/pyflakes/mod.rs
---
<filename>: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():
<filename>: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`.

45
crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__print_in_body_after_double_shadowing_except.snap Normal file
View file

@ -0,0 +1,45 @@
---
source: crates/ruff/src/rules/pyflakes/mod.rs
---
<filename>: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
<filename>: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

10
crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__print_after_shadowing_except.snap → crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__print_in_body_after_shadowing_except.snap
View file

@ -19,14 +19,6 @@ source: crates/ruff/src/rules/pyflakes/mod.rs
7 |+ except Exception:
8 8 | pass
9 9 |
10 10 | print(x)
<filename>: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

4
crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__print_in_if_else_after_shadowing_except.snap Normal file
View file

@ -0,0 +1,4 @@
---
source: crates/ruff/src/rules/pyflakes/mod.rs
---

4
crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__print_in_try_else_after_shadowing_except.snap Normal file
View file

@ -0,0 +1,4 @@
---
source: crates/ruff/src/rules/pyflakes/mod.rs
---

8
crates/ruff_python_semantic/src/binding.rs
View file

@ -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<BindingId>),
}
bitflags! {

92
crates/ruff_python_semantic/src/model.rs
View file

@ -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<BindingId> {
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,