Skip to content
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

Merged
merged 2 commits into from
Dec 16, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 63 additions & 7 deletions crates/biome_formatter/src/comments.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Contributor Author

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's isIndentableBlockComment naming.

All of the existing usages have also been renamed, of which there were only like 2 anyway.

if !comment.has_newline() {
return false;
}
Expand All @@ -1217,3 +1219,57 @@ pub fn is_doc_comment<L: Language>(comment: &SyntaxTriviaPieceComments<L>) -> bo
}
})
}

/// **TODO:** This is really JS-specific logic, both in syntax and semantics.
/// It should probably be moved to `biome_js_formatter` when possible, but is
/// currently tied to other behavior about formatting sets of comments (which
/// might also be best to move as well, since it relates to the same specific
/// behavior).
///
/// 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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be worth adding a TODO saying what you say here.

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('*')
}
})
}
93 changes: 82 additions & 11 deletions crates/biome_formatter/src/trivia.rs
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:/jsdoc/jsdoc.github.io/issues/40. Prettier also
/// implements the same behavior: https:/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
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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>(
Expand Down Expand Up @@ -37,14 +66,22 @@ where
FormatLeadingComments::Comments(comments) => comments,
};

for comment in leading_comments {
let mut leading_comments_iter = leading_comments.iter().peekable();
while let Some(comment) = leading_comments_iter.next() {
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_iter.peek().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()])?;
Expand Down Expand Up @@ -94,12 +131,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,
Expand All @@ -117,7 +159,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
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 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.

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()])?,
};

Expand All @@ -127,14 +187,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();
}

Expand Down Expand Up @@ -245,21 +307,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());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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()
Expand Down
6 changes: 3 additions & 3 deletions crates/biome_js_formatter/src/comments.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use crate::prelude::*;
use crate::utils::AnyJsConditional;
use biome_diagnostics_categories::category;
use biome_formatter::comments::is_doc_comment;
use biome_formatter::comments::is_alignable_comment;
use biome_formatter::{
comments::{
CommentKind, CommentPlacement, CommentStyle, CommentTextPosition, Comments,
Expand Down Expand Up @@ -33,12 +33,12 @@ impl FormatRule<SourceComment<JsLanguage>> for FormatJsLeadingComment {
comment: &SourceComment<JsLanguage>,
f: &mut Formatter<Self::Context>,
) -> FormatResult<()> {
if is_doc_comment(comment.piece()) {
if is_alignable_comment(comment.piece()) {
let mut source_offset = comment.piece().text_range().start();

let mut lines = comment.piece().text().lines();

// SAFETY: Safe, `is_doc_comment` only returns `true` for multiline comments
// SAFETY: Safe, `is_alignable_comment` only returns `true` for multiline comments
let first_line = lines.next().unwrap();
write!(f, [dynamic_text(first_line.trim_end(), source_offset)])?;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 */];

Expand Down

This file was deleted.

Loading