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.

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

Fix format named expr comments #5705

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,16 @@
if (
# 1
x # 2
# 2.5
:= # 3
# 3.5
y # 4
):
pass
...

# should not add brackets
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey. Just wanted to let you know that I'm working on changing NeedsParentheses to pass through the parent node. This should make it straightforward to implement the correct parentheses logic for the walrus operator.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

atm i can't even figure out what's causing these parens to be added. parenthesize gets set to "ifbreaks" and nothing is breaking...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR ships the infrastructure: #5708

The parentheses are added because FormatExpr first calls into NeedsParentheses to determine if parentheses are necessary

let parentheses = item.needs_parentheses(self.parenthesize, f.context());

And the implementation of NamedExpr returns Always

match default_expression_needs_parentheses(self.into(), parenthesize, context) {
// Unlike tuples, named expression parentheses are not part of the range even when
// mandatory. See [PEP 572](https://peps.python.org/pep-0572/) for details.
Parentheses::Optional => Parentheses::Always,
parentheses => parentheses,
}

We should implement the same rules as in https://github.com/psf/black/blob/d1248ca9beaf0ba526d265f4108836d89cf551b7/src/black/linegen.py#L1381-L1395 (requires access to the parent node)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aah. thanks

do i understand correctly that his is separate to (i.e. won't help with) parens for generator comprehensions (that we were discussing yesterday)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct. The problem is related but needs a separate fix (and I first need to figure out what even the expected output is... I have suspicion I know how it's supposed to behave but I haven't verified it yet)

if x := y:
...

y0 = (y1 := f(x))

Expand Down
65 changes: 65 additions & 0 deletions crates/ruff_python_formatter/src/comments/placement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ pub(super) fn place_comment<'a>(
handle_expr_if_comment,
handle_comprehension_comment,
handle_trailing_expression_starred_star_end_of_line_comment,
handle_walrus_comments,
];
for handler in HANDLERS {
comment = match handler(comment, locator) {
Expand Down Expand Up @@ -1232,6 +1233,70 @@ fn handle_trailing_expression_starred_star_end_of_line_comment<'a>(
CommentPlacement::leading(starred.as_any_node_ref(), comment)
}

/// Handle comments around `:=`
///
/// ```python
/// if (
/// x
/// # make trailing x (instead of leading y)
/// := # make dangling on the NamedExpr node
/// y
/// )
/// ...
/// ```
// TODO: can this also handle comments before colon in dicts
fn handle_walrus_comments<'a>(
comment: DecoratedComment<'a>,
locator: &Locator,
) -> CommentPlacement<'a> {
let AnyNodeRef::ExprNamedExpr(named) = comment.enclosing_node() else {
return CommentPlacement::Default(comment);
};

let walrus = find_only_token_in_range(
TextRange::new(named.target.range().end(), named.value.range().start()),
locator,
TokenKind::Walrus,
);

let is_own_line = comment.line_position().is_own_line();

// Comments between the target and the `:=`
// ```python
// [
// a
// # attach as trailing on the target `a` (instead of leading on the value `b`)
// :=
// in b
// ]
// ```
if comment.slice().end() < walrus.start() {
return if is_own_line {
CommentPlacement::trailing((&*named.target).into(), comment)
} else {
CommentPlacement::Default(comment)
};
}

// Comments after the `:=`
// ```python
// [
// a
// := # attach as dangling on the NamedExpr
// in b
// ]
// ```
if comment.slice().end() < named.value.range().start() {
return if is_own_line {
CommentPlacement::Default(comment)
} else {
return CommentPlacement::dangling(comment.enclosing_node(), comment);
};
}

CommentPlacement::Default(comment)
}

/// Looks for a token in the range that contains no other tokens except for parentheses outside
/// the expression ranges
fn find_only_token_in_range(range: TextRange, locator: &Locator, token_kind: TokenKind) -> Token {
Expand Down
44 changes: 33 additions & 11 deletions crates/ruff_python_formatter/src/expression/expr_named_expr.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
use crate::comments::dangling_comments;
use crate::context::PyFormatContext;
use crate::expression::parentheses::{
default_expression_needs_parentheses, NeedsParentheses, Parentheses, Parenthesize,
};
use crate::{AsFormat, FormatNodeRule, PyFormatter};
use ruff_formatter::prelude::{space, text};
use ruff_formatter::prelude::{soft_line_break, space, text};
use ruff_formatter::{write, Buffer, FormatResult};
use rustpython_parser::ast::ExprNamedExpr;

Expand All @@ -17,16 +18,37 @@ impl FormatNodeRule<ExprNamedExpr> for FormatExprNamedExpr {
value,
range: _,
} = item;
write!(
f,
[
target.format(),
space(),
text(":="),
space(),
value.format(),
]
)

write!(f, [target.format()])?;

let comments = f.context().comments().clone();
let trailing_target_comments = comments.trailing_comments(target.as_ref());
let leading_value_comments = comments.leading_comments(value.as_ref());
let dangling_item_comments = comments.dangling_comments(item);

if trailing_target_comments.is_empty() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i tried making a single write! call with these ifs inside, but that doesn't work since space() and soft_line_break have different types. is there some trick to make this work? (or do we prefer multiple write calls?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's format_with but personally i like this style better, multiple write calls are fine here, if required we can still optimize later

write!(f, [space()])?;
} else {
write!(f, [soft_line_break()])?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason for this being a soft line break?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as opposed to?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a hard_line_break, since as far as i can tell we always want to break after the comments if there are any. I realized it doesn't make a difference here because a trailing comment will expand the group, but it might be easier to read

}

write!(f, [text(":="), dangling_comments(dangling_item_comments)])?;

if leading_value_comments.is_empty() {
write!(f, [space()])?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens when there are dangling_item_comments but no leading leading_value_comments? this would make a good test case

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch. definitely something not what i expected. will continue investigating

} else {
write!(f, [soft_line_break()])?;
}
write!(f, [value.format()])
}

fn fmt_dangling_comments(
&self,
_node: &ExprNamedExpr,
_f: &mut PyFormatter,
) -> FormatResult<()> {
// Handled in `fmt_fields`
Ok(())
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
---
source: crates/ruff_python_formatter/src/trivia.rs
expression: test_case.tokens()
---
[
Token {
kind: Colon,
range: 0..1,
},
Token {
kind: Colon,
range: 1..2,
},
Token {
kind: Colon,
range: 2..3,
},
]
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
---
source: crates/ruff_python_formatter/src/trivia.rs
expression: test_case.tokens()
---
[
Token {
kind: Walrus,
range: 0..2,
},
Token {
kind: Walrus,
range: 2..4,
},
]
36 changes: 36 additions & 0 deletions crates/ruff_python_formatter/src/trivia.rs
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,10 @@ pub(crate) enum TokenKind {
/// `.`.
Dot,

/// `:=`.
/// TODO: name?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Walrus,

/// `else`
Else,

Expand Down Expand Up @@ -322,6 +326,15 @@ impl<'a> SimpleTokenizer<'a> {

'\\' => TokenKind::Continuation,

':' => {
if self.cursor.first() == '=' {
self.cursor.bump();
TokenKind::Walrus
} else {
TokenKind::Colon
}
}

c => {
let kind = if is_identifier_start(c) {
self.cursor.eat_while(is_identifier_continuation);
Expand Down Expand Up @@ -457,6 +470,9 @@ impl<'a> SimpleTokenizer<'a> {
self.cursor = savepoint;
TokenKind::Other
}
} else if c == '=' && self.cursor.last() == ':' {
self.cursor.bump_back();
TokenKind::Walrus
} else {
TokenKind::from_non_trivia_char(c)
};
Expand Down Expand Up @@ -687,6 +703,26 @@ mod tests {
test_case.assert_reverse_tokenization();
}

#[test]
fn tokenize_colon() {
let source = ":::";

let test_case = tokenize(source);

assert_debug_snapshot!(test_case.tokens());
test_case.assert_reverse_tokenization();
}

#[test]
fn tokenize_walrus() {
let source = ":=:=";

let test_case = tokenize(source);

assert_debug_snapshot!(test_case.tokens());
test_case.assert_reverse_tokenization();
}

#[test]
fn tokenize_continuation() {
let source = "( \\\n )";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,16 @@ y = 1
if (
# 1
x # 2
# 2.5
:= # 3
# 3.5
y # 4
):
pass
...

# should not add brackets
if x := y:
...

y0 = (y1 := f(x))

Expand All @@ -25,9 +31,17 @@ y = 1

if (
# 1
x := y # 2 # 3 # 4
x # 2
# 2.5
:= # 3
# 3.5
y # 4
):
pass
...

# should not add brackets
if (x := y):
...

y0 = (y1 := f(x))

Expand Down