Implement auto-fix for E711 and E712 (#784)

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` |  |
+| E711 | NoneComparison | Comparison to `None` should be `cond is None` | 🛠 |
-| E712 | TrueFalseComparison | Comparison to `True` should be `cond is True` |  |
+| 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()` |  |

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

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

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,
+        }
+    }
 }

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::plugins::literal_comparisons(
-                        pycodestyle::checks::literal_comparisons(
+                        self,
-                            left,
+                        expr,
-                            ops,
+                        left,
-                            comparators,
+                        ops,
-                            check_none_comparisons,
+                        comparators,
-                            check_true_false_comparisons,
+                        check_none_comparisons,
-                        )
+                        check_true_false_comparisons,
-                        .into_iter(),
+                    )
-                    );
                 }
 
                 if self.settings.enabled.contains(&CheckCode::F632) {

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<Check> {
-    let mut checks: Vec<Check> = 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<Check> {
     let mut checks: Vec<Check> = vec![];

src/pycodestyle/mod.rs

@@ -1 +1,2 @@
 pub mod checks;
+pub mod plugins;

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<String> {
+    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<usize, Cmpop> = FnvHashMap::default();
+    let mut checks: Vec<Check> = 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::<Vec<_>>();
+        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());
+}

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
 

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