Skip to content

Commit 893bdac

Browse files
committed
refactor(formatter): improve printing comments for special nodes (#14544)
Remove `SiblingNode` usages. It used to check comments for special nodes based on [Prettier's](https://github.com/prettier/prettier/blob/7584432401a47a26943dd7a9ca9a8e032ead7285/src/language-js/comments/handle-comments.js#L861) behavior, but we have moved all of the logic to the comments printing side, so `SiblingNode` becomes unnecessary; it will be removed in the next PR.
1 parent 0829d2c commit 893bdac

File tree

8 files changed

+244
-150
lines changed

8 files changed

+244
-150
lines changed

crates/oxc_formatter/src/formatter/comments.rs

Lines changed: 5 additions & 112 deletions
Original file line numberDiff line numberDiff line change
@@ -191,13 +191,10 @@ impl<'a> Comments<'a> {
191191
pub fn end_of_line_comments_after(&self, mut pos: u32) -> &'a [Comment] {
192192
let comments = self.unprinted_comments();
193193
for (index, comment) in comments.iter().enumerate() {
194-
if self
195-
.source_text
196-
.all_bytes_match(pos, comment.span.start, |b| matches!(b, b'\t' | b' ' | b')'))
197-
{
198-
if !self.source_text.is_own_line_comment(comment)
199-
&& (comment.is_line() || self.source_text.is_end_of_line_comment(comment))
200-
{
194+
if self.source_text.all_bytes_match(pos, comment.span.start, |b| {
195+
matches!(b, b'\t' | b' ' | b')' | b'=')
196+
}) {
197+
if comment.is_line() || self.source_text.is_end_of_line_comment(comment) {
201198
return &comments[..=index];
202199
}
203200
pos = comment.span.end;
@@ -288,7 +285,7 @@ impl<'a> Comments<'a> {
288285
/// Used when multiple comments are processed in batch. Each unit of `count`
289286
/// represents one comment that has been formatted and should be marked as processed.
290287
///
291-
/// Like [`increment_printed_count`], this is essential for maintaining the
288+
/// Like [`Comments::increment_printed_count`], this is essential for maintaining the
292289
/// integrity of the comment tracking system.
293290
#[inline]
294291
pub fn increase_printed_count_by(&mut self, count: usize) {
@@ -303,27 +300,6 @@ impl<'a> Comments<'a> {
303300
preceding_node: &SiblingNode<'a>,
304301
mut following_node: Option<&SiblingNode<'a>>,
305302
) -> &'a [Comment] {
306-
if !matches!(
307-
enclosing_node,
308-
SiblingNode::Program(_)
309-
| SiblingNode::BlockStatement(_)
310-
| SiblingNode::FunctionBody(_)
311-
| SiblingNode::TSModuleBlock(_)
312-
| SiblingNode::SwitchStatement(_)
313-
| SiblingNode::StaticBlock(_)
314-
) && matches!(following_node, Some(SiblingNode::EmptyStatement(_)))
315-
{
316-
let enclosing_span = enclosing_node.span();
317-
return self.comments_before(enclosing_span.end);
318-
}
319-
320-
// If preceding_node is a callee, let the following node handle its comments
321-
// Based on Prettier's comment handling logic
322-
if matches!(enclosing_node, SiblingNode::CallExpression(CallExpression { callee, ..}) | SiblingNode::NewExpression(NewExpression { callee, ..}) if callee.span().contains_inclusive(preceding_node.span()))
323-
{
324-
return &[];
325-
}
326-
327303
let comments = self.unprinted_comments();
328304
if comments.is_empty() {
329305
return &[];
@@ -377,78 +353,14 @@ impl<'a> Comments<'a> {
377353

378354
if source_text.is_own_line_comment(comment) {
379355
// Own line comments are typically leading comments for the next node
380-
381-
if matches!(enclosing_node, SiblingNode::IfStatement(stmt) if stmt.test.span() == preceding_span)
382-
|| matches!(enclosing_node, SiblingNode::WhileStatement(stmt) if stmt.test.span() == preceding_span)
383-
{
384-
return handle_if_and_while_statement_comments(
385-
following_span.start,
386-
comment_index,
387-
comments,
388-
source_text,
389-
);
390-
}
391-
392356
break;
393357
} else if self.source_text.is_end_of_line_comment(comment) {
394-
if let SiblingNode::IfStatement(if_stmt) = enclosing_node
395-
&& if_stmt.consequent.span() == preceding_span
396-
{
397-
// If comment is after the `else` keyword, it is not a trailing comment of consequent.
398-
if source_text[preceding_span.end as usize..comment.span.start as usize]
399-
.contains("else")
400-
{
401-
return &[];
402-
}
403-
}
404-
405-
if matches!(enclosing_node, SiblingNode::IfStatement(stmt) if stmt.test.span() == preceding_span)
406-
|| matches!(enclosing_node, SiblingNode::WhileStatement(stmt) if stmt.test.span() == preceding_span)
407-
{
408-
return handle_if_and_while_statement_comments(
409-
following_span.start,
410-
comment_index,
411-
comments,
412-
source_text,
413-
);
414-
}
415-
416-
// End-of-line comments in specific contexts should be leading comments
417-
if matches!(
418-
enclosing_node,
419-
SiblingNode::VariableDeclarator(_)
420-
| SiblingNode::AssignmentExpression(_)
421-
| SiblingNode::TSTypeAliasDeclaration(_)
422-
) && (comment.is_block()
423-
|| matches!(
424-
following_node,
425-
SiblingNode::ObjectExpression(_)
426-
| SiblingNode::ArrayExpression(_)
427-
| SiblingNode::TSTypeLiteral(_)
428-
| SiblingNode::TemplateLiteral(_)
429-
| SiblingNode::TaggedTemplateExpression(_)
430-
))
431-
{
432-
return &[];
433-
}
434358
return &comments[..=comment_index];
435359
}
436360

437361
comment_index += 1;
438362
}
439363

440-
if comment_index == 0 {
441-
// No comments to print
442-
return &[];
443-
}
444-
445-
if matches!(
446-
enclosing_node,
447-
SiblingNode::ImportDeclaration(_) | SiblingNode::ExportAllDeclaration(_)
448-
) {
449-
return &comments[..comment_index];
450-
}
451-
452364
// Find the first comment (from the end) that has non-whitespace/non-paren content after it
453365
let mut gap_end = following_span.start;
454366
for (idx, comment) in comments[..comment_index].iter().enumerate().rev() {
@@ -557,22 +469,3 @@ fn matches_pattern_at(bytes: &[u8], pos: usize, pattern: &[u8]) -> bool {
557469
bytes[pos..].starts_with(pattern)
558470
&& matches!(bytes.get(pos + pattern.len()), Some(b' ' | b'\t' | b'\n' | b'\r' | b'{'))
559471
}
560-
561-
/// Handles comment placement logic for if and while statements.
562-
fn handle_if_and_while_statement_comments<'a>(
563-
mut end: u32,
564-
comment_index: usize,
565-
comments: &'a [Comment],
566-
source_text: SourceText,
567-
) -> &'a [Comment] {
568-
// Handle pattern: `if (a /* comment */) // trailing comment`
569-
// Find the last comment that contains ')' between its end and the current end
570-
for (idx, comment) in comments[..=comment_index].iter().enumerate().rev() {
571-
if source_text.bytes_contain(comment.span.end, end, b')') {
572-
return &comments[..=idx];
573-
}
574-
end = comment.span.start;
575-
}
576-
577-
&[]
578-
}

crates/oxc_formatter/src/utils/assignment_like.rs

Lines changed: 69 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -9,10 +9,11 @@ use crate::{
99
formatter::{
1010
Buffer, BufferExtensions, Format, FormatResult, Formatter, VecBuffer,
1111
prelude::{FormatElements, format_once, line_suffix_boundary, *},
12-
trivia::{FormatLeadingComments, format_dangling_comments},
12+
trivia::{FormatLeadingComments, FormatTrailingComments},
1313
},
1414
generated::ast_nodes::{AstNode, AstNodes},
1515
options::Expand,
16+
parentheses::NeedsParentheses,
1617
utils::{
1718
format_node_without_trailing_comments::FormatNodeWithoutTrailingComments,
1819
member_chain::is_member_call_chain,
@@ -149,20 +150,66 @@ pub enum AssignmentLikeLayout {
149150
SuppressedInitializer,
150151
}
151152

152-
const MIN_OVERLAP_FOR_BREAK: u8 = 3;
153+
/// Based on Prettier's behavior:
154+
/// <https://github.com/prettier/prettier/blob/7584432401a47a26943dd7a9ca9a8e032ead7285/src/language-js/comments/handle-comments.js#L853-L883>
155+
fn format_left_trailing_comments(
156+
start: u32,
157+
should_print_as_leading: bool,
158+
f: &mut Formatter<'_, '_>,
159+
) -> FormatResult<()> {
160+
let end_of_line_comments = f.context().comments().end_of_line_comments_after(start);
161+
162+
let comments = if end_of_line_comments.is_empty() {
163+
let comments = f.context().comments().comments_before_character(start, b'=');
164+
if comments.iter().any(|c| f.source_text().is_own_line_comment(c)) { &[] } else { comments }
165+
} else if should_print_as_leading || end_of_line_comments.last().is_some_and(|c| c.is_block()) {
166+
// No trailing comments for these expressions or if the trailing comment is a block comment
167+
&[]
168+
} else {
169+
end_of_line_comments
170+
};
171+
172+
FormatTrailingComments::Comments(comments).fmt(f)
173+
}
174+
175+
fn should_print_as_leading(expr: &Expression) -> bool {
176+
matches!(
177+
expr,
178+
Expression::ObjectExpression(_)
179+
| Expression::ArrayExpression(_)
180+
| Expression::TemplateLiteral(_)
181+
| Expression::TaggedTemplateExpression(_)
182+
)
183+
}
153184

154185
impl<'a> AssignmentLike<'a, '_> {
155186
fn write_left(&self, f: &mut Formatter<'_, 'a>) -> FormatResult<bool> {
156187
match self {
157-
AssignmentLike::VariableDeclarator(variable_declarator) => {
158-
write!(f, variable_declarator.id())?;
188+
AssignmentLike::VariableDeclarator(declarator) => {
189+
if let Some(init) = &declarator.init {
190+
write!(f, [FormatNodeWithoutTrailingComments(&declarator.id())])?;
191+
format_left_trailing_comments(
192+
declarator.id.span().end,
193+
should_print_as_leading(init),
194+
f,
195+
)?;
196+
} else {
197+
write!(f, declarator.id())?;
198+
}
159199
Ok(false)
160200
}
161201
AssignmentLike::AssignmentExpression(assignment) => {
162-
write!(f, [assignment.left()]);
202+
write!(f, [FormatNodeWithoutTrailingComments(&assignment.left()),])?;
203+
format_left_trailing_comments(
204+
assignment.left.span().end,
205+
should_print_as_leading(&assignment.right),
206+
f,
207+
)?;
163208
Ok(false)
164209
}
165210
AssignmentLike::ObjectProperty(property) => {
211+
const MIN_OVERLAP_FOR_BREAK: u8 = 3;
212+
166213
let text_width_for_break =
167214
(f.options().indent_width.value() + MIN_OVERLAP_FOR_BREAK) as usize;
168215

@@ -226,15 +273,25 @@ impl<'a> AssignmentLike<'a, '_> {
226273
Ok(false) // Class properties don't use "short" key logic
227274
}
228275
AssignmentLike::TSTypeAliasDeclaration(declaration) => {
229-
write!(
276+
write!(f, [declaration.declare.then_some("declare "), "type "])?;
277+
278+
let start = if let Some(type_parameters) = &declaration.type_parameters() {
279+
write!(
280+
f,
281+
[declaration.id(), FormatNodeWithoutTrailingComments(type_parameters)]
282+
)?;
283+
type_parameters.span.end
284+
} else {
285+
write!(f, [FormatNodeWithoutTrailingComments(declaration.id())])?;
286+
declaration.id.span.end
287+
};
288+
289+
format_left_trailing_comments(
290+
start,
291+
matches!(&declaration.type_annotation, TSType::TSTypeLiteral(_)),
230292
f,
231-
[
232-
declaration.declare.then_some("declare "),
233-
"type ",
234-
declaration.id(),
235-
declaration.type_parameters()
236-
]
237293
)?;
294+
238295
Ok(false)
239296
}
240297
}

crates/oxc_formatter/src/utils/member_chain/chain_member.rs

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ use crate::{
77
prelude::*,
88
trivia::{FormatLeadingComments, FormatTrailingComments},
99
},
10-
generated::ast_nodes::AstNode,
10+
generated::ast_nodes::{AstNode, AstNodes},
1111
write,
1212
};
1313
use oxc_ast::{AstKind, ast::*};
@@ -77,15 +77,20 @@ impl<'a> Format<'a> for ChainMember<'a, '_> {
7777
member.property()
7878
]
7979
)?;
80-
member.format_trailing_comments(f)
81-
}
8280

81+
// `A.b /* comment */ (c)` -> `A.b(/* comment */ c)`
82+
if !matches!(member.parent, AstNodes::CallExpression(call) if call.type_arguments.is_none())
83+
{
84+
member.format_trailing_comments(f)?;
85+
}
86+
87+
Ok(())
88+
}
8389
Self::TSNonNullExpression(e) => {
8490
e.format_leading_comments(f)?;
8591
write!(f, ["!"])?;
8692
e.format_trailing_comments(f)
8793
}
88-
8994
Self::CallExpression { expression, position } => match *position {
9095
CallExpressionPosition::Start => write!(f, expression),
9196
CallExpressionPosition::Middle => {

crates/oxc_formatter/src/write/export_declarations.rs

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,10 @@ use crate::{
1212
},
1313
generated::ast_nodes::{AstNode, AstNodes},
1414
write,
15-
write::semicolon::OptionalSemicolon,
15+
write::{
16+
import_declaration::format_import_and_export_source_with_clause,
17+
semicolon::OptionalSemicolon,
18+
},
1619
};
1720

1821
use super::FormatWrite;
@@ -78,7 +81,10 @@ impl<'a> FormatWrite<'a> for AstNode<'a, ExportAllDeclaration<'a>> {
7881
if let Some(name) = &self.exported() {
7982
write!(f, ["as", space(), name, space()])?;
8083
}
81-
write!(f, ["from", space(), self.source(), self.with_clause(), OptionalSemicolon])
84+
write!(f, ["from", space()]);
85+
86+
format_import_and_export_source_with_clause(self.source(), self.with_clause(), f)?;
87+
write!(f, [OptionalSemicolon])
8288
}
8389
}
8490

@@ -88,7 +94,6 @@ impl<'a> FormatWrite<'a> for AstNode<'a, ExportNamedDeclaration<'a>> {
8894
let export_kind = self.export_kind();
8995
let specifiers = self.specifiers();
9096
let source = self.source();
91-
let with_clause = self.with_clause();
9297

9398
if let Some(decl) = declaration {
9499
format_export_keyword_with_class_decorators(
@@ -147,12 +152,10 @@ impl<'a> FormatWrite<'a> for AstNode<'a, ExportNamedDeclaration<'a>> {
147152
}
148153
write!(f, "}")?;
149154

155+
let with_clause = self.with_clause();
150156
if let Some(source) = source {
151-
write!(f, [space(), "from", space(), source])?;
152-
}
153-
154-
if let Some(with_clause) = with_clause {
155-
write!(f, [space(), with_clause])?;
157+
write!(f, [space(), "from", space()])?;
158+
format_import_and_export_source_with_clause(source, with_clause, f)?;
156159
}
157160
}
158161

crates/oxc_formatter/src/write/import_declaration.rs

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,25 @@ impl<'a> Format<'a> for ImportOrExportKind {
2525
}
2626
}
2727

28+
pub fn format_import_and_export_source_with_clause<'a>(
29+
source: &AstNode<'a, StringLiteral>,
30+
with_clause: Option<&AstNode<'a, WithClause>>,
31+
f: &mut Formatter<'_, 'a>,
32+
) -> FormatResult<()> {
33+
source.format_leading_comments(f)?;
34+
source.write(f)?;
35+
36+
if let Some(with_clause) = with_clause {
37+
if f.comments().has_comment_before(with_clause.span.start) {
38+
write!(f, [space()])?;
39+
}
40+
41+
write!(f, [with_clause])?;
42+
}
43+
44+
Ok(())
45+
}
46+
2847
impl<'a> FormatWrite<'a> for AstNode<'a, ImportDeclaration<'a>> {
2948
fn write(&self, f: &mut Formatter<'_, 'a>) -> FormatResult<()> {
3049
let decl = &format_once(|f| {
@@ -34,7 +53,9 @@ impl<'a> FormatWrite<'a> for AstNode<'a, ImportDeclaration<'a>> {
3453
write!(f, [specifiers, space(), "from", space()])?;
3554
}
3655

37-
write!(f, [self.source(), self.with_clause(), OptionalSemicolon])
56+
format_import_and_export_source_with_clause(self.source(), self.with_clause(), f)?;
57+
58+
write!(f, [OptionalSemicolon])
3859
});
3960

4061
write!(f, [labelled(LabelId::of(JsLabels::ImportDeclaration), decl)])

0 commit comments

Comments
 (0)