From c59e1ff0b5d63b539ec4df632f2ebbc15ba0d660 Mon Sep 17 00:00:00 2001 From: Harutaka Kawamura Date: Fri, 18 Nov 2022 01:43:44 +0900 Subject: [PATCH] Implement auto-fix for E711 and E712 (#784) --- README.md | 4 +- resources/test/fixtures/E711.py | 3 + resources/test/fixtures/E712.py | 3 + src/autofix/mod.rs | 11 + src/check_ast.rs | 19 +- src/pycodestyle/checks.rs | 112 +--------- src/pycodestyle/mod.rs | 1 + src/pycodestyle/plugins.rs | 203 ++++++++++++++++++ .../ruff__linter__tests__E711_E711.py.snap | 124 ++++++++++- .../ruff__linter__tests__E712_E712.py.snap | 140 +++++++++++- 10 files changed, 481 insertions(+), 139 deletions(-) create mode 100644 src/pycodestyle/plugins.rs diff --git a/README.md b/README.md index c9a7b8cfa9..2eb514f4cf 100644 --- a/README.md +++ b/README.md @@ -356,8 +356,8 @@ For more, see [pycodestyle](https://pypi.org/project/pycodestyle/2.9.1/) on PyPI | ---- | ---- | ------- | --- | | E402 | ModuleImportNotAtTopOfFile | Module level import not at top of file | | | E501 | LineTooLong | Line too long (89 > 88 characters) | | -| E711 | NoneComparison | Comparison to `None` should be `cond is None` | | -| E712 | TrueFalseComparison | Comparison to `True` should be `cond is True` | | +| E711 | NoneComparison | Comparison to `None` should be `cond is None` | 🛠 | +| E712 | TrueFalseComparison | Comparison to `True` should be `cond is True` | 🛠 | | E713 | NotInTest | Test for membership should be `not in` | | | E714 | NotIsTest | Test for object identity should be `is not` | | | E721 | TypeComparison | Do not compare types, use `isinstance()` | | diff --git a/resources/test/fixtures/E711.py b/resources/test/fixtures/E711.py index 0b141efcdc..f0b2eb8494 100644 --- a/resources/test/fixtures/E711.py +++ b/resources/test/fixtures/E711.py @@ -23,6 +23,9 @@ if None != res[1]: if None == res[1]: pass +if x == None != None: + pass + #: Okay if x not in y: pass diff --git a/resources/test/fixtures/E712.py b/resources/test/fixtures/E712.py index a3f97e19c7..818afddaef 100644 --- a/resources/test/fixtures/E712.py +++ b/resources/test/fixtures/E712.py @@ -22,6 +22,9 @@ var = 1 if cond == True else -1 if cond == False else cond if (True) == TrueElement or x == TrueElement: pass +if res == True != False: + pass + #: Okay if x not in y: pass diff --git a/src/autofix/mod.rs b/src/autofix/mod.rs index b9d5a92efb..ff56982819 100644 --- a/src/autofix/mod.rs +++ b/src/autofix/mod.rs @@ -50,4 +50,15 @@ impl Fix { applied: false, } } + + pub fn dummy(location: Location) -> Self { + Self { + patch: Patch { + content: "".to_string(), + location, + end_location: location, + }, + applied: false, + } + } } diff --git a/src/check_ast.rs b/src/check_ast.rs index 1aee834fa9..0976718dfa 100644 --- a/src/check_ast.rs +++ b/src/check_ast.rs @@ -1525,16 +1525,15 @@ where let check_none_comparisons = self.settings.enabled.contains(&CheckCode::E711); let check_true_false_comparisons = self.settings.enabled.contains(&CheckCode::E712); if check_none_comparisons || check_true_false_comparisons { - self.add_checks( - pycodestyle::checks::literal_comparisons( - left, - ops, - comparators, - check_none_comparisons, - check_true_false_comparisons, - ) - .into_iter(), - ); + pycodestyle::plugins::literal_comparisons( + self, + expr, + left, + ops, + comparators, + check_none_comparisons, + check_true_false_comparisons, + ) } if self.settings.enabled.contains(&CheckCode::F632) { diff --git a/src/pycodestyle/checks.rs b/src/pycodestyle/checks.rs index 764a9ede99..293dcfc581 100644 --- a/src/pycodestyle/checks.rs +++ b/src/pycodestyle/checks.rs @@ -1,9 +1,9 @@ use itertools::izip; use rustpython_ast::Location; -use rustpython_parser::ast::{Cmpop, Constant, Expr, ExprKind, Stmt, Unaryop}; +use rustpython_parser::ast::{Cmpop, Expr, ExprKind, Stmt, Unaryop}; use crate::ast::types::Range; -use crate::checks::{Check, CheckKind, RejectedCmpop}; +use crate::checks::{Check, CheckKind}; use crate::source_code_locator::SourceCodeLocator; fn is_ambiguous_name(name: &str) -> bool { @@ -97,114 +97,6 @@ pub fn not_tests( checks } -/// E711, E712 -pub fn literal_comparisons( - left: &Expr, - ops: &[Cmpop], - comparators: &[Expr], - check_none_comparisons: bool, - check_true_false_comparisons: bool, -) -> Vec { - let mut checks: Vec = vec![]; - - let op = ops.first().unwrap(); - let comparator = left; - - // Check `left`. - if check_none_comparisons - && matches!( - comparator.node, - ExprKind::Constant { - value: Constant::None, - kind: None - } - ) - { - if matches!(op, Cmpop::Eq) { - checks.push(Check::new( - CheckKind::NoneComparison(RejectedCmpop::Eq), - Range::from_located(comparator), - )); - } - if matches!(op, Cmpop::NotEq) { - checks.push(Check::new( - CheckKind::NoneComparison(RejectedCmpop::NotEq), - Range::from_located(comparator), - )); - } - } - - if check_true_false_comparisons { - if let ExprKind::Constant { - value: Constant::Bool(value), - kind: None, - } = comparator.node - { - if matches!(op, Cmpop::Eq) { - checks.push(Check::new( - CheckKind::TrueFalseComparison(value, RejectedCmpop::Eq), - Range::from_located(comparator), - )); - } - if matches!(op, Cmpop::NotEq) { - checks.push(Check::new( - CheckKind::TrueFalseComparison(value, RejectedCmpop::NotEq), - Range::from_located(comparator), - )); - } - } - } - - // Check each comparator in order. - for (op, comparator) in izip!(ops, comparators) { - if check_none_comparisons - && matches!( - comparator.node, - ExprKind::Constant { - value: Constant::None, - kind: None - } - ) - { - if matches!(op, Cmpop::Eq) { - checks.push(Check::new( - CheckKind::NoneComparison(RejectedCmpop::Eq), - Range::from_located(comparator), - )); - } - if matches!(op, Cmpop::NotEq) { - checks.push(Check::new( - CheckKind::NoneComparison(RejectedCmpop::NotEq), - Range::from_located(comparator), - )); - } - } - - if check_true_false_comparisons { - if let ExprKind::Constant { - value: Constant::Bool(value), - kind: None, - } = comparator.node - { - if matches!(op, Cmpop::Eq) { - checks.push(Check::new( - CheckKind::TrueFalseComparison(value, RejectedCmpop::Eq), - Range::from_located(comparator), - )); - } - if matches!(op, Cmpop::NotEq) { - checks.push(Check::new( - CheckKind::TrueFalseComparison(value, RejectedCmpop::NotEq), - Range::from_located(comparator), - )); - } - } - } - } - - checks -} - /// E721 pub fn type_comparison(ops: &[Cmpop], comparators: &[Expr], location: Range) -> Vec { let mut checks: Vec = vec![]; diff --git a/src/pycodestyle/mod.rs b/src/pycodestyle/mod.rs index f6b5329a02..6cab51a666 100644 --- a/src/pycodestyle/mod.rs +++ b/src/pycodestyle/mod.rs @@ -1 +1,2 @@ pub mod checks; +pub mod plugins; diff --git a/src/pycodestyle/plugins.rs b/src/pycodestyle/plugins.rs new file mode 100644 index 0000000000..d83d10b7df --- /dev/null +++ b/src/pycodestyle/plugins.rs @@ -0,0 +1,203 @@ +use fnv::FnvHashMap; +use itertools::izip; +use rustpython_parser::ast::{Cmpop, Constant, Expr, ExprKind}; + +use crate::ast::types::Range; +use crate::autofix::Fix; +use crate::check_ast::Checker; +use crate::checks::{Check, CheckKind, RejectedCmpop}; +use crate::code_gen::SourceGenerator; + +fn compare(left: &Expr, ops: &[Cmpop], comparators: &[Expr]) -> Option { + let cmp = Expr::new( + Default::default(), + Default::default(), + ExprKind::Compare { + left: Box::new(left.clone()), + ops: ops.to_vec(), + comparators: comparators.to_vec(), + }, + ); + let mut generator = SourceGenerator::new(); + if let Ok(()) = generator.unparse_expr(&cmp, 0) { + if let Ok(content) = generator.generate() { + return Some(content); + } + } + None +} + +/// E711, E712 +pub fn literal_comparisons( + checker: &mut Checker, + expr: &Expr, + left: &Expr, + ops: &[Cmpop], + comparators: &[Expr], + check_none_comparisons: bool, + check_true_false_comparisons: bool, +) { + // Mapping from (bad operator index) to (replacement operator). As we iterate + // through the list of operators, we apply "dummy" fixes for each error, + // then replace the entire expression at the end with one "real" fix, to + // avoid conflicts. + let mut bad_ops: FnvHashMap = FnvHashMap::default(); + let mut checks: Vec = vec![]; + + let op = ops.first().unwrap(); + let comparator = left; + + // Check `left`. + if check_none_comparisons + && matches!( + comparator.node, + ExprKind::Constant { + value: Constant::None, + kind: None + } + ) + { + if matches!(op, Cmpop::Eq) { + let mut check = Check::new( + CheckKind::NoneComparison(RejectedCmpop::Eq), + Range::from_located(comparator), + ); + if checker.patch() { + // Dummy replacement + check.amend(Fix::dummy(expr.location)); + bad_ops.insert(0, Cmpop::Is); + } + checks.push(check); + } + if matches!(op, Cmpop::NotEq) { + let mut check = Check::new( + CheckKind::NoneComparison(RejectedCmpop::NotEq), + Range::from_located(comparator), + ); + if checker.patch() { + check.amend(Fix::dummy(expr.location)); + bad_ops.insert(0, Cmpop::IsNot); + } + checks.push(check); + } + } + + if check_true_false_comparisons { + if let ExprKind::Constant { + value: Constant::Bool(value), + kind: None, + } = comparator.node + { + if matches!(op, Cmpop::Eq) { + let mut check = Check::new( + CheckKind::TrueFalseComparison(value, RejectedCmpop::Eq), + Range::from_located(comparator), + ); + if checker.patch() { + check.amend(Fix::dummy(expr.location)); + bad_ops.insert(0, Cmpop::Is); + } + checks.push(check); + } + if matches!(op, Cmpop::NotEq) { + let mut check = Check::new( + CheckKind::TrueFalseComparison(value, RejectedCmpop::NotEq), + Range::from_located(comparator), + ); + if checker.patch() { + check.amend(Fix::dummy(expr.location)); + bad_ops.insert(0, Cmpop::IsNot); + } + checks.push(check); + } + } + } + + // Check each comparator in order. + for (idx, (op, comparator)) in izip!(ops, comparators).enumerate() { + if check_none_comparisons + && matches!( + comparator.node, + ExprKind::Constant { + value: Constant::None, + kind: None + } + ) + { + if matches!(op, Cmpop::Eq) { + let mut check = Check::new( + CheckKind::NoneComparison(RejectedCmpop::Eq), + Range::from_located(comparator), + ); + if checker.patch() { + check.amend(Fix::dummy(expr.location)); + bad_ops.insert(idx, Cmpop::Is); + } + checks.push(check); + } + if matches!(op, Cmpop::NotEq) { + let mut check = Check::new( + CheckKind::NoneComparison(RejectedCmpop::NotEq), + Range::from_located(comparator), + ); + if checker.patch() { + check.amend(Fix::dummy(expr.location)); + bad_ops.insert(idx, Cmpop::IsNot); + } + checks.push(check); + } + } + + if check_true_false_comparisons { + if let ExprKind::Constant { + value: Constant::Bool(value), + kind: None, + } = comparator.node + { + if matches!(op, Cmpop::Eq) { + let mut check = Check::new( + CheckKind::TrueFalseComparison(value, RejectedCmpop::Eq), + Range::from_located(comparator), + ); + if checker.patch() { + check.amend(Fix::dummy(expr.location)); + bad_ops.insert(idx, Cmpop::Is); + } + checks.push(check); + } + if matches!(op, Cmpop::NotEq) { + let mut check = Check::new( + CheckKind::TrueFalseComparison(value, RejectedCmpop::NotEq), + Range::from_located(comparator), + ); + if checker.patch() { + check.amend(Fix::dummy(expr.location)); + bad_ops.insert(idx, Cmpop::IsNot); + } + checks.push(check); + } + } + } + } + + if !bad_ops.is_empty() { + // Replace the entire comparison expression. + let ops = ops + .iter() + .enumerate() + .map(|(idx, op)| bad_ops.get(&idx).unwrap_or(op)) + .cloned() + .collect::>(); + if let Some(content) = compare(left, &ops, comparators) { + if let Some(check) = checks.last_mut() { + check.fix = Some(Fix::replacement( + content, + expr.location, + expr.end_location.unwrap(), + )); + } + } + } + + checker.add_checks(checks.into_iter()); +} diff --git a/src/snapshots/ruff__linter__tests__E711_E711.py.snap b/src/snapshots/ruff__linter__tests__E711_E711.py.snap index 70c7f522a7..e99a697425 100644 --- a/src/snapshots/ruff__linter__tests__E711_E711.py.snap +++ b/src/snapshots/ruff__linter__tests__E711_E711.py.snap @@ -10,7 +10,16 @@ expression: checks end_location: row: 2 column: 14 - fix: ~ + fix: + patch: + content: res is None + location: + row: 2 + column: 3 + end_location: + row: 2 + column: 14 + applied: false - kind: NoneComparison: NotEq location: @@ -19,7 +28,16 @@ expression: checks end_location: row: 5 column: 14 - fix: ~ + fix: + patch: + content: res is not None + location: + row: 5 + column: 3 + end_location: + row: 5 + column: 14 + applied: false - kind: NoneComparison: Eq location: @@ -28,7 +46,16 @@ expression: checks end_location: row: 8 column: 7 - fix: ~ + fix: + patch: + content: None is res + location: + row: 8 + column: 3 + end_location: + row: 8 + column: 14 + applied: false - kind: NoneComparison: NotEq location: @@ -37,7 +64,16 @@ expression: checks end_location: row: 11 column: 7 - fix: ~ + fix: + patch: + content: None is not res + location: + row: 11 + column: 3 + end_location: + row: 11 + column: 14 + applied: false - kind: NoneComparison: Eq location: @@ -46,7 +82,16 @@ expression: checks end_location: row: 14 column: 17 - fix: ~ + fix: + patch: + content: "res[1] is None" + location: + row: 14 + column: 3 + end_location: + row: 14 + column: 17 + applied: false - kind: NoneComparison: NotEq location: @@ -55,7 +100,16 @@ expression: checks end_location: row: 17 column: 17 - fix: ~ + fix: + patch: + content: "res[1] is not None" + location: + row: 17 + column: 3 + end_location: + row: 17 + column: 17 + applied: false - kind: NoneComparison: NotEq location: @@ -64,7 +118,16 @@ expression: checks end_location: row: 20 column: 7 - fix: ~ + fix: + patch: + content: "None is not res[1]" + location: + row: 20 + column: 3 + end_location: + row: 20 + column: 17 + applied: false - kind: NoneComparison: Eq location: @@ -73,5 +136,50 @@ expression: checks end_location: row: 23 column: 7 - fix: ~ + fix: + patch: + content: "None is res[1]" + location: + row: 23 + column: 3 + end_location: + row: 23 + column: 17 + applied: false +- kind: + NoneComparison: Eq + location: + row: 26 + column: 8 + end_location: + row: 26 + column: 12 + fix: + patch: + content: "" + location: + row: 26 + column: 3 + end_location: + row: 26 + column: 3 + applied: false +- kind: + NoneComparison: NotEq + location: + row: 26 + column: 16 + end_location: + row: 26 + column: 20 + fix: + patch: + content: x is None is not None + location: + row: 26 + column: 3 + end_location: + row: 26 + column: 20 + applied: false diff --git a/src/snapshots/ruff__linter__tests__E712_E712.py.snap b/src/snapshots/ruff__linter__tests__E712_E712.py.snap index fcc0c5feaa..a46cf536e0 100644 --- a/src/snapshots/ruff__linter__tests__E712_E712.py.snap +++ b/src/snapshots/ruff__linter__tests__E712_E712.py.snap @@ -1,5 +1,6 @@ --- source: src/linter.rs +assertion_line: 531 expression: checks --- - kind: @@ -12,7 +13,16 @@ expression: checks end_location: row: 2 column: 14 - fix: ~ + fix: + patch: + content: res is True + location: + row: 2 + column: 3 + end_location: + row: 2 + column: 14 + applied: false - kind: TrueFalseComparison: - false @@ -23,7 +33,16 @@ expression: checks end_location: row: 5 column: 15 - fix: ~ + fix: + patch: + content: res is not False + location: + row: 5 + column: 3 + end_location: + row: 5 + column: 15 + applied: false - kind: TrueFalseComparison: - true @@ -34,7 +53,16 @@ expression: checks end_location: row: 8 column: 7 - fix: ~ + fix: + patch: + content: True is not res + location: + row: 8 + column: 3 + end_location: + row: 8 + column: 14 + applied: false - kind: TrueFalseComparison: - false @@ -45,7 +73,16 @@ expression: checks end_location: row: 11 column: 8 - fix: ~ + fix: + patch: + content: False is res + location: + row: 11 + column: 3 + end_location: + row: 11 + column: 15 + applied: false - kind: TrueFalseComparison: - true @@ -56,7 +93,16 @@ expression: checks end_location: row: 14 column: 17 - fix: ~ + fix: + patch: + content: "res[1] is True" + location: + row: 14 + column: 3 + end_location: + row: 14 + column: 17 + applied: false - kind: TrueFalseComparison: - false @@ -67,7 +113,16 @@ expression: checks end_location: row: 17 column: 18 - fix: ~ + fix: + patch: + content: "res[1] is not False" + location: + row: 17 + column: 3 + end_location: + row: 17 + column: 18 + applied: false - kind: TrueFalseComparison: - true @@ -78,7 +133,16 @@ expression: checks end_location: row: 20 column: 23 - fix: ~ + fix: + patch: + content: cond is True + location: + row: 20 + column: 11 + end_location: + row: 20 + column: 23 + applied: false - kind: TrueFalseComparison: - false @@ -89,7 +153,16 @@ expression: checks end_location: row: 20 column: 48 - fix: ~ + fix: + patch: + content: cond is False + location: + row: 20 + column: 35 + end_location: + row: 20 + column: 48 + applied: false - kind: TrueFalseComparison: - true @@ -100,5 +173,54 @@ expression: checks end_location: row: 22 column: 8 - fix: ~ + fix: + patch: + content: True is TrueElement + location: + row: 22 + column: 3 + end_location: + row: 22 + column: 24 + applied: false +- kind: + TrueFalseComparison: + - true + - Eq + location: + row: 25 + column: 10 + end_location: + row: 25 + column: 14 + fix: + patch: + content: "" + location: + row: 25 + column: 3 + end_location: + row: 25 + column: 3 + applied: false +- kind: + TrueFalseComparison: + - false + - NotEq + location: + row: 25 + column: 18 + end_location: + row: 25 + column: 23 + fix: + patch: + content: res is True is not False + location: + row: 25 + column: 3 + end_location: + row: 25 + column: 23 + applied: false