Automatically remove unused variables (#1683)

Closes #1460.

README.md

@@ -569,7 +569,7 @@ For more, see [Pyflakes](https://pypi.org/project/pyflakes/2.5.0/) on PyPI.
 | F821 | UndefinedName | Undefined name `...` |  |
 | F822 | UndefinedExport | Undefined name `...` in `__all__` |  |
 | F823 | UndefinedLocal | Local variable `...` referenced before assignment |  |
-| F841 | UnusedVariable | Local variable `...` is assigned to but never used |  |
+| F841 | UnusedVariable | Local variable `...` is assigned to but never used | 🛠 |
 | F842 | UnusedAnnotation | Local variable `...` is annotated but never used |  |
 | F901 | RaiseNotImplemented | `raise NotImplemented` should be `raise NotImplementedError` | 🛠 |
 

resources/test/fixtures/pyflakes/F841_3.py

@@ -0,0 +1,46 @@
+"""Test case for autofixing F841 violations."""
+
+
+def f():
+    x = 1
+    y = 2
+
+    z = 3
+    print(z)
+
+
+def f():
+    x: int = 1
+    y: int = 2
+
+    z: int = 3
+    print(z)
+
+
+def f():
+    with foo() as x1:
+        pass
+
+    with foo() as (x2, y2):
+        pass
+
+    with (foo() as x3, foo() as y3, foo() as z3):
+        pass
+
+
+def f():
+    (x1, y1) = (1, 2)
+    (x2, y2) = coords2 = (1, 2)
+    coords3 = (x3, y3) = (1, 2)
+
+
+def f():
+    try:
+        1 / 0
+    except ValueError as x1:
+        pass
+
+    try:
+        1 / 0
+    except (ValueError, ZeroDivisionError) as x2:
+        pass

src/ast/operations.rs

@@ -212,7 +212,7 @@ pub fn is_unpacking_assignment(parent: &Stmt, child: &Expr) -> bool {
                 ExprKind::Set { .. } | ExprKind::List { .. } | ExprKind::Tuple { .. }
             );
             // In `(a, b) = coords = (1, 2)`, `(a, b)` and `coords` are the targets, and
-            // `(a, b`) is a tuple. (We use "tuple" as a placeholder for any
+            // `(a, b)` is a tuple. (We use "tuple" as a placeholder for any
             // unpackable expression.)
             let targets_are_tuples = targets.iter().all(|item| {
                 matches!(

src/checkers/ast.rs

@@ -76,10 +76,10 @@ pub struct Checker<'a> {
     pub(crate) child_to_parent: FxHashMap<RefEquality<'a, Stmt>, RefEquality<'a, Stmt>>,
     pub(crate) bindings: Vec<Binding<'a>>,
     pub(crate) redefinitions: IntMap<usize, Vec<usize>>,
-    exprs: Vec<RefEquality<'a, Expr>>,
-    scopes: Vec<Scope<'a>>,
-    scope_stack: Vec<usize>,
-    dead_scopes: Vec<usize>,
+    pub(crate) exprs: Vec<RefEquality<'a, Expr>>,
+    pub(crate) scopes: Vec<Scope<'a>>,
+    pub(crate) scope_stack: Vec<usize>,
+    pub(crate) dead_scopes: Vec<usize>,
     deferred_string_type_definitions: Vec<(Range, &'a str, bool, DeferralContext<'a>)>,
     deferred_type_definitions: Vec<(&'a Expr, bool, DeferralContext<'a>)>,
     deferred_functions: Vec<(&'a Stmt, DeferralContext<'a>, VisibleScope)>,
@@ -3062,10 +3062,28 @@ where
                         } {
                             if self.bindings[*index].used.is_none() {
                                 if self.settings.enabled.contains(&CheckCode::F841) {
-                                    self.checks.push(Check::new(
+                                    let mut check = Check::new(
                                         CheckKind::UnusedVariable(name.to_string()),
                                         name_range,
-                                    ));
+                                    );
+                                    if self.patch(&CheckCode::F841) {
+                                        match pyflakes::fixes::remove_exception_handler_assignment(
+                                            excepthandler,
+                                            self.locator,
+                                        ) {
+                                            Ok(fix) => {
+                                                check.amend(fix);
+                                            }
+                                            Err(e) => {
+                                                error!(
+                                                    "Failed to remove exception handler \
+                                                     assignment: {}",
+                                                    e
+                                                );
+                                            }
+                                        }
+                                    }
+                                    self.checks.push(check);
                                 }
                             }
                         }
@@ -3801,18 +3819,10 @@ impl<'a> Checker<'a> {
             let scope_index = scopes[scopes.len() - 1];
             let parent_scope_index = scopes[scopes.len() - 2];
             if self.settings.enabled.contains(&CheckCode::F841) {
-                self.checks.extend(pyflakes::checks::unused_variable(
-                    &self.scopes[scope_index],
-                    &self.bindings,
-                    &self.settings.dummy_variable_rgx,
-                ));
+                pyflakes::plugins::unused_variable(self, scope_index);
             }
             if self.settings.enabled.contains(&CheckCode::F842) {
-                self.checks.extend(pyflakes::checks::unused_annotation(
-                    &self.scopes[scope_index],
-                    &self.bindings,
-                    &self.settings.dummy_variable_rgx,
-                ));
+                pyflakes::plugins::unused_annotation(self, scope_index);
             }
             if self.settings.enabled.contains(&CheckCode::ARG001)
                 || self.settings.enabled.contains(&CheckCode::ARG002)

src/linter.rs

@@ -527,7 +527,7 @@ pub fn test_path(path: &Path, settings: &Settings) -> Result<Vec<Check>> {
 
     // Detect autofixes that don't converge after multiple iterations.
     if checks.iter().any(|check| check.fix.is_some()) {
-        let max_iterations = 3;
+        let max_iterations = 10;
 
         let mut contents = contents.clone();
         let mut iterations = 0;

src/pyflakes/checks.rs

@@ -1,12 +1,11 @@
 use std::string::ToString;
 
-use regex::Regex;
 use rustpython_parser::ast::{
     Constant, Excepthandler, ExcepthandlerKind, Expr, ExprKind, Stmt, StmtKind,
 };
 
 use crate::ast::helpers::except_range;
-use crate::ast::types::{Binding, BindingKind, Range, Scope, ScopeKind};
+use crate::ast::types::{Binding, Range, Scope, ScopeKind};
 use crate::registry::{Check, CheckKind};
 use crate::source_code_locator::SourceCodeLocator;
 
@@ -52,65 +51,6 @@ pub fn undefined_local(name: &str, scopes: &[&Scope], bindings: &[Binding]) -> O
     None
 }
 
-/// F841
-pub fn unused_variable(
-    scope: &Scope,
-    bindings: &[Binding],
-    dummy_variable_rgx: &Regex,
-) -> Vec<Check> {
-    let mut checks: Vec<Check> = vec![];
-
-    if scope.uses_locals && matches!(scope.kind, ScopeKind::Function(..)) {
-        return checks;
-    }
-
-    for (name, binding) in scope
-        .values
-        .iter()
-        .map(|(name, index)| (name, &bindings[*index]))
-    {
-        if binding.used.is_none()
-            && matches!(binding.kind, BindingKind::Assignment)
-            && !dummy_variable_rgx.is_match(name)
-            && name != &"__tracebackhide__"
-            && name != &"__traceback_info__"
-            && name != &"__traceback_supplement__"
-        {
-            checks.push(Check::new(
-                CheckKind::UnusedVariable((*name).to_string()),
-                binding.range,
-            ));
-        }
-    }
-
-    checks
-}
-
-/// F842
-pub fn unused_annotation(
-    scope: &Scope,
-    bindings: &[Binding],
-    dummy_variable_rgx: &Regex,
-) -> Vec<Check> {
-    let mut checks: Vec<Check> = vec![];
-    for (name, binding) in scope
-        .values
-        .iter()
-        .map(|(name, index)| (name, &bindings[*index]))
-    {
-        if binding.used.is_none()
-            && matches!(binding.kind, BindingKind::Annotation)
-            && !dummy_variable_rgx.is_match(name)
-        {
-            checks.push(Check::new(
-                CheckKind::UnusedAnnotation((*name).to_string()),
-                binding.range,
-            ));
-        }
-    }
-    checks
-}
-
 /// F707
 pub fn default_except_not_last(
     handlers: &[Excepthandler],

src/pyflakes/fixes.rs

@@ -1,6 +1,8 @@
 use anyhow::{bail, Result};
 use libcst_native::{Call, Codegen, CodegenState, Dict, DictElement, Expression};
-use rustpython_ast::Expr;
+use rustpython_ast::{Excepthandler, Expr};
+use rustpython_parser::lexer;
+use rustpython_parser::lexer::Tok;
 
 use crate::ast::types::Range;
 use crate::autofix::Fix;
@@ -97,3 +99,34 @@ pub fn remove_unused_keyword_arguments_from_format_call(
         location.end_location,
     ))
 }
+
+/// Generate a `Fix` to remove the binding from an exception handler.
+pub fn remove_exception_handler_assignment(
+    excepthandler: &Excepthandler,
+    locator: &SourceCodeLocator,
+) -> Result<Fix> {
+    let contents = locator.slice_source_code_range(&Range::from_located(excepthandler));
+    let mut fix_start = None;
+    let mut fix_end = None;
+
+    // End of the token just before the `as` to the semicolon.
+    let mut prev = None;
+    for (start, tok, end) in
+        lexer::make_tokenizer_located(&contents, excepthandler.location).flatten()
+    {
+        if matches!(tok, Tok::As) {
+            fix_start = prev;
+        }
+        if matches!(tok, Tok::Colon) {
+            fix_end = Some(start);
+            break;
+        }
+        prev = Some(end);
+    }
+
+    if let (Some(start), Some(end)) = (fix_start, fix_end) {
+        Ok(Fix::deletion(start, end))
+    } else {
+        bail!("Could not find span of exception handler")
+    }
+}

src/pyflakes/mod.rs

@@ -103,6 +103,7 @@ mod tests {
     #[test_case(CheckCode::F841, Path::new("F841_0.py"); "F841_0")]
     #[test_case(CheckCode::F841, Path::new("F841_1.py"); "F841_1")]
     #[test_case(CheckCode::F841, Path::new("F841_2.py"); "F841_2")]
+    #[test_case(CheckCode::F841, Path::new("F841_3.py"); "F841_3")]
     #[test_case(CheckCode::F842, Path::new("F842.py"); "F842")]
     #[test_case(CheckCode::F901, Path::new("F901.py"); "F901")]
     fn checks(check_code: CheckCode, path: &Path) -> Result<()> {

src/pyflakes/plugins/mod.rs

@@ -12,6 +12,8 @@ pub(crate) use strings::{
     string_dot_format_extra_positional_arguments, string_dot_format_missing_argument,
     string_dot_format_mixing_automatic,
 };
+pub use unused_annotation::unused_annotation;
+pub use unused_variable::unused_variable;
 
 mod assert_tuple;
 mod f_string_missing_placeholders;
@@ -20,3 +22,5 @@ mod invalid_literal_comparisons;
 mod invalid_print_syntax;
 mod raise_not_implemented;
 mod strings;
+mod unused_annotation;
+mod unused_variable;

src/pyflakes/plugins/unused_annotation.rs

@@ -0,0 +1,23 @@
+use crate::ast::types::BindingKind;
+use crate::checkers::ast::Checker;
+use crate::registry::{Check, CheckKind};
+
+/// F842
+pub fn unused_annotation(checker: &mut Checker, scope: usize) {
+    let scope = &checker.scopes[scope];
+    for (name, binding) in scope
+        .values
+        .iter()
+        .map(|(name, index)| (name, &checker.bindings[*index]))
+    {
+        if binding.used.is_none()
+            && matches!(binding.kind, BindingKind::Annotation)
+            && !checker.settings.dummy_variable_rgx.is_match(name)
+        {
+            checker.checks.push(Check::new(
+                CheckKind::UnusedAnnotation((*name).to_string()),
+                binding.range,
+            ));
+        }
+    }
+}