mirror of
https://github.com/astral-sh/ruff.git
synced 2025-08-12 06:38:21 +00:00
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** |
This commit is contained in:
parent
a65efcf459
commit
11287f944f
9 changed files with 139 additions and 8 deletions
|
@ -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)
|
||||
}
|
||||
|
|
|
@ -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)
|
||||
}
|
||||
|
|
|
@ -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,
|
||||
|
|
Loading…
Add table
Add a link
Reference in a new issue