Skip to content

Commit

Permalink
Format all attribute dot comments manually
Browse files Browse the repository at this point in the history
  • Loading branch information
@charliermarsh
charliermarsh committed Aug 25, 2023
1 parent 281ce56 commit 6bcba68
Show file tree
Hide file tree
Showing 5 changed files with 215 additions and 127 deletions.
16 changes: 16 additions & 0 deletions crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/attribute.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,3 +109,19 @@
# regression: https://github.com/astral-sh/ruff/issues/6181
(#
()).a

(
(
a # trailing end-of-line
# trailing own-line
) # dangling before dot end-of-line
.b # trailing end-of-line
)

(
(
a
)
# dangling before dot own-line
.b
)
126 changes: 77 additions & 49 deletions crates/ruff_python_formatter/src/comments/placement.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@ use std::cmp::Ordering;
use ruff_python_ast::node::AnyNodeRef;
use ruff_python_ast::whitespace::indentation;
use ruff_python_ast::{self as ast, Comprehension, Expr, MatchCase, Parameters, Ranged};
use ruff_python_trivia::{indentation_at_offset, SimpleToken, SimpleTokenKind, SimpleTokenizer};
use ruff_python_trivia::{
find_only_token_in_range, indentation_at_offset, SimpleToken, SimpleTokenKind, SimpleTokenizer,
};
use ruff_source_file::Locator;
use ruff_text_size::{TextLen, TextRange};

Expand Down Expand Up @@ -177,7 +179,9 @@ fn handle_enclosed_comment<'a>(
AnyNodeRef::Comprehension(comprehension) => {
handle_comprehension_comment(comment, comprehension, locator)
}
AnyNodeRef::ExprAttribute(attribute) => handle_attribute_comment(comment, attribute),
AnyNodeRef::ExprAttribute(attribute) => {
handle_attribute_comment(comment, attribute, locator)
}
AnyNodeRef::ExprBinOp(binary_expression) => {
handle_trailing_binary_expression_left_or_operator_comment(
comment,
Expand Down Expand Up @@ -355,7 +359,7 @@ fn is_first_statement_in_body(statement: AnyNodeRef, has_body: AnyNodeRef) -> bo
| AnyNodeRef::ExceptHandlerExceptHandler(ast::ExceptHandlerExceptHandler {
body, ..
})
| AnyNodeRef::MatchCase(ast::MatchCase { body, .. })
| AnyNodeRef::MatchCase(MatchCase { body, .. })
| AnyNodeRef::StmtFunctionDef(ast::StmtFunctionDef { body, .. })
| AnyNodeRef::StmtClassDef(ast::StmtClassDef { body, .. }) => {
are_same_optional(statement, body.first())
Expand Down Expand Up @@ -974,47 +978,88 @@ fn handle_dict_unpacking_comment<'a>(
/// Own line comments coming after the node are always dangling comments
/// ```python
/// (
/// a
/// # trailing a comment
/// . # dangling comment
/// # or this
/// a # trailing comment on `a`
/// # dangling comment on the attribute
/// . # dangling comment on the attribute
/// # dangling comment on the attribute
/// b
/// )
/// ```
fn handle_attribute_comment<'a>(
comment: DecoratedComment<'a>,
attribute: &'a ast::ExprAttribute,
locator: &Locator,
) -> CommentPlacement<'a> {
if comment.preceding_node().is_none() {
// ```text
// ( value) . attr
// ^^^^ we're in this range
// ```

return CommentPlacement::leading(attribute.value.as_ref(), comment);
}

// ```text
// value . attr
// ^^^^^^^ we're in this range
// If the comment is parenthesized, use the parentheses to either attach it as a trailing
// comment on the value or a dangling comment on the attribute.
// For example, treat this as trailing:
// ```python
// (
// (
// value
// # comment
// )
// .attribute
// )
// ```
//
// However, treat this as dangling:
// ```python
// (
// (value)
// # comment
// .attribute
// )
// ```
if let Some(right_paren) = SimpleTokenizer::starts_at(attribute.value.end(), locator.contents())
.skip_trivia()
.take_while(|token| token.kind == SimpleTokenKind::RParen)
.last()
{
return if comment.start() < right_paren.start() {
CommentPlacement::trailing(attribute.value.as_ref(), comment)
} else {
CommentPlacement::dangling(comment.enclosing_node(), comment)
};
}

// If the comment precedes the `.`, treat it as trailing _if_ it's on the same line as the
// value. For example, treat this as trailing:
// ```python
// (
// value # comment
// .attribute
// )
// ```
//
// However, treat this as dangling:
// ```python
// (
// value
// # comment
// .attribute
// )
// ```
debug_assert!(
TextRange::new(attribute.value.end(), attribute.attr.start())
.contains(comment.slice().start())
);
if comment.line_position().is_end_of_line() {
// Attach as trailing comment to a. The specific placement is only relevant for fluent style
// ```python
// x322 = (
// a
// . # end-of-line dot comment 2
// b
// )
// ```
CommentPlacement::trailing(attribute.value.as_ref(), comment)
} else {
CommentPlacement::dangling(attribute, comment)
let dot_token = find_only_token_in_range(
TextRange::new(attribute.value.end(), attribute.attr.start()),
SimpleTokenKind::Dot,
locator.contents(),
);
if comment.end() < dot_token.start() {
return CommentPlacement::trailing(attribute.value.as_ref(), comment);
}
}

CommentPlacement::dangling(comment.enclosing_node(), comment)
}

/// Assign comments between `if` and `test` and `else` and `orelse` as leading to the respective
Expand Down Expand Up @@ -1051,7 +1096,7 @@ fn handle_expr_if_comment<'a>(
let if_token = find_only_token_in_range(
TextRange::new(body.end(), test.start()),
SimpleTokenKind::If,
locator,
locator.contents(),
);
// Between `if` and `test`
if if_token.range.start() < comment.slice().start() && comment.slice().start() < test.start() {
Expand All @@ -1061,7 +1106,7 @@ fn handle_expr_if_comment<'a>(
let else_token = find_only_token_in_range(
TextRange::new(test.end(), orelse.start()),
SimpleTokenKind::Else,
locator,
locator.contents(),
);
// Between `else` and `orelse`
if else_token.range.start() < comment.slice().start()
Expand Down Expand Up @@ -1133,7 +1178,7 @@ fn handle_with_item_comment<'a>(
let as_token = find_only_token_in_range(
TextRange::new(context_expr.end(), optional_vars.start()),
SimpleTokenKind::As,
locator,
locator.contents(),
);

if comment.end() < as_token.start() {
Expand Down Expand Up @@ -1232,7 +1277,7 @@ fn handle_named_expr_comment<'a>(
let colon_equal = find_only_token_in_range(
TextRange::new(target.end(), value.start()),
SimpleTokenKind::ColonEqual,
locator,
locator.contents(),
);

if comment.end() < colon_equal.start() {
Expand All @@ -1245,23 +1290,6 @@ fn handle_named_expr_comment<'a>(
}
}

/// 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,
token_kind: SimpleTokenKind,
locator: &Locator,
) -> SimpleToken {
let mut tokens = SimpleTokenizer::new(locator.contents(), range)
.skip_trivia()
.skip_while(|token| token.kind == SimpleTokenKind::RParen);
let token = tokens.next().expect("Expected a token");
debug_assert_eq!(token.kind(), token_kind);
let mut tokens = tokens.skip_while(|token| token.kind == SimpleTokenKind::LParen);
debug_assert_eq!(tokens.next(), None);
token
}

/// Attach an end-of-line comment immediately following an open bracket as a dangling comment on
/// enclosing node.
///
Expand Down Expand Up @@ -1456,7 +1484,7 @@ fn handle_comprehension_comment<'a>(
let in_token = find_only_token_in_range(
TextRange::new(comprehension.target.end(), comprehension.iter.start()),
SimpleTokenKind::In,
locator,
locator.contents(),
);

// Comments between the target and the `in`
Expand Down Expand Up @@ -1519,7 +1547,7 @@ fn handle_comprehension_comment<'a>(
let if_token = find_only_token_in_range(
TextRange::new(last_end, if_node.start()),
SimpleTokenKind::If,
locator,
locator.contents(),
);
if is_own_line {
if last_end < comment.slice().start() && comment.slice().start() < if_token.start() {
Expand Down
114 changes: 51 additions & 63 deletions crates/ruff_python_formatter/src/expression/expr_attribute.rs
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
use ruff_formatter::{write, FormatRuleWithOptions};
use ruff_python_ast::node::AnyNodeRef;
use ruff_python_ast::{Constant, Expr, ExprAttribute, ExprConstant};
use ruff_python_ast::{Constant, Expr, ExprAttribute, ExprConstant, Ranged};
use ruff_python_trivia::{find_only_token_in_range, SimpleTokenKind};
use ruff_text_size::TextRange;

use crate::comments::{leading_comments, trailing_comments, SourceComment};
use crate::comments::{dangling_comments, trailing_comments, SourceComment};
use crate::expression::parentheses::{
is_expression_parenthesized, NeedsParentheses, OptionalParentheses, Parentheses,
};
Expand Down Expand Up @@ -44,13 +46,6 @@ impl FormatNodeRule<ExprAttribute> for FormatExprAttribute {
})
);

let comments = f.context().comments().clone();
let dangling_comments = comments.dangling(item);
let leading_attribute_comments_start = dangling_comments
.partition_point(|comment| comment.line_position().is_end_of_line());
let (trailing_dot_comments, leading_attribute_comments) =
dangling_comments.split_at(leading_attribute_comments_start);

if needs_parentheses {
value.format().with_options(Parentheses::Always).fmt(f)?;
} else if call_chain_layout == CallChainLayout::Fluent {
Expand Down Expand Up @@ -88,55 +83,54 @@ impl FormatNodeRule<ExprAttribute> for FormatExprAttribute {
value.format().fmt(f)?;
}

if comments.has_trailing_own_line(value.as_ref()) {
hard_line_break().fmt(f)?;
}

if call_chain_layout == CallChainLayout::Fluent {
// Fluent style has line breaks before the dot
// ```python
// blogs3 = (
// Blog.objects.filter(
// entry__headline__contains="Lennon",
// )
// .filter(
// entry__pub_date__year=2008,
// )
// .filter(
// entry__pub_date__year=2008,
// )
// )
// ```
write!(
f,
[
(!leading_attribute_comments.is_empty()).then_some(hard_line_break()),
leading_comments(leading_attribute_comments),
text("."),
trailing_comments(trailing_dot_comments),
attr.format()
]
)
// Identify dangling comments before and after the dot:
// ```python
// (
// (
// a
// ) # `before_dot_end_of_line`
// # `before_dot_own_line`
// . # `after_dot_end_of_line`
// # `after_dot_own_line`
// b
// )
// ```
let comments = f.context().comments().clone();
let dangling = comments.dangling(item);
let (before_dot, after_dot) = if dangling.is_empty() {
(dangling, dangling)
} else {
// Regular style
// ```python
// blogs2 = Blog.objects.filter(
// entry__headline__contains="Lennon",
// ).filter(
// entry__pub_date__year=2008,
// )
// ```
write!(
f,
[
text("."),
trailing_comments(trailing_dot_comments),
(!leading_attribute_comments.is_empty()).then_some(hard_line_break()),
leading_comments(leading_attribute_comments),
attr.format()
]
let dot_token = find_only_token_in_range(
TextRange::new(item.value.end(), item.attr.start()),
SimpleTokenKind::Dot,
f.context().source(),
);
dangling.split_at(
dangling.partition_point(|comment| comment.start() < dot_token.start()),
)
}
};

let (before_dot_end_of_line, before_dot_own_line) = before_dot.split_at(
before_dot.partition_point(|comment| comment.line_position().is_end_of_line()),
);

let (after_dot_end_of_line, after_dot_own_line) = after_dot.split_at(
after_dot.partition_point(|comment| comment.line_position().is_end_of_line()),
);

write!(
f,
[
trailing_comments(before_dot_end_of_line),
(!before_dot.is_empty()).then_some(hard_line_break()),
dangling_comments(before_dot_own_line),
text("."),
trailing_comments(after_dot_end_of_line),
(!after_dot.is_empty()).then_some(hard_line_break()),
dangling_comments(after_dot_own_line),
attr.format()
]
)
});

let is_call_chain_root = self.call_chain_layout == CallChainLayout::Default
Expand Down Expand Up @@ -169,13 +163,7 @@ impl NeedsParentheses for ExprAttribute {
== CallChainLayout::Fluent
{
OptionalParentheses::Multiline
} else if context
.comments()
.dangling(self)
.iter()
.any(|comment| comment.line_position().is_own_line())
|| context.comments().has_trailing_own_line(self)
{
} else if context.comments().has_dangling(self) {
OptionalParentheses::Always
} else {
self.value.needs_parentheses(self.into(), context)
Expand Down
Loading

0 comments on commit 6bcba68

Please sign in to comment.