Fix SIM222 and SIM223 false negative

crates/ruff/resources/test/fixtures/flake8_simplify/SIM222.py

@@ -1,10 +1,10 @@
-if a or True:  # SIM223
+if a or True:  # SIM222
     pass
 
-if (a or b) or True:  # SIM223
+if (a or b) or True:  # SIM222
     pass
 
-if a or (b or True):  # SIM223
+if a or (b or True):  # SIM222
     pass
 
 if a and True:  # OK
@@ -16,3 +16,16 @@
 
 def validate(self, value):
     return json.loads(value) or True  # OK
+
+
+if a or f() or b or g() or True:  # OK
+    pass
+
+if a or f() or True or g() or b:  # SIM222
+    pass
+
+if True or f() or a or g() or b:  # SIM222
+    pass
+
+if a or True or f() or b or g():  # SIM222
+    pass

crates/ruff/resources/test/fixtures/flake8_simplify/SIM223.py

@@ -12,3 +12,15 @@
 
 if False:
     pass
+
+if a and f() and b and g() and False:  # OK
+    pass
+
+if a and f() and False and g() and b:  # SIM222
+    pass
+
+if False and f() and a and g() and b:  # SIM222
+    pass
+
+if a and False and f() and b and g():  # SIM222
+    pass

crates/ruff/src/rules/flake8_simplify/rules/ast_bool_op.rs

@@ -2,7 +2,11 @@ use std::collections::BTreeMap;
 use std::iter;
 
 use itertools::Either::{Left, Right};
-use rustpython_parser::ast::{Boolop, Cmpop, Constant, Expr, ExprContext, ExprKind, Unaryop};
+use itertools::Itertools;
+use ruff_python_ast::context::Context;
+use rustpython_parser::ast::{
+    Boolop, Cmpop, Constant, Expr, ExprContext, ExprKind, Location, Unaryop,
+};
 
 use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit};
 use ruff_macros::{derive_message_formats, violation};
@@ -475,56 +479,92 @@ pub fn expr_or_not_expr(checker: &mut Checker, expr: &Expr) {
     }
 }
 
-/// SIM222
-pub fn expr_or_true(checker: &mut Checker, expr: &Expr) {
-    let ExprKind::BoolOp { op: Boolop::Or, values, } = &expr.node else {
-        return;
+pub fn is_short_circuit(ctx: &Context, expr: &Expr) -> Option<(Location, Location)> {
+    let ExprKind::BoolOp { op, values, } = &expr.node else {
+        return None;
     };
-    if contains_effect(&checker.ctx, expr) {
-        return;
-    }
-    for value in values {
+    let short_circuit_value = match op {
+        Boolop::And => false,
+        Boolop::Or => true,
+    };
+
+    let mut location = expr.location;
+    for (value, next_value) in values.iter().tuple_windows() {
+        // Keep track of the location of the furthest-right, non-effectful expression.
+        if contains_effect(ctx, value) {
+            location = next_value.location;
+            continue;
+        }
+
+        // If the current expression is a constant, and it matches the short-circuit value, then
+        // we can return the location of the expression. This should only trigger if the
+        // short-circuit expression is the first expression in the list; otherwise, we'll see it
+        // as `next_value` before we see it as `value`.
         if let ExprKind::Constant {
-            value: Constant::Bool(true),
+            value: Constant::Bool(bool),
             ..
         } = &value.node
         {
-            let mut diagnostic = Diagnostic::new(ExprOrTrue, Range::from(value));
-            if checker.patch(diagnostic.kind.rule()) {
-                diagnostic.set_fix(Edit::replacement(
-                    "True".to_string(),
-                    expr.location,
-                    expr.end_location.unwrap(),
-                ));
+            if bool == &short_circuit_value {
+                return Some((location, expr.end_location.unwrap()));
             }
-            checker.diagnostics.push(diagnostic);
         }
-    }
-}
 
-/// SIM223
-pub fn expr_and_false(checker: &mut Checker, expr: &Expr) {
-    let ExprKind::BoolOp { op: Boolop::And, values, } = &expr.node else {
-        return;
-    };
-    if contains_effect(&checker.ctx, expr) {
-        return;
-    }
-    for value in values {
+        // If the next expression is a constant, and it matches the short-circuit value, then
+        // we can return the location of the expression.
         if let ExprKind::Constant {
-            value: Constant::Bool(false),
+            value: Constant::Bool(bool),
             ..
-        } = &value.node
+        } = &next_value.node
         {
-            let mut diagnostic = Diagnostic::new(ExprAndFalse, Range::from(value));
-            if checker.patch(diagnostic.kind.rule()) {
-                diagnostic.set_fix(Edit::replacement(
-                    "False".to_string(),
-                    expr.location,
-                    expr.end_location.unwrap(),
-                ));
+            if bool == &short_circuit_value {
+                return Some((location, expr.end_location.unwrap()));
             }
-            checker.diagnostics.push(diagnostic);
         }
     }
+    None
+}
+
+/// SIM222
+pub fn expr_or_true(checker: &mut Checker, expr: &Expr) {
+    let Some((location, end_location)) = is_short_circuit(&checker.ctx, expr) else {
+        return;
+    };
+    let mut diagnostic = Diagnostic::new(
+        ExprOrTrue,
+        Range {
+            location,
+            end_location,
+        },
+    );
+    if checker.patch(diagnostic.kind.rule()) {
+        diagnostic.set_fix(Edit::replacement(
+            "True".to_string(),
+            location,
+            end_location,
+        ));
+    }
+    checker.diagnostics.push(diagnostic);
+}
+
+/// SIM223
+pub fn expr_and_false(checker: &mut Checker, expr: &Expr) {
+    let Some((location, end_location)) = is_short_circuit(&checker.ctx, expr) else {
+        return;
+    };
+    let mut diagnostic = Diagnostic::new(
+        ExprAndFalse,
+        Range {
+            location,
+            end_location,
+        },
+    );
+    if checker.patch(diagnostic.kind.rule()) {
+        diagnostic.set_fix(Edit::replacement(
+            "False".to_string(),
+            location,
+            end_location,
+        ));
+    }
+    checker.diagnostics.push(diagnostic);
 }

crates/ruff/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM222_SIM222.py.snap

@@ -9,7 +9,7 @@ expression: diagnostics
     fixable: true
   location:
     row: 1
-    column: 8
+    column: 3
   end_location:
     row: 1
     column: 12
@@ -29,7 +29,7 @@ expression: diagnostics
     fixable: true
   location:
     row: 4
-    column: 15
+    column: 3
   end_location:
     row: 4
     column: 19
@@ -49,7 +49,7 @@ expression: diagnostics
     fixable: true
   location:
     row: 7
-    column: 14
+    column: 9
   end_location:
     row: 7
     column: 18
@@ -62,4 +62,64 @@ expression: diagnostics
       row: 7
       column: 18
   parent: ~
+- kind:
+    name: ExprOrTrue
+    body: "Use `True` instead of `... or True`"
+    suggestion: "Replace with `True`"
+    fixable: true
+  location:
+    row: 24
+    column: 15
+  end_location:
+    row: 24
+    column: 31
+  fix:
+    content: "True"
+    location:
+      row: 24
+      column: 15
+    end_location:
+      row: 24
+      column: 31
+  parent: ~
+- kind:
+    name: ExprOrTrue
+    body: "Use `True` instead of `... or True`"
+    suggestion: "Replace with `True`"
+    fixable: true
+  location:
+    row: 27
+    column: 3
+  end_location:
+    row: 27
+    column: 31
+  fix:
+    content: "True"
+    location:
+      row: 27
+      column: 3
+    end_location:
+      row: 27
+      column: 31
+  parent: ~
+- kind:
+    name: ExprOrTrue
+    body: "Use `True` instead of `... or True`"
+    suggestion: "Replace with `True`"
+    fixable: true
+  location:
+    row: 30
+    column: 3
+  end_location:
+    row: 30
+    column: 31
+  fix:
+    content: "True"
+    location:
+      row: 30
+      column: 3
+    end_location:
+      row: 30
+      column: 31
+  parent: ~
 

crates/ruff/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM223_SIM223.py.snap

@@ -9,7 +9,7 @@ expression: diagnostics
     fixable: true
   location:
     row: 1
-    column: 9
+    column: 3
   end_location:
     row: 1
     column: 14
@@ -29,7 +29,7 @@ expression: diagnostics
     fixable: true
   location:
     row: 4
-    column: 16
+    column: 3
   end_location:
     row: 4
     column: 21
@@ -49,7 +49,7 @@ expression: diagnostics
     fixable: true
   location:
     row: 7
-    column: 15
+    column: 9
   end_location:
     row: 7
     column: 20
@@ -62,4 +62,64 @@ expression: diagnostics
       row: 7
       column: 20
   parent: ~
+- kind:
+    name: ExprAndFalse
+    body: "Use `False` instead of `... and False`"
+    suggestion: "Replace with `False`"
+    fixable: true
+  location:
+    row: 19
+    column: 17
+  end_location:
+    row: 19
+    column: 36
+  fix:
+    content: "False"
+    location:
+      row: 19
+      column: 17
+    end_location:
+      row: 19
+      column: 36
+  parent: ~
+- kind:
+    name: ExprAndFalse
+    body: "Use `False` instead of `... and False`"
+    suggestion: "Replace with `False`"
+    fixable: true
+  location:
+    row: 22
+    column: 3
+  end_location:
+    row: 22
+    column: 36
+  fix:
+    content: "False"
+    location:
+      row: 22
+      column: 3
+    end_location:
+      row: 22
+      column: 36
+  parent: ~
+- kind:
+    name: ExprAndFalse
+    body: "Use `False` instead of `... and False`"
+    suggestion: "Replace with `False`"
+    fixable: true
+  location:
+    row: 25
+    column: 3
+  end_location:
+    row: 25
+    column: 36
+  fix:
+    content: "False"
+    location:
+      row: 25
+      column: 3
+    end_location:
+      row: 25
+      column: 36
+  parent: ~