Fix SIM222 and SIM223 false positive (#3832)

astral-sh · Mar 31, 2023 · fe38597 · fe38597
1 parent f3f9a9f
commit fe38597
Show file tree

Hide file tree

Showing 3 changed files with 39 additions and 6 deletions.
diff --git a/crates/ruff/resources/test/fixtures/flake8_simplify/SIM222.py b/crates/ruff/resources/test/fixtures/flake8_simplify/SIM222.py
@@ -29,3 +29,16 @@ def validate(self, value):
 
 if a or True or f() or b or g():  # SIM222
     pass
+
+
+if a and f() and b and g() and False:  # OK
+    pass
+
+if a and f() and False and g() and b:  # OK
+    pass
+
+if False and f() and a and g() and b:  # OK
+    pass
+
+if a and False and f() and b and g():  # OK
+    pass
diff --git a/crates/ruff/resources/test/fixtures/flake8_simplify/SIM223.py b/crates/ruff/resources/test/fixtures/flake8_simplify/SIM223.py
@@ -16,11 +16,24 @@
 if a and f() and b and g() and False:  # OK
     pass
 
-if a and f() and False and g() and b:  # SIM222
+if a and f() and False and g() and b:  # SIM223
     pass
 
-if False and f() and a and g() and b:  # SIM222
+if False and f() and a and g() and b:  # SIM223
     pass
 
-if a and False and f() and b and g():  # SIM222
+if a and False and f() and b and g():  # SIM223
+    pass
+
+
+if a or f() or b or g() or True:  # OK
+    pass
+
+if a or f() or True or g() or b:  # OK
+    pass
+
+if True or f() or a or g() or b:  # OK
+    pass
+
+if a or True or f() or b or g():  # OK
     pass
diff --git a/crates/ruff/src/rules/flake8_simplify/rules/ast_bool_op.rs b/crates/ruff/src/rules/flake8_simplify/rules/ast_bool_op.rs
@@ -503,10 +503,17 @@ pub fn expr_or_not_expr(checker: &mut Checker, expr: &Expr) {
     }
 }
 
-pub fn is_short_circuit(ctx: &Context, expr: &Expr) -> Option<(Location, Location)> {
+pub fn is_short_circuit(
+    ctx: &Context,
+    expr: &Expr,
+    expected_op: &Boolop,
+) -> Option<(Location, Location)> {
     let ExprKind::BoolOp { op, values, } = &expr.node else {
         return None;
     };
+    if op != expected_op {
+        return None;
+    }
     let short_circuit_value = match op {
         Boolop::And => false,
         Boolop::Or => true,
@@ -551,7 +558,7 @@ pub fn is_short_circuit(ctx: &Context, expr: &Expr) -> Option<(Location, Locatio
 
 /// SIM222
 pub fn expr_or_true(checker: &mut Checker, expr: &Expr) {
-    let Some((location, end_location)) = is_short_circuit(&checker.ctx, expr) else {
+    let Some((location, end_location)) = is_short_circuit(&checker.ctx, expr, &Boolop::Or) else {
         return;
     };
     let mut diagnostic = Diagnostic::new(
@@ -573,7 +580,7 @@ pub fn expr_or_true(checker: &mut Checker, expr: &Expr) {
 
 /// SIM223
 pub fn expr_and_false(checker: &mut Checker, expr: &Expr) {
-    let Some((location, end_location)) = is_short_circuit(&checker.ctx, expr) else {
+    let Some((location, end_location)) = is_short_circuit(&checker.ctx, expr, &Boolop::And) else {
         return;
     };
     let mut diagnostic = Diagnostic::new(