From 1e6df19a351170ded4b1e1a4e068993107bd1047 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Tue, 12 Sep 2023 08:39:57 +0200 Subject: [PATCH] Bool expression comment placement (#7269) --- .../test/fixtures/ruff/expression/binary.py | 61 ++++ .../ruff/expression/boolean_operation.py | 83 +++++ .../src/comments/placement.rs | 44 +++ .../src/expression/binary_like.rs | 291 +++++++++++++++++- .../src/expression/expr_bin_op.rs | 2 +- .../src/expression/expr_bool_op.rs | 94 +----- .../src/expression/expr_compare.rs | 2 +- .../src/expression/expr_subscript.rs | 19 +- .../format@expression__binary.py.snap | 144 +++++++-- ...rmat@expression__boolean_operation.py.snap | 166 ++++++++++ .../format@expression__compare.py.snap | 12 +- .../format@parentheses__nested.py.snap | 3 +- 12 files changed, 776 insertions(+), 145 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 f94b2d3452bf5..1ea6d78e1591c 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 @@ -320,6 +320,14 @@ (1 << (n + 2*n-1 + i+j)) # NE-SW ordinal for j in rangen] +rowuses = [((1 << j) # column ordinal + )| + ( + # comment + (1 << (n + i-j + n-1))) | # NW-SE ordinal + (1 << (n + 2*n-1 + i+j)) # NE-SW ordinal + for j in rangen] + skip_bytes = ( header.timecnt * 5 # Transition times and types + header.typecnt * 6 # Local time type records @@ -328,3 +336,56 @@ + header.isstdcnt # Standard/wall indicators + header.isutcnt # UT/local indicators ) + + +if ( + (1 + 2) # test + or (3 + 4) # other + or (4 + 5) # more +): + pass + + +if ( + (1 and 2) # test + + (3 and 4) # other + + (4 and 5) # more +): + pass + + +if ( + (1 + 2) # test + < (3 + 4) # other + > (4 + 5) # more +): + pass + + z = ( + a + + + # a: extracts this comment + ( + # b: and this comment + ( + # c: formats it as part of the expression + x and y + ) + ) + ) + +z = ( + ( + + ( + + x and y + # a: formats it as part of the expression + + ) + # b: extracts this comment + + ) + # c: and this comment + + a +) diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/boolean_operation.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/boolean_operation.py index c85f6a4daa653..0293ff7da61f2 100644 --- a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/boolean_operation.py +++ b/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/boolean_operation.py @@ -102,3 +102,86 @@ def test(): and {k.lower(): v for k, v in self.items()} == {k.lower(): v for k, v in other.items()} ) + + + +if "_continue" in request.POST or ( + # Redirecting after "Save as new". + "_saveasnew" in request.POST + and self.save_as_continue + and self.has_change_permission(request, obj) +): + pass + + +if True: + if False: + if True: + if ( + self.validate_max + and self.total_form_count() - len(self.deleted_forms) > self.max_num + ) or self.management_form.cleaned_data[ + TOTAL_FORM_COUNT + ] > self.absolute_max: + pass + + +if True: + if ( + reference_field_name is None + or + # Unspecified to_field(s). + to_fields is None + or + # Reference to primary key. + ( + None in to_fields + and (reference_field is None or reference_field.primary_key) + ) + or + # Reference to field. + reference_field_name in to_fields + ): + pass + + +field = opts.get_field(name) +if ( + field.is_relation + and + # Generic foreign keys OR reverse relations + ((field.many_to_one and not field.related_model) or field.one_to_many) +): + pass + + +if True: + return ( + filtered.exists() + and + # It may happen that the object is deleted from the DB right after + # this check, causing the subsequent UPDATE to return zero matching + # rows. The same result can occur in some rare cases when the + # database returns zero despite the UPDATE being executed + # successfully (a row is matched and updated). In order to + # distinguish these two cases, the object's existence in the + # database is again checked for if the UPDATE query returns 0. + (filtered._update(values) > 0 or filtered.exists()) + ) + + +if (self._proc is not None + # has the child process finished? + and self._returncode is None + # the child process has finished, but the + # transport hasn't been notified yet? + and self._proc.poll() is None): + pass + +if (self._proc + # has the child process finished? + * self._returncode + # the child process has finished, but the + # transport hasn't been notified yet? + + self._proc.poll()): + pass diff --git a/crates/ruff_python_formatter/src/comments/placement.rs b/crates/ruff_python_formatter/src/comments/placement.rs index e1f299e531ab2..f240cd0ffb36f 100644 --- a/crates/ruff_python_formatter/src/comments/placement.rs +++ b/crates/ruff_python_formatter/src/comments/placement.rs @@ -205,6 +205,9 @@ fn handle_enclosed_comment<'a>( locator, ) } + AnyNodeRef::ExprBoolOp(_) | AnyNodeRef::ExprCompare(_) => { + handle_trailing_binary_like_comment(comment, locator) + } AnyNodeRef::Keyword(keyword) => handle_keyword_comment(comment, keyword, locator), AnyNodeRef::PatternKeyword(pattern_keyword) => { handle_pattern_keyword_comment(comment, pattern_keyword, locator) @@ -836,6 +839,47 @@ fn handle_trailing_binary_expression_left_or_operator_comment<'a>( } } +/// Attaches comments between two bool or compare expression operands to the preceding operand if the comment is before the operator. +/// +/// ```python +/// a = ( +/// 5 > 3 +/// # trailing comment +/// and 3 == 3 +/// ) +/// ``` +fn handle_trailing_binary_like_comment<'a>( + comment: DecoratedComment<'a>, + locator: &Locator, +) -> CommentPlacement<'a> { + debug_assert!( + comment.enclosing_node().is_expr_bool_op() || comment.enclosing_node().is_expr_compare() + ); + + // Only if there's a preceding node (in which case, the preceding node is `left` or middle node). + let (Some(left_operand), Some(right_operand)) = + (comment.preceding_node(), comment.following_node()) + else { + return CommentPlacement::Default(comment); + }; + + let between_operands_range = TextRange::new(left_operand.end(), right_operand.start()); + + let mut tokens = SimpleTokenizer::new(locator.contents(), between_operands_range) + .skip_trivia() + .skip_while(|token| token.kind == SimpleTokenKind::RParen); + let operator_offset = tokens + .next() + .expect("Expected a token for the operator") + .start(); + + if comment.end() < operator_offset { + CommentPlacement::trailing(left_operand, comment) + } else { + CommentPlacement::Default(comment) + } +} + /// Handles own line comments on the module level before a class or function statement. /// A comment only becomes the leading comment of a class or function if it isn't separated by an empty /// line from the class. Comments that are separated by at least one empty line from the header of the diff --git a/crates/ruff_python_formatter/src/expression/binary_like.rs b/crates/ruff_python_formatter/src/expression/binary_like.rs index 09cfbc260480f..2f0989c1e8ea0 100644 --- a/crates/ruff_python_formatter/src/expression/binary_like.rs +++ b/crates/ruff_python_formatter/src/expression/binary_like.rs @@ -5,14 +5,18 @@ use smallvec::SmallVec; use ruff_formatter::write; use ruff_python_ast::{ - Constant, Expr, ExprAttribute, ExprBinOp, ExprCompare, ExprConstant, ExprUnaryOp, UnaryOp, + Constant, Expr, ExprAttribute, ExprBinOp, ExprBoolOp, ExprCompare, ExprConstant, ExprUnaryOp, + UnaryOp, }; +use ruff_python_trivia::{SimpleToken, SimpleTokenKind, SimpleTokenizer}; +use ruff_text_size::{Ranged, TextRange}; use crate::comments::{leading_comments, trailing_comments, Comments, SourceComment}; use crate::expression::parentheses::{ in_parentheses_only_group, in_parentheses_only_soft_line_break, in_parentheses_only_soft_line_break_or_space, is_expression_parenthesized, write_in_parentheses_only_group_end_tag, write_in_parentheses_only_group_start_tag, + Parentheses, }; use crate::expression::string::{AnyString, FormatString, StringLayout}; use crate::expression::OperatorPrecedence; @@ -20,8 +24,9 @@ use crate::prelude::*; #[derive(Copy, Clone, Debug)] pub(super) enum BinaryLike<'a> { - BinaryExpression(&'a ExprBinOp), - CompareExpression(&'a ExprCompare), + Binary(&'a ExprBinOp), + Compare(&'a ExprCompare), + Bool(&'a ExprBoolOp), } impl<'a> BinaryLike<'a> { @@ -84,6 +89,54 @@ impl<'a> BinaryLike<'a> { } } + fn recurse_bool<'a>( + bool_expression: &'a ExprBoolOp, + leading_comments: &'a [SourceComment], + trailing_comments: &'a [SourceComment], + comments: &'a Comments, + source: &str, + parts: &mut SmallVec<[OperandOrOperator<'a>; 8]>, + ) { + parts.reserve(bool_expression.values.len() * 2 - 1); + + if let Some((left, rest)) = bool_expression.values.split_first() { + rec( + Operand::Left { + expression: left, + leading_comments, + }, + comments, + source, + parts, + ); + + parts.push(OperandOrOperator::Operator(Operator { + symbol: OperatorSymbol::Bool(bool_expression.op), + trailing_comments: &[], + })); + + if let Some((right, middle)) = rest.split_last() { + for expression in middle { + rec(Operand::Middle { expression }, comments, source, parts); + parts.push(OperandOrOperator::Operator(Operator { + symbol: OperatorSymbol::Bool(bool_expression.op), + trailing_comments: &[], + })); + } + + rec( + Operand::Right { + expression: right, + trailing_comments, + }, + comments, + source, + parts, + ); + } + } + } + fn recurse_binary<'a>( binary: &'a ExprBinOp, leading_comments: &'a [SourceComment], @@ -164,6 +217,26 @@ impl<'a> BinaryLike<'a> { parts, ); } + Expr::BoolOp(bool_op) + if !is_expression_parenthesized(expression.into(), source) => + { + let leading_comments = operand + .leading_binary_comments() + .unwrap_or_else(|| comments.leading(bool_op)); + + let trailing_comments = operand + .trailing_binary_comments() + .unwrap_or_else(|| comments.trailing(bool_op)); + + recurse_bool( + bool_op, + leading_comments, + trailing_comments, + comments, + source, + parts, + ); + } _ => { parts.push(OperandOrOperator::Operand(operand)); } @@ -172,18 +245,25 @@ impl<'a> BinaryLike<'a> { let mut parts = SmallVec::new(); match self { - BinaryLike::BinaryExpression(binary) => { + BinaryLike::Binary(binary) => { // Leading and trailing comments are handled by the binary's ``FormatNodeRule` implementation. recurse_binary(binary, &[], &[], comments, source, &mut parts); } - BinaryLike::CompareExpression(compare) => { + BinaryLike::Compare(compare) => { // Leading and trailing comments are handled by the compare's ``FormatNodeRule` implementation. recurse_compare(compare, &[], &[], comments, source, &mut parts); } + BinaryLike::Bool(bool) => { + recurse_bool(bool, &[], &[], comments, source, &mut parts); + } } FlatBinaryExpression(parts) } + + const fn is_bool_op(self) -> bool { + matches!(self, BinaryLike::Bool(_)) + } } impl Format> for BinaryLike<'_> { @@ -191,6 +271,10 @@ impl Format> for BinaryLike<'_> { let comments = f.context().comments().clone(); let flat_binary = self.flatten(&comments, f.context().source()); + if self.is_bool_op() { + return in_parentheses_only_group(&&*flat_binary).fmt(f); + } + let source = f.context().source(); let mut string_operands = flat_binary .operands() @@ -265,8 +349,10 @@ impl Format> for BinaryLike<'_> { // previous iteration) write_in_parentheses_only_group_end_tag(f); - if operand.has_leading_comments(f.context().comments()) - || left_operator.has_trailing_comments() + if operand.has_unparenthesized_leading_comments( + f.context().comments(), + f.context().source(), + ) || left_operator.has_trailing_comments() { hard_line_break().fmt(f)?; } else { @@ -314,8 +400,11 @@ impl Format> for BinaryLike<'_> { if let Some(right_operator) = flat_binary.get_operator(index.right_operator()) { write_in_parentheses_only_group_start_tag(f); let right_operand = &flat_binary[right_operator_index.right_operand()]; - let right_operand_has_leading_comments = - right_operand.has_leading_comments(f.context().comments()); + let right_operand_has_leading_comments = right_operand + .has_unparenthesized_leading_comments( + f.context().comments(), + f.context().source(), + ); // Keep the operator on the same line if the right side has leading comments (and thus, breaks) if right_operand_has_leading_comments { @@ -326,7 +415,11 @@ impl Format> for BinaryLike<'_> { right_operator.fmt(f)?; - if right_operand_has_leading_comments + if (right_operand_has_leading_comments + && !is_expression_parenthesized( + right_operand.expression().into(), + f.context().source(), + )) || right_operator.has_trailing_comments() { hard_line_break().fmt(f)?; @@ -540,7 +633,7 @@ impl Format> for FlatBinaryExpressionSlice<'_> { fn fmt(&self, f: &mut Formatter) -> FormatResult<()> { // Single operand slice if let [OperandOrOperator::Operand(operand)] = &self.0 { - return operand.expression().format().fmt(f); + return operand.fmt(f); } let mut last_operator: Option = None; @@ -577,10 +670,11 @@ impl Format> for FlatBinaryExpressionSlice<'_> { operator_part.fmt(f)?; // Format the operator on its own line if the right side has any leading comments. - if right - .first_operand() - .has_leading_comments(f.context().comments()) - || operator_part.has_trailing_comments() + if operator_part.has_trailing_comments() + || right.first_operand().has_unparenthesized_leading_comments( + f.context().comments(), + f.context().source(), + ) { hard_line_break().fmt(f)?; } else if !is_pow { @@ -682,13 +776,33 @@ impl<'a> Operand<'a> { } } - fn has_leading_comments(&self, comments: &Comments) -> bool { + /// Returns `true` if the operand has any leading comments that are not parenthesized. + fn has_unparenthesized_leading_comments(&self, comments: &Comments, source: &str) -> bool { match self { Operand::Left { leading_comments, .. } => !leading_comments.is_empty(), Operand::Middle { expression } | Operand::Right { expression, .. } => { - comments.has_leading(*expression) + let leading = comments.leading(*expression); + if is_expression_parenthesized((*expression).into(), source) { + leading.iter().any(|comment| { + !comment.is_formatted() + && matches!( + SimpleTokenizer::new( + source, + TextRange::new(comment.end(), expression.start()), + ) + .skip_trivia() + .next(), + Some(SimpleToken { + kind: SimpleTokenKind::LParen, + .. + }) + ) + }) + } else { + !leading.is_empty() + } } } } @@ -713,6 +827,146 @@ impl<'a> Operand<'a> { } } +impl Format> for Operand<'_> { + fn fmt(&self, f: &mut Formatter>) -> FormatResult<()> { + let expression = self.expression(); + + return if is_expression_parenthesized(expression.into(), f.context().source()) { + let comments = f.context().comments().clone(); + let expression_comments = comments.leading_dangling_trailing(expression); + + // Format leading comments that are before the inner most `(` outside of the expression's parentheses. + // ```python + // z = ( + // a + // + + // # a: extracts this comment + // ( + // # b: and this comment + // ( + // # c: formats it as part of the expression + // x and y + // ) + // ) + // ) + // ``` + // + // Gets formatted as + // ```python + // z = ( + // a + // + + // # a: extracts this comment + // # b: and this comment + // ( + // # c: formats it as part of the expression + // x and y + // ) + // ) + // ``` + let leading = expression_comments.leading; + let leading_before_parentheses_end = leading + .iter() + .rposition(|comment| { + comment.is_unformatted() + && matches!( + SimpleTokenizer::new( + f.context().source(), + TextRange::new(comment.end(), expression.start()), + ) + .skip_trivia() + .next(), + Some(SimpleToken { + kind: SimpleTokenKind::LParen, + .. + }) + ) + }) + .map_or(0, |position| position + 1); + + let leading_before_parentheses = &leading[..leading_before_parentheses_end]; + + // Format trailing comments that are outside of the inner most `)` outside of the parentheses. + // ```python + // z = ( + // ( + // + // ( + // + // x and y + // # a: extracts this comment + // ) + // # b: and this comment + // ) + // # c: formats it as part of the expression + // + a + // ) + // ``` + // Gets formatted as + // ```python + // z = ( + // ( + // x and y + // # a: extracts this comment + // ) + // # b: and this comment + // # c: formats it as part of the expression + // + a + // ) + // ``` + let trailing = expression_comments.trailing; + + let trailing_after_parentheses_start = trailing + .iter() + .position(|comment| { + comment.is_unformatted() + && matches!( + SimpleTokenizer::new( + f.context().source(), + TextRange::new(expression.end(), comment.start()), + ) + .skip_trivia() + .next(), + Some(SimpleToken { + kind: SimpleTokenKind::RParen, + .. + }) + ) + }) + .unwrap_or(trailing.len()); + + let trailing_after_parentheses = &trailing[trailing_after_parentheses_start..]; + + // Mark the comment as formatted to avoid that the formatting of the expression + // formats the trailing comment inside of the parentheses. + for comment in trailing_after_parentheses { + comment.mark_formatted(); + } + + if !leading_before_parentheses.is_empty() { + leading_comments(leading_before_parentheses).fmt(f)?; + } + + expression + .format() + .with_options(Parentheses::Always) + .fmt(f)?; + + for comment in trailing_after_parentheses { + comment.mark_unformatted(); + } + + if !trailing_after_parentheses.is_empty() { + trailing_comments(trailing_after_parentheses).fmt(f)?; + } + + Ok(()) + } else { + expression.format().with_options(Parentheses::Never).fmt(f) + }; + } +} + #[derive(Debug)] struct Operator<'a> { symbol: OperatorSymbol, @@ -739,6 +993,7 @@ impl Format> for Operator<'_> { enum OperatorSymbol { Binary(ruff_python_ast::Operator), Comparator(ruff_python_ast::CmpOp), + Bool(ruff_python_ast::BoolOp), } impl OperatorSymbol { @@ -750,6 +1005,7 @@ impl OperatorSymbol { match self { OperatorSymbol::Binary(operator) => OperatorPrecedence::from(operator), OperatorSymbol::Comparator(_) => OperatorPrecedence::Comparator, + OperatorSymbol::Bool(_) => OperatorPrecedence::BooleanOperation, } } } @@ -759,6 +1015,7 @@ impl Format> for OperatorSymbol { match self { OperatorSymbol::Binary(operator) => operator.format().fmt(f), OperatorSymbol::Comparator(operator) => operator.format().fmt(f), + OperatorSymbol::Bool(bool) => bool.format().fmt(f), } } } diff --git a/crates/ruff_python_formatter/src/expression/expr_bin_op.rs b/crates/ruff_python_formatter/src/expression/expr_bin_op.rs index 3a87b6d69191f..8601f6b1df555 100644 --- a/crates/ruff_python_formatter/src/expression/expr_bin_op.rs +++ b/crates/ruff_python_formatter/src/expression/expr_bin_op.rs @@ -14,7 +14,7 @@ pub struct FormatExprBinOp; impl FormatNodeRule for FormatExprBinOp { #[inline] fn fmt_fields(&self, item: &ExprBinOp, f: &mut PyFormatter) -> FormatResult<()> { - BinaryLike::BinaryExpression(item).fmt(f) + BinaryLike::Binary(item).fmt(f) } fn fmt_dangling_comments( diff --git a/crates/ruff_python_formatter/src/expression/expr_bool_op.rs b/crates/ruff_python_formatter/src/expression/expr_bool_op.rs index ee96ddd369c23..27e511dd96b98 100644 --- a/crates/ruff_python_formatter/src/expression/expr_bool_op.rs +++ b/crates/ruff_python_formatter/src/expression/expr_bool_op.rs @@ -1,80 +1,18 @@ -use ruff_formatter::{write, FormatOwnedWithRule, FormatRefWithRule, FormatRuleWithOptions}; +use ruff_formatter::{FormatOwnedWithRule, FormatRefWithRule}; use ruff_python_ast::node::AnyNodeRef; -use ruff_python_ast::{BoolOp, Expr, ExprBoolOp}; +use ruff_python_ast::{BoolOp, ExprBoolOp}; -use crate::comments::leading_comments; -use crate::expression::parentheses::{ - in_parentheses_only_group, in_parentheses_only_soft_line_break_or_space, NeedsParentheses, - OptionalParentheses, -}; +use crate::expression::binary_like::BinaryLike; +use crate::expression::parentheses::{NeedsParentheses, OptionalParentheses}; use crate::prelude::*; -use super::parentheses::is_expression_parenthesized; - #[derive(Default)] -pub struct FormatExprBoolOp { - layout: BoolOpLayout, -} - -#[derive(Default, Copy, Clone)] -pub enum BoolOpLayout { - #[default] - Default, - Chained, -} - -impl FormatRuleWithOptions> for FormatExprBoolOp { - type Options = BoolOpLayout; - fn with_options(mut self, options: Self::Options) -> Self { - self.layout = options; - self - } -} +pub struct FormatExprBoolOp; impl FormatNodeRule for FormatExprBoolOp { + #[inline] fn fmt_fields(&self, item: &ExprBoolOp, f: &mut PyFormatter) -> FormatResult<()> { - let ExprBoolOp { - range: _, - op, - values, - } = item; - - let inner = format_with(|f: &mut PyFormatter| { - let mut values = values.iter(); - let comments = f.context().comments().clone(); - - let Some(first) = values.next() else { - return Ok(()); - }; - - FormatValue { value: first }.fmt(f)?; - - for value in values { - let leading_value_comments = comments.leading(value); - // Format the expressions leading comments **before** the operator - if leading_value_comments.is_empty() { - write!(f, [in_parentheses_only_soft_line_break_or_space()])?; - } else { - write!( - f, - [hard_line_break(), leading_comments(leading_value_comments)] - )?; - } - - write!(f, [op.format(), space()])?; - - FormatValue { value }.fmt(f)?; - } - - Ok(()) - }); - - if matches!(self.layout, BoolOpLayout::Chained) { - // Chained boolean operations should not be given a new group - inner.fmt(f) - } else { - in_parentheses_only_group(&inner).fmt(f) - } + BinaryLike::Bool(item).fmt(f) } } @@ -88,24 +26,6 @@ impl NeedsParentheses for ExprBoolOp { } } -struct FormatValue<'a> { - value: &'a Expr, -} - -impl Format> for FormatValue<'_> { - fn fmt(&self, f: &mut PyFormatter) -> FormatResult<()> { - match self.value { - Expr::BoolOp(bool_op) - if !is_expression_parenthesized(bool_op.into(), f.context().source()) => - { - // Mark chained boolean operations e.g. `x and y or z` and avoid creating a new group - write!(f, [bool_op.format().with_options(BoolOpLayout::Chained)]) - } - _ => write!(f, [in_parentheses_only_group(&self.value.format())]), - } - } -} - #[derive(Copy, Clone)] pub struct FormatBoolOp; diff --git a/crates/ruff_python_formatter/src/expression/expr_compare.rs b/crates/ruff_python_formatter/src/expression/expr_compare.rs index 9ad639a049d51..8ca05d864a922 100644 --- a/crates/ruff_python_formatter/src/expression/expr_compare.rs +++ b/crates/ruff_python_formatter/src/expression/expr_compare.rs @@ -15,7 +15,7 @@ pub struct FormatExprCompare; impl FormatNodeRule for FormatExprCompare { #[inline] fn fmt_fields(&self, item: &ExprCompare, f: &mut PyFormatter) -> FormatResult<()> { - BinaryLike::CompareExpression(item).fmt(f) + BinaryLike::Compare(item).fmt(f) } fn fmt_dangling_comments( diff --git a/crates/ruff_python_formatter/src/expression/expr_subscript.rs b/crates/ruff_python_formatter/src/expression/expr_subscript.rs index a7f0fa306b51e..1e2a5c3d94f1b 100644 --- a/crates/ruff_python_formatter/src/expression/expr_subscript.rs +++ b/crates/ruff_python_formatter/src/expression/expr_subscript.rs @@ -1,12 +1,11 @@ -use ruff_formatter::{format_args, write, FormatRuleWithOptions}; +use ruff_formatter::{write, FormatRuleWithOptions}; use ruff_python_ast::node::{AnyNodeRef, AstNode}; use ruff_python_ast::{Expr, ExprSubscript}; -use crate::comments::{trailing_comments, SourceComment}; - +use crate::comments::SourceComment; use crate::context::{NodeLevel, WithNodeLevel}; use crate::expression::expr_tuple::TupleParentheses; -use crate::expression::parentheses::{NeedsParentheses, OptionalParentheses}; +use crate::expression::parentheses::{parenthesized, NeedsParentheses, OptionalParentheses}; use crate::expression::CallChainLayout; use crate::prelude::*; @@ -67,15 +66,9 @@ impl FormatNodeRule for FormatExprSubscript { } }); - write!( - f, - [group(&format_args![ - token("["), - trailing_comments(dangling_comments), - soft_block_indent(&format_slice), - token("]") - ])] - ) + parenthesized("[", &format_slice, "]") + .with_dangling_comments(dangling_comments) + .fmt(f) } fn fmt_dangling_comments( 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 648aae2e6ce54..2e8975b24848f 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 @@ -326,6 +326,14 @@ rowuses = [(1 << j) | # column ordinal (1 << (n + 2*n-1 + i+j)) # NE-SW ordinal for j in rangen] +rowuses = [((1 << j) # column ordinal + )| + ( + # comment + (1 << (n + i-j + n-1))) | # NW-SE ordinal + (1 << (n + 2*n-1 + i+j)) # NE-SW ordinal + for j in rangen] + skip_bytes = ( header.timecnt * 5 # Transition times and types + header.typecnt * 6 # Local time type records @@ -334,6 +342,59 @@ skip_bytes = ( + header.isstdcnt # Standard/wall indicators + header.isutcnt # UT/local indicators ) + + +if ( + (1 + 2) # test + or (3 + 4) # other + or (4 + 5) # more +): + pass + + +if ( + (1 and 2) # test + + (3 and 4) # other + + (4 and 5) # more +): + pass + + +if ( + (1 + 2) # test + < (3 + 4) # other + > (4 + 5) # more +): + pass + + z = ( + a + + + # a: extracts this comment + ( + # b: and this comment + ( + # c: formats it as part of the expression + x and y + ) + ) + ) + +z = ( + ( + + ( + + x and y + # a: formats it as part of the expression + + ) + # b: extracts this comment + + ) + # c: and this comment + + a +) ``` ## Output @@ -565,19 +626,15 @@ if [ ... -if ( - [ - fffffffffffffffff, - gggggggggggggggggggg, - hhhhhhhhhhhhhhhhhhhhh, - iiiiiiiiiiiiiiii, - jjjjjjjjjjjjj, - ] - & - ( - # comment - a + b - ) +if [ + fffffffffffffffff, + gggggggggggggggggggg, + hhhhhhhhhhhhhhhhhhhhh, + iiiiiiiiiiiiiiii, + jjjjjjjjjjjjj, +] & ( + # comment + a + b ): ... @@ -706,8 +763,7 @@ expected_content = ( """ - % - ( + % ( # Needs parentheses self.base_url ) @@ -715,14 +771,21 @@ expected_content = ( rowuses = [ - ( - 1 << j # column ordinal - ) + (1 << j) # column ordinal | (1 << (n + i - j + n - 1)) # NW-SE ordinal | (1 << (n + 2 * n - 1 + i + j)) # NE-SW ordinal for j in rangen ] +rowuses = [ + (1 << j) # column ordinal + | + # comment + (1 << (n + i - j + n - 1)) # NW-SE ordinal + | (1 << (n + 2 * n - 1 + i + j)) # NE-SW ordinal + for j in rangen +] + skip_bytes = ( header.timecnt * 5 # Transition times and types + header.typecnt * 6 # Local time type records @@ -731,6 +794,51 @@ skip_bytes = ( + header.isstdcnt # Standard/wall indicators + header.isutcnt # UT/local indicators ) + + +if ( + (1 + 2) # test + or (3 + 4) # other + or (4 + 5) # more +): + pass + + +if ( + (1 and 2) # test + + (3 and 4) # other + + (4 and 5) # more +): + pass + + +if ( + (1 + 2) # test + < (3 + 4) # other + > (4 + 5) # more +): + pass + +z = ( + a + + + # a: extracts this comment + # b: and this comment + ( + # c: formats it as part of the expression + x and y + ) +) + +z = ( + ( + x and y + # a: formats it as part of the expression + ) + # b: extracts this comment + # c: and this comment + + a +) ``` diff --git a/crates/ruff_python_formatter/tests/snapshots/format@expression__boolean_operation.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@expression__boolean_operation.py.snap index 3f3f171e494de..e62f59f804623 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@expression__boolean_operation.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@expression__boolean_operation.py.snap @@ -108,6 +108,89 @@ def test(): and {k.lower(): v for k, v in self.items()} == {k.lower(): v for k, v in other.items()} ) + + + +if "_continue" in request.POST or ( + # Redirecting after "Save as new". + "_saveasnew" in request.POST + and self.save_as_continue + and self.has_change_permission(request, obj) +): + pass + + +if True: + if False: + if True: + if ( + self.validate_max + and self.total_form_count() - len(self.deleted_forms) > self.max_num + ) or self.management_form.cleaned_data[ + TOTAL_FORM_COUNT + ] > self.absolute_max: + pass + + +if True: + if ( + reference_field_name is None + or + # Unspecified to_field(s). + to_fields is None + or + # Reference to primary key. + ( + None in to_fields + and (reference_field is None or reference_field.primary_key) + ) + or + # Reference to field. + reference_field_name in to_fields + ): + pass + + +field = opts.get_field(name) +if ( + field.is_relation + and + # Generic foreign keys OR reverse relations + ((field.many_to_one and not field.related_model) or field.one_to_many) +): + pass + + +if True: + return ( + filtered.exists() + and + # It may happen that the object is deleted from the DB right after + # this check, causing the subsequent UPDATE to return zero matching + # rows. The same result can occur in some rare cases when the + # database returns zero despite the UPDATE being executed + # successfully (a row is matched and updated). In order to + # distinguish these two cases, the object's existence in the + # database is again checked for if the UPDATE query returns 0. + (filtered._update(values) > 0 or filtered.exists()) + ) + + +if (self._proc is not None + # has the child process finished? + and self._returncode is None + # the child process has finished, but the + # transport hasn't been notified yet? + and self._proc.poll() is None): + pass + +if (self._proc + # has the child process finished? + * self._returncode + # the child process has finished, but the + # transport hasn't been notified yet? + + self._proc.poll()): + pass ``` ## Output @@ -234,6 +317,89 @@ def test(): return isinstance(other, Mapping) and {k.lower(): v for k, v in self.items()} == { k.lower(): v for k, v in other.items() } + + +if "_continue" in request.POST or ( + # Redirecting after "Save as new". + "_saveasnew" in request.POST + and self.save_as_continue + and self.has_change_permission(request, obj) +): + pass + + +if True: + if False: + if True: + if ( + self.validate_max + and self.total_form_count() - len(self.deleted_forms) > self.max_num + ) or self.management_form.cleaned_data[ + TOTAL_FORM_COUNT + ] > self.absolute_max: + pass + + +if True: + if ( + reference_field_name is None + or + # Unspecified to_field(s). + to_fields is None + or + # Reference to primary key. + (None in to_fields and (reference_field is None or reference_field.primary_key)) + or + # Reference to field. + reference_field_name in to_fields + ): + pass + + +field = opts.get_field(name) +if ( + field.is_relation + and + # Generic foreign keys OR reverse relations + ((field.many_to_one and not field.related_model) or field.one_to_many) +): + pass + + +if True: + return ( + filtered.exists() + and + # It may happen that the object is deleted from the DB right after + # this check, causing the subsequent UPDATE to return zero matching + # rows. The same result can occur in some rare cases when the + # database returns zero despite the UPDATE being executed + # successfully (a row is matched and updated). In order to + # distinguish these two cases, the object's existence in the + # database is again checked for if the UPDATE query returns 0. + (filtered._update(values) > 0 or filtered.exists()) + ) + + +if ( + self._proc is not None + # has the child process finished? + and self._returncode is None + # the child process has finished, but the + # transport hasn't been notified yet? + and self._proc.poll() is None +): + pass + +if ( + self._proc + # has the child process finished? + * self._returncode + # the child process has finished, but the + # transport hasn't been notified yet? + + self._proc.poll() +): + pass ``` diff --git a/crates/ruff_python_formatter/tests/snapshots/format@expression__compare.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@expression__compare.py.snap index 288046cbef8d3..bb3dabb8ac432 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@expression__compare.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@expression__compare.py.snap @@ -339,13 +339,13 @@ ct_match = ( == self.get_content_type[obj, rel_obj, using, instance._state.db].id ) -ct_match = { - aaaaaaaaaaaaaaaa -} == self.get_content_type[obj, rel_obj, using, instance._state.db].id +ct_match = {aaaaaaaaaaaaaaaa} == self.get_content_type[ + obj, rel_obj, using, instance._state.db +].id -ct_match = ( - aaaaaaaaaaaaaaaa -) == self.get_content_type[obj, rel_obj, using, instance._state.db].id +ct_match = (aaaaaaaaaaaaaaaa) == self.get_content_type[ + obj, rel_obj, using, instance._state.db +].id # comments diff --git a/crates/ruff_python_formatter/tests/snapshots/format@parentheses__nested.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@parentheses__nested.py.snap index 2106ed7f658e2..b9052fb1ac4e2 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@parentheses__nested.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@parentheses__nested.py.snap @@ -90,8 +90,7 @@ a = ( + b + c + d - + - ( # Hello + + ( # Hello e + f + g ) )