From 5a6dc4ad6466c621078cd03ac5ecbcda075b5bc6 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Thu, 14 Dec 2023 12:37:42 +0800 Subject: [PATCH] Fix `can_omit_optional_parentheses` for expressions with a right most fstring --- .../test/fixtures/ruff/expression/binary.py | 12 +++++++ .../src/expression/mod.rs | 36 ++++++++++--------- .../format@expression__binary.py.snap | 24 +++++++++++++ 3 files changed, 55 insertions(+), 17 deletions(-) diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/binary.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/binary.py index 5d91dc500a930..83c6f0ff9fc3a 100644 --- a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/binary.py +++ b/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/binary.py @@ -319,6 +319,18 @@ ) ) +# Skip FString content when determining whether to omit optional parentheses or not.0 +# The below expression should be parenthesized because it ends with an fstring and starts with a name. +# (Call expressions at the beginning don't count as parenthesized because they don't start with parens). +assert ( + format.format_event(spec) + == f'Event("_remove_cookie", {{key:`testkey`,options:{json.dumps(options)}}})' +) +# Avoid parentheses for this example because it starts with a tuple expression. +assert ( + (spec, format) + == f'Event("_remove_cookie", {{key:`testkey`,options:{json.dumps(options)}}})' +) rowuses = [(1 << j) | # column ordinal (1 << (n + i-j + n-1)) | # NW-SE ordinal diff --git a/crates/ruff_python_formatter/src/expression/mod.rs b/crates/ruff_python_formatter/src/expression/mod.rs index 084a138912b70..391a61fd26af5 100644 --- a/crates/ruff_python_formatter/src/expression/mod.rs +++ b/crates/ruff_python_formatter/src/expression/mod.rs @@ -552,17 +552,13 @@ fn can_omit_optional_parentheses(expr: &Expr, context: &PyFormatContext) -> bool // Only use the layout if the first expression starts with parentheses // or the last expression ends with parentheses of some sort, and // those parentheses are non-empty. - if visitor + visitor .last .is_some_and(|last| is_parenthesized(last, context)) - { - true - } else { - visitor + || visitor .first .expression() .is_some_and(|first| is_parenthesized(first, context)) - } } } @@ -689,16 +685,6 @@ impl<'input> CanOmitOptionalParenthesesVisitor<'input> { // Don't walk the slice, because the slice is always parenthesized. return; } - Expr::UnaryOp(ast::ExprUnaryOp { - range: _, - op, - operand: _, - }) => { - if op.is_invert() { - self.update_max_precedence(OperatorPrecedence::BitwiseInversion); - } - self.first.set_if_none(First::Token); - } // `[a, b].test.test[300].dot` Expr::Attribute(ast::ExprAttribute { @@ -727,10 +713,23 @@ impl<'input> CanOmitOptionalParenthesesVisitor<'input> { } Expr::FString(ast::ExprFString { value, .. }) if value.is_implicit_concatenated() => { self.update_max_precedence(OperatorPrecedence::String); + return; } // Expressions with sub expressions but a preceding token // Mark this expression as first expression and not the sub expression. + // Visit the sub-expressions because the sub expressions may be the end of the entire expression. + Expr::UnaryOp(ast::ExprUnaryOp { + range: _, + op, + operand: _, + }) => { + if op.is_invert() { + self.update_max_precedence(OperatorPrecedence::BitwiseInversion); + } + self.first.set_if_none(First::Token); + } + Expr::Lambda(_) | Expr::Await(_) | Expr::Yield(_) @@ -739,6 +738,7 @@ impl<'input> CanOmitOptionalParenthesesVisitor<'input> { self.first.set_if_none(First::Token); } + // Terminal nodes or nodes that wrap a sub-expression (where the sub expression can never be the end). Expr::Tuple(_) | Expr::NamedExpr(_) | Expr::GeneratorExp(_) @@ -751,7 +751,9 @@ impl<'input> CanOmitOptionalParenthesesVisitor<'input> { | Expr::EllipsisLiteral(_) | Expr::Name(_) | Expr::Slice(_) - | Expr::IpyEscapeCommand(_) => {} + | Expr::IpyEscapeCommand(_) => { + return; + } }; walk_expr(self, expr); diff --git a/crates/ruff_python_formatter/tests/snapshots/format@expression__binary.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@expression__binary.py.snap index e245378863ad4..1d59b5b13a1bc 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@expression__binary.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@expression__binary.py.snap @@ -325,6 +325,18 @@ expected_content = ( ) ) +# Skip FString content when determining whether to omit optional parentheses or not.0 +# The below expression should be parenthesized because it ends with an fstring and starts with a name. +# (Call expressions at the beginning don't count as parenthesized because they don't start with parens). +assert ( + format.format_event(spec) + == f'Event("_remove_cookie", {{key:`testkey`,options:{json.dumps(options)}}})' +) +# Avoid parentheses for this example because it starts with a tuple expression. +assert ( + (spec, format) + == f'Event("_remove_cookie", {{key:`testkey`,options:{json.dumps(options)}}})' +) rowuses = [(1 << j) | # column ordinal (1 << (n + i-j + n-1)) | # NW-SE ordinal @@ -790,6 +802,18 @@ expected_content = ( ) ) +# Skip FString content when determining whether to omit optional parentheses or not.0 +# The below expression should be parenthesized because it ends with an fstring and starts with a name. +# (Call expressions at the beginning don't count as parenthesized because they don't start with parens). +assert ( + format.format_event(spec) + == f'Event("_remove_cookie", {{key:`testkey`,options:{json.dumps(options)}}})' +) +# Avoid parentheses for this example because it starts with a tuple expression. +assert ( + spec, + format, +) == f'Event("_remove_cookie", {{key:`testkey`,options:{json.dumps(options)}}})' rowuses = [ (1 << j) # column ordinal