From 11287f944fbdd320fa0a556180382fabc59be5b2 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 14 Sep 2023 05:05:37 -0400 Subject: [PATCH] Avoid re-parenthesizing call chains whose inner values are parenthesized (#7373) ## Summary Given a statement like: ```python result = ( f(111111111111111111111111111111111111111111111111111111111111111111111111111111111) + 1 )() ``` When we go to parenthesize the target of the assignment, we use `maybe_parenthesize_expression` with `Parenthesize::IfBreaks`. This then checks if the call on the right-hand side needs to be parenthesized, the implementation of which looks like: ```rust impl NeedsParentheses for ExprCall { fn needs_parentheses( &self, _parent: AnyNodeRef, context: &PyFormatContext, ) -> OptionalParentheses { if CallChainLayout::from_expression(self.into(), context.source()) == CallChainLayout::Fluent { OptionalParentheses::Multiline } else if context.comments().has_dangling(self) { OptionalParentheses::Always } else { self.func.needs_parentheses(self.into(), context) } } } ``` Checking for `self.func.needs_parentheses(self.into(), context)` is problematic, since, as in the example above, `self.func` may _already_ be parenthesized -- in which case, we _don't_ want to parenthesize the entire expression. If we do, we end up with this non-ideal formatting: ```python result = ( ( f( 111111111111111111111111111111111111111111111111111111111111111111111111111111111 ) + 1 )() ) ``` This PR modifies the `NeedsParentheses` implementations for call chain expressions to return `Never` if the inner expression has its own parentheses, in which case, the formatting implementations for those expressions will preserve them anyway. Closes https://github.com/astral-sh/ruff/issues/7370. ## Test Plan Zulip improves a bit, everything else is unchanged. Before: | project | similarity index | total files | changed files | |--------------|------------------:|------------------:|------------------:| | cpython | 0.76083 | 1789 | 1632 | | django | 0.99981 | 2760 | 40 | | transformers | 0.99944 | 2587 | 413 | | twine | 1.00000 | 33 | 0 | | typeshed | 0.99983 | 3496 | 18 | | warehouse | 0.99834 | 648 | 20 | | zulip | 0.99956 | 1437 | 23 | After: | project | similarity index | total files | changed files | |--------------|------------------:|------------------:|------------------:| | cpython | 0.76083 | 1789 | 1632 | | django | 0.99981 | 2760 | 40 | | transformers | 0.99944 | 2587 | 413 | | twine | 1.00000 | 33 | 0 | | typeshed | 0.99983 | 3496 | 18 | | warehouse | 0.99834 | 648 | 20 | | **zulip** | **0.99962** | **1437** | **22** | --- .../fixtures/ruff/expression/attribute.py | 31 +++++++++- .../test/fixtures/ruff/expression/call.py | 5 ++ .../fixtures/ruff/expression/subscript.py | 5 ++ .../src/expression/expr_attribute.rs | 2 + .../src/expression/expr_call.rs | 6 +- .../src/expression/expr_subscript.rs | 6 +- .../format@expression__attribute.py.snap | 57 +++++++++++++++++-- .../snapshots/format@expression__call.py.snap | 11 ++++ .../format@expression__subscript.py.snap | 24 ++++++++ 9 files changed, 139 insertions(+), 8 deletions(-) create mode 100644 crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/subscript.py create mode 100644 crates/ruff_python_formatter/tests/snapshots/format@expression__subscript.py.snap diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/attribute.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/attribute.py index 0219429b29..6312b607d5 100644 --- a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/attribute.py +++ b/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/attribute.py @@ -36,7 +36,7 @@ a.aaaaaaaaaaaaaaaaaaaaa.lllllllllllllllllllllllllllloooooooooong.chaaaaaaaaaaaaa # Test that we add parentheses around the outermost attribute access in an attribute # chain if and only if we need them, that is if there are own line comments inside # the chain. -x1 = ( +x10 = ( a .b # comment 1 @@ -46,6 +46,27 @@ x1 = ( .d ) +x11 = ( + a + .b + # comment 1 + . # comment 2 + # comment 3 + c + ).d + +x12 = ( + ( + a + .b + # comment 1 + . # comment 2 + # comment 3 + c + ) + .d +) + x20 = ( a .b @@ -106,7 +127,7 @@ x6 = ( a.b ) -# regression: https://github.com/astral-sh/ruff/issues/6181 +# Regression test for: https://github.com/astral-sh/ruff/issues/6181 (# ()).a @@ -125,3 +146,9 @@ x6 = ( # dangling before dot own-line .b ) + +# Regression test for: https://github.com/astral-sh/ruff/issues/7370 +result = ( + f(111111111111111111111111111111111111111111111111111111111111111111111111111111111) + + 1 +).bit_length() diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/call.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/call.py index 7123bd85d4..7690b5e0ac 100644 --- a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/call.py +++ b/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/call.py @@ -265,3 +265,8 @@ f( # a kwargs, ) +# Regression test for: https://github.com/astral-sh/ruff/issues/7370 +result = ( + f(111111111111111111111111111111111111111111111111111111111111111111111111111111111) + + 1 +)() diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/subscript.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/subscript.py new file mode 100644 index 0000000000..679f43acb5 --- /dev/null +++ b/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/subscript.py @@ -0,0 +1,5 @@ +# Regression test for: https://github.com/astral-sh/ruff/issues/7370 +result = ( + f(111111111111111111111111111111111111111111111111111111111111111111111111111111111) + + 1 +)[0] diff --git a/crates/ruff_python_formatter/src/expression/expr_attribute.rs b/crates/ruff_python_formatter/src/expression/expr_attribute.rs index 3b11cde300..37abc02b0a 100644 --- a/crates/ruff_python_formatter/src/expression/expr_attribute.rs +++ b/crates/ruff_python_formatter/src/expression/expr_attribute.rs @@ -150,6 +150,8 @@ impl NeedsParentheses for ExprAttribute { OptionalParentheses::Always } else if self.value.is_name_expr() { OptionalParentheses::BestFit + } else if is_expression_parenthesized(self.value.as_ref().into(), context.source()) { + OptionalParentheses::Never } else { self.value.needs_parentheses(self.into(), context) } diff --git a/crates/ruff_python_formatter/src/expression/expr_call.rs b/crates/ruff_python_formatter/src/expression/expr_call.rs index abb17570d8..6df21ba328 100644 --- a/crates/ruff_python_formatter/src/expression/expr_call.rs +++ b/crates/ruff_python_formatter/src/expression/expr_call.rs @@ -3,7 +3,9 @@ use ruff_python_ast::node::AnyNodeRef; use ruff_python_ast::{Expr, ExprCall}; use crate::comments::{dangling_comments, SourceComment}; -use crate::expression::parentheses::{NeedsParentheses, OptionalParentheses}; +use crate::expression::parentheses::{ + is_expression_parenthesized, NeedsParentheses, OptionalParentheses, +}; use crate::expression::CallChainLayout; use crate::prelude::*; @@ -86,6 +88,8 @@ impl NeedsParentheses for ExprCall { OptionalParentheses::Multiline } else if context.comments().has_dangling(self) { OptionalParentheses::Always + } else if is_expression_parenthesized(self.func.as_ref().into(), context.source()) { + OptionalParentheses::Never } else { self.func.needs_parentheses(self.into(), context) } diff --git a/crates/ruff_python_formatter/src/expression/expr_subscript.rs b/crates/ruff_python_formatter/src/expression/expr_subscript.rs index 682849f5de..c751165937 100644 --- a/crates/ruff_python_formatter/src/expression/expr_subscript.rs +++ b/crates/ruff_python_formatter/src/expression/expr_subscript.rs @@ -4,7 +4,9 @@ use ruff_python_ast::{Expr, ExprSubscript}; use crate::comments::SourceComment; use crate::expression::expr_tuple::TupleParentheses; -use crate::expression::parentheses::{parenthesized, NeedsParentheses, OptionalParentheses}; +use crate::expression::parentheses::{ + is_expression_parenthesized, parenthesized, NeedsParentheses, OptionalParentheses, +}; use crate::expression::CallChainLayout; use crate::prelude::*; @@ -81,6 +83,8 @@ impl NeedsParentheses for ExprSubscript { == CallChainLayout::Fluent { OptionalParentheses::Multiline + } else if is_expression_parenthesized(self.value.as_ref().into(), context.source()) { + OptionalParentheses::Never } else { match self.value.needs_parentheses(self.into(), context) { OptionalParentheses::BestFit => OptionalParentheses::Never, diff --git a/crates/ruff_python_formatter/tests/snapshots/format@expression__attribute.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@expression__attribute.py.snap index 14f17fc663..0e4951e92a 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@expression__attribute.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@expression__attribute.py.snap @@ -42,7 +42,7 @@ a.aaaaaaaaaaaaaaaaaaaaa.lllllllllllllllllllllllllllloooooooooong.chaaaaaaaaaaaaa # Test that we add parentheses around the outermost attribute access in an attribute # chain if and only if we need them, that is if there are own line comments inside # the chain. -x1 = ( +x10 = ( a .b # comment 1 @@ -52,6 +52,27 @@ x1 = ( .d ) +x11 = ( + a + .b + # comment 1 + . # comment 2 + # comment 3 + c + ).d + +x12 = ( + ( + a + .b + # comment 1 + . # comment 2 + # comment 3 + c + ) + .d +) + x20 = ( a .b @@ -112,7 +133,7 @@ x6 = ( a.b ) -# regression: https://github.com/astral-sh/ruff/issues/6181 +# Regression test for: https://github.com/astral-sh/ruff/issues/6181 (# ()).a @@ -131,6 +152,12 @@ x6 = ( # dangling before dot own-line .b ) + +# Regression test for: https://github.com/astral-sh/ruff/issues/7370 +result = ( + f(111111111111111111111111111111111111111111111111111111111111111111111111111111111) + + 1 +).bit_length() ``` ## Output @@ -173,7 +200,7 @@ a.aaaaaaaaaaaaaaaaaaaaa.lllllllllllllllllllllllllllloooooooooong.chaaaaaaaaaaaaa # Test that we add parentheses around the outermost attribute access in an attribute # chain if and only if we need them, that is if there are own line comments inside # the chain. -x1 = ( +x10 = ( a.b # comment 1 . # comment 2 @@ -181,6 +208,22 @@ x1 = ( c.d ) +x11 = ( + a.b + # comment 1 + . # comment 2 + # comment 3 + c +).d + +x12 = ( + a.b + # comment 1 + . # comment 2 + # comment 3 + c +).d + x20 = a.b x21 = ( # leading name own line @@ -227,7 +270,7 @@ x6 = ( a.b ) -# regression: https://github.com/astral-sh/ruff/issues/6181 +# Regression test for: https://github.com/astral-sh/ruff/issues/6181 ( # () ).a @@ -245,6 +288,12 @@ x6 = ( # dangling before dot own-line .b ) + +# Regression test for: https://github.com/astral-sh/ruff/issues/7370 +result = ( + f(111111111111111111111111111111111111111111111111111111111111111111111111111111111) + + 1 +).bit_length() ``` diff --git a/crates/ruff_python_formatter/tests/snapshots/format@expression__call.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@expression__call.py.snap index 1e4b44427b..3f19463966 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@expression__call.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@expression__call.py.snap @@ -271,6 +271,11 @@ f( # a kwargs, ) +# Regression test for: https://github.com/astral-sh/ruff/issues/7370 +result = ( + f(111111111111111111111111111111111111111111111111111111111111111111111111111111111) + + 1 +)() ``` ## Output @@ -537,6 +542,12 @@ f( # a ** # h kwargs, ) + +# Regression test for: https://github.com/astral-sh/ruff/issues/7370 +result = ( + f(111111111111111111111111111111111111111111111111111111111111111111111111111111111) + + 1 +)() ``` diff --git a/crates/ruff_python_formatter/tests/snapshots/format@expression__subscript.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@expression__subscript.py.snap new file mode 100644 index 0000000000..102175f3d5 --- /dev/null +++ b/crates/ruff_python_formatter/tests/snapshots/format@expression__subscript.py.snap @@ -0,0 +1,24 @@ +--- +source: crates/ruff_python_formatter/tests/fixtures.rs +input_file: crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/subscript.py +--- +## Input +```py +# Regression test for: https://github.com/astral-sh/ruff/issues/7370 +result = ( + f(111111111111111111111111111111111111111111111111111111111111111111111111111111111) + + 1 +)[0] +``` + +## Output +```py +# Regression test for: https://github.com/astral-sh/ruff/issues/7370 +result = ( + f(111111111111111111111111111111111111111111111111111111111111111111111111111111111) + + 1 +)[0] +``` + + +