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): Use fluid assignment layout when left side is breakable #1021

Merged
merged 3 commits into from
Dec 4, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
17 changes: 17 additions & 0 deletions crates/biome_formatter/src/format_element.rs
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,10 @@ impl FormatElement {
pub const fn is_space(&self) -> bool {
matches!(self, FormatElement::Space)
}

pub const fn is_line(&self) -> bool {
matches!(self, FormatElement::Line(_))
}
Comment on lines +236 to +238
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kinda surprised this didn't exist before. Turns out it's not actually necessary because may_directly_break does this check itself, but I figure this makes sense to keep around anyway.

}

impl FormatElements for FormatElement {
Expand All @@ -253,6 +257,10 @@ impl FormatElements for FormatElement {
}
}

fn may_directly_break(&self) -> bool {
matches!(self, FormatElement::Line(_))
}

fn has_label(&self, label_id: LabelId) -> bool {
match self {
FormatElement::Tag(Tag::StartLabelled(actual)) => *actual == label_id,
Expand Down Expand Up @@ -341,6 +349,15 @@ pub trait FormatElements {
/// lines if this element is part of a group and the group doesn't fit on a single line.
fn will_break(&self) -> bool;

/// Returns true if this [FormatElement] has the potential to break across multiple lines when printed.
/// This is the case _only_ if this format element recursively contains a [FormatElement::Line].
///
/// It's possible for [FormatElements::will_break] to return true while this function returns false,
/// such as when the group contains a [crate::builders::expand_parent] or some text within the group
/// contains a newline. Neither of those cases directly contain a [FormatElement::Line], and so they
/// do not _directly_ break.
fn may_directly_break(&self) -> bool;
Copy link
Member

Choose a reason for hiding this comment

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

I don't mind the name of the function, but I wonder if at this point, we should rename will_break in will_inderectly_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.

Honestly I don't know which is the better name between will_break, will_directly_break or will_indirectly_break...because they're all both accurate and inaccurate in different ways lol. indirectly i think implies that the element won't break itself, but some child element does, or something like that. directly implies that the element itself does, but children might also break instead. the plain will_break at least covers both of those, despite being a little more ambiguous to read.

Copy link
Member

Choose a reason for hiding this comment

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

You're right. I hope to find better names , because the current ones aren't very good and the prettier code base doesn't help


/// Returns true if the element has the given label.
fn has_label(&self, label: LabelId) -> bool;

Expand Down
32 changes: 32 additions & 0 deletions crates/biome_formatter/src/format_element/document.rs
Original file line number Diff line number Diff line change
Expand Up @@ -576,6 +576,38 @@ impl FormatElements for [FormatElement] {
false
}

fn may_directly_break(&self) -> bool {
use Tag::*;
let mut ignore_depth = 0usize;

for element in self {
match element {
// Line suffix
// Ignore if any of its content breaks
FormatElement::Tag(StartLineSuffix) => {
ignore_depth += 1;
}
FormatElement::Tag(EndLineSuffix) => {
ignore_depth -= 1;
}
FormatElement::Interned(interned) if ignore_depth == 0 => {
if interned.may_directly_break() {
return true;
}
}
Comment on lines +584 to +597
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'd like to not duplicate this code again, but idk a cleaner way to write it out. I think this state management is still necessary for this function, since it also shouldn't be inspecting text content on suffixes, but I'm not 100% sure.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine :) the duplicated code isn't that much


element if ignore_depth == 0 && element.may_directly_break() => {
return true;
}
_ => continue,
}
}

debug_assert_eq!(ignore_depth, 0, "Unclosed start container");

false
}

fn has_label(&self, expected: LabelId) -> bool {
self.first()
.map_or(false, |element| element.has_label(expected))
Expand Down
26 changes: 15 additions & 11 deletions crates/biome_js_formatter/src/utils/assignment_like.rs
Original file line number Diff line number Diff line change
Expand Up @@ -658,6 +658,7 @@ impl AnyJsAssignmentLike {
fn layout(
&self,
is_left_short: bool,
left_may_break: bool,
f: &mut Formatter<JsFormatContext>,
) -> FormatResult<AssignmentLikeLayout> {
if self.has_only_left_hand_side() {
Expand Down Expand Up @@ -726,17 +727,19 @@ impl AnyJsAssignmentLike {
return Ok(AssignmentLikeLayout::BreakAfterOperator);
}

if matches!(
right_expression,
Some(
AnyJsExpression::JsClassExpression(_)
| AnyJsExpression::JsTemplateExpression(_)
| AnyJsExpression::AnyJsLiteralExpression(
AnyJsLiteralExpression::JsBooleanLiteralExpression(_)
| AnyJsLiteralExpression::JsNumberLiteralExpression(_)
)
if !left_may_break
&& matches!(
right_expression,
Some(
AnyJsExpression::JsClassExpression(_)
| AnyJsExpression::JsTemplateExpression(_)
| AnyJsExpression::AnyJsLiteralExpression(
AnyJsLiteralExpression::JsBooleanLiteralExpression(_)
| AnyJsLiteralExpression::JsNumberLiteralExpression(_)
)
)
)
) {
{
return Ok(AssignmentLikeLayout::NeverBreakAfterOperator);
}

Expand Down Expand Up @@ -1011,11 +1014,12 @@ impl Format<JsFormatContext> for AnyJsAssignmentLike {
let mut buffer = VecBuffer::new(f.state_mut());
let is_left_short = self.write_left(&mut Formatter::new(&mut buffer))?;
let formatted_left = buffer.into_vec();
let left_may_break = formatted_left.may_directly_break();

// Compare name only if we are in a position of computing it.
// If not (for example, left is not an identifier), then let's fallback to false,
// so we can continue the chain of checks
let layout = self.layout(is_left_short, f)?;
let layout = self.layout(is_left_short, left_may_break, f)?;

let left = format_once(|f| f.write_elements(formatted_left));
let right = format_with(|f| self.write_right(f, layout));
Expand Down
4 changes: 4 additions & 0 deletions crates/biome_js_formatter/src/utils/format_modifiers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,10 @@ where
let modifiers = sort_modifiers_by_precedence(&self.list);
let should_expand = should_expand_decorators(&self.list);

if self.list.is_empty() {
return Ok(());
}
Comment on lines +54 to +56
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Returning early here is important, because otherwise this node returns a group that always has a soft_line_break_or_space, which causes may_directly_break to return true. An example case this affects is:

class Test {
  prop1 = // comment
    true;
}

Without this early escape, the IR gets written as:

group(expand: propagated, [
  group(expand: true, [soft_line_break_or_space]),
  " prop1",
  line_suffix([" // comment"]),
  expand_parent
]),
" =  true"

and that group with just the soft line is this modifier list. With this early return, that group no longer exists, meaning the outer group won't contain any potential breaks, and the comments get formatted as expected by Prettier.

Copy link
Member

Choose a reason for hiding this comment

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

This is a great comment, and a I think we should have it inside the code :) (maybe a variation of that)


// need to use peek the iterator to check if the current node is a decorator and don't advance the iterator
let mut iter = modifiers.into_iter().peekable();
let decorators = format_once(|f| {
Expand Down
17 changes: 6 additions & 11 deletions crates/biome_js_formatter/tests/quick_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,12 @@ mod language {
fn quick_test() {
let src = r#"

const makeSomeFunction =
(services = {logger:null}) =>
(a, b, c) =>
services.logger(a,b,c)

const makeSomeFunction2 =
(services = {
logger: null
}) =>
(a, b, c) =>
services.logger(a, b, c)
class Test {
prop2 = // test
2;
prop5 // test
= 5 // a
}

"#;
let syntax = JsFileSource::tsx();
Expand Down

This file was deleted.