fix: Don't omit optional parentheses for subscripts (#7380)

crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/slice.py

@@ -91,3 +91,10 @@ f = "f"[:,]
 g1 = "g"[(1):(2)]
 g2 = "g"[(1):(2):(3)]
 
+# Don't omit optional parentheses for subscripts
+# https://github.com/astral-sh/ruff/issues/7319
+def f():
+    return (
+        package_version is not None
+        and package_version.split(".")[:2] == package_info.version.split(".")[:2]
+    )

crates/ruff_python_formatter/src/expression/expr_subscript.rs

@@ -3,7 +3,6 @@ use ruff_python_ast::node::{AnyNodeRef, AstNode};
 use ruff_python_ast::{Expr, ExprSubscript};
 
 use crate::comments::SourceComment;
-use crate::context::{NodeLevel, WithNodeLevel};
 use crate::expression::expr_tuple::TupleParentheses;
 use crate::expression::parentheses::{parenthesized, NeedsParentheses, OptionalParentheses};
 use crate::expression::CallChainLayout;
@@ -41,24 +40,14 @@ impl FormatNodeRule<ExprSubscript> for FormatExprSubscript {
             "A subscript expression can only have a single dangling comment, the one after the bracket"
         );
 
-        let format_value = format_with(|f| match value.as_ref() {
-            Expr::Attribute(expr) => expr.format().with_options(call_chain_layout).fmt(f),
-            Expr::Call(expr) => expr.format().with_options(call_chain_layout).fmt(f),
-            Expr::Subscript(expr) => expr.format().with_options(call_chain_layout).fmt(f),
-            _ => value.format().fmt(f),
-        });
-
-        if let NodeLevel::Expression(Some(_)) = f.context().node_level() {
-            // Enforce the optional parentheses for parenthesized values.
-            let mut f = WithNodeLevel::new(NodeLevel::Expression(None), f);
-            write!(f, [format_value])?;
-        } else {
-            format_value.fmt(f)?;
+        match value.as_ref() {
+            Expr::Attribute(expr) => expr.format().with_options(call_chain_layout).fmt(f)?,
+            Expr::Call(expr) => expr.format().with_options(call_chain_layout).fmt(f)?,
+            Expr::Subscript(expr) => expr.format().with_options(call_chain_layout).fmt(f)?,
+            _ => value.format().fmt(f)?,
         }
 
         let format_slice = format_with(|f: &mut PyFormatter| {
-            let mut f = WithNodeLevel::new(NodeLevel::ParenthesizedExpression, f);
-
             if let Expr::Tuple(tuple) = slice.as_ref() {
                 write!(f, [tuple.format().with_options(TupleParentheses::Preserve)])
             } else {

crates/ruff_python_formatter/src/expression/mod.rs

@@ -385,14 +385,21 @@ fn can_omit_optional_parentheses(expr: &Expr, context: &PyFormatContext) -> bool
         // Only use the more complex IR when there is any expression that we can possibly split by
         false
     } else {
+        fn is_parenthesized(expr: &Expr, context: &PyFormatContext) -> bool {
+            // Don't break subscripts except in parenthesized context. It looks weird.
+            !matches!(expr, Expr::Subscript(_))
+                && has_parentheses(expr, context).is_some_and(OwnParentheses::is_non_empty)
+        }
+
         // Only use the layout if the first or last expression has parentheses of some sort, and
         // those parentheses are non-empty.
-        let first_parenthesized = visitor.first.is_some_and(|first| {
-            has_parentheses(first, context).is_some_and(OwnParentheses::is_non_empty)
-        });
-        let last_parenthesized = visitor.last.is_some_and(|last| {
-            has_parentheses(last, context).is_some_and(OwnParentheses::is_non_empty)
-        });
+        let first_parenthesized = visitor
+            .first
+            .is_some_and(|first| is_parenthesized(first, context));
+        let last_parenthesized = visitor
+            .last
+            .is_some_and(|last| is_parenthesized(last, context));
+
         first_parenthesized || last_parenthesized
     }
 }
@@ -502,6 +509,7 @@ impl<'input> CanOmitOptionalParenthesesVisitor<'input> {
                 self.any_parenthesized_expressions = true;
                 // Only walk the function, the subscript is always parenthesized
                 self.visit_expr(value);
+                self.last = Some(expr);
                 // Don't walk the slice, because the slice is always parenthesized.
                 return;
             }

crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__expression.py.snap

@@ -275,23 +275,7 @@ last_call()
  flags & ~select.EPOLLIN and waiters.write_task is not None
  lambda arg: None
  lambda a=True: a
-@@ -39,10 +39,11 @@
- lambda a, b, c=True, *, d=(1 << v2), e="str": a
- lambda a, b, c=True, *vararg, d=(v1 << 2), e="str", **kwargs: a + b
- manylambdas = lambda x=lambda y=lambda z=1: z: y(): x()
--foo = lambda port_id, ignore_missing: {
--    "port1": port1_resource,
--    "port2": port2_resource,
--}[port_id]
-+foo = (
-+    lambda port_id, ignore_missing: {"port1": port1_resource, "port2": port2_resource}[
-+        port_id
-+    ]
-+)
- 1 if True else 2
- str or None if True else str or bytes or None
- (str or None) if True else (str or bytes or None)
-@@ -115,7 +116,7 @@
+@@ -115,7 +115,7 @@
      arg,
      another,
      kwarg="hey",
@@ -346,11 +330,10 @@ lambda a, b, c=True: a
 lambda a, b, c=True, *, d=(1 << v2), e="str": a
 lambda a, b, c=True, *vararg, d=(v1 << 2), e="str", **kwargs: a + b
 manylambdas = lambda x=lambda y=lambda z=1: z: y(): x()
-foo = (
-    lambda port_id, ignore_missing: {"port1": port1_resource, "port2": port2_resource}[
-        port_id
-    ]
-)
+foo = lambda port_id, ignore_missing: {
+    "port1": port1_resource,
+    "port2": port2_resource,
+}[port_id]
 1 if True else 2
 str or None if True else str or bytes or None
 (str or None) if True else (str or bytes or None)

crates/ruff_python_formatter/tests/snapshots/format@expression__compare.py.snap

@@ -324,13 +324,13 @@ ct_match = (
     aaaaaaaaaaact_id == self.get_content_type(obj=rel_obj, using=instance._state.db)[id]
 )
 
-ct_match = {
-    aaaaaaaaaaaaaaaa
-} == self.get_content_type(obj=rel_obj, using=instance._state.db)[id]
+ct_match = {aaaaaaaaaaaaaaaa} == self.get_content_type(
+    obj=rel_obj, using=instance._state.db
+)[id]
 
-ct_match = (
-    aaaaaaaaaaaaaaaa
-) == self.get_content_type(obj=rel_obj, using=instance._state.db)[id]
+ct_match = (aaaaaaaaaaaaaaaa) == self.get_content_type(
+    obj=rel_obj, using=instance._state.db
+)[id]
 
 # Subscripts expressions with trailing attributes.
 

crates/ruff_python_formatter/tests/snapshots/format@expression__slice.py.snap

@@ -97,6 +97,13 @@ f = "f"[:,]
 g1 = "g"[(1):(2)]
 g2 = "g"[(1):(2):(3)]
 
+# Don't omit optional parentheses for subscripts
+# https://github.com/astral-sh/ruff/issues/7319
+def f():
+    return (
+        package_version is not None
+        and package_version.split(".")[:2] == package_info.version.split(".")[:2]
+    )
 ```
 
 ## Output
@@ -191,6 +198,15 @@ f = "f"[:,]
 # Regression test for https://github.com/astral-sh/ruff/issues/5733
 g1 = "g"[(1):(2)]
 g2 = "g"[(1):(2):(3)]
+
+
+# Don't omit optional parentheses for subscripts
+# https://github.com/astral-sh/ruff/issues/7319
+def f():
+    return (
+        package_version is not None
+        and package_version.split(".")[:2] == package_info.version.split(".")[:2]
+    )
 ```