Avoid suggesting 'is' for constant literals (#3146)

astral-sh · Feb 22, 2023 · 726adb7 · 726adb7
1 parent dbdfdeb
commit 726adb7
Show file tree

Hide file tree

Showing 3 changed files with 58 additions and 110 deletions.
diff --git a/crates/ruff/resources/test/fixtures/pycodestyle/constant_literals.py b/crates/ruff/resources/test/fixtures/pycodestyle/constant_literals.py
@@ -17,20 +17,16 @@
     pass
 
 ###
-# Unfixable errors
+# Non-errors
 ###
-if "abc" == None:  # E711
+if "abc" == None:
     pass
-if None == "abc":  # E711
+if None == "abc":
     pass
-if "abc" == False:  # E712
+if "abc" == False:
     pass
-if False == "abc":  # E712
+if False == "abc":
     pass
-
-###
-# Non-errors
-###
 if "def" == "abc":
     pass
 if False is None:

diff --git a/crates/ruff/src/rules/pycodestyle/rules/literal_comparisons.rs b/crates/ruff/src/rules/pycodestyle/rules/literal_comparisons.rs
@@ -102,68 +102,72 @@ pub fn literal_comparisons(
     // Check `left`.
     let mut comparator = left;
     let next = &comparators[0];
-    if check_none_comparisons
-        && matches!(
-            comparator.node,
-            ExprKind::Constant {
-                value: Constant::None,
-                kind: None
-            }
-        )
-    {
-        if matches!(op, Cmpop::Eq) {
-            let diagnostic =
-                Diagnostic::new(NoneComparison(op.into()), Range::from_located(comparator));
-            if checker.patch(diagnostic.kind.rule()) && !helpers::is_constant_non_singleton(next) {
-                bad_ops.insert(0, Cmpop::Is);
-            }
-            diagnostics.push(diagnostic);
-        }
-        if matches!(op, Cmpop::NotEq) {
-            let diagnostic =
-                Diagnostic::new(NoneComparison(op.into()), Range::from_located(comparator));
-            if checker.patch(diagnostic.kind.rule()) && !helpers::is_constant_non_singleton(next) {
-                bad_ops.insert(0, Cmpop::IsNot);
-            }
-            diagnostics.push(diagnostic);
-        }
-    }
 
-    if check_true_false_comparisons {
-        if let ExprKind::Constant {
-            value: Constant::Bool(value),
-            kind: None,
-        } = comparator.node
+    if !helpers::is_constant_non_singleton(next) {
+        if check_none_comparisons
+            && matches!(
+                comparator.node,
+                ExprKind::Constant {
+                    value: Constant::None,
+                    kind: None
+                }
+            )
         {
             if matches!(op, Cmpop::Eq) {
-                let diagnostic = Diagnostic::new(
-                    TrueFalseComparison(value, op.into()),
-                    Range::from_located(comparator),
-                );
-                if checker.patch(diagnostic.kind.rule())
-                    && !helpers::is_constant_non_singleton(next)
-                {
+                let diagnostic =
+                    Diagnostic::new(NoneComparison(op.into()), Range::from_located(comparator));
+                if checker.patch(diagnostic.kind.rule()) {
                     bad_ops.insert(0, Cmpop::Is);
                 }
                 diagnostics.push(diagnostic);
             }
             if matches!(op, Cmpop::NotEq) {
-                let diagnostic = Diagnostic::new(
-                    TrueFalseComparison(value, op.into()),
-                    Range::from_located(comparator),
-                );
-                if checker.patch(diagnostic.kind.rule())
-                    && !helpers::is_constant_non_singleton(next)
-                {
+                let diagnostic =
+                    Diagnostic::new(NoneComparison(op.into()), Range::from_located(comparator));
+                if checker.patch(diagnostic.kind.rule()) {
                     bad_ops.insert(0, Cmpop::IsNot);
                 }
                 diagnostics.push(diagnostic);
             }
         }
+
+        if check_true_false_comparisons {
+            if let ExprKind::Constant {
+                value: Constant::Bool(value),
+                kind: None,
+            } = comparator.node
+            {
+                if matches!(op, Cmpop::Eq) {
+                    let diagnostic = Diagnostic::new(
+                        TrueFalseComparison(value, op.into()),
+                        Range::from_located(comparator),
+                    );
+                    if checker.patch(diagnostic.kind.rule()) {
+                        bad_ops.insert(0, Cmpop::Is);
+                    }
+                    diagnostics.push(diagnostic);
+                }
+                if matches!(op, Cmpop::NotEq) {
+                    let diagnostic = Diagnostic::new(
+                        TrueFalseComparison(value, op.into()),
+                        Range::from_located(comparator),
+                    );
+                    if checker.patch(diagnostic.kind.rule()) {
+                        bad_ops.insert(0, Cmpop::IsNot);
+                    }
+                    diagnostics.push(diagnostic);
+                }
+            }
+        }
     }
 
     // Check each comparator in order.
     for (idx, (op, next)) in izip!(ops, comparators).enumerate() {
+        if helpers::is_constant_non_singleton(comparator) {
+            comparator = next;
+            continue;
+        }
+
         if check_none_comparisons
             && matches!(
                 next.node,
@@ -176,19 +180,15 @@ pub fn literal_comparisons(
             if matches!(op, Cmpop::Eq) {
                 let diagnostic =
                     Diagnostic::new(NoneComparison(op.into()), Range::from_located(next));
-                if checker.patch(diagnostic.kind.rule())
-                    && !helpers::is_constant_non_singleton(comparator)
-                {
+                if checker.patch(diagnostic.kind.rule()) {
                     bad_ops.insert(idx, Cmpop::Is);
                 }
                 diagnostics.push(diagnostic);
             }
             if matches!(op, Cmpop::NotEq) {
                 let diagnostic =
                     Diagnostic::new(NoneComparison(op.into()), Range::from_located(next));
-                if checker.patch(diagnostic.kind.rule())
-                    && !helpers::is_constant_non_singleton(comparator)
-                {
+                if checker.patch(diagnostic.kind.rule()) {
                     bad_ops.insert(idx, Cmpop::IsNot);
                 }
                 diagnostics.push(diagnostic);
@@ -206,9 +206,7 @@ pub fn literal_comparisons(
                         TrueFalseComparison(value, op.into()),
                         Range::from_located(next),
                     );
-                    if checker.patch(diagnostic.kind.rule())
-                        && !helpers::is_constant_non_singleton(comparator)
-                    {
+                    if checker.patch(diagnostic.kind.rule()) {
                         bad_ops.insert(idx, Cmpop::Is);
                     }
                     diagnostics.push(diagnostic);
@@ -218,9 +216,7 @@ pub fn literal_comparisons(
                         TrueFalseComparison(value, op.into()),
                         Range::from_located(next),
                     );
-                    if checker.patch(diagnostic.kind.rule())
-                        && !helpers::is_constant_non_singleton(comparator)
-                    {
+                    if checker.patch(diagnostic.kind.rule()) {
                         bad_ops.insert(idx, Cmpop::IsNot);
                     }
                     diagnostics.push(diagnostic);

diff --git a/...f/src/rules/pycodestyle/snapshots/ruff__rules__pycodestyle__tests__constant_literals.snap b/...f/src/rules/pycodestyle/snapshots/ruff__rules__pycodestyle__tests__constant_literals.snap
@@ -164,48 +164,4 @@ expression: diagnostics
       row: 16
       column: 16
   parent: ~
-- kind:
-    NoneComparison: Eq
-  location:
-    row: 22
-    column: 12
-  end_location:
-    row: 22
-    column: 16
-  fix: ~
-  parent: ~
-- kind:
-    NoneComparison: Eq
-  location:
-    row: 24
-    column: 3
-  end_location:
-    row: 24
-    column: 7
-  fix: ~
-  parent: ~
-- kind:
-    TrueFalseComparison:
-      - false
-      - Eq
-  location:
-    row: 26
-    column: 12
-  end_location:
-    row: 26
-    column: 17
-  fix: ~
-  parent: ~
-- kind:
-    TrueFalseComparison:
-      - false
-      - Eq
-  location:
-    row: 28
-    column: 3
-  end_location:
-    row: 28
-    column: 8
-  fix: ~
-  parent: ~