Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Restore existing bindings when unbinding caught exceptions #5256

Merged
merged 2 commits into from
Jun 21, 2023
Merged

Restore existing bindings when unbinding caught exceptions #5256

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b36e441
Restore existing bindings when unbinding caught exceptions
charliermarsh Jun 21, 2023
62b8ddb
No clone
charliermarsh Jun 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 11 additions & 9 deletions crates/ruff/src/checkers/ast/mod.rs
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
44 changes: 44 additions & 0 deletions ...napshots/ruff__rules__pyflakes__tests__load_after_multiple_unbinds_from_module_scope.snap
View file
Open in desktop
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`.


32 changes: 32 additions & 0 deletions .../pyflakes/snapshots/ruff__rules__pyflakes__tests__load_after_unbind_from_class_scope.snap
View file
Open in desktop
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
|


24 changes: 24 additions & 0 deletions ...pyflakes/snapshots/ruff__rules__pyflakes__tests__load_after_unbind_from_module_scope.snap
View file
Open in desktop
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`.


44 changes: 44 additions & 0 deletions ...s/snapshots/ruff__rules__pyflakes__tests__load_after_unbind_from_nested_module_scope.snap
View file
Open in desktop
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`.


45 changes: 45 additions & 0 deletions .../snapshots/ruff__rules__pyflakes__tests__print_in_body_after_double_shadowing_except.snap
View file
Open in desktop
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