Skip to content

Commit 95ef9dd

Browse files
azjezzazjezz
committed
fix(formatter): ensure idempotent formatting for trailing comments after breaking parameter lists
closes #788 Signed-off-by: azjezz <[email protected]>
1 parent 774186d commit 95ef9dd

File tree

6 files changed

+97
-19
lines changed

6 files changed

+97
-19
lines changed

‎crates/formatter/src/internal/comment/format.rs‎

Lines changed: 44 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ impl<'arena> FormatterState<'_, 'arena> {
5555
///
5656
/// `true` if the next substantive line is a comment line, `false` otherwise.
5757
pub(crate) fn is_followed_by_comment_on_next_line(&self, span: Span) -> bool {
58-
let Some(first_char_offset) = self.skip_spaces(Some(span.end.offset), false) else {
58+
let Some(first_char_offset) = self.skip_spaces(Some(span.end_offset()), false) else {
5959
return false;
6060
};
6161

@@ -93,15 +93,15 @@ impl<'arena> FormatterState<'_, 'arena> {
9393
break;
9494
}
9595

96-
if comment.end <= range.start.offset {
96+
if comment.end <= range.start_offset() {
9797
if flags.contains(CommentFlags::Leading) && comment.matches_flags(flags) {
9898
return true;
9999
}
100-
} else if range.end.offset < comment.start && self.is_insignificant(range.end.offset, comment.start) {
100+
} else if range.end_offset() < comment.start && self.is_insignificant(range.end_offset(), comment.start) {
101101
if flags.contains(CommentFlags::Trailing) && comment.matches_flags(flags) {
102102
return true;
103103
}
104-
} else if comment.end <= range.end.offset {
104+
} else if comment.end <= range.end_offset() {
105105
if flags.contains(CommentFlags::Dangling) && comment.matches_flags(flags) {
106106
return true;
107107
}
@@ -117,11 +117,11 @@ impl<'arena> FormatterState<'_, 'arena> {
117117
#[inline]
118118
pub fn has_inner_comment(&self, range: Span) -> bool {
119119
for comment in self.remaining_comments() {
120-
if comment.start > range.end.offset {
120+
if comment.start > range.end_offset() {
121121
break;
122122
}
123123

124-
if comment.start >= range.start.offset && comment.end <= range.end.offset {
124+
if comment.start >= range.start_offset() && comment.end <= range.end_offset() {
125125
return true;
126126
}
127127
}
@@ -141,7 +141,7 @@ impl<'arena> FormatterState<'_, 'arena> {
141141
while let Some(trivia) = self.all_comments.get(self.next_comment_index) {
142142
let comment = Comment::from_trivia(self.file, trivia);
143143

144-
if comment.end <= range.start.offset {
144+
if comment.end <= range.start_offset() {
145145
// Check if comment is in an ignore region - if so, preserve as-is
146146
if self.get_ignore_region_for(comment.start).is_some() {
147147
self.print_preserved_leading_comment(&mut parts, comment);
@@ -165,7 +165,7 @@ impl<'arena> FormatterState<'_, 'arena> {
165165
while let Some(trivia) = self.all_comments.get(self.next_comment_index) {
166166
let comment = Comment::from_trivia(self.file, trivia);
167167

168-
if range.end.offset < comment.start && self.is_insignificant(range.end.offset, comment.start) {
168+
if range.end_offset() < comment.start && self.is_insignificant(range.end_offset(), comment.start) {
169169
// Check if comment is in an ignore region - if so, preserve as-is
170170
if self.get_ignore_region_for(comment.start).is_some() {
171171
self.print_preserved_trailing_comment(&mut parts, comment);
@@ -331,7 +331,7 @@ impl<'arena> FormatterState<'_, 'arena> {
331331
for trivia in &self.all_comments[self.next_comment_index..] {
332332
let comment = Comment::from_trivia(self.file, trivia);
333333

334-
if comment.start >= range.start.offset && comment.end <= range.end.offset {
334+
if comment.start >= range.start_offset() && comment.end <= range.end_offset() {
335335
must_break = must_break || !comment.is_block;
336336
if !should_indent && self.is_next_line_empty(trivia.span) {
337337
parts.push(Document::Array(
@@ -387,7 +387,7 @@ impl<'arena> FormatterState<'_, 'arena> {
387387
for trivia in &self.all_comments[self.next_comment_index..] {
388388
let comment = Comment::from_trivia(self.file, trivia);
389389

390-
if comment.end <= range.end.offset {
390+
if comment.end <= range.end_offset() {
391391
if !indented && self.is_next_line_empty(trivia.span) {
392392
parts.push(Document::Array(
393393
vec![in self.arena; self.print_comment(comment), Document::Line(Line::hard())],
@@ -435,9 +435,9 @@ impl<'arena> FormatterState<'_, 'arena> {
435435
for trivia in &self.all_comments[self.next_comment_index..] {
436436
let comment = Comment::from_trivia(self.file, trivia);
437437

438-
if comment.start >= after.end.offset
439-
&& comment.end <= before.start.offset
440-
&& self.is_insignificant(after.end.offset, comment.start)
438+
if comment.start >= after.end_offset()
439+
&& comment.end <= before.start_offset()
440+
&& self.is_insignificant(after.end_offset(), comment.start)
441441
{
442442
parts.push(self.print_comment(comment));
443443
consumed_count += 1;
@@ -462,6 +462,37 @@ impl<'arena> FormatterState<'_, 'arena> {
462462
]))
463463
}
464464

465+
/// Prints trailing comments that appear between two nodes, suitable for use after `{`.
466+
/// Unlike `print_dangling_comments_between_nodes`, this uses LineSuffix for inline comments
467+
/// to keep them on the same line as the preceding content.
468+
#[must_use]
469+
pub(crate) fn print_trailing_comments_between_nodes(
470+
&mut self,
471+
after: Span,
472+
before: Span,
473+
) -> Option<Document<'arena>> {
474+
let mut parts = vec![in self.arena];
475+
let mut previous_comment: Option<Comment> = None;
476+
477+
while let Some(trivia) = self.all_comments.get(self.next_comment_index) {
478+
let comment = Comment::from_trivia(self.file, trivia);
479+
480+
let is_between = comment.start >= after.end_offset() && comment.end <= before.start_offset();
481+
let gap_is_ok =
482+
after.end_offset() == comment.start || self.is_insignificant(after.end_offset(), comment.start);
483+
484+
if is_between && gap_is_ok {
485+
let previous = self.print_trailing_comment(&mut parts, comment, previous_comment);
486+
previous_comment = Some(previous);
487+
self.next_comment_index += 1;
488+
} else {
489+
break;
490+
}
491+
}
492+
493+
if parts.is_empty() { None } else { Some(Document::Array(parts)) }
494+
}
495+
465496
#[must_use]
466497
fn print_comment(&self, comment: Comment) -> Document<'arena> {
467498
let content = &self.source_text[comment.start as usize..comment.end as usize];

‎crates/formatter/src/internal/format/function_like.rs‎

Lines changed: 37 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,16 @@ impl<'arena> FunctionLikeParts<'arena> {
118118
}
119119
}
120120

121+
fn get_signature_end_span(&self) -> Span {
122+
if let Some(return_type) = self.return_type_hint {
123+
return_type.span()
124+
} else if let Some(use_clause) = self.use_clause {
125+
use_clause.span()
126+
} else {
127+
self.parameter_list.span()
128+
}
129+
}
130+
121131
fn format_attributes(&self, f: &mut FormatterState<'_, 'arena>) -> Document<'arena> {
122132
let mut attributes = vec![in f.arena];
123133
for attribute_list in self.attribute_lists {
@@ -241,6 +251,7 @@ impl<'arena> FunctionLikeParts<'arena> {
241251
settings: FunctionLikeSettings,
242252
signature_id: GroupIdentifier,
243253
parameter_list_will_break: Option<bool>,
254+
dangling_comments: Option<Document<'arena>>,
244255
) -> Document<'arena> {
245256
match self.body {
246257
FunctionLikeBody::Abstract(span) => format_token(f, span, ";"),
@@ -249,11 +260,22 @@ impl<'arena> FunctionLikeParts<'arena> {
249260
let spacing =
250261
self.format_brace_spacing(f, settings, signature_id, parameter_list_will_break, inlined_braces);
251262

252-
Document::Group(Group::new(vec![
253-
in f.arena;
254-
spacing,
255-
block.format(f),
256-
]))
263+
if let Some(comments) = dangling_comments {
264+
let block_doc = block.format(f);
265+
266+
Document::Group(Group::new(vec![
267+
in f.arena;
268+
spacing,
269+
comments,
270+
block_doc,
271+
]))
272+
} else {
273+
Document::Group(Group::new(vec![
274+
in f.arena;
275+
spacing,
276+
block.format(f),
277+
]))
278+
}
257279
}
258280
}
259281
}
@@ -273,8 +295,17 @@ impl<'arena> FunctionLikeParts<'arena> {
273295
None
274296
};
275297

298+
let dangling_comments = match (&self.body, parameter_list_will_break) {
299+
(_, Some(false)) => None,
300+
(FunctionLikeBody::Block(block), _) => {
301+
let signature_end = self.get_signature_end_span();
302+
f.print_trailing_comments_between_nodes(signature_end, block.left_brace)
303+
}
304+
(FunctionLikeBody::Abstract(_), _) => None,
305+
};
306+
276307
let (signature, signature_id) = self.format_signature(f, settings.space_before_params);
277-
let body = self.format_body(f, settings, signature_id, parameter_list_will_break);
308+
let body = self.format_body(f, settings, signature_id, parameter_list_will_break, dangling_comments);
278309

279310
Document::Group(Group::new(vec![
280311
in f.arena;

‎crates/formatter/tests/cases/issue_788/after.php‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
<?php
2+
3+
function foo(
4+
$aaaaaaaaaaaaaaaaaaaaaaaaa1,
5+
$aaaaaaaaaaaaaaaaaaaaaaaaa2,
6+
$aaaaaaaaaaaaaaaaaaaaaaaaa3,
7+
$aaaaaaaaaaaaaaaaaaaaaaaaa4,
8+
) { //: void
9+
}

‎crates/formatter/tests/cases/issue_788/before.php‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
<?php
2+
3+
function foo($aaaaaaaaaaaaaaaaaaaaaaaaa1,$aaaaaaaaaaaaaaaaaaaaaaaaa2,$aaaaaaaaaaaaaaaaaaaaaaaaa3,$aaaaaaaaaaaaaaaaaaaaaaaaa4)//: void
4+
{
5+
}

‎crates/formatter/tests/cases/issue_788/settings.inc‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
FormatSettings::default()

‎crates/formatter/tests/mod.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -302,6 +302,7 @@ test_case!(issue_640);
302302
test_case!(issue_683);
303303
test_case!(issue_727);
304304
test_case!(issue_738);
305+
test_case!(issue_788);
305306

306307
#[test]
307308
fn test_all_test_cases_are_ran() {

0 commit comments

Comments
 (0)