Skip to content

Commit

Permalink
Restore existing bindings when unbinding caught exceptions (#5256)
Browse files Browse the repository at this point in the history
## 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.
  • Loading branch information
charliermarsh committed Jun 21, 2023
1 parent d99b3bf commit ecf61d4
Show file tree
Hide file tree
Showing 13 changed files with 429 additions and 25 deletions.
20 changes: 11 additions & 9 deletions crates/ruff/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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) {
Expand All @@ -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),
}
Expand Down Expand Up @@ -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();
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff/src/renamer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
}
Expand Down
125 changes: 124 additions & 1 deletion crates/ruff/src/rules/pyflakes/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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#"
Expand All @@ -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);
Expand Down
Original file line number Diff line number Diff line change
@@ -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`.


Original file line number Diff line number Diff line change
@@ -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
|


Original file line number Diff line number Diff line change
@@ -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`.


Original file line number Diff line number Diff line change
@@ -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`.


Original file line number Diff line number Diff line change
@@ -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


0 comments on commit ecf61d4

Please sign in to comment.