Skip to content

Commit 34c5a2f

Browse files
committed
fix(formatter): incorrect comments checking (#12356)
Due to our printing comments architecture is different from `Biome`, so it may incorrectly implement the comments checking parts. But fortunately, those will be caught as the port work gradually nears done.
1 parent f6e2f29 commit 34c5a2f

File tree

11 files changed

+245
-196
lines changed

11 files changed

+245
-196
lines changed

crates/oxc_formatter/src/formatter/builders.rs

Lines changed: 48 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,12 @@
1-
use std::{borrow::Cow, cell::Cell, num::NonZeroU8};
1+
use std::{backtrace, borrow::Cow, cell::Cell, num::NonZeroU8};
22

33
use Tag::{
44
EndAlign, EndConditionalContent, EndDedent, EndEntry, EndFill, EndGroup, EndIndent,
55
EndIndentIfGroupBreaks, EndLabelled, EndLineSuffix, StartAlign, StartConditionalContent,
66
StartDedent, StartEntry, StartFill, StartGroup, StartIndent, StartIndentIfGroupBreaks,
77
StartLabelled, StartLineSuffix,
88
};
9-
use oxc_span::Span;
9+
use oxc_span::{GetSpan, Span};
1010
use oxc_syntax::identifier::{is_line_terminator, is_white_space_single_line};
1111

1212
use super::{
@@ -2454,37 +2454,63 @@ where
24542454

24552455
/// Get the number of line breaks between two consecutive SyntaxNodes in the tree
24562456
pub fn has_lines_before(&self, span: Span) -> bool {
2457-
get_lines_before(span.start, self.fmt) > 1
2457+
get_lines_before(span, self.fmt) > 1
24582458
}
24592459
}
24602460

24612461
/// Get the number of line breaks between two consecutive SyntaxNodes in the tree
2462-
pub fn get_lines_before(start: u32, f: &Formatter) -> usize {
2463-
// Count the newlines in the leading trivia of the next node
2462+
pub fn get_lines_before(span: Span, f: &Formatter) -> usize {
2463+
let mut start = span.start;
2464+
2465+
// Should skip the leading comments of the node.
24642466
let comments = f.comments().unprinted_comments();
2465-
let start = if let Some(comment) = comments.first() {
2466-
if comment.span.end < start { comment.span.start } else { start }
2467-
} else {
2468-
start
2469-
};
2470-
2471-
f.source_text()[..start as usize]
2472-
// TODO: This is a workaround.
2473-
// Trim `(` because we turned off `preserveParens`, which causes us to not have a parenthesis node,
2474-
// but you will still find `(` or `)` in the source text.
2475-
.trim_end_matches(|c| is_white_space_single_line(c) || c == '(')
2476-
.chars()
2477-
.rev()
2478-
.take_while(|c| is_line_terminator(*c))
2479-
.count()
2467+
if let Some(comment) = comments.first() {
2468+
if comment.span.end < start {
2469+
start = comment.span.start;
2470+
}
2471+
}
2472+
2473+
// Count the newlines in the leading trivia of the next node
2474+
let mut count = 0;
2475+
let mut right_parent_start = span.end as usize;
2476+
for c in f.source_text()[..start as usize].chars().rev() {
2477+
if is_white_space_single_line(c) {
2478+
continue;
2479+
}
2480+
2481+
if c == '(' {
2482+
// We don't have a parenthesis node when `preserveParens` is turned off,
2483+
// but we will find the `(` and `)` around the node if it exists.
2484+
// If we find a `(`, we try to find the matching `)` and reset the count.
2485+
// This is necessary to avoid counting the newlines inside the parenthesis.
2486+
2487+
let Some((pos, ')')) =
2488+
f.source_text()[right_parent_start..].trim_start().chars().enumerate().next()
2489+
else {
2490+
return count;
2491+
};
2492+
2493+
right_parent_start = pos;
2494+
count = 0;
2495+
continue;
2496+
}
2497+
2498+
if !is_line_terminator(c) {
2499+
return count;
2500+
}
2501+
2502+
count += 1;
2503+
}
2504+
2505+
count
24802506
}
24812507

24822508
/// Get the number of line breaks between two consecutive SyntaxNodes in the tree
24832509
pub fn get_lines_after(end: u32, source_text: &str) -> usize {
24842510
source_text[end as usize..]
2485-
.trim_start_matches(is_white_space_single_line)
24862511
.chars()
2487-
.take_while(|c| is_line_terminator(*c))
2512+
.filter(|&c| !is_white_space_single_line(c))
2513+
.take_while(|&c| is_line_terminator(c))
24882514
.count()
24892515
}
24902516

crates/oxc_formatter/src/formatter/comments/mod.rs

Lines changed: 117 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,10 @@ use std::{
66
};
77

88
use oxc_allocator::Vec;
9-
use oxc_ast::{Comment, CommentKind, ast};
9+
use oxc_ast::{
10+
Comment, CommentKind,
11+
ast::{self, CallExpression, NewExpression},
12+
};
1013
use oxc_span::{GetSpan, Span};
1114

1215
use crate::{
@@ -30,42 +33,40 @@ impl<'a> Comments<'a> {
3033
Comments { source_text, comments, printed_count: 0 }
3134
}
3235

36+
/// Returns comments that not printed yet.
3337
#[inline]
3438
pub fn unprinted_comments(&self) -> &'a [Comment] {
3539
&self.comments[self.printed_count..]
3640
}
3741

42+
/// Returns comments that were printed already.
43+
#[inline]
44+
pub fn printed_comments(&self) -> &'a [Comment] {
45+
&self.comments[..self.printed_count]
46+
}
47+
3848
/// Returns the comments that after the given `start` position, even if they were already printed.
39-
pub fn comments_after(&self, start: u32) -> &'a [Comment] {
49+
pub fn comments_after(&self, pos: u32) -> &'a [Comment] {
4050
let mut index = self.printed_count;
4151

4252
// No comments or all are printed already
4353
if index == self.comments.len() {
4454
return &[];
4555
}
4656

47-
let forward = self.comments[index].span.end < start;
48-
// Skip comments that before start
49-
if forward {
50-
while index < self.comments.len() - 1 {
51-
if self.comments[index + 1].span.end <= start {
52-
index += 1;
53-
continue;
54-
}
55-
break;
56-
}
57-
} else {
58-
// Find comments that after start
59-
while index > 0 {
60-
if self.comments[index - 1].span.start > start {
61-
index -= 1;
62-
continue;
63-
}
64-
break;
65-
}
57+
// You may want to check comments after your are printing the node,
58+
// so it may have a lot of comments that don't print yet.
59+
//
60+
// Skip comments that before pos
61+
while index < self.comments.len() - 1 && self.comments[index].span.end < pos {
62+
index += 1;
6663
}
6764

68-
&self.comments[index..]
65+
if self.comments[index].span.end < pos {
66+
&self.comments[index + 1..]
67+
} else {
68+
&self.comments[index..]
69+
}
6970
}
7071

7172
#[inline]
@@ -243,6 +244,100 @@ impl<'a> Comments<'a> {
243244
pub fn increment_printed_count(&mut self) {
244245
self.printed_count += 1;
245246
}
247+
248+
pub fn get_trailing_comments(
249+
&self,
250+
enclosing_node: &SiblingNode<'a>,
251+
preceding_node: &SiblingNode<'a>,
252+
following_node: Option<&SiblingNode<'a>>,
253+
) -> &'a [Comment] {
254+
// The preceding_node is the callee of the call expression or new expression, let following node to print it.
255+
// Based on https://github.com/prettier/prettier/blob/7584432401a47a26943dd7a9ca9a8e032ead7285/src/language-js/comments/handle-comments.js#L726-L741
256+
if matches!(enclosing_node, SiblingNode::CallExpression(CallExpression { callee, ..}) | SiblingNode::NewExpression(NewExpression { callee, ..}) if callee.span().contains_inclusive(preceding_node.span()))
257+
{
258+
return &[];
259+
}
260+
261+
let comments = self.unprinted_comments();
262+
if comments.is_empty() {
263+
return &[];
264+
}
265+
266+
let source_text = self.source_text;
267+
let preceding_span = preceding_node.span();
268+
269+
// All of the comments before this node are printed already.
270+
debug_assert!(
271+
comments.first().is_none_or(|comment| comment.span.end > preceding_span.start)
272+
);
273+
274+
let Some(following_node) = following_node else {
275+
let enclosing_span = enclosing_node.span();
276+
return comments
277+
.iter()
278+
.enumerate()
279+
.take_while(|(_, comment)| comment.span.end <= enclosing_span.end)
280+
.last()
281+
.map_or(&[], |(index, _)| &comments[..=index]);
282+
};
283+
284+
let following_span = following_node.span();
285+
let mut comment_index = 0;
286+
while let Some(comment) = comments.get(comment_index) {
287+
// Check if the comment is before the following node's span
288+
if comment.span.end > following_span.start {
289+
break;
290+
}
291+
292+
if is_own_line_comment(comment, source_text) {
293+
// TODO: describe the logic here
294+
// Reached an own line comment, which means it is the leading comment for the next node.
295+
break;
296+
} else if is_end_of_line_comment(comment, source_text) {
297+
// Should be a leading comment of following node.
298+
// Based on https://github.com/prettier/prettier/blob/7584432401a47a26943dd7a9ca9a8e032ead7285/src/language-js/comments/handle-comments.js#L852-L883
299+
if matches!(
300+
enclosing_node,
301+
SiblingNode::VariableDeclarator(_)
302+
| SiblingNode::AssignmentExpression(_)
303+
| SiblingNode::TSTypeAliasDeclaration(_)
304+
) && (comment.is_block()
305+
|| matches!(
306+
following_node,
307+
SiblingNode::ObjectExpression(_)
308+
| SiblingNode::ArrayExpression(_)
309+
| SiblingNode::TSTypeLiteral(_)
310+
| SiblingNode::TemplateLiteral(_)
311+
| SiblingNode::TaggedTemplateExpression(_)
312+
))
313+
{
314+
return &[];
315+
}
316+
return &comments[..=comment_index];
317+
}
318+
319+
comment_index += 1;
320+
}
321+
322+
if comment_index == 0 {
323+
// No comments to print
324+
return &[];
325+
}
326+
327+
let mut gap_end = following_span.start;
328+
for cur_index in (0..comment_index).rev() {
329+
let comment = &comments[cur_index];
330+
let gap_str = Span::new(comment.span.end, gap_end).source_text(source_text);
331+
if gap_str.as_bytes().iter().all(|&b| matches!(b, b' ' | b'(')) {
332+
gap_end = comment.span.start;
333+
} else {
334+
// If there is a non-whitespace character, we stop here
335+
return &comments[..=cur_index];
336+
}
337+
}
338+
339+
&[]
340+
}
246341
}
247342

248343
#[inline]

0 commit comments

Comments
 (0)