Avoid fixing E711 and E712 issues that would cause F632 (#1248)

resources/test/fixtures/pycodestyle/constant_literals.py

@@ -0,0 +1,39 @@
+###
+# Errors
+###
+if "abc" is "def":  # F632 (fix)
+    pass
+if "abc" is None:  # F632 (fix, but leaves behind unfixable E711)
+    pass
+if None is "abc":  # F632 (fix, but leaves behind unfixable E711)
+    pass
+if "abc" is False:  # F632 (fix, but leaves behind unfixable E712)
+    pass
+if False is "abc":  # F632 (fix, but leaves behind unfixable E712)
+    pass
+if False == None:  # E711, E712 (fix)
+    pass
+if None == False:  # E711, E712 (fix)
+    pass
+
+###
+# Unfixable errors
+###
+if "abc" == None:  # E711
+    pass
+if None == "abc":  # E711
+    pass
+if "abc" == False:  # E712
+    pass
+if False == "abc":  # E712
+    pass
+
+###
+# Non-errors
+###
+if "def" == "abc":
+    pass
+if False is None:
+    pass
+if None is False:
+    pass

src/ast/helpers.rs

@@ -3,7 +3,7 @@ use once_cell::sync::Lazy;
 use regex::Regex;
 use rustc_hash::{FxHashMap, FxHashSet};
 use rustpython_ast::{
-    Arguments, Excepthandler, ExcepthandlerKind, Expr, ExprKind, Location, Stmt, StmtKind,
+    Arguments, Constant, Excepthandler, ExcepthandlerKind, Expr, ExprKind, Location, Stmt, StmtKind,
 };
 use rustpython_parser::lexer;
 use rustpython_parser::lexer::Tok;
@@ -152,15 +152,12 @@ pub fn match_call_path(
 
 static DUNDER_REGEX: Lazy<Regex> = Lazy::new(|| Regex::new(r"__[^\s]+__").unwrap());
 
-pub fn is_assignment_to_a_dunder(node: &StmtKind) -> bool {
+/// Return `true` if the `Stmt` is an assignment to a dunder (like `__all__`).
+pub fn is_assignment_to_a_dunder(stmt: &Stmt) -> bool {
     // Check whether it's an assignment to a dunder, with or without a type
     // annotation. This is what pycodestyle (as of 2.9.1) does.
-    match node {
+    match &stmt.node {
-        StmtKind::Assign {
+        StmtKind::Assign { targets, .. } => {
-            targets,
-            value: _,
-            type_comment: _,
-        } => {
             if targets.len() != 1 {
                 return false;
             }
@@ -169,12 +166,7 @@ pub fn is_assignment_to_a_dunder(node: &StmtKind) -> bool {
                 _ => false,
             }
         }
-        StmtKind::AnnAssign {
+        StmtKind::AnnAssign { target, .. } => match &target.node {
-            target,
-            annotation: _,
-            value: _,
-            simple: _,
-        } => match &target.node {
             ExprKind::Name { id, ctx: _ } => DUNDER_REGEX.is_match(id),
             _ => false,
         },
@@ -182,6 +174,32 @@ pub fn is_assignment_to_a_dunder(node: &StmtKind) -> bool {
     }
 }
 
+/// Return `true` if the `Expr` is a singleton (`None`, `True`, `False`, or
+/// `...`).
+pub fn is_singleton(expr: &Expr) -> bool {
+    matches!(
+        expr.node,
+        ExprKind::Constant {
+            value: Constant::None | Constant::Bool(_) | Constant::Ellipsis,
+            ..
+        }
+    )
+}
+
+/// Return `true` if the `Expr` is a constant or tuple of constants.
+pub fn is_constant(expr: &Expr) -> bool {
+    match &expr.node {
+        ExprKind::Constant { .. } => true,
+        ExprKind::Tuple { elts, .. } => elts.iter().all(is_constant),
+        _ => false,
+    }
+}
+
+/// Return `true` if the `Expr` is a non-singleton constant.
+pub fn is_constant_non_singleton(expr: &Expr) -> bool {
+    is_constant(expr) && !is_singleton(expr)
+}
+
 /// Extract the names of all handled exceptions.
 pub fn extract_handler_names(handlers: &[Excepthandler]) -> Vec<Vec<&str>> {
     let mut handler_names = vec![];
@@ -232,7 +250,6 @@ pub fn collect_arg_names<'a>(arguments: &'a Arguments) -> FxHashSet<&'a str> {
 
 /// Returns `true` if a call is an argumented `super` invocation.
 pub fn is_super_call_with_arguments(func: &Expr, args: &[Expr]) -> bool {
-    // Check: is this a `super` call?
     if let ExprKind::Name { id, .. } = &func.node {
         id == "super" && !args.is_empty()
     } else {

src/check_ast.rs

@@ -223,10 +223,10 @@ where
             StmtKind::Import { .. } => {
                 self.futures_allowed = false;
             }
-            node => {
+            _ => {
                 self.futures_allowed = false;
                 if !self.seen_import_boundary
-                    && !helpers::is_assignment_to_a_dunder(node)
+                    && !helpers::is_assignment_to_a_dunder(stmt)
                     && !operations::in_nested_block(
                         &mut self.parents.iter().rev().map(|node| node.0),
                     )

src/pycodestyle/mod.rs

@@ -44,4 +44,16 @@ mod tests {
         insta::assert_yaml_snapshot!(snapshot, checks);
         Ok(())
     }
+
+    #[test]
+    fn constant_literals() -> Result<()> {
+        let mut checks = test_path(
+            Path::new("./resources/test/fixtures/pycodestyle/constant_literals.py"),
+            &settings::Settings::for_rules(vec![CheckCode::E711, CheckCode::E712, CheckCode::F632]),
+            true,
+        )?;
+        checks.sort_by_key(|check| check.location);
+        insta::assert_yaml_snapshot!(checks);
+        Ok(())
+    }
 }

src/pycodestyle/plugins.rs

@@ -5,6 +5,7 @@ use rustc_hash::FxHashMap;
 use rustpython_ast::{Arguments, Location, StmtKind};
 use rustpython_parser::ast::{Cmpop, Constant, Expr, ExprKind, Stmt, Unaryop};
 
+use crate::ast::helpers;
 use crate::ast::helpers::{match_leading_content, match_trailing_content};
 use crate::ast::types::Range;
 use crate::ast::whitespace::leading_space;
@@ -46,9 +47,10 @@ pub fn literal_comparisons(
     let mut checks: Vec<Check> = vec![];
 
     let op = ops.first().unwrap();
-    let comparator = left;
 
     // Check `left`.
+    let mut comparator = left;
+    let next = &comparators[0];
     if check_none_comparisons
         && matches!(
             comparator.node,
@@ -63,7 +65,7 @@ pub fn literal_comparisons(
                 CheckKind::NoneComparison(RejectedCmpop::Eq),
                 Range::from_located(comparator),
             );
-            if checker.patch(check.kind.code()) {
+            if checker.patch(check.kind.code()) && !helpers::is_constant_non_singleton(next) {
                 bad_ops.insert(0, Cmpop::Is);
             }
             checks.push(check);
@@ -73,7 +75,7 @@ pub fn literal_comparisons(
                 CheckKind::NoneComparison(RejectedCmpop::NotEq),
                 Range::from_located(comparator),
             );
-            if checker.patch(check.kind.code()) {
+            if checker.patch(check.kind.code()) && !helpers::is_constant_non_singleton(next) {
                 bad_ops.insert(0, Cmpop::IsNot);
             }
             checks.push(check);
@@ -91,7 +93,7 @@ pub fn literal_comparisons(
                     CheckKind::TrueFalseComparison(value, RejectedCmpop::Eq),
                     Range::from_located(comparator),
                 );
-                if checker.patch(check.kind.code()) {
+                if checker.patch(check.kind.code()) && !helpers::is_constant_non_singleton(next) {
                     bad_ops.insert(0, Cmpop::Is);
                 }
                 checks.push(check);
@@ -101,7 +103,7 @@ pub fn literal_comparisons(
                     CheckKind::TrueFalseComparison(value, RejectedCmpop::NotEq),
                     Range::from_located(comparator),
                 );
-                if checker.patch(check.kind.code()) {
+                if checker.patch(check.kind.code()) && !helpers::is_constant_non_singleton(next) {
                     bad_ops.insert(0, Cmpop::IsNot);
                 }
                 checks.push(check);
@@ -110,10 +112,10 @@ pub fn literal_comparisons(
     }
 
     // Check each comparator in order.
-    for (idx, (op, comparator)) in izip!(ops, comparators).enumerate() {
+    for (idx, (op, next)) in izip!(ops, comparators).enumerate() {
         if check_none_comparisons
             && matches!(
-                comparator.node,
+                next.node,
                 ExprKind::Constant {
                     value: Constant::None,
                     kind: None
@@ -123,9 +125,11 @@ pub fn literal_comparisons(
             if matches!(op, Cmpop::Eq) {
                 let check = Check::new(
                     CheckKind::NoneComparison(RejectedCmpop::Eq),
-                    Range::from_located(comparator),
+                    Range::from_located(next),
                 );
-                if checker.patch(check.kind.code()) {
+                if checker.patch(check.kind.code())
+                    && !helpers::is_constant_non_singleton(comparator)
+                {
                     bad_ops.insert(idx, Cmpop::Is);
                 }
                 checks.push(check);
@@ -133,9 +137,11 @@ pub fn literal_comparisons(
             if matches!(op, Cmpop::NotEq) {
                 let check = Check::new(
                     CheckKind::NoneComparison(RejectedCmpop::NotEq),
-                    Range::from_located(comparator),
+                    Range::from_located(next),
                 );
-                if checker.patch(check.kind.code()) {
+                if checker.patch(check.kind.code())
+                    && !helpers::is_constant_non_singleton(comparator)
+                {
                     bad_ops.insert(idx, Cmpop::IsNot);
                 }
                 checks.push(check);
@@ -146,14 +152,16 @@ pub fn literal_comparisons(
             if let ExprKind::Constant {
                 value: Constant::Bool(value),
                 kind: None,
-            } = comparator.node
+            } = next.node
             {
                 if matches!(op, Cmpop::Eq) {
                     let check = Check::new(
                         CheckKind::TrueFalseComparison(value, RejectedCmpop::Eq),
-                        Range::from_located(comparator),
+                        Range::from_located(next),
                     );
-                    if checker.patch(check.kind.code()) {
+                    if checker.patch(check.kind.code())
+                        && !helpers::is_constant_non_singleton(comparator)
+                    {
                         bad_ops.insert(idx, Cmpop::Is);
                     }
                     checks.push(check);
@@ -161,15 +169,19 @@ pub fn literal_comparisons(
                 if matches!(op, Cmpop::NotEq) {
                     let check = Check::new(
                         CheckKind::TrueFalseComparison(value, RejectedCmpop::NotEq),
-                        Range::from_located(comparator),
+                        Range::from_located(next),
                     );
-                    if checker.patch(check.kind.code()) {
+                    if checker.patch(check.kind.code())
+                        && !helpers::is_constant_non_singleton(comparator)
+                    {
                         bad_ops.insert(idx, Cmpop::IsNot);
                     }
                     checks.push(check);
                 }
             }
         }
+
+        comparator = next;
     }
 
     if !bad_ops.is_empty() {

src/pycodestyle/snapshots/ruff__pycodestyle__tests__constant_literals.snap

@@ -0,0 +1,188 @@
+---
+source: src/pycodestyle/mod.rs
+expression: checks
+---
+- kind: IsLiteral
+  location:
+    row: 4
+    column: 3
+  end_location:
+    row: 4
+    column: 17
+  fix:
+    content: "=="
+    location:
+      row: 4
+      column: 9
+    end_location:
+      row: 4
+      column: 11
+- kind: IsLiteral
+  location:
+    row: 6
+    column: 3
+  end_location:
+    row: 6
+    column: 16
+  fix:
+    content: "=="
+    location:
+      row: 6
+      column: 9
+    end_location:
+      row: 6
+      column: 11
+- kind: IsLiteral
+  location:
+    row: 8
+    column: 3
+  end_location:
+    row: 8
+    column: 16
+  fix:
+    content: "=="
+    location:
+      row: 8
+      column: 8
+    end_location:
+      row: 8
+      column: 10
+- kind: IsLiteral
+  location:
+    row: 10
+    column: 3
+  end_location:
+    row: 10
+    column: 17
+  fix:
+    content: "=="
+    location:
+      row: 10
+      column: 9
+    end_location:
+      row: 10
+      column: 11
+- kind: IsLiteral
+  location:
+    row: 12
+    column: 3
+  end_location:
+    row: 12
+    column: 17
+  fix:
+    content: "=="
+    location:
+      row: 12
+      column: 9
+    end_location:
+      row: 12
+      column: 11
+- kind:
+    TrueFalseComparison:
+      - false
+      - Eq
+  location:
+    row: 14
+    column: 3
+  end_location:
+    row: 14
+    column: 8
+  fix:
+    content: False is None
+    location:
+      row: 14
+      column: 3
+    end_location:
+      row: 14
+      column: 16
+- kind:
+    NoneComparison: Eq
+  location:
+    row: 14
+    column: 12
+  end_location:
+    row: 14
+    column: 16
+  fix:
+    content: False is None
+    location:
+      row: 14
+      column: 3
+    end_location:
+      row: 14
+      column: 16
+- kind:
+    NoneComparison: Eq
+  location:
+    row: 16
+    column: 3
+  end_location:
+    row: 16
+    column: 7
+  fix:
+    content: None is False
+    location:
+      row: 16
+      column: 3
+    end_location:
+      row: 16
+      column: 16
+- kind:
+    TrueFalseComparison:
+      - false
+      - Eq
+  location:
+    row: 16
+    column: 11
+  end_location:
+    row: 16
+    column: 16
+  fix:
+    content: None is False
+    location:
+      row: 16
+      column: 3
+    end_location:
+      row: 16
+      column: 16
+- kind:
+    NoneComparison: Eq
+  location:
+    row: 22
+    column: 12
+  end_location:
+    row: 22
+    column: 16
+  fix: ~
+- kind:
+    NoneComparison: Eq
+  location:
+    row: 24
+    column: 3
+  end_location:
+    row: 24
+    column: 7
+  fix: ~
+- kind:
+    TrueFalseComparison:
+      - false
+      - Eq
+  location:
+    row: 26
+    column: 12
+  end_location:
+    row: 26
+    column: 17
+  fix: ~
+- kind:
+    TrueFalseComparison:
+      - false
+      - Eq
+  location:
+    row: 28
+    column: 3
+  end_location:
+    row: 28
+    column: 8
+  fix: ~
+

src/pyflakes/plugins/invalid_literal_comparisons.rs

@@ -1,6 +1,6 @@
 use itertools::izip;
 use once_cell::unsync::Lazy;
-use rustpython_ast::{Cmpop, Constant, Expr, ExprKind};
+use rustpython_ast::{Cmpop, Expr};
 
 use crate::ast::helpers;
 use crate::ast::operations::locate_cmpops;
@@ -9,28 +9,6 @@ use crate::autofix::Fix;
 use crate::check_ast::Checker;
 use crate::checks::{Check, CheckKind};
 
-fn is_singleton(expr: &Expr) -> bool {
-    matches!(
-        expr.node,
-        ExprKind::Constant {
-            value: Constant::None | Constant::Bool(_) | Constant::Ellipsis,
-            ..
-        }
-    )
-}
-
-fn is_constant(expr: &Expr) -> bool {
-    match &expr.node {
-        ExprKind::Constant { .. } => true,
-        ExprKind::Tuple { elts, .. } => elts.iter().all(is_constant),
-        _ => false,
-    }
-}
-
-fn is_constant_non_singleton(expr: &Expr) -> bool {
-    is_constant(expr) && !is_singleton(expr)
-}
-
 /// F632
 pub fn invalid_literal_comparison(
     checker: &mut Checker,
@@ -43,7 +21,8 @@ pub fn invalid_literal_comparison(
     let mut left = left;
     for (index, (op, right)) in izip!(ops, comparators).enumerate() {
         if matches!(op, Cmpop::Is | Cmpop::IsNot)
-            && (is_constant_non_singleton(left) || is_constant_non_singleton(right))
+            && (helpers::is_constant_non_singleton(left)
+                || helpers::is_constant_non_singleton(right))
         {
             let mut check = Check::new(CheckKind::IsLiteral, location);
             if checker.patch(check.kind.code()) {