fix(js_formatter): handle nestling comments to support jsdoc overloads #1195
Conversation
✅ Deploy Preview for biomejs canceled.
|
github-actions
bot
added
A-Formatter
| /// /* | ||
| /// * | ||
| /// this line doesn't start with a star | ||
| /// */ | ||
| /// "#))); | ||
| /// ``` | ||
| pub fn is_doc_comment<L: Language>(comment: &SyntaxTriviaPieceComments<L>) -> bool { | ||
| pub fn is_alignable_comment<L: Language>(comment: &SyntaxTriviaPieceComments<L>) -> bool { |
There was a problem hiding this comment.
I renamed this to is_alignable_comment, since it's technically not the same as a doc comment when there's only one star, but we can still format the alignment even in that case. This also more closely matches Prettier's isIndentableBlockComment naming.
All of the existing usages have also been renamed, of which there were only like 2 anyway.
| /// */ | ||
| /// "#))); | ||
| /// ``` | ||
| pub fn is_doc_comment<L: Language>(comment: &SyntaxTriviaPieceComments<L>) -> bool { |
There was a problem hiding this comment.
This is now truly checking for "doc" comments, as defined by JSDoc's syntax. Again, I think this should probably be in the biome_js_formatter crate rather than here, but all of the logic that uses it is in this generic one right now, too. Once we start implementing HTML formatting this will likely have to get moved, but that's a future time.
There was a problem hiding this comment.
It might be worth adding a TODO saying what you say here.
| first.has_newline() | ||
| && second.has_newline() | ||
| && (second.text_range().start()).sub(first.text_range().end()) == TextSize::from(0) | ||
| && is_doc_comment(first) | ||
| && is_doc_comment(second) |
There was a problem hiding this comment.
I tried ordering this to make it do the least work possible. I imagine the text range stuff gets inlined into just integer comparisons and a subtraction by the compiler, but i'm not totally sure. Probably doesn't really matter since this function is unlikely to be called in most cases anyway.
crates/biome_formatter/src/trivia.rs
Outdated
| @@ -37,14 +66,23 @@ where | |||
| FormatLeadingComments::Comments(comments) => comments, | |||
| }; | |||
|
|
|||
| for comment in leading_comments { | |||
| for (index, comment) in leading_comments.iter().enumerate() { | |||
There was a problem hiding this comment.
No idea what the best way to do this is. It feels awkward to have to directly get the iterated and then enumerate it, but it's probably the most efficient way to get both the current and the next item? This is my lack of Rust experience coming through.
There was a problem hiding this comment.
If you need both the current and next item, you can use peekable.
let mut leading_comments_iter = leading_comments.iter().peekable();
for comment in leading_comments_iter {
let next_comment = leading_comments_iter.peek();
}| 0 | 1 => write!(f, [hard_line_break()])?, | ||
| _ if should_nestle => {} |
There was a problem hiding this comment.
This is the biggest change from what existed before. We used to say that all trailing comments got put on their own lines. This now says that comments starting on the same line as the previous comment will always stay on that same line, and will just have a space if they aren't nestled.
afaict, that matches Prettier's behavior in all cases, and all of the tests still pass, so I assume that's true.
| let format_dangling_comments = format_with(|f| { | ||
| // Write all comments up to the first skipped token trivia or the token | ||
| let mut join = f.join_with(hard_line_break()); |
There was a problem hiding this comment.
General question: are JoinBuilders particularly efficient for building here? Or just convenient? This can't use a joiner anymore since the joining element can be different based on the context, but idk if that has a performance hit or not.
The convenience is nice to not have to implement the awkward conditional below on Line 326 lol, but oh well.
| @@ -187,8 +187,7 @@ function name( | |||
|
|
|||
| /* leading of opening */ /* trailing of opening */ 4 + 3; | |||
|
|
|||
| /* leading of closing */ | |||
| /* trailing of closing */ | |||
| /* leading of closing */ /* trailing of closing */ | |||
There was a problem hiding this comment.
This is the only change that came from that adjustment to the trailing comment logic. It matches Prettier's output now.
|
!bench_formatter |
Formatter Benchmark Results |
| /// */ | ||
| /// "#))); | ||
| /// ``` | ||
| pub fn is_doc_comment<L: Language>(comment: &SyntaxTriviaPieceComments<L>) -> bool { |
There was a problem hiding this comment.
It might be worth adding a TODO saying what you say here.
crates/biome_formatter/src/trivia.rs
Outdated
| @@ -37,14 +66,23 @@ where | |||
| FormatLeadingComments::Comments(comments) => comments, | |||
| }; | |||
|
|
|||
| for comment in leading_comments { | |||
| for (index, comment) in leading_comments.iter().enumerate() { | |||
There was a problem hiding this comment.
If you need both the current and next item, you can use peekable.
let mut leading_comments_iter = leading_comments.iter().peekable();
for comment in leading_comments_iter {
let next_comment = leading_comments_iter.peek();
}
Summary
There were a few diffs in the Prettier report that dealt with JSDoc nestling comments. These are a wholly-undocumented featured of JSDoc that's been around for more than a decade now (jsdoc/jsdoc.github.io#40) that causes JSDoc to consider adjacent doc-style comments as overloads for the same subject, rather than as distinct things.
A nestled comment is one that appears immediately after a previous doc comment, with absolutely no space in between, like:
Formatting for these comments must preserve the adjacency for JSDoc to understand it as an overload.
Unfortunately, this is a little awkward to implement. So awkward that Prettier actually just merges the comments into a single comment directly in the parsed syntax tree. But I don't think that's the greatest approach, since it loses the context that they are actually distinct comments. Parsing JSDoc from there in the future then becomes awkward, too, since you have to split on the
*//**content rather than just using consecutive leading comments to determine the overloading. But that's all a discussion for another time.In this PR, I implemented support for preserving these nestled comments. It's currently in
biome_formatter, but I think it should really live inbiome_js_formatter. The only reason it's not is because all of the formatting forLeading,Trailing, andDanglingcomment sets is done in the generic formatter, while individual comments are done in the language-specific one. I think that should probably change in the future as we eventually support languages that use comment syntax other than//and/**/, as it's otherwise hardcoded in there right now.This implementation in generally brings our comment formatting a little closer to prettier overall.
Test Plan
Three Prettier diff snapshots are gone! There's also one spec test that changed, but I checked that against the output from Prettier and the change matches their output, so I think it's a positive change (other things in that file are still formatted quite differently).