language-servers/ruff
1
0
Fork 0
mirror of https://github.com/astral-sh/ruff.git synced 2025-08-03 18:28:56 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 3

Respect parenthesized generators in has_own_parentheses (#8100)

Browse source 
## Summary

When analyzing:

```python
if "root" not in (
    long_tree_name_tree.split("/")[0]
    for long_tree_name_tree in really_really_long_variable_name
):
    msg = "Could not find root. Please try a different forest."
    raise ValueError(msg)
```

We missed that the generator expression is parenthesized, because the
parentheses are _part_ of the generator -- so
`is_expression_parenthesized` returns `False`. We needed to take into
account that generators and tuples may or may not be parenthesized when
determining whether we can omit parentheses while splitting an
expression.

Closes https://github.com/astral-sh/ruff/issues/8090.

## Test Plan

No changes in similarity.

Before:

| project | similarity index | total files | changed files |

|----------------|------------------:|------------------:|------------------:|
| cpython | 0.75803 | 1799 | 1647 |
| django | 0.99983 | 2772 | 34 |
| home-assistant | 0.99953 | 10596 | 186 |
| poetry | 0.99891 | 317 | 17 |
| transformers | 0.99966 | 2657 | 330 |
| twine | 1.00000 | 33 | 0 |
| typeshed | 0.99978 | 3669 | 20 |
| warehouse | 0.99977 | 654 | 13 |
| zulip | 0.99970 | 1459 | 22 |

After:

| project | similarity index | total files | changed files |

|----------------|------------------:|------------------:|------------------:|
| cpython | 0.75803 | 1799 | 1647 |
| django | 0.99983 | 2772 | 34 |
| home-assistant | 0.99953 | 10596 | 186 |
| poetry | 0.99891 | 317 | 17 |
| transformers | 0.99966 | 2657 | 330 |
| twine | 1.00000 | 33 | 0 |
| typeshed | 0.99978 | 3669 | 20 |
| warehouse | 0.99977 | 654 | 13 |
| zulip | 0.99970 | 1459 | 22 |
This commit is contained in:
Charlie Marsh 2023-10-22 16:58:25 -07:00 • committed by GitHub
parent bcaac9693b
commit 95702e408f
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
5 changed files with 72 additions and 20 deletions
Download patch file Download diff file Expand all files Collapse all files

2
crates/ruff_python_formatter/src/expression/expr_tuple.rs
View file

@ -224,7 +224,7 @@ impl NeedsParentheses for ExprTuple {
/// Return `true` if a tuple is parenthesized in the source code.
pub(crate) fn is_tuple_parenthesized(tuple: &ExprTuple, source: &str) -> bool {
let Some(elt) = tuple.elts.first() else {
return false;
return true;
};
// Count the number of open parentheses between the start of the tuple and the first element.

49
crates/ruff_python_formatter/src/expression/mod.rs
View file

@ -14,6 +14,8 @@ use ruff_text_size::Ranged;
use crate::builders::parenthesize_if_expands;
use crate::comments::{leading_comments, trailing_comments, LeadingDanglingTrailingComments};
use crate::context::{NodeLevel, WithNodeLevel};
use crate::expression::expr_generator_exp::is_generator_parenthesized;
use crate::expression::expr_tuple::is_tuple_parenthesized;
use crate::expression::parentheses::{
is_expression_parenthesized, optional_parentheses, parenthesized, NeedsParentheses,
OptionalParentheses, Parentheses, Parenthesize,
@ -510,7 +512,7 @@ fn can_omit_optional_parentheses(expr: &Expr, context: &PyFormatContext) -> bool
if visitor.max_precedence == OperatorPrecedence::None {
true
} else if visitor.pax_precedence_count > 1 {
} else if visitor.max_precedence_count > 1 {
false
} else if visitor.max_precedence == OperatorPrecedence::Attribute {
true
@ -540,7 +542,7 @@ fn can_omit_optional_parentheses(expr: &Expr, context: &PyFormatContext) -> bool
#[derive(Clone, Debug)]
struct CanOmitOptionalParenthesesVisitor<'input> {
max_precedence: OperatorPrecedence,
pax_precedence_count: u32,
max_precedence_count: u32,
any_parenthesized_expressions: bool,
last: Option<&'input Expr>,
first: Option<&'input Expr>,
@ -552,7 +554,7 @@ impl<'input> CanOmitOptionalParenthesesVisitor<'input> {
Self {
context,
max_precedence: OperatorPrecedence::None,
pax_precedence_count: 0,
max_precedence_count: 0,
any_parenthesized_expressions: false,
last: None,
first: None,
@ -566,11 +568,11 @@ impl<'input> CanOmitOptionalParenthesesVisitor<'input> {
fn update_max_precedence_with_count(&mut self, precedence: OperatorPrecedence, count: u32) {
match self.max_precedence.cmp(&precedence) {
Ordering::Less => {
self.pax_precedence_count = count;
self.max_precedence_count = count;
self.max_precedence = precedence;
}
Ordering::Equal => {
self.pax_precedence_count += count;
self.max_precedence_count += count;
}
Ordering::Greater => {}
}
@ -581,7 +583,6 @@ impl<'input> CanOmitOptionalParenthesesVisitor<'input> {
match expr {
Expr::Dict(_)
| Expr::List(_)
| Expr::Tuple(_)
| Expr::Set(_)
| Expr::ListComp(_)
| Expr::SetComp(_)
@ -590,6 +591,21 @@ impl<'input> CanOmitOptionalParenthesesVisitor<'input> {
// The values are always parenthesized, don't visit.
return;
}
Expr::Tuple(tuple) if is_tuple_parenthesized(tuple, self.context.source()) => {
self.any_parenthesized_expressions = true;
// The values are always parenthesized, don't visit.
return;
}
Expr::GeneratorExp(generator)
if is_generator_parenthesized(generator, self.context.source()) =>
{
self.any_parenthesized_expressions = true;
// The values are always parenthesized, don't visit.
return;
}
// It's impossible for a file smaller or equal to 4GB to contain more than 2^32 comparisons
// because each comparison requires a left operand, and `n` `operands` and right sides.
#[allow(clippy::cast_possible_truncation)]
@ -690,7 +706,8 @@ impl<'input> CanOmitOptionalParenthesesVisitor<'input> {
self.update_max_precedence(OperatorPrecedence::String);
}
Expr::NamedExpr(_)
Expr::Tuple(_)
| Expr::NamedExpr(_)
| Expr::GeneratorExp(_)
| Expr::Lambda(_)
| Expr::Await(_)
@ -914,10 +931,14 @@ pub(crate) fn has_own_parentheses(
Some(OwnParentheses::NonEmpty)
}
Expr::GeneratorExp(generator)
if is_generator_parenthesized(generator, context.source()) =>
{
Some(OwnParentheses::NonEmpty)
}
// These expressions must contain _some_ child or trivia token in order to be non-empty.
Expr::List(ast::ExprList { elts, .. })
| Expr::Set(ast::ExprSet { elts, .. })
| Expr::Tuple(ast::ExprTuple { elts, .. }) => {
Expr::List(ast::ExprList { elts, .. }) | Expr::Set(ast::ExprSet { elts, .. }) => {
if !elts.is_empty() || context.comments().has_dangling(AnyNodeRef::from(expr)) {
Some(OwnParentheses::NonEmpty)
} else {
@ -925,6 +946,14 @@ pub(crate) fn has_own_parentheses(
}
}
Expr::Tuple(tuple) if is_tuple_parenthesized(tuple, context.source()) => {
if !tuple.elts.is_empty() || context.comments().has_dangling(AnyNodeRef::from(expr)) {
Some(OwnParentheses::NonEmpty)
} else {
Some(OwnParentheses::Empty)
}
}
Expr::Dict(ast::ExprDict { keys, .. }) => {
if !keys.is_empty() || context.comments().has_dangling(AnyNodeRef::from(expr)) {
Some(OwnParentheses::NonEmpty)