Add parentheses when simplifying conditions in SIM222 (#7117)

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

@@ -152,3 +152,11 @@ if (a or [1] or True or [2]) == (a or [1]):  # SIM222
 
 if f(a or [1] or True or [2]):  # SIM222
     pass
+
+# Regression test for: https://github.com/astral-sh/ruff/issues/7099
+def secondToTime(s0: int) -> (int, int, int) or str:
+    m, s = divmod(s0, 60)
+
+
+def secondToTime(s0: int) -> ((int, int, int) or str):
+    m, s = divmod(s0, 60)

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

@@ -11,6 +11,8 @@ use ruff_diagnostics::{AlwaysAutofixableViolation, AutofixKind, Diagnostic, Edit
 use ruff_macros::{derive_message_formats, violation};
 use ruff_python_ast::comparable::ComparableExpr;
 use ruff_python_ast::helpers::{contains_effect, Truthiness};
+use ruff_python_ast::parenthesize::parenthesized_range;
+use ruff_python_codegen::Generator;
 
 use crate::checkers::ast::Checker;
 use crate::registry::AsRule;
@@ -692,12 +694,12 @@ pub(crate) fn expr_or_not_expr(checker: &mut Checker, expr: &Expr) {
     }
 }
 
-pub(crate) fn get_short_circuit_edit(
+fn get_short_circuit_edit(
     expr: &Expr,
     range: TextRange,
     truthiness: Truthiness,
     in_boolean_test: bool,
-    checker: &Checker,
+    generator: Generator,
 ) -> Edit {
     let content = if in_boolean_test {
         match truthiness {
@@ -708,9 +710,17 @@ pub(crate) fn get_short_circuit_edit(
             }
         }
     } else {
-        checker.generator().expr(expr)
+        generator.expr(expr)
     };
-    Edit::range_replacement(content, range)
+    Edit::range_replacement(
+        if matches!(expr, Expr::Tuple(ast::ExprTuple { elts, ctx: _, range: _}) if !elts.is_empty())
+        {
+            format!("({content})")
+        } else {
+            content
+        },
+        range,
+    )
 }
 
 fn is_short_circuit(
@@ -734,7 +744,7 @@ fn is_short_circuit(
         BoolOp::Or => Truthiness::Truthy,
     };
 
-    let mut location = expr.start();
+    let mut furthest = expr;
     let mut edit = None;
     let mut remove = None;
 
@@ -749,7 +759,7 @@ fn is_short_circuit(
             && (!checker.semantic().in_boolean_test()
                 || contains_effect(value, |id| checker.semantic().is_builtin(id)))
         {
-            location = next_value.start();
+            furthest = next_value;
             continue;
         }
 
@@ -758,17 +768,19 @@ fn is_short_circuit(
         // 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 value_truthiness == short_circuit_truthiness {
-            remove = Some(if location == value.start() {
-                ContentAround::After
-            } else {
-                ContentAround::Both
-            });
+            remove = Some(ContentAround::After);
+
             edit = Some(get_short_circuit_edit(
                 value,
-                TextRange::new(location, expr.end()),
+                TextRange::new(
+                    parenthesized_range(furthest.into(), expr.into(), checker.locator().contents())
+                        .unwrap_or(furthest.range())
+                        .start(),
+                    expr.end(),
+                ),
                 short_circuit_truthiness,
                 checker.semantic().in_boolean_test(),
-                checker,
+                checker.generator(),
             ));
             break;
         }
@@ -776,17 +788,22 @@ fn is_short_circuit(
         // If the next expression is a constant, and it matches the short-circuit value, then
         // we can return the location of the expression.
         if next_value_truthiness == short_circuit_truthiness {
-            remove = Some(if index == values.len() - 2 {
+            remove = Some(if index + 1 == values.len() - 1 {
                 ContentAround::Before
             } else {
                 ContentAround::Both
             });
             edit = Some(get_short_circuit_edit(
                 next_value,
-                TextRange::new(location, expr.end()),
+                TextRange::new(
+                    parenthesized_range(furthest.into(), expr.into(), checker.locator().contents())
+                        .unwrap_or(furthest.range())
+                        .start(),
+                    expr.end(),
+                ),
                 short_circuit_truthiness,
                 checker.semantic().in_boolean_test(),
-                checker,
+                checker.generator(),
             ));
             break;
         }

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

@@ -534,7 +534,7 @@ SIM222.py:85:6: SIM222 [*] Use `True` instead of `... or True`
 87 87 | a or (1,) or True or (2,)  # SIM222
 88 88 | 
 
-SIM222.py:87:6: SIM222 [*] Use `1,` instead of `1, or ...`
+SIM222.py:87:6: SIM222 [*] Use `(1,)` instead of `(1,) or ...`
    |
 85 | a or tuple(()) or True  # SIM222
 86 | 
@@ -543,14 +543,14 @@ SIM222.py:87:6: SIM222 [*] Use `1,` instead of `1, or ...`
 88 | 
 89 | a or tuple((1,)) or True or tuple((2,))  # SIM222
    |
-   = help: Replace with `1,`
+   = help: Replace with `(1,)`
 
 ℹ Suggested fix
 84 84 | 
 85 85 | a or tuple(()) or True  # SIM222
 86 86 | 
 87    |-a or (1,) or True or (2,)  # SIM222
-   87 |+a or 1,  # SIM222
+   87 |+a or (1,)  # SIM222
 88 88 | 
 89 89 | a or tuple((1,)) or True or tuple((2,))  # SIM222
 90 90 | 
@@ -1003,5 +1003,42 @@ SIM222.py:153:11: SIM222 [*] Use `[1]` instead of `[1] or ...`
 153     |-if f(a or [1] or True or [2]):  # SIM222
     153 |+if f(a or [1]):  # SIM222
 154 154 |     pass
+155 155 | 
+156 156 | # Regression test for: https://github.com/astral-sh/ruff/issues/7099
+
+SIM222.py:157:30: SIM222 [*] Use `(int, int, int)` instead of `(int, int, int) or ...`
+    |
+156 | # Regression test for: https://github.com/astral-sh/ruff/issues/7099
+157 | def secondToTime(s0: int) -> (int, int, int) or str:
+    |                              ^^^^^^^^^^^^^^^^^^^^^^ SIM222
+158 |     m, s = divmod(s0, 60)
+    |
+    = help: Replace with `(int, int, int)`
+
+ℹ Suggested fix
+154 154 |     pass
+155 155 | 
+156 156 | # Regression test for: https://github.com/astral-sh/ruff/issues/7099
+157     |-def secondToTime(s0: int) -> (int, int, int) or str:
+    157 |+def secondToTime(s0: int) -> (int, int, int):
+158 158 |     m, s = divmod(s0, 60)
+159 159 | 
+160 160 | 
+
+SIM222.py:161:31: SIM222 [*] Use `(int, int, int)` instead of `(int, int, int) or ...`
+    |
+161 | def secondToTime(s0: int) -> ((int, int, int) or str):
+    |                               ^^^^^^^^^^^^^^^^^^^^^^ SIM222
+162 |     m, s = divmod(s0, 60)
+    |
+    = help: Replace with `(int, int, int)`
+
+ℹ Suggested fix
+158 158 |     m, s = divmod(s0, 60)
+159 159 | 
+160 160 | 
+161     |-def secondToTime(s0: int) -> ((int, int, int) or str):
+    161 |+def secondToTime(s0: int) -> ((int, int, int)):
+162 162 |     m, s = divmod(s0, 60)
 
 

crates/ruff_python_codegen/src/generator.rs

@@ -1,6 +1,5 @@
 //! Generate Python source code from an abstract syntax tree (AST).
 
-use ruff_python_ast::{ParameterWithDefault, TypeParams};
 use std::ops::Deref;
 
 use ruff_python_ast::{
@@ -8,8 +7,8 @@ use ruff_python_ast::{
     ExceptHandler, Expr, Identifier, MatchCase, Operator, Parameter, Parameters, Pattern, Stmt,
     Suite, TypeParam, TypeParamParamSpec, TypeParamTypeVar, TypeParamTypeVarTuple, WithItem,
 };
+use ruff_python_ast::{ParameterWithDefault, TypeParams};
 use ruff_python_literal::escape::{AsciiEscape, Escape, UnicodeEscape};
-
 use ruff_source_file::LineEnding;
 
 use super::stylist::{Indentation, Quote, Stylist};
@@ -1381,12 +1380,12 @@ impl<'a> Generator<'a> {
 mod tests {
     use ruff_python_ast::{Mod, ModModule};
     use ruff_python_parser::{self, parse_suite, Mode};
-
     use ruff_source_file::LineEnding;
 
-    use super::Generator;
     use crate::stylist::{Indentation, Quote};
 
+    use super::Generator;
+
     fn round_trip(contents: &str) -> String {
         let indentation = Indentation::default();
         let quote = Quote::default();