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

Use reserved width to include line suffix measurement #6901

Merged
merged 11 commits into from
Aug 30, 2023
Original file line number Diff line number Diff line change
Expand Up @@ -66,3 +66,12 @@
h1 = ((((1, 2))))
h2 = ((((1, "qweiurpoiqwurepqiurpqirpuqoiwrupqoirupqoirupqoiurpqiorupwqiourpqurpqurpqurpqurpqurpqurüqurqpuriq"))))
h3 = 1, "qweiurpoiqwurepqiurpqirpuqoiwrupqoirupqoirupqoiurpqiorupwqiourpqurpqurpqurpqurpqurpqurüqurqpuriq"

i1 = ("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",) # This should break

# As of adding this fixture `black` replaces the non-breaking space with a space if followed by a space.
# https://github.com/psf/black/blob/b4dca26c7d93f930bbd5a7b552807370b60d4298/src/black/comments.py#L122-L129
i2 = ("",) #  type: Add space before leading NBSP followed by spaces
i3 = ("",) #type: A space is added
i4 = ("",) #  type: Add space before leading NBSP followed by a space
i5 = ("",) # type: Add space before leading NBSP
cnpryer marked this conversation as resolved.
Show resolved Hide resolved
204 changes: 148 additions & 56 deletions crates/ruff_python_formatter/src/comments/format.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use std::borrow::Cow;

use ruff_text_size::{Ranged, TextLen, TextRange, TextSize};

use ruff_formatter::{format_args, write, FormatError, SourceCode};
Expand Down Expand Up @@ -155,19 +157,22 @@ impl Format<PyFormatContext<'_>> for FormatTrailingComments<'_> {
empty_lines(lines_before_comment),
format_comment(trailing)
],
// Reserving width isn't necessary because we don't split
// comments and the empty lines expand any enclosing group.
0
),
expand_parent()
]
)?;
} else {
write!(
f,
[
line_suffix(&format_args![space(), space(), format_comment(trailing)], 0),
expand_parent()
]
)?;
// A trailing comment at the end of a line has a reserved width to
// consider during line measurement.
// ```python
// tup = (
// "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
// ) # Some comment
// ```
trailing_end_of_line_comment(trailing).fmt(f)?;
}

trailing.mark_formatted();
Expand Down Expand Up @@ -266,13 +271,7 @@ impl Format<PyFormatContext<'_>> for FormatDanglingOpenParenthesisComments<'_> {
"Expected dangling comment to be at the end of the line"
);

write!(
f,
[
line_suffix(&format_args!(space(), space(), format_comment(comment)), 0),
expand_parent()
]
)?;
trailing_end_of_line_comment(comment).fmt(f)?;
comment.mark_formatted();
}

Expand All @@ -295,50 +294,11 @@ pub(crate) struct FormatComment<'a> {
impl Format<PyFormatContext<'_>> for FormatComment<'_> {
fn fmt(&self, f: &mut PyFormatter) -> FormatResult<()> {
let slice = self.comment.slice();
let comment_text = slice.text(SourceCode::new(f.context().source()));

let trimmed = comment_text.trim_end();
let trailing_whitespace_len = comment_text.text_len() - trimmed.text_len();

let Some(content) = trimmed.strip_prefix('#') else {
return Err(FormatError::syntax_error(
"Didn't find expected comment token `#`",
));
};

// Fast path for correctly formatted comments:
// * Start with a `#` and are followed by a space
// * Have no trailing whitespace.
if trailing_whitespace_len == TextSize::new(0) && content.starts_with(' ') {
return source_text_slice(slice.range(), ContainsNewlines::No).fmt(f);
}
let source = SourceCode::new(f.context().source());

write!(f, [source_position(slice.start()), text("#")])?;
let normalized_comment = normalize_comment(self.comment, source)?;

// Starts with a non breaking space
let start_offset =
if content.starts_with('\u{A0}') && !content.trim_start().starts_with("type:") {
// Replace non-breaking space with a space (if not followed by a normal space)
"#\u{A0}".text_len()
} else {
'#'.text_len()
};

// Add a space between the `#` and the text if the source contains none.
if !content.is_empty() && !content.starts_with([' ', '!', ':', '#', '\'']) {
write!(f, [space()])?;
}

let start = slice.start() + start_offset;
let end = slice.end() - trailing_whitespace_len;

write!(
f,
[
source_text_slice(TextRange::new(start, end), ContainsNewlines::No),
source_position(slice.end())
]
)
format_normalized_comment(normalized_comment, slice.start()).fmt(f)
}
}

Expand Down Expand Up @@ -376,3 +336,135 @@ impl Format<PyFormatContext<'_>> for FormatEmptyLines {
}
}
}

/// A helper that constructs a formattable element using a reserved-width line-suffix
/// for normalized comments.
///
/// * Black normalization of `SourceComment`.
/// * Line suffix with reserved width for the final, normalized content.
/// * Expands parent node.
pub(crate) const fn trailing_end_of_line_comment(
comment: &SourceComment,
) -> FormatTrailingEndOfLineComment {
FormatTrailingEndOfLineComment { comment }
}

pub(crate) struct FormatTrailingEndOfLineComment<'a> {
comment: &'a SourceComment,
}

impl Format<PyFormatContext<'_>> for FormatTrailingEndOfLineComment<'_> {
fn fmt(&self, f: &mut PyFormatter) -> FormatResult<()> {
let slice = self.comment.slice();
let source = SourceCode::new(f.context().source());

let normalized_comment = normalize_comment(self.comment, source)?;
let reserved_width = normalized_comment.text_len().to_u32() + 2; // Account for two added spaces
cnpryer marked this conversation as resolved.
Show resolved Hide resolved

write!(
f,
[
line_suffix(
&format_args![
space(),
space(),
format_normalized_comment(normalized_comment, slice.start())
],
reserved_width
),
expand_parent()
MichaReiser marked this conversation as resolved.
Show resolved Hide resolved
]
)
}
}

/// A helper that constructs formattable normalized comment text as efficiently as
/// possible.
///
/// * If the content is unaltered then format with source text slice strategy and no
/// unnecessary allocations.
/// * If the content is modified then make as few allocations as possible and use
/// a dynamic text element at the original slice's start position.
pub(crate) const fn format_normalized_comment(
MichaReiser marked this conversation as resolved.
Show resolved Hide resolved
comment: Cow<'_, str>,
start_position: TextSize,
) -> FormatNormalizedComment<'_> {
FormatNormalizedComment {
comment,
start_position,
}
}

pub(crate) struct FormatNormalizedComment<'a> {
comment: Cow<'a, str>,
start_position: TextSize,
}

impl Format<PyFormatContext<'_>> for FormatNormalizedComment<'_> {
fn fmt(&self, f: &mut Formatter<PyFormatContext>) -> FormatResult<()> {
match self.comment {
Cow::Borrowed(borrowed) => source_text_slice(
TextRange::at(self.start_position, borrowed.text_len()),
ContainsNewlines::No,
)
.fmt(f),

Cow::Owned(ref owned) => dynamic_text(owned, Some(self.start_position)).fmt(f),
}
}
}

/// A helper for normalizing comments efficiently.
///
/// * Return as fast as possible without making unnecessary allocations.
/// * Trim any trailing whitespace.
/// * Normalize for a leading '# '.
/// * Retain non-breaking spaces for 'type:' pragmas by leading with '# \u{A0}'.
fn normalize_comment<'a>(
comment: &'a SourceComment,
source: SourceCode<'a>,
) -> FormatResult<Cow<'a, str>> {
let slice = comment.slice();
let comment_text = slice.text(source);

let trimmed = comment_text.trim_end();
let trailing_whitespace_len = comment_text.text_len() - trimmed.text_len();

let Some(content) = trimmed.strip_prefix('#') else {
return Err(FormatError::syntax_error(
"Didn't find expected comment token `#`",
));
};

if content.is_empty() {
return Ok(Cow::Borrowed("#"));
}

// Fast path for correctly formatted comments:
// * Start with a `# '.
// * Have no trailing whitespace.
if trailing_whitespace_len == TextSize::new(0) && content.starts_with(' ') {
return Ok(Cow::Borrowed(comment_text));
}

if content.starts_with('\u{A0}') {
let trimmed = content.trim_start_matches('\u{A0}');

// Black adds a space before the non-breaking space if part of a type pragma.
if trimmed.trim_start().starts_with("type:") {
return Ok(Cow::Borrowed(comment_text));
}

// Black replaces the non-breaking space with a space if followed by a space.
if trimmed.starts_with(' ') {
return Ok(Cow::Owned(std::format!("# {trimmed}")));
}
}

// We normalize the leading space if we can.
if !content.starts_with([' ', '!', ':', '#', '\'']) {
return Ok(Cow::Owned(std::format!("# {}", content.trim_start())));
}

Ok(Cow::Owned(std::format!("#{content}")))
}
11 changes: 7 additions & 4 deletions crates/ruff_python_formatter/src/expression/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -247,10 +247,13 @@ impl Format<PyFormatContext<'_>> for MaybeParenthesizeExpression<'_> {
if format_expression.inspect(f)?.will_break() {
// The group here is necessary because `format_expression` may contain IR elements
// that refer to the group id
group(&format_expression)
.with_group_id(Some(group_id))
.should_expand(true)
.fmt(f)
group(&format_args![
text("("),
soft_block_indent(&format_expression),
text(")")
])
.with_group_id(Some(group_id))
.fmt(f)
} else {
// Only add parentheses if it makes the expression fit on the line.
// Using the flat version as the most expanded version gives a left-to-right splitting behavior
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ aaaaaaaaaaaaa, bbbbbbbbb = map(list, map(itertools.chain.from_iterable, zip(*ite
)


@@ -108,11 +112,18 @@
@@ -108,11 +112,20 @@
"xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
)

Expand All @@ -176,7 +176,10 @@ aaaaaaaaaaaaa, bbbbbbbbb = map(list, map(itertools.chain.from_iterable, zip(*ite
+ ], # type: ignore
)

aaaaaaaaaaaaa, bbbbbbbbb = map(list, map(itertools.chain.from_iterable, zip(*items))) # type: ignore[arg-type]
-aaaaaaaaaaaaa, bbbbbbbbb = map(list, map(itertools.chain.from_iterable, zip(*items))) # type: ignore[arg-type]
+aaaaaaaaaaaaa, bbbbbbbbb = map(
+ list, map(itertools.chain.from_iterable, zip(*items))
+) # type: ignore[arg-type]
```

## Ruff Output
Expand Down Expand Up @@ -310,7 +313,9 @@ call_to_some_function_asdf(
], # type: ignore
)

aaaaaaaaaaaaa, bbbbbbbbb = map(list, map(itertools.chain.from_iterable, zip(*items))) # type: ignore[arg-type]
aaaaaaaaaaaaa, bbbbbbbbb = map(
list, map(itertools.chain.from_iterable, zip(*items))
) # type: ignore[arg-type]
```

## Black Output
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,18 @@ last_call()
) # note: no trailing comma pre-3.6
call(*gidgets[:2])
call(a, *gidgets[:2])
@@ -328,13 +329,18 @@
@@ -142,7 +143,9 @@
xxxx_xxxxx_xxxx_xxx: Callable[..., List[SomeClass]] = classmethod( # type: ignore
sync(async_xxxx_xxx_xxxx_xxxxx_xxxx_xxx.__func__)
)
-xxxx_xxx_xxxx_xxxxx_xxxx_xxx: Callable[..., List[SomeClass]] = classmethod( # type: ignore
+xxxx_xxx_xxxx_xxxxx_xxxx_xxx: Callable[
+ ..., List[SomeClass]
+] = classmethod( # type: ignore
sync(async_xxxx_xxx_xxxx_xxxxx_xxxx_xxx.__func__)
)
xxxx_xxx_xxxx_xxxxx_xxxx_xxx: Callable[..., List[SomeClass]] = classmethod(
@@ -328,13 +331,18 @@
):
return True
if (
Expand All @@ -322,7 +333,7 @@ last_call()
^ aaaaaaaa.i << aaaaaaaa.k >> aaaaaaaa.l**aaaaaaaa.m // aaaaaaaa.n
):
return True
@@ -342,7 +348,8 @@
@@ -342,7 +350,8 @@
~aaaaaaaaaaaaaaaa.a
+ aaaaaaaaaaaaaaaa.b
- aaaaaaaaaaaaaaaa.c * aaaaaaaaaaaaaaaa.d @ aaaaaaaaaaaaaaaa.e
Expand Down Expand Up @@ -482,7 +493,9 @@ very_long_variable_name_filters: t.List[
xxxx_xxxxx_xxxx_xxx: Callable[..., List[SomeClass]] = classmethod( # type: ignore
sync(async_xxxx_xxx_xxxx_xxxxx_xxxx_xxx.__func__)
)
xxxx_xxx_xxxx_xxxxx_xxxx_xxx: Callable[..., List[SomeClass]] = classmethod( # type: ignore
xxxx_xxx_xxxx_xxxxx_xxxx_xxx: Callable[
..., List[SomeClass]
] = classmethod( # type: ignore
sync(async_xxxx_xxx_xxxx_xxxxx_xxxx_xxx.__func__)
)
xxxx_xxx_xxxx_xxxxx_xxxx_xxx: Callable[..., List[SomeClass]] = classmethod(
Expand Down
Loading
Loading