From 3d5bc1f51f521339d0c4437417f483e29cf87e7b Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 5 Oct 2022 14:08:40 -0400 Subject: [PATCH] Migrate Checker logic to independent plugins (#331) --- src/ast/checks.rs | 62 +++----- src/check_ast.rs | 171 ++++------------------ src/lib.rs | 1 + src/plugins.rs | 17 +++ src/plugins/assert_equals.rs | 23 +++ src/plugins/assert_tuple.rs | 13 ++ src/plugins/if_tuple.rs | 13 ++ src/plugins/invalid_print_syntax.rs | 23 +++ src/plugins/print_call.rs | 46 ++++++ src/plugins/super_call_with_parameters.rs | 31 ++++ src/plugins/useless_metaclass_type.rs | 44 ++++++ src/plugins/useless_object_inheritance.rs | 29 ++++ 12 files changed, 288 insertions(+), 185 deletions(-) create mode 100644 src/plugins.rs create mode 100644 src/plugins/assert_equals.rs create mode 100644 src/plugins/assert_tuple.rs create mode 100644 src/plugins/if_tuple.rs create mode 100644 src/plugins/invalid_print_syntax.rs create mode 100644 src/plugins/print_call.rs create mode 100644 src/plugins/super_call_with_parameters.rs create mode 100644 src/plugins/useless_metaclass_type.rs create mode 100644 src/plugins/useless_object_inheritance.rs diff --git a/src/ast/checks.rs b/src/ast/checks.rs index 5e5b927d6d..dc4211b1fb 100644 --- a/src/ast/checks.rs +++ b/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, Fix, RejectedCmpop}; +use crate::checks::{Check, CheckKind, RejectedCmpop}; use crate::python::builtins::BUILTINS; /// Check IfTuple compliance. @@ -178,13 +176,9 @@ pub fn check_ambiguous_function_name(name: &str, location: Range) -> Option Option { 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 AssertEquals compliance. -pub fn check_assert_equals(expr: &Expr, autofix: &fixer::Mode) -> Option { +pub fn check_assert_equals(expr: &Expr) -> Option { if let ExprKind::Attribute { value, attr, .. } = &expr.node { if attr == "assertEquals" { if let ExprKind::Name { id, .. } = &value.node { if id == "self" { - let mut check = - Check::new(CheckKind::NoAssertEquals, Range::from_located(expr)); - if matches!(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, - }); - } - return Some(check); + return Some(Check::new( + CheckKind::NoAssertEquals, + Range::from_located(expr), + )); } } } @@ -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 { +pub fn check_print_call( + expr: &Expr, + func: &Expr, + check_print: bool, + check_pprint: bool, +) -> Option { 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 { 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), diff --git a/src/check_ast.rs b/src/check_ast.rs index 9a28206173..804a8301fa 100644 --- a/src/check_ast.rs +++ b/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 = 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, + // Edit tracking. + // TODO(charlie): Instead of exposing deletions, wrap in a public API. + pub(crate) deletions: BTreeSet, // 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>, - parent_stack: Vec, + pub(crate) parents: Vec<&'a Stmt>, + pub(crate) parent_stack: Vec, scopes: Vec, scope_stack: Vec, dead_scopes: Vec, @@ -50,7 +55,7 @@ struct Checker<'a> { deferred_functions: Vec<(&'a Stmt, Vec, Vec)>, deferred_lambdas: Vec<(&'a Expr, Vec, Vec)>, deferred_assignments: Vec, - // Derivative state. + // Internal, derivative state. in_f_string: Option, 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, } impl<'a> Checker<'a> { @@ -366,19 +369,7 @@ where .. } => { if self.settings.enabled.contains(&CheckCode::R001) { - let scope = - &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); - } + plugins::useless_object_inheritance(self, stmt, name, bases, keywords); } 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) = - checks::check_if_tuple(test, self.locate_check(Range::from_located(stmt))) - { - self.checks.push(check); - } + plugins::if_tuple(self, stmt, test); } } StmtKind::Assert { test, .. } => { - if self - .settings - .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); - } + if self.settings.enabled.contains(&CheckCode::F631) { + plugins::assert_tuple(self, stmt, test); } } 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( - 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); - } + plugins::useless_metaclass_type(self, stmt, value, targets); } } 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) { - self.checks.push(check) - } + plugins::assert_equals(self, func); } // 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. - // (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) - } - } + plugins::super_call_with_parameters(self, expr, func, args); } // 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) { - 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) - } + plugins::print_call(self, expr, func); } 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 { - 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), - )); - } - } - } + plugins::invalid_print_syntax(self, 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(); diff --git a/src/lib.rs b/src/lib.rs index 892ea209f3..82932f8de8 100644 --- a/src/lib.rs +++ b/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; diff --git a/src/plugins.rs b/src/plugins.rs new file mode 100644 index 0000000000..0d8cfe156f --- /dev/null +++ b/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; diff --git a/src/plugins/assert_equals.rs b/src/plugins/assert_equals.rs new file mode 100644 index 0000000000..a6720c4b50 --- /dev/null +++ b/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); + } +} diff --git a/src/plugins/assert_tuple.rs b/src/plugins/assert_tuple.rs new file mode 100644 index 0000000000..a01f1894f8 --- /dev/null +++ b/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); + } +} diff --git a/src/plugins/if_tuple.rs b/src/plugins/if_tuple.rs new file mode 100644 index 0000000000..649ed6ef5f --- /dev/null +++ b/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); + } +} diff --git a/src/plugins/invalid_print_syntax.rs b/src/plugins/invalid_print_syntax.rs new file mode 100644 index 0000000000..8ab870ac4a --- /dev/null +++ b/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), + )); + } + } + } +} diff --git a/src/plugins/print_call.rs b/src/plugins/print_call.rs new file mode 100644 index 0000000000..cc93258fe6 --- /dev/null +++ b/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); + } +} diff --git a/src/plugins/super_call_with_parameters.rs b/src/plugins/super_call_with_parameters.rs new file mode 100644 index 0000000000..2477641c08 --- /dev/null +++ b/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, +) { + // 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) + } + } +} diff --git a/src/plugins/useless_metaclass_type.rs b/src/plugins/useless_metaclass_type.rs new file mode 100644 index 0000000000..d54d962b48 --- /dev/null +++ b/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, +) { + 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); + } +} diff --git a/src/plugins/useless_object_inheritance.rs b/src/plugins/useless_object_inheritance.rs new file mode 100644 index 0000000000..34a24dbb54 --- /dev/null +++ b/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); + } +}