From 2b676e8403e8d42044dc1a57a9c3f3c40457c014 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Thu, 26 Oct 2023 12:22:53 +0900 Subject: [PATCH] Fix parenthesizing of expression starting with unary expressions --- .../test/fixtures/ruff/expression/unary.py | 13 ++++ .../src/expression/mod.rs | 68 ++++++++++++++----- crates/ruff_python_formatter/src/lib.rs | 13 ++-- .../format@expression__unary.py.snap | 39 +++++++++-- 4 files changed, 104 insertions(+), 29 deletions(-) diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/unary.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/unary.py index dd53db4884adc..5c8ab8d8b31ab 100644 --- a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/unary.py +++ b/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/unary.py @@ -180,3 +180,16 @@ ): msg = "Could not find root. Please try a different forest." raise ValueError(msg) + +# Regression for https://github.com/astral-sh/ruff/issues/8183 +def foo(): + while ( + not (aaaaaaaaaaaaaaaaaaaaa(bbbbbbbb, ccccccc)) and dddddddddd < eeeeeeeeeeeeeee + ): + pass + +def foo(): + while ( + not (aaaaaaaaaaaaaaaaaaaaa[bbbbbbbb, ccccccc]) and dddddddddd < eeeeeeeeeeeeeee + ): + pass diff --git a/crates/ruff_python_formatter/src/expression/mod.rs b/crates/ruff_python_formatter/src/expression/mod.rs index 48ced2960c9d1..c7dae45612333 100644 --- a/crates/ruff_python_formatter/src/expression/mod.rs +++ b/crates/ruff_python_formatter/src/expression/mod.rs @@ -526,16 +526,20 @@ fn can_omit_optional_parentheses(expr: &Expr, context: &PyFormatContext) -> bool && has_parentheses(expr, context).is_some_and(OwnParentheses::is_non_empty) } - // Only use the layout if the first or last expression has parentheses of some sort, and + // 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. - let first_parenthesized = visitor - .first - .is_some_and(|first| is_parenthesized(first, context)); - let last_parenthesized = visitor + if visitor .last - .is_some_and(|last| is_parenthesized(last, context)); - - first_parenthesized || last_parenthesized + .is_some_and(|last| is_parenthesized(last, context)) + { + true + } else { + visitor + .first + .expression() + .is_some_and(|first| is_parenthesized(first, context)) + } } } @@ -545,7 +549,7 @@ struct CanOmitOptionalParenthesesVisitor<'input> { max_precedence_count: u32, any_parenthesized_expressions: bool, last: Option<&'input Expr>, - first: Option<&'input Expr>, + first: First<'input>, context: &'input PyFormatContext<'input>, } @@ -557,7 +561,7 @@ impl<'input> CanOmitOptionalParenthesesVisitor<'input> { max_precedence_count: 0, any_parenthesized_expressions: false, last: None, - first: None, + first: First::None, } } @@ -670,6 +674,7 @@ impl<'input> CanOmitOptionalParenthesesVisitor<'input> { if op.is_invert() { self.update_max_precedence(OperatorPrecedence::BitwiseInversion); } + self.first.set_if_none(First::Token); } // `[a, b].test.test[300].dot` @@ -706,17 +711,22 @@ impl<'input> CanOmitOptionalParenthesesVisitor<'input> { self.update_max_precedence(OperatorPrecedence::String); } - Expr::Tuple(_) - | Expr::NamedExpr(_) - | Expr::GeneratorExp(_) - | Expr::Lambda(_) + // Expressions with sub expressions but a preceding token + // Mark this expression as first expression and not the sub expression. + Expr::Lambda(_) | Expr::Await(_) | Expr::Yield(_) | Expr::YieldFrom(_) + | Expr::Starred(_) => { + self.first.set_if_none(First::Token); + } + + Expr::Tuple(_) + | Expr::NamedExpr(_) + | Expr::GeneratorExp(_) | Expr::FormattedValue(_) | Expr::FString(_) | Expr::Constant(_) - | Expr::Starred(_) | Expr::Name(_) | Expr::Slice(_) | Expr::IpyEscapeCommand(_) => {} @@ -741,8 +751,32 @@ impl<'input> PreorderVisitor<'input> for CanOmitOptionalParenthesesVisitor<'inpu self.visit_subexpression(expr); } - if self.first.is_none() { - self.first = Some(expr); + self.first.set_if_none(First::Expression(expr)); + } +} + +#[derive(Copy, Clone, Debug)] +enum First<'a> { + None, + + /// Expression starts with a non-parentheses token. E.g. `not a` + Token, + + Expression(&'a Expr), +} + +impl<'a> First<'a> { + #[inline] + fn set_if_none(&mut self, first: First<'a>) { + if matches!(self, First::None) { + *self = first; + } + } + + fn expression(self) -> Option<&'a Expr> { + match self { + First::None | First::Token => None, + First::Expression(expr) => Some(expr), } } } diff --git a/crates/ruff_python_formatter/src/lib.rs b/crates/ruff_python_formatter/src/lib.rs index 7697789fe072c..9f360b230cc73 100644 --- a/crates/ruff_python_formatter/src/lib.rs +++ b/crates/ruff_python_formatter/src/lib.rs @@ -206,14 +206,11 @@ if True: #[test] fn quick_test() { let source = r#" -def main() -> None: - if True: - some_very_long_variable_name_abcdefghijk = Foo() - some_very_long_variable_name_abcdefghijk = some_very_long_variable_name_abcdefghijk[ - some_very_long_variable_name_abcdefghijk.some_very_long_attribute_name - == "This is a very long string abcdefghijk" - ] - +def foo(): + while ( + aaaaaaaaaaaaaaaaaaaaa(bbbbbbbb, cccccccccccccc) and dddddddddd < eeeeeeeeeeeeeee + ): + pass "#; let source_type = PySourceType::Python; let (tokens, comment_ranges) = tokens_and_ranges(source, source_type).unwrap(); diff --git a/crates/ruff_python_formatter/tests/snapshots/format@expression__unary.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@expression__unary.py.snap index d2f0bac0abf9e..6146fd82d5424 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@expression__unary.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@expression__unary.py.snap @@ -186,6 +186,19 @@ if "root" not in ( ): msg = "Could not find root. Please try a different forest." raise ValueError(msg) + +# Regression for https://github.com/astral-sh/ruff/issues/8183 +def foo(): + while ( + not (aaaaaaaaaaaaaaaaaaaaa(bbbbbbbb, ccccccc)) and dddddddddd < eeeeeeeeeeeeeee + ): + pass + +def foo(): + while ( + not (aaaaaaaaaaaaaaaaaaaaa[bbbbbbbb, ccccccc]) and dddddddddd < eeeeeeeeeeeeeee + ): + pass ``` ## Output @@ -292,10 +305,13 @@ if aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa & ( ): pass -if not ( - aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa - + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb -) & aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa: +if ( + not ( + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb + ) + & aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa +): pass @@ -383,6 +399,21 @@ if "root" not in ( ): msg = "Could not find root. Please try a different forest." raise ValueError(msg) + + +# Regression for https://github.com/astral-sh/ruff/issues/8183 +def foo(): + while ( + not (aaaaaaaaaaaaaaaaaaaaa(bbbbbbbb, ccccccc)) and dddddddddd < eeeeeeeeeeeeeee + ): + pass + + +def foo(): + while ( + not (aaaaaaaaaaaaaaaaaaaaa[bbbbbbbb, ccccccc]) and dddddddddd < eeeeeeeeeeeeeee + ): + pass ```