Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Respect parenthesized generators in has_own_parentheses #8100

Merged
merged 1 commit into from
Oct 22, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@
a:
pass

# Regression: https://github.com/astral-sh/ruff/issues/5338
# Regression test for: https://github.com/astral-sh/ruff/issues/5338
if a and not aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa & aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa:
pass

Expand All @@ -162,7 +162,7 @@
):
pass

# https://github.com/astral-sh/ruff/issues/7448
# Regression test for: https://github.com/astral-sh/ruff/issues/7448
x = (
# a
not # b
Expand All @@ -172,3 +172,11 @@
True
)
)

# Regression test for: https://github.com/astral-sh/ruff/issues/8090
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)
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An empty tuple has to be parenthesized. Otherwise, it... can't exist?

};

// Count the number of open parentheses between the start of the tuple and the first element.
Expand Down
49 changes: 39 additions & 10 deletions crates/ruff_python_formatter/src/expression/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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 {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this was a typo, but please correct me if I'm wrong.

false
} else if visitor.max_precedence == OperatorPrecedence::Attribute {
true
Expand Down Expand Up @@ -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>,
Expand All @@ -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,
Expand All @@ -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 => {}
}
Expand All @@ -581,7 +583,6 @@ impl<'input> CanOmitOptionalParenthesesVisitor<'input> {
match expr {
Expr::Dict(_)
| Expr::List(_)
| Expr::Tuple(_)
| Expr::Set(_)
| Expr::ListComp(_)
| Expr::SetComp(_)
Expand All @@ -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)]
Expand Down Expand Up @@ -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(_)
Expand Down Expand Up @@ -914,17 +931,29 @@ 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 {
Some(OwnParentheses::Empty)
}
}

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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -475,9 +475,8 @@ aaaaaaaaaaaaaa + {
aaaaaaaaaaaaaa + [
a for x in bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
]
(
aaaaaaaaaaaaaa
+ (a for x in bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb)
aaaaaaaaaaaaaa + (
a for x in bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
)
aaaaaaaaaaaaaa + {
a for x in bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ if not \
a:
pass

# Regression: https://github.com/astral-sh/ruff/issues/5338
# Regression test for: https://github.com/astral-sh/ruff/issues/5338
if a and not aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa & aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa:
pass

Expand All @@ -168,7 +168,7 @@ if True:
):
pass

# https://github.com/astral-sh/ruff/issues/7448
# Regression test for: https://github.com/astral-sh/ruff/issues/7448
x = (
# a
not # b
Expand All @@ -178,6 +178,14 @@ x = (
True
)
)

# Regression test for: https://github.com/astral-sh/ruff/issues/8090
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)
```

## Output
Expand Down Expand Up @@ -328,7 +336,7 @@ if (
if not a:
pass

# Regression: https://github.com/astral-sh/ruff/issues/5338
# Regression test for: https://github.com/astral-sh/ruff/issues/5338
if (
a
and not aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
Expand Down Expand Up @@ -357,7 +365,7 @@ if True:
):
pass

# https://github.com/astral-sh/ruff/issues/7448
# Regression test for: https://github.com/astral-sh/ruff/issues/7448
x = (
# a
# b
Expand All @@ -367,6 +375,14 @@ x = (
True
)
)

# Regression test for: https://github.com/astral-sh/ruff/issues/8090
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)
```


Expand Down
Loading