Migrate Checker logic to independent plugins (#331)

src/ast/checks.rs

@@ -4,15 +4,13 @@ use itertools::izip;
 use regex::Regex;
 use rustpython_parser::ast::{
     Arg, ArgData, Arguments, Cmpop, Constant, Excepthandler, ExcepthandlerKind, Expr, ExprKind,
-    Keyword, Location, Stmt, StmtKind, Unaryop,
+    Stmt, StmtKind, Unaryop,
 };
 
-use crate::ast::operations::SourceCodeLocator;
 use crate::ast::types::{
     Binding, BindingKind, CheckLocator, FunctionScope, Range, Scope, ScopeKind,
 };
-use crate::autofix::{fixer, fixes};
+use crate::checks::{Check, CheckKind, RejectedCmpop};
-use crate::checks::{Check, CheckKind, Fix, RejectedCmpop};
 use crate::python::builtins::BUILTINS;
 
 /// Check IfTuple compliance.
@@ -178,13 +176,9 @@ pub fn check_ambiguous_function_name(name: &str, location: Range) -> Option<Chec
 
 /// Check UselessObjectInheritance compliance.
 pub fn check_useless_object_inheritance(
-    stmt: &Stmt,
     name: &str,
     bases: &[Expr],
-    keywords: &[Keyword],
     scope: &Scope,
-    locator: &mut SourceCodeLocator,
-    autofix: &fixer::Mode,
 ) -> Option<Check> {
     for expr in bases {
         if let ExprKind::Name { id, .. } = &expr.node {
@@ -195,22 +189,10 @@ pub fn check_useless_object_inheritance(
                         kind: BindingKind::Builtin,
                         ..
                     }) => {
-                        let mut check = Check::new(
+                        return Some(Check::new(
                             CheckKind::UselessObjectInheritance(name.to_string()),
                             Range::from_located(expr),
-                        );
+                        ));
-                        if matches!(autofix, fixer::Mode::Generate | fixer::Mode::Apply) {
-                            if let Some(fix) = fixes::remove_class_def_base(
-                                locator,
-                                &stmt.location,
-                                expr.location,
-                                bases,
-                                keywords,
-                            ) {
-                                check.amend(fix);
-                            }
-                        }
-                        return Some(check);
                     }
                     _ => {}
                 }
@@ -298,28 +280,15 @@ pub fn check_duplicate_arguments(arguments: &Arguments) -> Vec<Check> {
 }
 
 /// Check AssertEquals compliance.
-pub fn check_assert_equals(expr: &Expr, autofix: &fixer::Mode) -> Option<Check> {
+pub fn check_assert_equals(expr: &Expr) -> Option<Check> {
     if let ExprKind::Attribute { value, attr, .. } = &expr.node {
         if attr == "assertEquals" {
             if let ExprKind::Name { id, .. } = &value.node {
                 if id == "self" {
-                    let mut check =
+                    return Some(Check::new(
-                        Check::new(CheckKind::NoAssertEquals, Range::from_located(expr));
+                        CheckKind::NoAssertEquals,
-                    if matches!(autofix, fixer::Mode::Generate | fixer::Mode::Apply) {
+                        Range::from_located(expr),
-                        check.amend(Fix {
+                    ));
-                            content: "assertEqual".to_string(),
-                            location: Location::new(
-                                expr.location.row(),
-                                expr.location.column() + 1,
-                            ),
-                            end_location: Location::new(
-                                expr.location.row(),
-                                expr.location.column() + 1 + "assertEquals".len(),
-                            ),
-                            applied: false,
-                        });
-                    }
-                    return Some(check);
                 }
             }
         }
@@ -791,11 +760,16 @@ pub fn check_super_args(
 
 // flake8-print
 /// Check whether a function call is a `print` or `pprint` invocation
-pub fn check_print_call(expr: &Expr, func: &Expr) -> Option<Check> {
+pub fn check_print_call(
+    expr: &Expr,
+    func: &Expr,
+    check_print: bool,
+    check_pprint: bool,
+) -> Option<Check> {
     if let ExprKind::Name { id, .. } = &func.node {
-        if id == "print" {
+        if check_print && id == "print" {
             return Some(Check::new(CheckKind::PrintFound, Range::from_located(expr)));
-        } else if id == "pprint" {
+        } else if check_pprint && id == "pprint" {
             return Some(Check::new(
                 CheckKind::PPrintFound,
                 Range::from_located(expr),
@@ -805,7 +779,7 @@ pub fn check_print_call(expr: &Expr, func: &Expr) -> Option<Check> {
 
     if let ExprKind::Attribute { value, attr, .. } = &func.node {
         if let ExprKind::Name { id, .. } = &value.node {
-            if id == "pprint" && attr == "pprint" {
+            if check_pprint && id == "pprint" && attr == "pprint" {
                 return Some(Check::new(
                     CheckKind::PPrintFound,
                     Range::from_located(expr),

src/check_ast.rs

@@ -21,6 +21,7 @@ use crate::ast::visitor::{walk_excepthandler, Visitor};
 use crate::ast::{checks, operations, visitor};
 use crate::autofix::{fixer, fixes};
 use crate::checks::{Check, CheckCode, CheckKind};
+use crate::plugins;
 use crate::python::builtins::{BUILTINS, MAGIC_GLOBALS};
 use crate::python::future::ALL_FEATURE_NAMES;
 use crate::python::typing;
@@ -30,18 +31,22 @@ pub const GLOBAL_SCOPE_INDEX: usize = 0;
 
 static DUNDER_REGEX: Lazy<Regex> = Lazy::new(|| Regex::new(r"__[^\s]+__").unwrap());
 
-struct Checker<'a> {
+pub struct Checker<'a> {
     // Input data.
-    locator: SourceCodeLocator<'a>,
-    settings: &'a Settings,
-    autofix: &'a fixer::Mode,
     path: &'a Path,
+    // TODO(charlie): Separate immutable from mutable state. (None of these should ever change.)
+    pub(crate) locator: SourceCodeLocator<'a>,
+    pub(crate) settings: &'a Settings,
+    pub(crate) autofix: &'a fixer::Mode,
     // Computed checks.
     checks: Vec<Check>,
+    // Edit tracking.
+    // TODO(charlie): Instead of exposing deletions, wrap in a public API.
+    pub(crate) deletions: BTreeSet<usize>,
     // Retain all scopes and parent nodes, along with a stack of indexes to track which are active
     // at various points in time.
-    parents: Vec<&'a Stmt>,
+    pub(crate) parents: Vec<&'a Stmt>,
-    parent_stack: Vec<usize>,
+    pub(crate) parent_stack: Vec<usize>,
     scopes: Vec<Scope>,
     scope_stack: Vec<usize>,
     dead_scopes: Vec<usize>,
@@ -50,7 +55,7 @@ struct Checker<'a> {
     deferred_functions: Vec<(&'a Stmt, Vec<usize>, Vec<usize>)>,
     deferred_lambdas: Vec<(&'a Expr, Vec<usize>, Vec<usize>)>,
     deferred_assignments: Vec<usize>,
-    // Derivative state.
+    // Internal, derivative state.
     in_f_string: Option<Range>,
     in_annotation: bool,
     in_literal: bool,
@@ -58,8 +63,6 @@ struct Checker<'a> {
     seen_docstring: bool,
     futures_allowed: bool,
     annotations_future_enabled: bool,
-    // Edit tracking.
-    deletions: BTreeSet<usize>,
 }
 
 impl<'a> Checker<'a> {
@@ -366,19 +369,7 @@ where
                 ..
             } => {
                 if self.settings.enabled.contains(&CheckCode::R001) {
-                    let scope =
+                    plugins::useless_object_inheritance(self, stmt, name, bases, keywords);
-                        &self.scopes[*(self.scope_stack.last().expect("No current scope found."))];
-                    if let Some(check) = checks::check_useless_object_inheritance(
-                        stmt,
-                        name,
-                        bases,
-                        keywords,
-                        scope,
-                        &mut self.locator,
-                        self.autofix,
-                    ) {
-                        self.checks.push(check);
-                    }
                 }
 
                 if self.settings.enabled.contains(&CheckCode::E742) {
@@ -597,25 +588,12 @@ where
             }
             StmtKind::If { test, .. } => {
                 if self.settings.enabled.contains(&CheckCode::F634) {
-                    if let Some(check) =
+                    plugins::if_tuple(self, stmt, test);
-                        checks::check_if_tuple(test, self.locate_check(Range::from_located(stmt)))
-                    {
-                        self.checks.push(check);
-                    }
                 }
             }
             StmtKind::Assert { test, .. } => {
-                if self
+                if self.settings.enabled.contains(&CheckCode::F631) {
-                    .settings
+                    plugins::assert_tuple(self, stmt, test);
-                    .enabled
-                    .contains(CheckKind::AssertTuple.code())
-                {
-                    if let Some(check) = checks::check_assert_tuple(
-                        test,
-                        self.locate_check(Range::from_located(stmt)),
-                    ) {
-                        self.checks.push(check);
-                    }
                 }
             }
             StmtKind::Try { handlers, .. } => {
@@ -635,35 +613,7 @@ where
                     }
                 }
                 if self.settings.enabled.contains(&CheckCode::U001) {
-                    if let Some(mut check) = checks::check_useless_metaclass_type(
+                    plugins::useless_metaclass_type(self, stmt, value, targets);
-                        targets,
-                        value,
-                        self.locate_check(Range::from_located(stmt)),
-                    ) {
-                        if matches!(self.autofix, fixer::Mode::Generate | fixer::Mode::Apply) {
-                            let context = self.binding_context();
-                            let deleted: Vec<&Stmt> = self
-                                .deletions
-                                .iter()
-                                .map(|index| self.parents[*index])
-                                .collect();
-
-                            match fixes::remove_stmt(
-                                self.parents[context.defined_by],
-                                context.defined_in.map(|index| self.parents[index]),
-                                &deleted,
-                            ) {
-                                Ok(fix) => {
-                                    if fix.content.is_empty() || fix.content == "pass" {
-                                        self.deletions.insert(context.defined_by);
-                                    }
-                                    check.amend(fix)
-                                }
-                                Err(e) => error!("Failed to fix unused imports: {}", e),
-                            }
-                        }
-                        self.checks.push(check);
-                    }
                 }
             }
             StmtKind::AnnAssign { value, .. } => {
@@ -782,73 +732,19 @@ where
             },
             ExprKind::Call { func, args, .. } => {
                 if self.settings.enabled.contains(&CheckCode::R002) {
-                    if let Some(check) = checks::check_assert_equals(func, self.autofix) {
+                    plugins::assert_equals(self, func);
-                        self.checks.push(check)
-                    }
                 }
 
                 // flake8-super
                 if self.settings.enabled.contains(&CheckCode::SPR001) {
-                    // Only bother going through the super check at all if we're in a `super` call.
+                    plugins::super_call_with_parameters(self, expr, func, args);
-                    // (We check this in `check_super_args` too, so this is just an optimization.)
-                    if checks::is_super_call_with_arguments(func, args) {
-                        let scope = &mut self.scopes
-                            [*(self.scope_stack.last().expect("No current scope found."))];
-                        let parents: Vec<&Stmt> = self
-                            .parent_stack
-                            .iter()
-                            .map(|index| self.parents[*index])
-                            .collect();
-                        if let Some(mut check) =
-                            checks::check_super_args(scope, &parents, expr, func, args)
-                        {
-                            if matches!(self.autofix, fixer::Mode::Generate | fixer::Mode::Apply) {
-                                if let Some(fix) =
-                                    fixes::remove_super_arguments(&mut self.locator, expr)
-                                {
-                                    check.amend(fix);
-                                }
-                            }
-                            self.checks.push(check)
-                        }
-                    }
                 }
 
                 // flake8-print
                 if self.settings.enabled.contains(&CheckCode::T201)
                     || self.settings.enabled.contains(&CheckCode::T203)
                 {
-                    if let Some(mut check) = checks::check_print_call(expr, func) {
+                    plugins::print_call(self, expr, func);
-                        if matches!(self.autofix, fixer::Mode::Generate | fixer::Mode::Apply) {
-                            let context = self.binding_context();
-                            if matches!(
-                                self.parents[context.defined_by].node,
-                                StmtKind::Expr { .. }
-                            ) {
-                                let deleted: Vec<&Stmt> = self
-                                    .deletions
-                                    .iter()
-                                    .map(|index| self.parents[*index])
-                                    .collect();
-
-                                match fixes::remove_stmt(
-                                    self.parents[context.defined_by],
-                                    context.defined_in.map(|index| self.parents[index]),
-                                    &deleted,
-                                ) {
-                                    Ok(fix) => {
-                                        if fix.content.is_empty() || fix.content == "pass" {
-                                            self.deletions.insert(context.defined_by);
-                                        }
-                                        check.amend(fix)
-                                    }
-                                    Err(e) => error!("Failed to fix unused imports: {}", e),
-                                }
-                            }
-                        }
-
-                        self.checks.push(check)
-                    }
                 }
 
                 if let ExprKind::Name { id, ctx } = &func.node {
@@ -916,22 +812,7 @@ where
                 ..
             } => {
                 if self.settings.enabled.contains(&CheckCode::F633) {
-                    if let ExprKind::Name { id, .. } = &left.node {
+                    plugins::invalid_print_syntax(self, left);
-                        if id == "print" {
-                            let scope = &self.scopes
-                                [*(self.scope_stack.last().expect("No current scope found."))];
-                            if let Some(Binding {
-                                kind: BindingKind::Builtin,
-                                ..
-                            }) = scope.values.get("print")
-                            {
-                                self.checks.push(Check::new(
-                                    CheckKind::InvalidPrintSyntax,
-                                    Range::from_located(left),
-                                ));
-                            }
-                        }
-                    }
                 }
             }
             ExprKind::UnaryOp { op, operand } => {
@@ -1306,6 +1187,10 @@ impl CheckLocator for Checker<'_> {
 }
 
 impl<'a> Checker<'a> {
+    pub fn add_check(&mut self, check: Check) {
+        self.checks.push(check);
+    }
+
     fn push_parent(&mut self, parent: &'a Stmt) {
         self.parent_stack.push(self.parents.len());
         self.parents.push(parent);
@@ -1355,7 +1240,11 @@ impl<'a> Checker<'a> {
         }
     }
 
-    fn binding_context(&self) -> BindingContext {
+    pub fn current_scope(&self) -> &Scope {
+        &self.scopes[*(self.scope_stack.last().expect("No current scope found."))]
+    }
+
+    pub fn binding_context(&self) -> BindingContext {
         let mut rev = self.parent_stack.iter().rev().fuse();
         let defined_by = *rev.next().expect("Expected to bind within a statement.");
         let defined_in = rev.next().cloned();

src/lib.rs

@@ -21,6 +21,7 @@ pub mod linter;
 pub mod logging;
 pub mod message;
 mod noqa;
+mod plugins;
 pub mod printer;
 pub mod pyproject;
 mod python;

src/plugins.rs

@@ -0,0 +1,17 @@
+mod assert_equals;
+mod assert_tuple;
+mod if_tuple;
+mod invalid_print_syntax;
+mod print_call;
+mod super_call_with_parameters;
+mod useless_metaclass_type;
+mod useless_object_inheritance;
+
+pub use assert_equals::assert_equals;
+pub use assert_tuple::assert_tuple;
+pub use if_tuple::if_tuple;
+pub use invalid_print_syntax::invalid_print_syntax;
+pub use print_call::print_call;
+pub use super_call_with_parameters::super_call_with_parameters;
+pub use useless_metaclass_type::useless_metaclass_type;
+pub use useless_object_inheritance::useless_object_inheritance;

src/plugins/assert_equals.rs

@@ -0,0 +1,23 @@
+use rustpython_ast::{Expr, Location};
+
+use crate::ast::checks;
+use crate::autofix::fixer;
+use crate::check_ast::Checker;
+use crate::checks::Fix;
+
+pub fn assert_equals(checker: &mut Checker, expr: &Expr) {
+    if let Some(mut check) = checks::check_assert_equals(expr) {
+        if matches!(checker.autofix, fixer::Mode::Generate | fixer::Mode::Apply) {
+            check.amend(Fix {
+                content: "assertEqual".to_string(),
+                location: Location::new(expr.location.row(), expr.location.column() + 1),
+                end_location: Location::new(
+                    expr.location.row(),
+                    expr.location.column() + 1 + "assertEquals".len(),
+                ),
+                applied: false,
+            });
+        }
+        checker.add_check(check);
+    }
+}

src/plugins/assert_tuple.rs

@@ -0,0 +1,13 @@
+use rustpython_ast::{Expr, Stmt};
+
+use crate::ast::checks;
+use crate::ast::types::{CheckLocator, Range};
+use crate::check_ast::Checker;
+
+pub fn assert_tuple(checker: &mut Checker, stmt: &Stmt, test: &Expr) {
+    if let Some(check) =
+        checks::check_assert_tuple(test, checker.locate_check(Range::from_located(stmt)))
+    {
+        checker.add_check(check);
+    }
+}

src/plugins/if_tuple.rs

@@ -0,0 +1,13 @@
+use rustpython_ast::{Expr, Stmt};
+
+use crate::ast::checks;
+use crate::ast::types::{CheckLocator, Range};
+use crate::check_ast::Checker;
+
+pub fn if_tuple(checker: &mut Checker, stmt: &Stmt, test: &Expr) {
+    if let Some(check) =
+        checks::check_if_tuple(test, checker.locate_check(Range::from_located(stmt)))
+    {
+        checker.add_check(check);
+    }
+}

src/plugins/invalid_print_syntax.rs

@@ -0,0 +1,23 @@
+use rustpython_ast::{Expr, ExprKind};
+
+use crate::ast::types::{Binding, BindingKind, Range};
+use crate::check_ast::Checker;
+use crate::checks::{Check, CheckKind};
+
+pub fn invalid_print_syntax(checker: &mut Checker, left: &Expr) {
+    if let ExprKind::Name { id, .. } = &left.node {
+        if id == "print" {
+            let scope = checker.current_scope();
+            if let Some(Binding {
+                kind: BindingKind::Builtin,
+                ..
+            }) = scope.values.get("print")
+            {
+                checker.add_check(Check::new(
+                    CheckKind::InvalidPrintSyntax,
+                    Range::from_located(left),
+                ));
+            }
+        }
+    }
+}

src/plugins/print_call.rs

@@ -0,0 +1,46 @@
+use log::error;
+use rustpython_ast::{Expr, Stmt, StmtKind};
+
+use crate::ast::checks;
+use crate::autofix::{fixer, fixes};
+use crate::check_ast::Checker;
+use crate::checks::CheckCode;
+
+pub fn print_call(checker: &mut Checker, expr: &Expr, func: &Expr) {
+    if let Some(mut check) = checks::check_print_call(
+        expr,
+        func,
+        checker.settings.enabled.contains(&CheckCode::T201),
+        checker.settings.enabled.contains(&CheckCode::T203),
+    ) {
+        if matches!(checker.autofix, fixer::Mode::Generate | fixer::Mode::Apply) {
+            let context = checker.binding_context();
+            if matches!(
+                checker.parents[context.defined_by].node,
+                StmtKind::Expr { .. }
+            ) {
+                let deleted: Vec<&Stmt> = checker
+                    .deletions
+                    .iter()
+                    .map(|index| checker.parents[*index])
+                    .collect();
+
+                match fixes::remove_stmt(
+                    checker.parents[context.defined_by],
+                    context.defined_in.map(|index| checker.parents[index]),
+                    &deleted,
+                ) {
+                    Ok(fix) => {
+                        if fix.content.is_empty() || fix.content == "pass" {
+                            checker.deletions.insert(context.defined_by);
+                        }
+                        check.amend(fix)
+                    }
+                    Err(e) => error!("Failed to fix unused imports: {}", e),
+                }
+            }
+        }
+
+        checker.add_check(check);
+    }
+}

src/plugins/super_call_with_parameters.rs

@@ -0,0 +1,31 @@
+use rustpython_ast::{Expr, Stmt};
+
+use crate::ast::checks;
+use crate::autofix::{fixer, fixes};
+use crate::check_ast::Checker;
+
+pub fn super_call_with_parameters(
+    checker: &mut Checker,
+    expr: &Expr,
+    func: &Expr,
+    args: &Vec<Expr>,
+) {
+    // Only bother going through the super check at all if we're in a `super` call.
+    // (We check this in `check_super_args` too, so this is just an optimization.)
+    if checks::is_super_call_with_arguments(func, args) {
+        let scope = checker.current_scope();
+        let parents: Vec<&Stmt> = checker
+            .parent_stack
+            .iter()
+            .map(|index| checker.parents[*index])
+            .collect();
+        if let Some(mut check) = checks::check_super_args(scope, &parents, expr, func, args) {
+            if matches!(checker.autofix, fixer::Mode::Generate | fixer::Mode::Apply) {
+                if let Some(fix) = fixes::remove_super_arguments(&mut checker.locator, expr) {
+                    check.amend(fix);
+                }
+            }
+            checker.add_check(check)
+        }
+    }
+}

src/plugins/useless_metaclass_type.rs

@@ -0,0 +1,44 @@
+use log::error;
+use rustpython_ast::{Expr, Stmt};
+
+use crate::ast::checks;
+use crate::ast::types::{CheckLocator, Range};
+use crate::autofix::{fixer, fixes};
+use crate::check_ast::Checker;
+
+pub fn useless_metaclass_type(
+    checker: &mut Checker,
+    stmt: &Stmt,
+    value: &Expr,
+    targets: &Vec<Expr>,
+) {
+    if let Some(mut check) = checks::check_useless_metaclass_type(
+        targets,
+        value,
+        checker.locate_check(Range::from_located(stmt)),
+    ) {
+        if matches!(checker.autofix, fixer::Mode::Generate | fixer::Mode::Apply) {
+            let context = checker.binding_context();
+            let deleted: Vec<&Stmt> = checker
+                .deletions
+                .iter()
+                .map(|index| checker.parents[*index])
+                .collect();
+
+            match fixes::remove_stmt(
+                checker.parents[context.defined_by],
+                context.defined_in.map(|index| checker.parents[index]),
+                &deleted,
+            ) {
+                Ok(fix) => {
+                    if fix.content.is_empty() || fix.content == "pass" {
+                        checker.deletions.insert(context.defined_by);
+                    }
+                    check.amend(fix)
+                }
+                Err(e) => error!("Failed to fix unused imports: {}", e),
+            }
+        }
+        checker.add_check(check);
+    }
+}

src/plugins/useless_object_inheritance.rs

@@ -0,0 +1,29 @@
+use rustpython_ast::{Expr, Keyword, Stmt};
+
+use crate::ast::checks;
+use crate::autofix::{fixer, fixes};
+use crate::check_ast::Checker;
+
+pub fn useless_object_inheritance(
+    checker: &mut Checker,
+    stmt: &Stmt,
+    name: &str,
+    bases: &[Expr],
+    keywords: &[Keyword],
+) {
+    let scope = checker.current_scope();
+    if let Some(mut check) = checks::check_useless_object_inheritance(name, bases, scope) {
+        if matches!(checker.autofix, fixer::Mode::Generate | fixer::Mode::Apply) {
+            if let Some(fix) = fixes::remove_class_def_base(
+                &mut checker.locator,
+                &stmt.location,
+                check.location,
+                bases,
+                keywords,
+            ) {
+                check.amend(fix);
+            }
+        }
+        checker.add_check(check);
+    }
+}