From a65efcf4590f290917d496d0706b2f9522d67291 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Thu, 14 Sep 2023 10:43:53 +0200 Subject: [PATCH] fix: Don't omit optional parentheses for subscripts (#7380) --- .../test/fixtures/ruff/expression/slice.py | 7 +++++ .../src/expression/expr_subscript.rs | 21 ++++----------- .../src/expression/mod.rs | 20 +++++++++----- ...atibility@simple_cases__expression.py.snap | 27 ++++--------------- .../format@expression__compare.py.snap | 12 ++++----- .../format@expression__slice.py.snap | 16 +++++++++++ 6 files changed, 53 insertions(+), 50 deletions(-) diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/slice.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/slice.py index 3b30f67cd4..059322a8a3 100644 --- a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/slice.py +++ b/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] + ) diff --git a/crates/ruff_python_formatter/src/expression/expr_subscript.rs b/crates/ruff_python_formatter/src/expression/expr_subscript.rs index 1e2a5c3d94..682849f5de 100644 --- a/crates/ruff_python_formatter/src/expression/expr_subscript.rs +++ b/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 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 { diff --git a/crates/ruff_python_formatter/src/expression/mod.rs b/crates/ruff_python_formatter/src/expression/mod.rs index 413b732a53..ba2b979fe5 100644 --- a/crates/ruff_python_formatter/src/expression/mod.rs +++ b/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; } diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__expression.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__expression.py.snap index 0ee2ffdceb..43e5c13e54 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__expression.py.snap +++ b/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) diff --git a/crates/ruff_python_formatter/tests/snapshots/format@expression__compare.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@expression__compare.py.snap index bb3dabb8ac..6498068ee8 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@expression__compare.py.snap +++ b/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. diff --git a/crates/ruff_python_formatter/tests/snapshots/format@expression__slice.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@expression__slice.py.snap index 482cd452c1..6a89f8e147 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@expression__slice.py.snap +++ b/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] + ) ```