-
-
Notifications
You must be signed in to change notification settings - Fork 541
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
fix(js_formatter): handle nestling comments to support jsdoc overloads #1195
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1174,35 +1174,37 @@ where | |
} | ||
} | ||
|
||
/// Returns `true` if `comment` is a multi line block comment: | ||
/// Returns `true` if `comment` is a multi line block comment where each line | ||
/// starts with a star (`*`). These comments can be formatted to always have | ||
/// the leading stars line up in a column. | ||
/// | ||
/// # Examples | ||
/// | ||
/// ```rs,ignore | ||
/// assert!(is_doc_comment(&parse_comment(r#" | ||
/// assert!(is_alignable_comment(&parse_comment(r#" | ||
/// /** | ||
/// * Multiline doc comment | ||
/// */ | ||
/// "#))); | ||
/// | ||
/// assert!(is_doc_comment(&parse_comment(r#" | ||
/// assert!(is_alignable_comment(&parse_comment(r#" | ||
/// /* | ||
/// * Single star | ||
/// */ | ||
/// "#))); | ||
/// | ||
/// | ||
/// // Non doc-comments | ||
/// assert!(!is_doc_comment(&parse_comment(r#"/** has no line break */"#))); | ||
/// // Non indentable-comments | ||
/// assert!(!is_alignable_comment(&parse_comment(r#"/** has no line break */"#))); | ||
/// | ||
/// assert!(!is_doc_comment(&parse_comment(r#" | ||
/// assert!(!is_alignable_comment(&parse_comment(r#" | ||
/// /* | ||
/// * | ||
/// 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 { | ||
if !comment.has_newline() { | ||
return false; | ||
} | ||
|
@@ -1217,3 +1219,51 @@ pub fn is_doc_comment<L: Language>(comment: &SyntaxTriviaPieceComments<L>) -> bo | |
} | ||
}) | ||
} | ||
|
||
/// Returns `true` if `comment` is a documentation-style comment, specifically | ||
/// matching the JSDoc format where the comment: | ||
/// - spans over multiple lines | ||
/// - starts with two stars (like `/**`) | ||
/// | ||
/// This is a special case of [self::is_alignable_comment]. | ||
/// | ||
/// # Examples | ||
/// | ||
/// ```rs,ignore | ||
/// assert!(is_doc_comment(&parse_comment(r#" | ||
/// /** | ||
/// * Multiline doc comment | ||
/// */ | ||
/// "#))); | ||
/// | ||
/// // Non doc-comments | ||
/// assert!(!is_doc_comment(&parse_comment(r#" | ||
/// /* | ||
/// * Single star | ||
/// */ | ||
/// "#))); | ||
/// | ||
/// assert!(!is_doc_comment(&parse_comment(r#"/** has no line break */"#))); | ||
/// | ||
/// assert!(!is_doc_comment(&parse_comment(r#" | ||
/// /** | ||
/// * | ||
/// this line doesn't start with a star | ||
/// */ | ||
/// "#))); | ||
/// ``` | ||
pub fn is_doc_comment<L: Language>(comment: &SyntaxTriviaPieceComments<L>) -> bool { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is now truly checking for "doc" comments, as defined by JSDoc's syntax. Again, I think this should probably be in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be worth adding a |
||
if !comment.has_newline() { | ||
return false; | ||
} | ||
|
||
let text = comment.text(); | ||
|
||
text.lines().enumerate().all(|(index, line)| { | ||
if index == 0 { | ||
line.starts_with("/**") | ||
} else { | ||
line.trim_start().starts_with('*') | ||
} | ||
}) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,15 +1,44 @@ | ||
//! Provides builders for comments and skipped token trivia. | ||
|
||
use crate::comments::is_doc_comment; | ||
use crate::format_element::tag::VerbatimKind; | ||
use crate::prelude::*; | ||
use crate::{ | ||
comments::{CommentKind, CommentStyle}, | ||
write, Argument, Arguments, CstFormatContext, FormatRefWithRule, GroupId, SourceComment, | ||
TextRange, | ||
}; | ||
use biome_rowan::{Language, SyntaxNode, SyntaxToken}; | ||
use biome_rowan::{Language, SyntaxNode, SyntaxToken, TextSize}; | ||
#[cfg(debug_assertions)] | ||
use std::cell::Cell; | ||
use std::ops::Sub; | ||
|
||
/// Returns true if: | ||
/// - `next_comment` is Some, and | ||
/// - both comments are documentation comments, and | ||
/// - both comments are multiline, and | ||
/// - the two comments are immediately adjacent to each other, with no characters between them. | ||
/// | ||
/// In this case, the comments are considered "nestled" - a pattern that JSDoc uses to represent | ||
/// overloaded types, which get merged together to create the final type for the subject. The | ||
/// comments must be kept immediately adjacent after formatting to preserve this behavior. | ||
/// | ||
/// There isn't much documentation about this behavior, but it is mentioned on the JSDoc repo | ||
/// for documentation: https://github.com/jsdoc/jsdoc.github.io/issues/40. Prettier also | ||
/// implements the same behavior: https://github.com/prettier/prettier/pull/13445/files#diff-3d5eaa2a1593372823589e6e55e7ca905f7c64203ecada0aa4b3b0cdddd5c3ddR160-R178 | ||
fn should_nestle_adjacent_doc_comments<L: Language>( | ||
first_comment: &SourceComment<L>, | ||
second_comment: &SourceComment<L>, | ||
) -> bool { | ||
let first = first_comment.piece(); | ||
let second = second_comment.piece(); | ||
|
||
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) | ||
Comment on lines
+36
to
+40
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} | ||
|
||
/// Formats the leading comments of `node` | ||
pub const fn format_leading_comments<L: Language>( | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No idea what the best way to do this is. It feels awkward to have to directly get the iterated and then There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you need both the current and next item, you can use let mut leading_comments_iter = leading_comments.iter().peekable();
for comment in leading_comments_iter {
let next_comment = leading_comments_iter.peek();
} |
||
let format_comment = FormatRefWithRule::new(comment, Context::CommentRule::default()); | ||
write!(f, [format_comment])?; | ||
|
||
match comment.kind() { | ||
CommentKind::Block | CommentKind::InlineBlock => { | ||
match comment.lines_after() { | ||
0 => write!(f, [space()])?, | ||
0 => { | ||
let should_nestle = | ||
leading_comments | ||
.get(index + 1) | ||
.map_or(false, |next_comment| { | ||
should_nestle_adjacent_doc_comments(comment, next_comment) | ||
}); | ||
|
||
write!(f, [maybe_space(!should_nestle)])?; | ||
} | ||
1 => { | ||
if comment.lines_before() == 0 { | ||
write!(f, [soft_line_break_or_space()])?; | ||
|
@@ -94,12 +132,17 @@ where | |
}; | ||
|
||
let mut total_lines_before = 0; | ||
let mut previous_comment: Option<&SourceComment<Context::Language>> = None; | ||
|
||
for comment in trailing_comments { | ||
total_lines_before += comment.lines_before(); | ||
|
||
let format_comment = FormatRefWithRule::new(comment, Context::CommentRule::default()); | ||
|
||
let should_nestle = previous_comment.map_or(false, |previous_comment| { | ||
should_nestle_adjacent_doc_comments(previous_comment, comment) | ||
}); | ||
|
||
// This allows comments at the end of nested structures: | ||
// { | ||
// x: 1, | ||
|
@@ -117,7 +160,25 @@ where | |
[ | ||
line_suffix(&format_with(|f| { | ||
match comment.lines_before() { | ||
0 | 1 => write!(f, [hard_line_break()])?, | ||
_ if should_nestle => {} | ||
Comment on lines
-120
to
+162
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 afaict, that matches Prettier's behavior in all cases, and all of the tests still pass, so I assume that's true. |
||
0 => { | ||
// If the comment is immediately following a block-like comment, | ||
// then it can stay on the same line with just a space between. | ||
// Otherwise, it gets a hard break. | ||
// | ||
// /** hello */ // hi | ||
// /** | ||
// * docs | ||
// */ /* still on the same line */ | ||
if previous_comment.map_or(false, |previous_comment| { | ||
previous_comment.kind().is_line() | ||
}) { | ||
write!(f, [hard_line_break()])?; | ||
} else { | ||
write!(f, [space()])?; | ||
} | ||
} | ||
1 => write!(f, [hard_line_break()])?, | ||
_ => write!(f, [empty_line()])?, | ||
}; | ||
|
||
|
@@ -127,14 +188,16 @@ where | |
] | ||
)?; | ||
} else { | ||
let content = format_with(|f| write!(f, [space(), format_comment])); | ||
let content = | ||
format_with(|f| write!(f, [maybe_space(!should_nestle), format_comment])); | ||
if comment.kind().is_line() { | ||
write!(f, [line_suffix(&content), expand_parent()])?; | ||
} else { | ||
write!(f, [content])?; | ||
} | ||
} | ||
|
||
previous_comment = Some(comment); | ||
comment.mark_formatted(); | ||
} | ||
|
||
|
@@ -245,21 +308,30 @@ where | |
if dangling_comments.is_empty() { | ||
return Ok(()); | ||
} | ||
|
||
// Write all comments up to the first skipped token trivia or the token | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. General question: are The convenience is nice to not have to implement the awkward conditional below on Line 326 lol, but oh well. |
||
let mut previous_comment: Option<&SourceComment<Context::Language>> = None; | ||
|
||
for comment in dangling_comments { | ||
let format_comment = | ||
FormatRefWithRule::new(comment, Context::CommentRule::default()); | ||
join.entry(&format_comment); | ||
|
||
let should_nestle = previous_comment.map_or(false, |previous_comment| { | ||
should_nestle_adjacent_doc_comments(previous_comment, comment) | ||
}); | ||
|
||
write!( | ||
f, | ||
[ | ||
(previous_comment.is_some() && !should_nestle).then_some(hard_line_break()), | ||
format_comment | ||
] | ||
)?; | ||
|
||
previous_comment = Some(comment); | ||
comment.mark_formatted(); | ||
} | ||
|
||
join.finish()?; | ||
|
||
if matches!(self.indent(), DanglingIndentMode::Soft) | ||
&& dangling_comments | ||
.last() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the only change that came from that adjustment to the trailing comment logic. It matches Prettier's output now. |
||
|
||
[3 /* trailing num */ /* trailing sep */]; | ||
|
||
|
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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'sisIndentableBlockComment
naming.All of the existing usages have also been renamed, of which there were only like 2 anyway.