From c0d79808d2436e448b28978b7de9bb90faa3144c Mon Sep 17 00:00:00 2001 From: konstin Date: Mon, 9 Oct 2023 18:56:24 +0200 Subject: [PATCH 01/10] Comments outside expression parentheses --- crates/ruff_formatter/src/lib.rs | 4 + .../test/fixtures/ruff/expression/unary.py | 11 + .../expression_parentheses_comments.py | 113 +++++++++ .../test/fixtures/ruff/statement/with.py | 4 - .../src/comments/placement.rs | 40 ++-- .../src/expression/mod.rs | 200 +++++++++++++--- crates/ruff_python_formatter/src/lib.rs | 1 - ...aneous__long_strings_flag_disabled.py.snap | 15 +- .../snapshots/format@expression__call.py.snap | 5 +- .../format@expression__unary.py.snap | 57 +++-- ...s__expression_parentheses_comments.py.snap | 224 ++++++++++++++++++ .../snapshots/format@statement__with.py.snap | 10 - 12 files changed, 589 insertions(+), 95 deletions(-) create mode 100644 crates/ruff_python_formatter/resources/test/fixtures/ruff/parentheses/expression_parentheses_comments.py create mode 100644 crates/ruff_python_formatter/tests/snapshots/format@parentheses__expression_parentheses_comments.py.snap diff --git a/crates/ruff_formatter/src/lib.rs b/crates/ruff_formatter/src/lib.rs index 556b864931ab5..dfac3df3fc345 100644 --- a/crates/ruff_formatter/src/lib.rs +++ b/crates/ruff_formatter/src/lib.rs @@ -575,6 +575,10 @@ where context: PhantomData, } } + + pub fn rule(&self) -> &R { + &self.rule + } } impl FormatRefWithRule<'_, T, R, C> 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 9ae2022ff5583..5169b8a19b4d8 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 @@ -161,3 +161,14 @@ + "WARNING: Removing listed files. Do you really want to continue. yes/n)? " ): pass + +# https://github.com/astral-sh/ruff/issues/7448 +x = ( + # a + not # b + # c + ( # d + # e + True + ) +) \ No newline at end of file diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/parentheses/expression_parentheses_comments.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/parentheses/expression_parentheses_comments.py new file mode 100644 index 0000000000000..a13d869763368 --- /dev/null +++ b/crates/ruff_python_formatter/resources/test/fixtures/ruff/parentheses/expression_parentheses_comments.py @@ -0,0 +1,113 @@ +list_with_parenthesized_elements1 = [ + # comment leading outer + ( + # comment leading inner + 1 + 2 # comment trailing inner + ) # comment trailing outer +] + +list_with_parenthesized_elements2 = [ + # leading outer + (1 + 2) +] +list_with_parenthesized_elements3 = [ + # leading outer + (1 + 2) # trailing outer +] +list_with_parenthesized_elements4 = [ + # leading outer + (1 + 2), # trailing outer +] +list_with_parenthesized_elements5 = [ + (1), # trailing outer + (2), # trailing outer +] + +nested_parentheses1 = ( + ( + ( + 1 + ) # i + ) # j +) # k +nested_parentheses2 = [ + ( + ( + ( + 1 + ) # i + # i2 + ) # j + # j2 + ) # k + # k2 +] +nested_parentheses3 = ( + ( # a + ( # b + 1 + ) # i + ) # j +) # k +nested_parentheses4 = [ + # a + ( # b + # c + ( # d + # e + ( #f + 1 + ) # i + # i2 + ) # j + # j2 + ) # k + # k2 +] + + +x = ( + # unary comment + not + # in-between comment + ( + # leading inner + "a" + ), + not # in-between comment + ( + # leading inner + "b" + ), + not + ( # in-between comment + # leading inner + "c" + ), + # 1 + not # 2 + ( # 3 + # 4 + "d" + ) +) + +if ( + # unary comment + not + # in-between comment + ( + # leading inner + 1 + ) +): + pass + +# Make sure we keep a inside the parentheses +# https://github.com/astral-sh/ruff/issues/7892 +x = ( + # a + ( # b + 1 + ) +) \ No newline at end of file diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/with.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/with.py index f4b729a3baaf5..4dc166009a363 100644 --- a/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/with.py +++ b/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/with.py @@ -86,10 +86,6 @@ ) ): pass -with (a # trailing same line comment - # trailing own line comment - ) as b: pass - with ( a # trailing same line comment # trailing own line comment diff --git a/crates/ruff_python_formatter/src/comments/placement.rs b/crates/ruff_python_formatter/src/comments/placement.rs index e45312a1532ed..acb8b9db00bb2 100644 --- a/crates/ruff_python_formatter/src/comments/placement.rs +++ b/crates/ruff_python_formatter/src/comments/placement.rs @@ -1878,8 +1878,7 @@ fn handle_lambda_comment<'a>( CommentPlacement::Default(comment) } -/// Attach trailing end-of-line comments on the operator as dangling comments on the enclosing -/// node. +/// Move comment between a unary op and its operand before the unary op by marking them as trailing. /// /// For example, given: /// ```python @@ -1896,26 +1895,27 @@ fn handle_unary_op_comment<'a>( unary_op: &'a ast::ExprUnaryOp, locator: &Locator, ) -> CommentPlacement<'a> { - if comment.line_position().is_own_line() { - return CommentPlacement::Default(comment); - } - - if comment.start() > unary_op.operand.start() { - return CommentPlacement::Default(comment); - } - - let tokenizer = SimpleTokenizer::new( + let mut tokenizer = SimpleTokenizer::new( locator.contents(), - TextRange::new(comment.start(), unary_op.operand.start()), - ); - if tokenizer - .skip_trivia() - .any(|token| token.kind == SimpleTokenKind::LParen) - { - return CommentPlacement::Default(comment); + TextRange::new(unary_op.start(), unary_op.operand.start()), + ) + .skip_trivia(); + let op_token = tokenizer.next(); + debug_assert!(op_token.is_some_and(|token| matches!( + token.kind, + SimpleTokenKind::Tilde + | SimpleTokenKind::Not + | SimpleTokenKind::Plus + | SimpleTokenKind::Minus + ))); + let up_to = tokenizer + .find(|token| token.kind == SimpleTokenKind::LParen) + .map_or(unary_op.operand.start(), |lparen| lparen.start()); + if comment.end() < up_to { + CommentPlacement::leading(unary_op, comment) + } else { + CommentPlacement::Default(comment) } - - CommentPlacement::dangling(comment.enclosing_node(), comment) } /// Attach an end-of-line comment immediately following an open bracket as a dangling comment on diff --git a/crates/ruff_python_formatter/src/expression/mod.rs b/crates/ruff_python_formatter/src/expression/mod.rs index bc1bd2050ab8f..d4cdb1f29570b 100644 --- a/crates/ruff_python_formatter/src/expression/mod.rs +++ b/crates/ruff_python_formatter/src/expression/mod.rs @@ -1,6 +1,5 @@ use std::cmp::Ordering; - -use itertools::Itertools; +use std::slice; use ruff_formatter::{ write, FormatOwnedWithRule, FormatRefWithRule, FormatRule, FormatRuleWithOptions, @@ -9,10 +8,11 @@ use ruff_python_ast as ast; use ruff_python_ast::visitor::preorder::{walk_expr, PreorderVisitor}; use ruff_python_ast::AnyNodeRef; use ruff_python_ast::{Constant, Expr, ExpressionRef, Operator}; -use ruff_python_trivia::CommentRanges; +use ruff_python_trivia::{BackwardsTokenizer, CommentRanges, SimpleTokenKind, SimpleTokenizer}; +use ruff_text_size::{Ranged, TextLen, TextRange}; use crate::builders::parenthesize_if_expands; -use crate::comments::leading_comments; +use crate::comments::{leading_comments, trailing_comments, Comments}; use crate::context::{NodeLevel, WithNodeLevel}; use crate::expression::parentheses::{ is_expression_parenthesized, optional_parentheses, parenthesized, NeedsParentheses, @@ -102,6 +102,38 @@ impl FormatRule> for FormatExpr { Expr::Slice(expr) => expr.format().fmt(f), Expr::IpyEscapeCommand(expr) => expr.format().fmt(f), }); + let format_expr_bare = format_with(|f| match expression { + Expr::BoolOp(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::NamedExpr(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::BinOp(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::UnaryOp(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::Lambda(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::IfExp(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::Dict(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::Set(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::ListComp(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::SetComp(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::DictComp(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::GeneratorExp(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::Await(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::Yield(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::YieldFrom(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::Compare(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::Call(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::FormattedValue(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::FString(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::Constant(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::Attribute(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::Subscript(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::Starred(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::Name(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::List(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::Tuple(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::Slice(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::IpyEscapeCommand(expr) => { + FormatNodeRule::fmt_fields(expr.format().rule(), expr, f) + } + }); let parenthesize = match parentheses { Parentheses::Preserve => is_expression_parenthesized( @@ -113,32 +145,12 @@ impl FormatRule> for FormatExpr { // Fluent style means we already have parentheses Parentheses::Never => false, }; - if parenthesize { - // Any comments on the open parenthesis of a `node`. - // - // For example, `# comment` in: - // ```python - // ( # comment - // foo.bar - // ) - // ``` let comments = f.context().comments().clone(); - let leading = comments.leading(expression); - if let Some((index, open_parenthesis_comment)) = leading - .iter() - .find_position(|comment| comment.line_position().is_end_of_line()) - { - write!( - f, - [ - leading_comments(&leading[..index]), - parenthesized("(", &format_expr, ")") - .with_dangling_comments(std::slice::from_ref(open_parenthesis_comment)) - ] - ) - } else { + if !comments.has_leading(expression) && !comments.has_trailing(expression) { parenthesized("(", &format_expr, ")").fmt(f) + } else { + format_with_parentheses_comments(expression, f, &format_expr_bare, &comments) } } else { let level = match f.context().node_level() { @@ -155,6 +167,140 @@ impl FormatRule> for FormatExpr { } } +/// The comments below are trailing on the addition, but it's also outside the +/// parentheses +/// ```python +/// x = [ +/// # comment leading +/// (1 + 2) # comment trailing +/// ] +/// ``` +/// as opposed to +/// ```python +/// x = [( +/// # comment leading +/// 1 + 2 # comment trailing +/// )] +/// ``` +/// , where the comments are inside the parentheses. That is also affects list +/// formatting, where we want to avoid moving the comments after the comma inside +/// the parentheses: +/// ```python +/// data = [ +/// ( +/// b"\x01\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00" +/// b"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" +/// ), # Point (0 0) +/// ] +/// ``` +/// We could mark those comments as trailing in list but it's easier to handle +/// them here too. +/// +/// So given +/// ```python +/// x = [ +/// # comment leading outer +/// ( +/// # comment leading inner +/// 1 + 2 # comment trailing inner +/// ) # comment trailing outer +/// ] +/// ``` +/// we want to keep the inner an outer comments outside the parentheses and the inner ones inside. +/// This is independent of whether they are own line or end-of-line comments, though end-of-line +/// comments can become own line comments when we discard nested parentheses. +/// +/// Style decision: When there are multiple nested parentheses around an expression, we consider the +/// outermost parentheses the relevant ones and discard the others. +fn format_with_parentheses_comments< + 'ast, + S: Fn(&mut Formatter>) -> FormatResult<()>, +>( + expression: &Expr, + f: &mut PyFormatter<'ast, '_>, + format_expr_bare: &FormatWith, S>, + comments: &Comments, +) -> FormatResult<()> { + let trailing = comments.trailing(expression); + let leading = comments.leading(expression); + // TODO: deduplicate + let right_tokenizer = SimpleTokenizer::new( + f.context().source(), + TextRange::new(expression.end(), f.context().source().text_len()), + ) + .skip_trivia() + .take_while(|token| token.kind == SimpleTokenKind::RParen); + + let left_tokenizer = BackwardsTokenizer::up_to( + expression.start(), + f.context().source(), + f.context().comments().ranges(), + ) + .skip_trivia() + .take_while(|token| token.kind == SimpleTokenKind::LParen); + + // Zip closing parenthesis with opening parenthesis. The order is intentional, as testing for + // closing parentheses is cheaper, and `zip` will avoid progressing the `left_tokenizer` if + // the `right_tokenizer` is exhausted. + let range_with_parens = right_tokenizer + .zip(left_tokenizer) + .last() + .map(|(right, left)| TextRange::new(left.start(), right.end())); + + let (leading_split, trailing_split) = if let Some(range_with_parens) = range_with_parens { + let leading_split = + leading.partition_point(|comment| comment.start() < range_with_parens.start()); + let trailing_split = + trailing.partition_point(|comment| comment.start() < range_with_parens.end()); + (leading_split, trailing_split) + } else { + (0, trailing.len()) + }; + + let leading_outer = &leading[..leading_split]; + let leading_inner = &leading[leading_split..]; + let trailing_inner = &trailing[..trailing_split]; + let trailing_outer = &trailing[trailing_split..]; + + // + let (parentheses_comment, leading_inner) = match leading_inner.split_first() { + Some((first, rest)) if first.line_position().is_end_of_line() => { + (slice::from_ref(first), rest) + } + _ => (Default::default(), leading), + }; + + leading_comments(leading_outer).fmt(f)?; + + // Custom FormatNodeRule::fmt patch + let custom_format_node_rule_fmt = format_with(|f| { + // No need to handle suppression comments, those are statement only + leading_comments(leading_inner).fmt(f)?; + + let is_source_map_enabled = f.options().source_map_generation().is_enabled(); + + if is_source_map_enabled { + source_position(expression.start()).fmt(f)?; + } + + format_expr_bare.fmt(f)?; + + if is_source_map_enabled { + source_position(expression.end()).fmt(f)?; + } + + trailing_comments(trailing_inner).fmt(f) + }); + + // The actual parenthesized formatting + parenthesized("(", &custom_format_node_rule_fmt, ")") + .with_dangling_comments(parentheses_comment) + .fmt(f)?; + trailing_comments(trailing_outer).fmt(f)?; + + Ok(()) +} + /// Wraps an expression in an optional parentheses except if its [`NeedsParentheses::needs_parentheses`] implementation /// indicates that it is okay to omit the parentheses. For example, parentheses can always be omitted for lists, /// because they already bring their own parentheses. diff --git a/crates/ruff_python_formatter/src/lib.rs b/crates/ruff_python_formatter/src/lib.rs index e92ec0028b3a2..4271d3aa48197 100644 --- a/crates/ruff_python_formatter/src/lib.rs +++ b/crates/ruff_python_formatter/src/lib.rs @@ -60,7 +60,6 @@ where } self.fmt_fields(node, f)?; - self.fmt_dangling_comments(node_comments.dangling, f)?; if is_source_map_enabled { source_position(node.end()).fmt(f)?; diff --git a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@miscellaneous__long_strings_flag_disabled.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@miscellaneous__long_strings_flag_disabled.py.snap index 8be3c1f3eab42..991b20198c79e 100644 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@miscellaneous__long_strings_flag_disabled.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@miscellaneous__long_strings_flag_disabled.py.snap @@ -320,17 +320,6 @@ long_unmergable_string_with_pragma = ( "formatting" ) -@@ -221,8 +217,8 @@ - func_with_bad_comma( - ( - "This is a really long string argument to a function that has a trailing comma" -- " which should NOT be there." -- ), # comment after comma -+ " which should NOT be there." # comment after comma -+ ), - ) - - func_with_bad_parens_that_wont_fit_in_one_line( ``` ## Ruff Output @@ -555,8 +544,8 @@ func_with_bad_comma( func_with_bad_comma( ( "This is a really long string argument to a function that has a trailing comma" - " which should NOT be there." # comment after comma - ), + " which should NOT be there." + ), # comment after comma ) func_with_bad_parens_that_wont_fit_in_one_line( diff --git a/crates/ruff_python_formatter/tests/snapshots/format@expression__call.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@expression__call.py.snap index 136b44be6d934..861c4f0553512 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@expression__call.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@expression__call.py.snap @@ -458,8 +458,9 @@ func( ) func( - # outer comment - ( # inner comment + ( + # outer comment + # inner comment [] ) ) 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 147fad105ece3..8816e9487f51f 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 @@ -167,7 +167,17 @@ if True: + "WARNING: Removing listed files. Do you really want to continue. yes/n)? " ): pass -``` + +# https://github.com/astral-sh/ruff/issues/7448 +x = ( + # a + not # b + # c + ( # d + # e + True + ) +)``` ## Output ```py @@ -217,35 +227,31 @@ if +( pass if ( - not # comment - aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + not aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb ): pass if ( - ~ # comment - aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + ~aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb ): pass if ( - - # comment - aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + -aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb ): pass if ( - + # comment - aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + +aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb ): pass @@ -254,8 +260,8 @@ if ( if ( # unary comment + # operand comment not ( - # operand comment # comment aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb @@ -286,28 +292,31 @@ if not ( ## Trailing operator comments -if ( - not aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa # comment +if ( # comment + not aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb ): pass if ( - ~aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa # comment + # comment + ~aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb ): pass if ( - -aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa # comment + # comment + -aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb ): pass if ( - +aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa # comment + # comment + +aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb ): pass @@ -327,13 +336,14 @@ if ( pass if ( - not # comment - a + not a ): pass -if not a: # comment +if ( # comment + not a +): pass # Regression test for: https://github.com/astral-sh/ruff/issues/7423 @@ -345,6 +355,17 @@ if True: + "WARNING: Removing listed files. Do you really want to continue. yes/n)? " ): pass + +# https://github.com/astral-sh/ruff/issues/7448 +x = ( + # a + # b + # c + not ( # d + # e + True + ) +) ``` diff --git a/crates/ruff_python_formatter/tests/snapshots/format@parentheses__expression_parentheses_comments.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@parentheses__expression_parentheses_comments.py.snap new file mode 100644 index 0000000000000..8e09c943f61d4 --- /dev/null +++ b/crates/ruff_python_formatter/tests/snapshots/format@parentheses__expression_parentheses_comments.py.snap @@ -0,0 +1,224 @@ +--- +source: crates/ruff_python_formatter/tests/fixtures.rs +input_file: crates/ruff_python_formatter/resources/test/fixtures/ruff/parentheses/expression_parentheses_comments.py +--- +## Input +```py +list_with_parenthesized_elements1 = [ + # comment leading outer + ( + # comment leading inner + 1 + 2 # comment trailing inner + ) # comment trailing outer +] + +list_with_parenthesized_elements2 = [ + # leading outer + (1 + 2) +] +list_with_parenthesized_elements3 = [ + # leading outer + (1 + 2) # trailing outer +] +list_with_parenthesized_elements4 = [ + # leading outer + (1 + 2), # trailing outer +] +list_with_parenthesized_elements5 = [ + (1), # trailing outer + (2), # trailing outer +] + +nested_parentheses1 = ( + ( + ( + 1 + ) # i + ) # j +) # k +nested_parentheses2 = [ + ( + ( + ( + 1 + ) # i + # i2 + ) # j + # j2 + ) # k + # k2 +] +nested_parentheses3 = ( + ( # a + ( # b + 1 + ) # i + ) # j +) # k +nested_parentheses4 = [ + # a + ( # b + # c + ( # d + # e + ( #f + 1 + ) # i + # i2 + ) # j + # j2 + ) # k + # k2 +] + + +x = ( + # unary comment + not + # in-between comment + ( + # leading inner + "a" + ), + not # in-between comment + ( + # leading inner + "b" + ), + not + ( # in-between comment + # leading inner + "c" + ), + # 1 + not # 2 + ( # 3 + # 4 + "d" + ) +) + +if ( + # unary comment + not + # in-between comment + ( + # leading inner + 1 + ) +): + pass + +# Make sure we keep a inside the parentheses +# https://github.com/astral-sh/ruff/issues/7892 +x = ( + # a + ( # b + 1 + ) +)``` + +## Output +```py +list_with_parenthesized_elements1 = [ + # comment leading outer + ( + # comment leading inner + 1 + 2 # comment trailing inner + ) # comment trailing outer +] + +list_with_parenthesized_elements2 = [ + # leading outer + (1 + 2) +] +list_with_parenthesized_elements3 = [ + # leading outer + (1 + 2) # trailing outer +] +list_with_parenthesized_elements4 = [ + # leading outer + (1 + 2), # trailing outer +] +list_with_parenthesized_elements5 = [ + (1), # trailing outer + (2), # trailing outer +] + +nested_parentheses1 = ( + 1 # i # j +) # k +nested_parentheses2 = [ + ( + 1 # i + # i2 + # j + # j2 + ) # k + # k2 +] +nested_parentheses3 = ( # a + # b + 1 # i # j +) # k +nested_parentheses4 = [ + # a + ( # b + # c + # d + # e + # f + 1 # i + # i2 + # j + # j2 + ) # k + # k2 +] + + +x = ( + # unary comment + # in-between comment + not ( + # leading inner + "a" + ), + # in-between comment + not ( + # leading inner + "b" + ), + not ( # in-between comment + # leading inner + "c" + ), + # 1 + # 2 + not ( # 3 + # 4 + "d" + ), +) + +if ( + # unary comment + # in-between comment + not ( + # leading inner + 1 + ) +): + pass + +# Make sure we keep a inside the parentheses +# https://github.com/astral-sh/ruff/issues/7892 +x = ( + # a + # b + 1 +) +``` + + + diff --git a/crates/ruff_python_formatter/tests/snapshots/format@statement__with.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@statement__with.py.snap index d7fc6346957ac..145e04e5d4ffe 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@statement__with.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@statement__with.py.snap @@ -92,10 +92,6 @@ with ( ) ): pass -with (a # trailing same line comment - # trailing own line comment - ) as b: pass - with ( a # trailing same line comment # trailing own line comment @@ -420,12 +416,6 @@ with ( ) as b: pass -with ( - a # trailing same line comment - # trailing own line comment -) as b: - pass - with ( ( a From 38d46833d648a7f8d49f7cb65b03e169417fb6d7 Mon Sep 17 00:00:00 2001 From: konstin Date: Wed, 11 Oct 2023 15:32:49 +0200 Subject: [PATCH 02/10] Code style --- .../test/fixtures/ruff/expression/unary.py | 2 +- .../expression_parentheses_comments.py | 2 +- .../src/expression/mod.rs | 108 ++++++++++-------- .../format@expression__unary.py.snap | 3 +- ...s__expression_parentheses_comments.py.snap | 3 +- 5 files changed, 67 insertions(+), 51 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 5169b8a19b4d8..1fb69736bd602 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 @@ -171,4 +171,4 @@ # e True ) -) \ No newline at end of file +) diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/parentheses/expression_parentheses_comments.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/parentheses/expression_parentheses_comments.py index a13d869763368..c86279119fd41 100644 --- a/crates/ruff_python_formatter/resources/test/fixtures/ruff/parentheses/expression_parentheses_comments.py +++ b/crates/ruff_python_formatter/resources/test/fixtures/ruff/parentheses/expression_parentheses_comments.py @@ -110,4 +110,4 @@ ( # b 1 ) -) \ No newline at end of file +) diff --git a/crates/ruff_python_formatter/src/expression/mod.rs b/crates/ruff_python_formatter/src/expression/mod.rs index d4cdb1f29570b..87410817e6814 100644 --- a/crates/ruff_python_formatter/src/expression/mod.rs +++ b/crates/ruff_python_formatter/src/expression/mod.rs @@ -102,39 +102,6 @@ impl FormatRule> for FormatExpr { Expr::Slice(expr) => expr.format().fmt(f), Expr::IpyEscapeCommand(expr) => expr.format().fmt(f), }); - let format_expr_bare = format_with(|f| match expression { - Expr::BoolOp(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), - Expr::NamedExpr(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), - Expr::BinOp(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), - Expr::UnaryOp(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), - Expr::Lambda(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), - Expr::IfExp(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), - Expr::Dict(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), - Expr::Set(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), - Expr::ListComp(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), - Expr::SetComp(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), - Expr::DictComp(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), - Expr::GeneratorExp(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), - Expr::Await(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), - Expr::Yield(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), - Expr::YieldFrom(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), - Expr::Compare(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), - Expr::Call(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), - Expr::FormattedValue(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), - Expr::FString(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), - Expr::Constant(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), - Expr::Attribute(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), - Expr::Subscript(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), - Expr::Starred(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), - Expr::Name(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), - Expr::List(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), - Expr::Tuple(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), - Expr::Slice(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), - Expr::IpyEscapeCommand(expr) => { - FormatNodeRule::fmt_fields(expr.format().rule(), expr, f) - } - }); - let parenthesize = match parentheses { Parentheses::Preserve => is_expression_parenthesized( expression.into(), @@ -150,7 +117,7 @@ impl FormatRule> for FormatExpr { if !comments.has_leading(expression) && !comments.has_trailing(expression) { parenthesized("(", &format_expr, ")").fmt(f) } else { - format_with_parentheses_comments(expression, f, &format_expr_bare, &comments) + format_with_parentheses_comments(expression, &comments, f) } } else { let level = match f.context().node_level() { @@ -212,17 +179,12 @@ impl FormatRule> for FormatExpr { /// /// Style decision: When there are multiple nested parentheses around an expression, we consider the /// outermost parentheses the relevant ones and discard the others. -fn format_with_parentheses_comments< - 'ast, - S: Fn(&mut Formatter>) -> FormatResult<()>, ->( +fn format_with_parentheses_comments( expression: &Expr, - f: &mut PyFormatter<'ast, '_>, - format_expr_bare: &FormatWith, S>, comments: &Comments, + f: &mut PyFormatter, ) -> FormatResult<()> { - let trailing = comments.trailing(expression); - let leading = comments.leading(expression); + // First part: Split the comments // TODO: deduplicate let right_tokenizer = SimpleTokenizer::new( f.context().source(), @@ -247,6 +209,9 @@ fn format_with_parentheses_comments< .last() .map(|(right, left)| TextRange::new(left.start(), right.end())); + let leading = comments.leading(expression); + let trailing = comments.trailing(expression); + let (leading_split, trailing_split) = if let Some(range_with_parens) = range_with_parens { let leading_split = leading.partition_point(|comment| comment.start() < range_with_parens.start()); @@ -262,7 +227,13 @@ fn format_with_parentheses_comments< let trailing_inner = &trailing[..trailing_split]; let trailing_outer = &trailing[trailing_split..]; - // + // Preserve an opening parentheses comment + // ```python + // a = ( # opening parentheses comment + // # leading inner + // 1 + // ) + // ``` let (parentheses_comment, leading_inner) = match leading_inner.split_first() { Some((first, rest)) if first.line_position().is_end_of_line() => { (slice::from_ref(first), rest) @@ -270,10 +241,53 @@ fn format_with_parentheses_comments< _ => (Default::default(), leading), }; + // Second Part: Format + + // The code order is a bit strange here, we format: + // * outer leading comment + // * opening parenthesis + // * opening parenthesis comment + // * inner leading comments + // * the expression itself + // * inner trailing comments + // * the closing parenthesis + // * outer trailing comments + + let fmt_fields = format_with(|f| match expression { + Expr::BoolOp(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::NamedExpr(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::BinOp(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::UnaryOp(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::Lambda(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::IfExp(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::Dict(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::Set(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::ListComp(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::SetComp(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::DictComp(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::GeneratorExp(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::Await(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::Yield(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::YieldFrom(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::Compare(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::Call(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::FormattedValue(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::FString(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::Constant(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::Attribute(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::Subscript(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::Starred(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::Name(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::List(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::Tuple(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::Slice(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + Expr::IpyEscapeCommand(expr) => FormatNodeRule::fmt_fields(expr.format().rule(), expr, f), + }); + leading_comments(leading_outer).fmt(f)?; - // Custom FormatNodeRule::fmt patch - let custom_format_node_rule_fmt = format_with(|f| { + // Custom FormatNodeRule::fmt variant that only formats the inner comments + let format_node_rule_fmt = format_with(|f| { // No need to handle suppression comments, those are statement only leading_comments(leading_inner).fmt(f)?; @@ -283,7 +297,7 @@ fn format_with_parentheses_comments< source_position(expression.start()).fmt(f)?; } - format_expr_bare.fmt(f)?; + fmt_fields.fmt(f)?; if is_source_map_enabled { source_position(expression.end()).fmt(f)?; @@ -293,7 +307,7 @@ fn format_with_parentheses_comments< }); // The actual parenthesized formatting - parenthesized("(", &custom_format_node_rule_fmt, ")") + parenthesized("(", &format_node_rule_fmt, ")") .with_dangling_comments(parentheses_comment) .fmt(f)?; trailing_comments(trailing_outer).fmt(f)?; 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 8816e9487f51f..9a291501337aa 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 @@ -177,7 +177,8 @@ x = ( # e True ) -)``` +) +``` ## Output ```py diff --git a/crates/ruff_python_formatter/tests/snapshots/format@parentheses__expression_parentheses_comments.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@parentheses__expression_parentheses_comments.py.snap index 8e09c943f61d4..f90a84c74c0f4 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@parentheses__expression_parentheses_comments.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@parentheses__expression_parentheses_comments.py.snap @@ -116,7 +116,8 @@ x = ( ( # b 1 ) -)``` +) +``` ## Output ```py From 53603bbf281c719ad0796cfff6b99408b6ad562b Mon Sep 17 00:00:00 2001 From: konstin Date: Wed, 11 Oct 2023 16:01:34 +0200 Subject: [PATCH 03/10] Document missing parent problem --- .../src/expression/mod.rs | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/crates/ruff_python_formatter/src/expression/mod.rs b/crates/ruff_python_formatter/src/expression/mod.rs index 87410817e6814..2ef78cf65b504 100644 --- a/crates/ruff_python_formatter/src/expression/mod.rs +++ b/crates/ruff_python_formatter/src/expression/mod.rs @@ -185,7 +185,24 @@ fn format_with_parentheses_comments( f: &mut PyFormatter, ) -> FormatResult<()> { // First part: Split the comments - // TODO: deduplicate + + // TODO: This is copied from `parenthesized_range`, except that we don't have the parent, which + // is a problem: + // ```python + // f( + // # a + // (a) + // ) + // ``` + // gets formatted as + // ```python + // f( + // ( + // # a + // a + // ) + // ) + // ``` let right_tokenizer = SimpleTokenizer::new( f.context().source(), TextRange::new(expression.end(), f.context().source().text_len()), From 4fc4e43360a3fd4a787220e958d8ee0ce94c3974 Mon Sep 17 00:00:00 2001 From: konsti Date: Tue, 17 Oct 2023 12:17:08 +0200 Subject: [PATCH 04/10] Update crates/ruff_python_formatter/src/expression/mod.rs Co-authored-by: Micha Reiser --- crates/ruff_python_formatter/src/expression/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/ruff_python_formatter/src/expression/mod.rs b/crates/ruff_python_formatter/src/expression/mod.rs index 2ef78cf65b504..8120947b131ff 100644 --- a/crates/ruff_python_formatter/src/expression/mod.rs +++ b/crates/ruff_python_formatter/src/expression/mod.rs @@ -203,9 +203,9 @@ fn format_with_parentheses_comments( // ) // ) // ``` - let right_tokenizer = SimpleTokenizer::new( + let right_tokenizer = SimpleTokenizer::starts_at( + expression.end(), f.context().source(), - TextRange::new(expression.end(), f.context().source().text_len()), ) .skip_trivia() .take_while(|token| token.kind == SimpleTokenKind::RParen); From c2251513a261ac018fab54b5d70294d0b65ffbda Mon Sep 17 00:00:00 2001 From: konstin Date: Tue, 17 Oct 2023 12:30:27 +0200 Subject: [PATCH 05/10] Review --- .../src/expression/mod.rs | 36 +++++++++---------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/crates/ruff_python_formatter/src/expression/mod.rs b/crates/ruff_python_formatter/src/expression/mod.rs index 8120947b131ff..f7804079fa76f 100644 --- a/crates/ruff_python_formatter/src/expression/mod.rs +++ b/crates/ruff_python_formatter/src/expression/mod.rs @@ -12,7 +12,7 @@ use ruff_python_trivia::{BackwardsTokenizer, CommentRanges, SimpleTokenKind, Sim use ruff_text_size::{Ranged, TextLen, TextRange}; use crate::builders::parenthesize_if_expands; -use crate::comments::{leading_comments, trailing_comments, Comments}; +use crate::comments::{leading_comments, trailing_comments, LeadingDanglingTrailingComments}; use crate::context::{NodeLevel, WithNodeLevel}; use crate::expression::parentheses::{ is_expression_parenthesized, optional_parentheses, parenthesized, NeedsParentheses, @@ -113,11 +113,12 @@ impl FormatRule> for FormatExpr { Parentheses::Never => false, }; if parenthesize { - let comments = f.context().comments().clone(); - if !comments.has_leading(expression) && !comments.has_trailing(expression) { + let comment = f.context().comments().clone(); + let node_comments = comment.leading_dangling_trailing(expression); + if !node_comments.has_leading() && !node_comments.has_trailing() { parenthesized("(", &format_expr, ")").fmt(f) } else { - format_with_parentheses_comments(expression, &comments, f) + format_with_parentheses_comments(expression, &node_comments, f) } } else { let level = match f.context().node_level() { @@ -181,7 +182,7 @@ impl FormatRule> for FormatExpr { /// outermost parentheses the relevant ones and discard the others. fn format_with_parentheses_comments( expression: &Expr, - comments: &Comments, + node_comments: &LeadingDanglingTrailingComments, f: &mut PyFormatter, ) -> FormatResult<()> { // First part: Split the comments @@ -226,23 +227,22 @@ fn format_with_parentheses_comments( .last() .map(|(right, left)| TextRange::new(left.start(), right.end())); - let leading = comments.leading(expression); - let trailing = comments.trailing(expression); - let (leading_split, trailing_split) = if let Some(range_with_parens) = range_with_parens { - let leading_split = - leading.partition_point(|comment| comment.start() < range_with_parens.start()); - let trailing_split = - trailing.partition_point(|comment| comment.start() < range_with_parens.end()); + let leading_split = node_comments + .leading + .partition_point(|comment| comment.start() < range_with_parens.start()); + let trailing_split = node_comments + .trailing + .partition_point(|comment| comment.start() < range_with_parens.end()); (leading_split, trailing_split) } else { - (0, trailing.len()) + (0, node_comments.trailing.len()) }; - let leading_outer = &leading[..leading_split]; - let leading_inner = &leading[leading_split..]; - let trailing_inner = &trailing[..trailing_split]; - let trailing_outer = &trailing[trailing_split..]; + let leading_outer = &node_comments.leading[..leading_split]; + let leading_inner = &node_comments.leading[leading_split..]; + let trailing_inner = &node_comments.trailing[..trailing_split]; + let trailing_outer = &node_comments.trailing[trailing_split..]; // Preserve an opening parentheses comment // ```python @@ -255,7 +255,7 @@ fn format_with_parentheses_comments( Some((first, rest)) if first.line_position().is_end_of_line() => { (slice::from_ref(first), rest) } - _ => (Default::default(), leading), + _ => (Default::default(), node_comments.leading), }; // Second Part: Format From b8587254bde4732a91731a240ba02a570727416f Mon Sep 17 00:00:00 2001 From: konstin Date: Tue, 17 Oct 2023 12:31:21 +0200 Subject: [PATCH 06/10] clippy --- crates/ruff_python_formatter/src/expression/mod.rs | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/crates/ruff_python_formatter/src/expression/mod.rs b/crates/ruff_python_formatter/src/expression/mod.rs index f7804079fa76f..4ee420a5a61f8 100644 --- a/crates/ruff_python_formatter/src/expression/mod.rs +++ b/crates/ruff_python_formatter/src/expression/mod.rs @@ -9,7 +9,7 @@ use ruff_python_ast::visitor::preorder::{walk_expr, PreorderVisitor}; use ruff_python_ast::AnyNodeRef; use ruff_python_ast::{Constant, Expr, ExpressionRef, Operator}; use ruff_python_trivia::{BackwardsTokenizer, CommentRanges, SimpleTokenKind, SimpleTokenizer}; -use ruff_text_size::{Ranged, TextLen, TextRange}; +use ruff_text_size::{Ranged, TextRange}; use crate::builders::parenthesize_if_expands; use crate::comments::{leading_comments, trailing_comments, LeadingDanglingTrailingComments}; @@ -204,12 +204,9 @@ fn format_with_parentheses_comments( // ) // ) // ``` - let right_tokenizer = SimpleTokenizer::starts_at( - expression.end(), - f.context().source(), - ) - .skip_trivia() - .take_while(|token| token.kind == SimpleTokenKind::RParen); + let right_tokenizer = SimpleTokenizer::starts_at(expression.end(), f.context().source()) + .skip_trivia() + .take_while(|token| token.kind == SimpleTokenKind::RParen); let left_tokenizer = BackwardsTokenizer::up_to( expression.start(), From 6fd44eecdbd827cc22f6bcf35e360d2e7efc841a Mon Sep 17 00:00:00 2001 From: konstin Date: Tue, 17 Oct 2023 12:48:05 +0200 Subject: [PATCH 07/10] comment --- crates/ruff_python_formatter/src/expression/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/ruff_python_formatter/src/expression/mod.rs b/crates/ruff_python_formatter/src/expression/mod.rs index 4ee420a5a61f8..1eb15b9f04865 100644 --- a/crates/ruff_python_formatter/src/expression/mod.rs +++ b/crates/ruff_python_formatter/src/expression/mod.rs @@ -187,8 +187,8 @@ fn format_with_parentheses_comments( ) -> FormatResult<()> { // First part: Split the comments - // TODO: This is copied from `parenthesized_range`, except that we don't have the parent, which - // is a problem: + // TODO(konstin): This is copied from `parenthesized_range`, except that we don't have the + // parent, which is a problem: // ```python // f( // # a From 3a45c14e29f729a4b15a1dc089d4a241148ef091 Mon Sep 17 00:00:00 2001 From: konsti Date: Thu, 19 Oct 2023 10:19:47 +0200 Subject: [PATCH 08/10] Update crates/ruff_python_formatter/src/expression/mod.rs Co-authored-by: Micha Reiser --- crates/ruff_python_formatter/src/expression/mod.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/crates/ruff_python_formatter/src/expression/mod.rs b/crates/ruff_python_formatter/src/expression/mod.rs index 1eb15b9f04865..3e6275b7a1c6b 100644 --- a/crates/ruff_python_formatter/src/expression/mod.rs +++ b/crates/ruff_python_formatter/src/expression/mod.rs @@ -236,10 +236,8 @@ fn format_with_parentheses_comments( (0, node_comments.trailing.len()) }; - let leading_outer = &node_comments.leading[..leading_split]; - let leading_inner = &node_comments.leading[leading_split..]; - let trailing_inner = &node_comments.trailing[..trailing_split]; - let trailing_outer = &node_comments.trailing[trailing_split..]; + let (leading_outer, leading_inner) = node_comments.leading.split_at(leading_split); + let (trailing_inner, trailing_outer) = node_comments.trailing.split_at(trailing_split); // Preserve an opening parentheses comment // ```python From 3dea4268e9ebffddaf40d2af632c52324ba59406 Mon Sep 17 00:00:00 2001 From: konsti Date: Thu, 19 Oct 2023 10:20:03 +0200 Subject: [PATCH 09/10] Update crates/ruff_python_formatter/src/expression/mod.rs Co-authored-by: Micha Reiser --- crates/ruff_python_formatter/src/expression/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ruff_python_formatter/src/expression/mod.rs b/crates/ruff_python_formatter/src/expression/mod.rs index 3e6275b7a1c6b..661ec44e145ed 100644 --- a/crates/ruff_python_formatter/src/expression/mod.rs +++ b/crates/ruff_python_formatter/src/expression/mod.rs @@ -250,7 +250,7 @@ fn format_with_parentheses_comments( Some((first, rest)) if first.line_position().is_end_of_line() => { (slice::from_ref(first), rest) } - _ => (Default::default(), node_comments.leading), + _ => (&[], node_comments.leading), }; // Second Part: Format From e4ef5b040bf8a34fedef635c635711bc2f1ca81b Mon Sep 17 00:00:00 2001 From: konstin Date: Thu, 19 Oct 2023 11:04:18 +0200 Subject: [PATCH 10/10] Review: Introduce `parentheses_iterator` --- crates/ruff_python_ast/src/parenthesize.rs | 73 ++++++++++++------- .../src/expression/mod.rs | 35 +++------ 2 files changed, 57 insertions(+), 51 deletions(-) diff --git a/crates/ruff_python_ast/src/parenthesize.rs b/crates/ruff_python_ast/src/parenthesize.rs index 0cf8e57a7e6a2..36ec307d73bfc 100644 --- a/crates/ruff_python_ast/src/parenthesize.rs +++ b/crates/ruff_python_ast/src/parenthesize.rs @@ -4,35 +4,44 @@ use ruff_text_size::{Ranged, TextLen, TextRange}; use crate::AnyNodeRef; use crate::ExpressionRef; -/// Returns the [`TextRange`] of a given expression including parentheses, if the expression is -/// parenthesized; or `None`, if the expression is not parenthesized. -pub fn parenthesized_range( - expr: ExpressionRef, - parent: AnyNodeRef, - comment_ranges: &CommentRanges, - source: &str, -) -> Option { - // If the parent is a node that brings its own parentheses, exclude the closing parenthesis - // from our search range. Otherwise, we risk matching on calls, like `func(x)`, for which - // the open and close parentheses are part of the `Arguments` node. - // - // There are a few other nodes that may have their own parentheses, but are fine to exclude: - // - `Parameters`: The parameters to a function definition. Any expressions would represent - // default arguments, and so must be preceded by _at least_ the parameter name. As such, - // we won't mistake any parentheses for the opening and closing parentheses on the - // `Parameters` node itself. - // - `Tuple`: The elements of a tuple. The only risk is a single-element tuple (e.g., `(x,)`), - // which must have a trailing comma anyway. - let exclusive_parent_end = if parent.is_arguments() { - parent.end() - ")".text_len() +/// Returns an iterator over the ranges of the optional parentheses surrounding an expression. +/// +/// E.g. for `((f()))` with `f()` as expression, the iterator returns the ranges (1, 6) and (0, 7). +/// +/// Note that without a parent the range can be inaccurate, e.g. `f(a)` we falsely return a set of +/// parentheses around `a` even if the parentheses actually belong to `f`. That is why you should +/// generally prefer [`parenthesized_range`]. +pub fn parentheses_iterator<'a>( + expr: ExpressionRef<'a>, + parent: Option, + comment_ranges: &'a CommentRanges, + source: &'a str, +) -> impl Iterator + 'a { + let right_tokenizer = if let Some(parent) = parent { + // If the parent is a node that brings its own parentheses, exclude the closing parenthesis + // from our search range. Otherwise, we risk matching on calls, like `func(x)`, for which + // the open and close parentheses are part of the `Arguments` node. + // + // There are a few other nodes that may have their own parentheses, but are fine to exclude: + // - `Parameters`: The parameters to a function definition. Any expressions would represent + // default arguments, and so must be preceded by _at least_ the parameter name. As such, + // we won't mistake any parentheses for the opening and closing parentheses on the + // `Parameters` node itself. + // - `Tuple`: The elements of a tuple. The only risk is a single-element tuple (e.g., `(x,)`), + // which must have a trailing comma anyway. + let exclusive_parent_end = if parent.is_arguments() { + parent.end() - ")".text_len() + } else { + parent.end() + }; + SimpleTokenizer::new(source, TextRange::new(expr.end(), exclusive_parent_end)) } else { - parent.end() + SimpleTokenizer::starts_at(expr.end(), source) }; - let right_tokenizer = - SimpleTokenizer::new(source, TextRange::new(expr.end(), exclusive_parent_end)) - .skip_trivia() - .take_while(|token| token.kind == SimpleTokenKind::RParen); + let right_tokenizer = right_tokenizer + .skip_trivia() + .take_while(|token| token.kind == SimpleTokenKind::RParen); let left_tokenizer = BackwardsTokenizer::up_to(expr.start(), source, comment_ranges) .skip_trivia() @@ -43,6 +52,16 @@ pub fn parenthesized_range( // the `right_tokenizer` is exhausted. right_tokenizer .zip(left_tokenizer) - .last() .map(|(right, left)| TextRange::new(left.start(), right.end())) } + +/// Returns the [`TextRange`] of a given expression including parentheses, if the expression is +/// parenthesized; or `None`, if the expression is not parenthesized. +pub fn parenthesized_range( + expr: ExpressionRef, + parent: AnyNodeRef, + comment_ranges: &CommentRanges, + source: &str, +) -> Option { + parentheses_iterator(expr, Some(parent), comment_ranges, source).last() +} diff --git a/crates/ruff_python_formatter/src/expression/mod.rs b/crates/ruff_python_formatter/src/expression/mod.rs index 661ec44e145ed..0f6550654db23 100644 --- a/crates/ruff_python_formatter/src/expression/mod.rs +++ b/crates/ruff_python_formatter/src/expression/mod.rs @@ -5,11 +5,11 @@ use ruff_formatter::{ write, FormatOwnedWithRule, FormatRefWithRule, FormatRule, FormatRuleWithOptions, }; use ruff_python_ast as ast; +use ruff_python_ast::parenthesize::parentheses_iterator; use ruff_python_ast::visitor::preorder::{walk_expr, PreorderVisitor}; -use ruff_python_ast::AnyNodeRef; -use ruff_python_ast::{Constant, Expr, ExpressionRef, Operator}; -use ruff_python_trivia::{BackwardsTokenizer, CommentRanges, SimpleTokenKind, SimpleTokenizer}; -use ruff_text_size::{Ranged, TextRange}; +use ruff_python_ast::{AnyNodeRef, Constant, Expr, ExpressionRef, Operator}; +use ruff_python_trivia::CommentRanges; +use ruff_text_size::Ranged; use crate::builders::parenthesize_if_expands; use crate::comments::{leading_comments, trailing_comments, LeadingDanglingTrailingComments}; @@ -187,8 +187,7 @@ fn format_with_parentheses_comments( ) -> FormatResult<()> { // First part: Split the comments - // TODO(konstin): This is copied from `parenthesized_range`, except that we don't have the - // parent, which is a problem: + // TODO(konstin): We don't have the parent, which is a problem: // ```python // f( // # a @@ -204,25 +203,13 @@ fn format_with_parentheses_comments( // ) // ) // ``` - let right_tokenizer = SimpleTokenizer::starts_at(expression.end(), f.context().source()) - .skip_trivia() - .take_while(|token| token.kind == SimpleTokenKind::RParen); - - let left_tokenizer = BackwardsTokenizer::up_to( - expression.start(), - f.context().source(), + let range_with_parens = parentheses_iterator( + expression.into(), + None, f.context().comments().ranges(), + f.context().source(), ) - .skip_trivia() - .take_while(|token| token.kind == SimpleTokenKind::LParen); - - // Zip closing parenthesis with opening parenthesis. The order is intentional, as testing for - // closing parentheses is cheaper, and `zip` will avoid progressing the `left_tokenizer` if - // the `right_tokenizer` is exhausted. - let range_with_parens = right_tokenizer - .zip(left_tokenizer) - .last() - .map(|(right, left)| TextRange::new(left.start(), right.end())); + .last(); let (leading_split, trailing_split) = if let Some(range_with_parens) = range_with_parens { let leading_split = node_comments @@ -250,7 +237,7 @@ fn format_with_parentheses_comments( Some((first, rest)) if first.line_position().is_end_of_line() => { (slice::from_ref(first), rest) } - _ => (&[], node_comments.leading), + _ => (Default::default(), node_comments.leading), }; // Second Part: Format