Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ignore the open parentheses when measuring BestFitsParenthesize #8940

Closed
wants to merge 3 commits into from
Closed

Ignore the open parentheses when measuring BestFitsParenthesize #8940

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1de4fcd
Refactor the comment handling of a statement's last expression
MichaReiser Nov 30, 2023
7d948b2
Inline comments for type alias similar to assignment
MichaReiser Dec 1, 2023
d013b71
Ignore the open parentheses when measuring `BestFitsParenthesize`
MichaReiser Dec 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions crates/ruff_formatter/src/builders.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1451,11 +1451,12 @@ impl<Context> std::fmt::Debug for Group<'_, Context> {
}
}

/// Content that may get parenthesized if it exceeds the configured line width but only if the parenthesized
/// layout doesn't exceed the line width too, in which case it falls back to the flat layout.
/// Content that may get parenthesized if it exceeds the configured line width but only if the parenthesized content
/// doesn't exceed the line width too, in which case it falls back to the flat layout.
///
/// This IR is identical to the following [`best_fitting`] layout but is implemented as custom IR for
/// best performance.
/// This IR is almost identical to the following [`best_fitting`] layout but is implemented as custom IR for
/// best performance. It differs in that the open parentheses is ignored when testing if the content fits because
/// the goal is to only parenthesize the content if parenthesizing makes the content fit.
///
/// ```rust
/// # use ruff_formatter::prelude::*;
Expand Down
11 changes: 7 additions & 4 deletions crates/ruff_formatter/src/printer/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,9 @@ impl<'a> Printer<'a> {
args.with_measure_mode(MeasureMode::AllLines),
);

queue.extend_back(&[OPEN_PAREN, INDENT, HARD_LINE_BREAK]);
// Don't measure the opening parentheses. Only concerned whether parenthesizing
// makes the content fit.
queue.extend_back(&[INDENT, HARD_LINE_BREAK]);
let fits_expanded = self.fits(queue, stack)?;
queue.pop_slice();
stack.pop(TagKind::BestFitParenthesize)?;
Expand Down Expand Up @@ -1299,10 +1301,11 @@ impl<'a, 'print> FitsMeasurer<'a, 'print> {
self.state.line_width = 0;
self.state.pending_indent = unindented.indention();

return Ok(self.fits_text(Text::Token(")"), unindented));
// Don't measure the `)` similar to the open parentheses but increment the line width and indent level.
self.fits_text(Text::Token(")"), unindented);
} else {
self.stack.pop(TagKind::BestFitParenthesize)?;
}

self.stack.pop(TagKind::BestFitParenthesize)?;
}

FormatElement::Tag(StartConditionalGroup(group)) => {
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/type_alias.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,12 @@
type Xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx[Aaaaaaaaaaaaaaaaaaaaaaaaaaaa] = int
type Xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx[Aaaaaaaaaaaaaaaaaaaaaaaaaaaa, Bbbbbbbbbbbbb] = int
type Xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx = Tttttttttttttttttttttttttttttttttttttttttttttttttttttttt
type Xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx = Tttttttttttttttttttttttttttttttttttttttttttttttttttttttt # with comment

# long value
type X = Ttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttt
type X = Aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | Bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb | Ccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc
type XXXXXXXXXXXXX = Tttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttt # with comment

# soft keyword as alias name
type type = int
Expand Down
142 changes: 6 additions & 136 deletions crates/ruff_python_formatter/src/expression/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,7 @@ use ruff_python_trivia::CommentRanges;
use ruff_text_size::Ranged;

use crate::builders::parenthesize_if_expands;
use crate::comments::{
leading_comments, trailing_comments, LeadingDanglingTrailingComments, SourceComment,
};
use crate::comments::{leading_comments, trailing_comments, LeadingDanglingTrailingComments};
use crate::context::{NodeLevel, WithNodeLevel};
use crate::expression::expr_generator_exp::is_generator_parenthesized;
use crate::expression::expr_tuple::is_tuple_parenthesized;
Expand Down Expand Up @@ -434,106 +432,16 @@ impl Format<PyFormatContext<'_>> for MaybeParenthesizeExpression<'_> {
}

Parenthesize::IfBreaks => {
// Is the expression the last token in the parent statement.
// Excludes `await` and `yield` for which Black doesn't seem to apply the layout?
let last_expression = parent.is_stmt_assign()
|| parent.is_stmt_ann_assign()
|| parent.is_stmt_aug_assign()
|| parent.is_stmt_return();

// Format the statements and value's trailing end of line comments:
// * after the expression if the expression needs no parentheses (necessary or the `expand_parent` makes the group never fit).
// * inside the parentheses if the expression exceeds the line-width.
//
// ```python
// a = long # with_comment
// b = (
// short # with_comment
// )
//
// # formatted
// a = (
// long # with comment
// )
// b = short # with comment
// ```
// This matches Black's formatting with the exception that ruff applies this style also for
// attribute chains and non-fluent call expressions. See https://github.com/psf/black/issues/4001#issuecomment-1786681792
//
// This logic isn't implemented in [`place_comment`] by associating trailing statement comments to the expression because
// doing so breaks the suite empty lines formatting that relies on trailing comments to be stored on the statement.
let (inline_comments, expression_trailing_comments) = if last_expression
&& !(
// Ignore non-fluent attribute chains for black compatibility.
// See https://github.com/psf/black/issues/4001#issuecomment-1786681792
expression.is_attribute_expr()
|| expression.is_call_expr()
|| expression.is_yield_from_expr()
|| expression.is_yield_expr()
|| expression.is_await_expr()
) {
let parent_trailing_comments = comments.trailing(*parent);
let after_end_of_line = parent_trailing_comments
.partition_point(|comment| comment.line_position().is_end_of_line());
let (stmt_inline_comments, _) =
parent_trailing_comments.split_at(after_end_of_line);

let after_end_of_line = node_comments
.trailing
.partition_point(|comment| comment.line_position().is_end_of_line());

let (expression_inline_comments, expression_trailing_comments) =
node_comments.trailing.split_at(after_end_of_line);

(
OptionalParenthesesInlinedComments {
expression: expression_inline_comments,
statement: stmt_inline_comments,
},
expression_trailing_comments,
)
if node_comments.has_trailing() {
expression.format().with_options(Parentheses::Always).fmt(f)
} else {
(
OptionalParenthesesInlinedComments::default(),
node_comments.trailing,
)
};

if expression_trailing_comments.is_empty() {
// The group id is necessary because the nested expressions may reference it.
let group_id = f.group_id("optional_parentheses");
let f = &mut WithNodeLevel::new(NodeLevel::Expression(Some(group_id)), f);

best_fit_parenthesize(&format_with(|f| {
inline_comments.mark_formatted();

expression
.format()
.with_options(Parentheses::Never)
.fmt(f)?;

if !inline_comments.is_empty() {
// If the expressions exceeds the line width, format the comments in the parentheses
if_group_breaks(&inline_comments)
.with_group_id(Some(group_id))
.fmt(f)?;
}

Ok(())
}))
.with_group_id(Some(group_id))
.fmt(f)?;

if !inline_comments.is_empty() {
// If the line fits into the line width, format the comments after the parenthesized expression
if_group_fits_on_line(&inline_comments)
.with_group_id(Some(group_id))
.fmt(f)?;
}

Ok(())
} else {
expression.format().with_options(Parentheses::Always).fmt(f)
best_fit_parenthesize(&expression.format().with_options(Parentheses::Never))
.with_group_id(Some(group_id))
.fmt(f)
}
}
},
Expand Down Expand Up @@ -1248,41 +1156,3 @@ impl From<Operator> for OperatorPrecedence {
}
}
}

#[derive(Debug, Default)]
struct OptionalParenthesesInlinedComments<'a> {
expression: &'a [SourceComment],
statement: &'a [SourceComment],
}

impl<'a> OptionalParenthesesInlinedComments<'a> {
fn is_empty(&self) -> bool {
self.expression.is_empty() && self.statement.is_empty()
}

fn iter_comments(&self) -> impl Iterator<Item = &'a SourceComment> {
self.expression.iter().chain(self.statement)
}

fn mark_formatted(&self) {
for comment in self.iter_comments() {
comment.mark_formatted();
}
}
}

impl Format<PyFormatContext<'_>> for OptionalParenthesesInlinedComments<'_> {
fn fmt(&self, f: &mut Formatter<PyFormatContext<'_>>) -> FormatResult<()> {
for comment in self.iter_comments() {
comment.mark_unformatted();
}

write!(
f,
[
trailing_comments(self.expression),
trailing_comments(self.statement)
]
)
}
}
6 changes: 2 additions & 4 deletions crates/ruff_python_formatter/src/statement/stmt_ann_assign.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,8 @@ use ruff_formatter::write;
use ruff_python_ast::StmtAnnAssign;

use crate::comments::{SourceComment, SuppressionKind};

use crate::expression::maybe_parenthesize_expression;
use crate::expression::parentheses::Parenthesize;
use crate::prelude::*;
use crate::statement::stmt_assign::FormatStatementsLastExpression;
use crate::statement::trailing_semicolon;

#[derive(Default)]
Expand Down Expand Up @@ -33,7 +31,7 @@ impl FormatNodeRule<StmtAnnAssign> for FormatStmtAnnAssign {
space(),
token("="),
space(),
maybe_parenthesize_expression(value, item, Parenthesize::IfBreaks)
FormatStatementsLastExpression::new(value, item)
]
)?;
}
Expand Down
Loading
Loading