From 8206d7e6280b372aeec3b55e75d6ef128fd59f80 Mon Sep 17 00:00:00 2001 From: Jon Egeland Date: Sat, 2 Dec 2023 20:38:37 +0000 Subject: [PATCH 1/2] fix(js_formatter): Use fluid assignment layout when left side is breakable --- crates/biome_formatter/src/format_element.rs | 17 +++++ .../src/format_element/document.rs | 32 +++++++++ .../src/utils/assignment_like.rs | 26 ++++--- .../src/utils/format_modifiers.rs | 4 ++ crates/biome_js_formatter/tests/quick_test.rs | 17 ++--- .../js/assignment/issue-15534.js.snap | 68 ------------------- 6 files changed, 74 insertions(+), 90 deletions(-) delete mode 100644 crates/biome_js_formatter/tests/specs/prettier/js/assignment/issue-15534.js.snap diff --git a/crates/biome_formatter/src/format_element.rs b/crates/biome_formatter/src/format_element.rs index a38434996cac..70a5c6bc9ece 100644 --- a/crates/biome_formatter/src/format_element.rs +++ b/crates/biome_formatter/src/format_element.rs @@ -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(_)) + } } impl FormatElements for FormatElement { @@ -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, @@ -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; + /// Returns true if the element has the given label. fn has_label(&self, label: LabelId) -> bool; diff --git a/crates/biome_formatter/src/format_element/document.rs b/crates/biome_formatter/src/format_element/document.rs index 33d2c1055c96..75ad96e3d2f6 100644 --- a/crates/biome_formatter/src/format_element/document.rs +++ b/crates/biome_formatter/src/format_element/document.rs @@ -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; + } + } + + 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)) diff --git a/crates/biome_js_formatter/src/utils/assignment_like.rs b/crates/biome_js_formatter/src/utils/assignment_like.rs index 6dd50cd32f16..ff06444e5421 100644 --- a/crates/biome_js_formatter/src/utils/assignment_like.rs +++ b/crates/biome_js_formatter/src/utils/assignment_like.rs @@ -658,6 +658,7 @@ impl AnyJsAssignmentLike { fn layout( &self, is_left_short: bool, + left_may_break: bool, f: &mut Formatter, ) -> FormatResult { if self.has_only_left_hand_side() { @@ -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); } @@ -1011,11 +1014,12 @@ impl Format 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)); diff --git a/crates/biome_js_formatter/src/utils/format_modifiers.rs b/crates/biome_js_formatter/src/utils/format_modifiers.rs index 3e9d15790adb..be758ac29f1f 100644 --- a/crates/biome_js_formatter/src/utils/format_modifiers.rs +++ b/crates/biome_js_formatter/src/utils/format_modifiers.rs @@ -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(()); + } + // 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| { diff --git a/crates/biome_js_formatter/tests/quick_test.rs b/crates/biome_js_formatter/tests/quick_test.rs index e71fdc0ab672..2c9b21d2d70e 100644 --- a/crates/biome_js_formatter/tests/quick_test.rs +++ b/crates/biome_js_formatter/tests/quick_test.rs @@ -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(); diff --git a/crates/biome_js_formatter/tests/specs/prettier/js/assignment/issue-15534.js.snap b/crates/biome_js_formatter/tests/specs/prettier/js/assignment/issue-15534.js.snap deleted file mode 100644 index c783ee10b96b..000000000000 --- a/crates/biome_js_formatter/tests/specs/prettier/js/assignment/issue-15534.js.snap +++ /dev/null @@ -1,68 +0,0 @@ ---- -source: crates/biome_formatter_test/src/snapshot_builder.rs -info: js/assignment/issue-15534.js ---- - -# Input - -```js -params["redirectTo"] = `${window.location.pathname}${window.location.search}${window.location.hash}`; - -params["redirectTo"]["codePointAt"]["name"] = - `${window.location.pathname}${window.location.search}${window.location.hash}`; - -params.redirectTo.bar.bar.ba.barab["foo"].abr = - `${window.location.pathname}${window.location.search}${window.location.hash}`; - -``` - - -# Prettier differences - -```diff ---- Prettier -+++ Biome -@@ -1,8 +1,11 @@ --params["redirectTo"] = -- `${window.location.pathname}${window.location.search}${window.location.hash}`; -+params[ -+ "redirectTo" -+] = `${window.location.pathname}${window.location.search}${window.location.hash}`; - --params["redirectTo"]["codePointAt"]["name"] = -- `${window.location.pathname}${window.location.search}${window.location.hash}`; -+params["redirectTo"]["codePointAt"][ -+ "name" -+] = `${window.location.pathname}${window.location.search}${window.location.hash}`; - --params.redirectTo.bar.bar.ba.barab["foo"].abr = -- `${window.location.pathname}${window.location.search}${window.location.hash}`; -+params.redirectTo.bar.bar.ba.barab[ -+ "foo" -+].abr = `${window.location.pathname}${window.location.search}${window.location.hash}`; -``` - -# Output - -```js -params[ - "redirectTo" -] = `${window.location.pathname}${window.location.search}${window.location.hash}`; - -params["redirectTo"]["codePointAt"][ - "name" -] = `${window.location.pathname}${window.location.search}${window.location.hash}`; - -params.redirectTo.bar.bar.ba.barab[ - "foo" -].abr = `${window.location.pathname}${window.location.search}${window.location.hash}`; -``` - -# Lines exceeding max width of 80 characters -``` - 3: ] = `${window.location.pathname}${window.location.search}${window.location.hash}`; - 7: ] = `${window.location.pathname}${window.location.search}${window.location.hash}`; - 11: ].abr = `${window.location.pathname}${window.location.search}${window.location.hash}`; -``` - - From 692d439f37a92b378412c0bd01e8cef4f8347a76 Mon Sep 17 00:00:00 2001 From: Jon Egeland Date: Mon, 4 Dec 2023 19:03:03 +0000 Subject: [PATCH 2/2] add comment for early return --- .../src/utils/format_modifiers.rs | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/crates/biome_js_formatter/src/utils/format_modifiers.rs b/crates/biome_js_formatter/src/utils/format_modifiers.rs index be758ac29f1f..a4b76b385d27 100644 --- a/crates/biome_js_formatter/src/utils/format_modifiers.rs +++ b/crates/biome_js_formatter/src/utils/format_modifiers.rs @@ -26,6 +26,31 @@ where let modifiers = sort_modifiers_by_precedence(&self.list); let should_expand = should_expand_decorators(&self.list); + // Returning early here is important, because otherwise this node + // returns a group that always has a soft line break, which causes + // `may_directly_break` to return true, even if there is no + // possibility for the break to be used, since it's at the start of + // a line with no content before it. An example case this affects is: + // + // ```js + // class Test { + // prop1 = // comment + // true; + // } + // ``` + // + // Here, the modifier list is present before `prop1`, but part of the + // statement as a whole. The statement checks if it _may_ break when + // determining how to position the trailing comment. If it does break, + // the comment is placed on a new line and the value on a line after + // that. But if it doesn't break, then the whole statement can remnain + // on a single line, which is desirable and important for preserving + // semantics of things like ignore comments. + // + // ```js + // class Test { + // prop1 = true; // comment + // } if self.list.is_empty() { return Ok(()); }