Skip to content

Commit 30bec3f

Browse files
authored
Only omit optinal parens if the expression ends or starts with a parenthesized expression
<!-- Thank you for contributing to Ruff! To help us out with reviewing, please consider the following: - Does this pull request include a summary of the change? (See below.) - Does this pull request include a descriptive title? - Does this pull request include references to any relevant issues? --> ## Summary This PR matches Black' behavior where it only omits the optional parentheses if the expression starts or ends with a parenthesized expression: ```python a + [aaa, bbb, cccc] * c # Don't omit [aaa, bbb, cccc] + a * c # Split a + c * [aaa, bbb, ccc] # Split ``` <!-- What's the purpose of the change? What does it do, and why? --> ## Test Plan This improves the Jaccard index from 0.945 to 0.946
1 parent 8b9193a commit 30bec3f

File tree

3 files changed

+43
-159
lines changed

3 files changed

+43
-159
lines changed

crates/ruff_python_formatter/src/expression/mod.rs

Lines changed: 34 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -198,37 +198,30 @@ impl<'ast> IntoFormat<PyFormatContext<'ast>> for Expr {
198198
///
199199
/// This mimics Black's [`_maybe_split_omitting_optional_parens`](https://github.com/psf/black/blob/d1248ca9beaf0ba526d265f4108836d89cf551b7/src/black/linegen.py#L746-L820)
200200
fn can_omit_optional_parentheses(expr: &Expr, context: &PyFormatContext) -> bool {
201-
let mut visitor = MaxOperatorPriorityVisitor::new(context.source());
202-
201+
let mut visitor = CanOmitOptionalParenthesesVisitor::new(context.source());
203202
visitor.visit_subexpression(expr);
204-
205-
let (max_operator_priority, operation_count, any_parenthesized_expression) = visitor.finish();
206-
207-
if operation_count > 1 {
208-
false
209-
} else if max_operator_priority == OperatorPriority::Attribute {
210-
true
211-
} else {
212-
// Only use the more complex IR when there is any expression that we can possibly split by
213-
any_parenthesized_expression
214-
}
203+
visitor.can_omit()
215204
}
216205

217206
#[derive(Clone, Debug)]
218-
struct MaxOperatorPriorityVisitor<'input> {
207+
struct CanOmitOptionalParenthesesVisitor<'input> {
219208
max_priority: OperatorPriority,
220209
max_priority_count: u32,
221210
any_parenthesized_expressions: bool,
211+
last: Option<&'input Expr>,
212+
first: Option<&'input Expr>,
222213
source: &'input str,
223214
}
224215

225-
impl<'input> MaxOperatorPriorityVisitor<'input> {
216+
impl<'input> CanOmitOptionalParenthesesVisitor<'input> {
226217
fn new(source: &'input str) -> Self {
227218
Self {
228219
source,
229220
max_priority: OperatorPriority::None,
230221
max_priority_count: 0,
231222
any_parenthesized_expressions: false,
223+
last: None,
224+
first: None,
232225
}
233226
}
234227

@@ -305,6 +298,7 @@ impl<'input> MaxOperatorPriorityVisitor<'input> {
305298
self.any_parenthesized_expressions = true;
306299
// Only walk the function, the arguments are always parenthesized
307300
self.visit_expr(func);
301+
self.last = Some(expr);
308302
return;
309303
}
310304
Expr::Subscript(_) => {
@@ -351,23 +345,41 @@ impl<'input> MaxOperatorPriorityVisitor<'input> {
351345
walk_expr(self, expr);
352346
}
353347

354-
fn finish(self) -> (OperatorPriority, u32, bool) {
355-
(
356-
self.max_priority,
357-
self.max_priority_count,
358-
self.any_parenthesized_expressions,
359-
)
348+
fn can_omit(self) -> bool {
349+
if self.max_priority_count > 1 {
350+
false
351+
} else if self.max_priority == OperatorPriority::Attribute {
352+
true
353+
} else if !self.any_parenthesized_expressions {
354+
// Only use the more complex IR when there is any expression that we can possibly split by
355+
false
356+
} else {
357+
// Only use the layout if the first or last expression has parentheses of some sort.
358+
let first_parenthesized = self
359+
.first
360+
.map_or(false, |first| has_parentheses(first, self.source));
361+
let last_parenthesized = self
362+
.last
363+
.map_or(false, |last| has_parentheses(last, self.source));
364+
first_parenthesized || last_parenthesized
365+
}
360366
}
361367
}
362368

363-
impl<'input> PreorderVisitor<'input> for MaxOperatorPriorityVisitor<'input> {
369+
impl<'input> PreorderVisitor<'input> for CanOmitOptionalParenthesesVisitor<'input> {
364370
fn visit_expr(&mut self, expr: &'input Expr) {
371+
self.last = Some(expr);
372+
365373
// Rule only applies for non-parenthesized expressions.
366374
if is_expression_parenthesized(AnyNodeRef::from(expr), self.source) {
367375
self.any_parenthesized_expressions = true;
368376
} else {
369377
self.visit_subexpression(expr);
370378
}
379+
380+
if self.first.is_none() {
381+
self.first = Some(expr);
382+
}
371383
}
372384
}
373385

crates/ruff_python_formatter/src/lib.rs

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -280,8 +280,15 @@ if True:
280280
#[test]
281281
fn quick_test() {
282282
let src = r#"
283-
def foo() -> tuple[int, int, int,]:
284-
return 2
283+
if a * [
284+
bbbbbbbbbbbbbbbbbbbbbb,
285+
cccccccccccccccccccccccccccccdddddddddddddddddddddddddd,
286+
] + a * e * [
287+
ffff,
288+
gggg,
289+
hhhhhhhhhhhhhh,
290+
] * c:
291+
pass
285292
286293
"#;
287294
// Tokenize once

crates/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__trailing_comma_optional_parens1.py.snap

Lines changed: 0 additions & 135 deletions
This file was deleted.

0 commit comments

Comments
 (0)