From 235020c9f1e5526d4d05f8d41b1bb86fdb5972b8 Mon Sep 17 00:00:00 2001 From: Jon Egeland Date: Sat, 9 Dec 2023 21:37:12 +0000 Subject: [PATCH 1/3] fix(js_formatter): fix array fill elements expanding from leading comments --- crates/biome_formatter/src/printer/mod.rs | 98 +++++++++++++------ .../src/js/lists/array_element_list.rs | 5 +- .../src/jsx/lists/child_list.rs | 24 ++++- crates/biome_js_formatter/tests/quick_test.rs | 9 +- .../fill-array-comments.js.snap | 3 +- .../js/arrays/numbers-negative.js.snap | 73 -------------- .../numbers-with-tricky-comments.js.snap | 57 ----------- 7 files changed, 104 insertions(+), 165 deletions(-) delete mode 100644 crates/biome_js_formatter/tests/specs/prettier/js/arrays/numbers-negative.js.snap delete mode 100644 crates/biome_js_formatter/tests/specs/prettier/js/arrays/numbers-with-tricky-comments.js.snap diff --git a/crates/biome_formatter/src/printer/mod.rs b/crates/biome_formatter/src/printer/mod.rs index 826cc5653372..c88ef3ded0e3 100644 --- a/crates/biome_formatter/src/printer/mod.rs +++ b/crates/biome_formatter/src/printer/mod.rs @@ -105,29 +105,38 @@ impl<'a> Printer<'a> { } => self.print_text(slice, Some(*source_position)), FormatElement::Line(line_mode) => { - if args.mode().is_flat() - && matches!(line_mode, LineMode::Soft | LineMode::SoftOrSpace) - { - if line_mode == &LineMode::SoftOrSpace && self.state.line_width > 0 { - self.state.pending_space = true; + if args.mode().is_flat() { + match line_mode { + LineMode::Soft | LineMode::SoftOrSpace => { + if line_mode == &LineMode::SoftOrSpace && self.state.line_width > 0 { + self.state.pending_space = true; + } + return Ok(()); + } + LineMode::Hard | LineMode::Empty => { + self.state.measured_group_fits = false; + } } - } else if self.state.line_suffixes.has_pending() { + } + + if self.state.line_suffixes.has_pending() { self.flush_line_suffixes(queue, stack, Some(element)); - } else { - // Only print a newline if the current line isn't already empty - if self.state.line_width > 0 { - self.print_str("\n"); - } + return Ok(()); + } - // Print a second line break if this is an empty line - if line_mode == &LineMode::Empty && !self.state.has_empty_line { - self.print_str("\n"); - self.state.has_empty_line = true; - } + // Only print a newline if the current line isn't already empty + if self.state.line_width > 0 { + self.print_str("\n"); + } - self.state.pending_space = false; - self.state.pending_indent = args.indention(); + // Print a second line break if this is an empty line + if line_mode == &LineMode::Empty && !self.state.has_empty_line { + self.print_str("\n"); + self.state.has_empty_line = true; } + + self.state.pending_space = false; + self.state.pending_indent = args.indention(); } FormatElement::ExpandParent => { @@ -858,8 +867,6 @@ struct FitsMeasurer<'a, 'print> { bomb: DebugDropBomb, } -impl<'a, 'print> FitsMeasurer<'a, 'print> {} - impl<'a, 'print> FitsMeasurer<'a, 'print> { fn new_flat( print_queue: &'print PrintQueue<'a>, @@ -912,9 +919,7 @@ impl<'a, 'print> FitsMeasurer<'a, 'print> { while let Some(element) = self.queue.pop() { match self.fits_element(element)? { Fits::Yes => return Ok(true), - Fits::No => { - return Ok(false); - } + Fits::No => return Ok(false), Fits::Maybe => { if predicate.is_end(element)? { break; @@ -989,11 +994,48 @@ impl<'a, 'print> FitsMeasurer<'a, 'print> { } LineMode::Soft => {} LineMode::Hard | LineMode::Empty => { - return Ok(if self.must_be_flat { - Fits::No - } else { - Fits::Yes - }); + // Even in flat mode, content that _directly_ contains a hard or empty + // line is considered to fit when a hard break is reached, since that + // break is always going to exist, regardless of the print mode. + // This is particularly important for `Fill` entries, where _ungrouped_ + // content that contains hard breaks shouldn't force the surrounding + // elements to also expand. Example: + // [ + // -1, -2, 3 + // // leading comment + // -4, -5, -6 + // ] + // Here, `-4` contains a hardline because of the leading comment, but that + // doesn't cause the element (`-4`) nor the separator (`,`) to print in + // expanded mode, allowing the rest of the elements to fill in. If this + // _did_ respect `must_be_flat` and returned `Fits::No` instead, the result + // would put the `-4` on its own line, which is not preferable (at least, + // it doesn't match Prettier): + // [ + // -1, -2, 3 + // // leading comment + // -4, + // -5, -6 + // ] + // The perception here is that most comments inline for fills are used to + // separate _groups_ rather than to single out an individual element. + // + // The alternative case is when the fill entry is grouped, in which case + // this fit returns `Yes`, but the parent group is already known to + // expand _because_ of this hard line, and so the fill entry and separator + // are automatically printed in expanded mode anyway, and this fit check + // has no bearing on that (so it doesn't need to care about flat or not): + //
+ // + // + // {" "} + // ({variable}) + //
+ // Here the `...` _is_ grouped and contains a hardline, so it + // is known to break and _not_ fit already because the check is performed + // on the group. But within the group itself, the content with hardlines + // (the `\n\n`) _does_ fit, for the same logic in the first case. + return Ok(Fits::Yes); } } } else { diff --git a/crates/biome_js_formatter/src/js/lists/array_element_list.rs b/crates/biome_js_formatter/src/js/lists/array_element_list.rs index 3ef36ef3398b..0a7a88e4fafd 100644 --- a/crates/biome_js_formatter/src/js/lists/array_element_list.rs +++ b/crates/biome_js_formatter/src/js/lists/array_element_list.rs @@ -45,8 +45,11 @@ impl FormatRule for FormatJsArrayElementList { ) { filler.entry( &format_once(|f| { - if get_lines_before(element?.syntax()) > 1 { + let element = element?; + if get_lines_before(element.syntax()) > 1 { write!(f, [empty_line()]) + } else if f.comments().has_leading_own_line_comment(element.syntax()) { + write!(f, [hard_line_break()]) } else { write!(f, [soft_line_break_or_space()]) } diff --git a/crates/biome_js_formatter/src/jsx/lists/child_list.rs b/crates/biome_js_formatter/src/jsx/lists/child_list.rs index 671c564778d8..f128598cfceb 100644 --- a/crates/biome_js_formatter/src/jsx/lists/child_list.rs +++ b/crates/biome_js_formatter/src/jsx/lists/child_list.rs @@ -665,7 +665,29 @@ impl Format for FormatMultilineChildren { Ok(()) }); - write!(f, [block_indent(&format_inner)]) + // This indent is wrapped with a group to ensure that the print mode is + // set to `Expanded` when the group prints and will guarantee that the + // content _does not_ fit when printed as part of a `Fill`. Example: + //
+ // + // + // {" "} + // ({variable}) + //
+ // The `...` is the element that gets wrapped in the group + // by this line. Importantly, it contains a hard line break, and because + // [FitsMeasurer::fits_element] considers all hard lines as `Fits::Yes`, + // it will cause the element and the following separator to be printed + // in flat mode due to the logic of `Fill`. But because the we know the + // item breaks over multiple lines, we want it to _not_ fit and print + // both the content and the separator in Expanded mode, keeping the + // formatting as shown above. + // + // The `group` here allows us to opt-in to telling the `FitsMeasurer` + // that content that breaks shouldn't be considered flat and should be + // expanded. This is in contrast to something like a concise array fill, + // which _does_ allow breaks to fit and preserves density. + write!(f, [group(&block_indent(&format_inner))]) } } diff --git a/crates/biome_js_formatter/tests/quick_test.rs b/crates/biome_js_formatter/tests/quick_test.rs index 00544a75f4d9..1608925caaef 100644 --- a/crates/biome_js_formatter/tests/quick_test.rs +++ b/crates/biome_js_formatter/tests/quick_test.rs @@ -9,13 +9,16 @@ mod language { include!("language.rs"); } -#[ignore] +// #[ignore] #[test] // use this test check if your snippet prints as you wish, without using a snapshot fn quick_test() { let src = r#" - ((C) => (props) => ); - (({C}) => (props) => ); +
+ + + ({variable}) +
; "#; let source_type = JsFileSource::tsx(); let tree = parse( diff --git a/crates/biome_js_formatter/tests/specs/js/module/prettier-differences/fill-array-comments.js.snap b/crates/biome_js_formatter/tests/specs/js/module/prettier-differences/fill-array-comments.js.snap index 545372062c35..541f11d4e289 100644 --- a/crates/biome_js_formatter/tests/specs/js/module/prettier-differences/fill-array-comments.js.snap +++ b/crates/biome_js_formatter/tests/specs/js/module/prettier-differences/fill-array-comments.js.snap @@ -45,8 +45,7 @@ Bracket same line: false // This is the case because Prettier doesn't add virtual groups around `fill` elements, making it return `true` when it // encounters the first hard line break. As it happens, this line comment contains a hard line break, making // Prettier believe that the `-3` with this leading comment all fits on the line, which, obviously, isn't the case. - -3, - -2, + -3, -2, ]; ``` diff --git a/crates/biome_js_formatter/tests/specs/prettier/js/arrays/numbers-negative.js.snap b/crates/biome_js_formatter/tests/specs/prettier/js/arrays/numbers-negative.js.snap deleted file mode 100644 index 5031cbdfa5a7..000000000000 --- a/crates/biome_js_formatter/tests/specs/prettier/js/arrays/numbers-negative.js.snap +++ /dev/null @@ -1,73 +0,0 @@ ---- -source: crates/biome_formatter_test/src/snapshot_builder.rs -info: js/arrays/numbers-negative.js ---- - -# Input - -```js -const numbers1 = [-2017,-506252,-744011292,-7224,-70.4,-83353.6,-708.4,-174023963.52,-40385, -// comment1 --380014, --253951682,-728,-15.84,-2058467564.56,-43,-33,-85134845,-67092,-1,-78820379,-2371.6,-16,7, -// comment2 --62454,-4282239912, --10816495.36,0.88,-100622682,8.8,-67087.68000000001,-3758276,-25.5211,-54,-1184265243,-46073628,-280423.44, --41833463,-27961.12,-305.36,-199875.28]; - -const numbers2 = [-234, -342 // comment3 -, -223, -333333.33,12345] - -``` - - -# Prettier differences - -```diff ---- Prettier -+++ Biome -@@ -2,10 +2,12 @@ - -2017, -506252, -744011292, -7224, -70.4, -83353.6, -708.4, -174023963.52, - -40385, - // comment1 -- -380014, -253951682, -728, -15.84, -2058467564.56, -43, -33, -85134845, -- -67092, -1, -78820379, -2371.6, -16, 7, -+ -380014, -+ -253951682, -728, -15.84, -2058467564.56, -43, -33, -85134845, -67092, -1, -+ -78820379, -2371.6, -16, 7, - // comment2 -- -62454, -4282239912, -10816495.36, 0.88, -100622682, 8.8, -67087.68000000001, -+ -62454, -+ -4282239912, -10816495.36, 0.88, -100622682, 8.8, -67087.68000000001, - -3758276, -25.5211, -54, -1184265243, -46073628, -280423.44, -41833463, - -27961.12, -305.36, -199875.28, - ]; -``` - -# Output - -```js -const numbers1 = [ - -2017, -506252, -744011292, -7224, -70.4, -83353.6, -708.4, -174023963.52, - -40385, - // comment1 - -380014, - -253951682, -728, -15.84, -2058467564.56, -43, -33, -85134845, -67092, -1, - -78820379, -2371.6, -16, 7, - // comment2 - -62454, - -4282239912, -10816495.36, 0.88, -100622682, 8.8, -67087.68000000001, - -3758276, -25.5211, -54, -1184265243, -46073628, -280423.44, -41833463, - -27961.12, -305.36, -199875.28, -]; - -const numbers2 = [ - -234, - -342, // comment3 - -223, - -333333.33, - 12345, -]; -``` - - diff --git a/crates/biome_js_formatter/tests/specs/prettier/js/arrays/numbers-with-tricky-comments.js.snap b/crates/biome_js_formatter/tests/specs/prettier/js/arrays/numbers-with-tricky-comments.js.snap deleted file mode 100644 index cd85f7b2e43a..000000000000 --- a/crates/biome_js_formatter/tests/specs/prettier/js/arrays/numbers-with-tricky-comments.js.snap +++ /dev/null @@ -1,57 +0,0 @@ ---- -source: crates/biome_formatter_test/src/snapshot_builder.rs -info: js/arrays/numbers-with-tricky-comments.js ---- - -# Input - -```js -const lazyCatererNumbers = [1, 2, 4, 7, 11, 16, 22, 29, 37, 46, -56, 67, 79, 92, 106, 121, 137, 154, 172, 191, 211, 232, 254, 277, 301, 326, 352, 379, 407, 436, 466, /*block*/ -// line -497, 529, 562, 596, 631, 667, 704, 742, 781, -821, 862, 904, 947, 991, 1036, 1082, 1129, 1177, 1226, -// line 2 -1276, 1327, 1379]; - -``` - - -# Prettier differences - -```diff ---- Prettier -+++ Biome -@@ -2,8 +2,10 @@ - 1, 2, 4, 7, 11, 16, 22, 29, 37, 46, 56, 67, 79, 92, 106, 121, 137, 154, 172, - 191, 211, 232, 254, 277, 301, 326, 352, 379, 407, 436, 466 /*block*/, - // line -- 497, 529, 562, 596, 631, 667, 704, 742, 781, 821, 862, 904, 947, 991, 1036, -- 1082, 1129, 1177, 1226, -+ 497, -+ 529, 562, 596, 631, 667, 704, 742, 781, 821, 862, 904, 947, 991, 1036, 1082, -+ 1129, 1177, 1226, - // line 2 -- 1276, 1327, 1379, -+ 1276, -+ 1327, 1379, - ]; -``` - -# Output - -```js -const lazyCatererNumbers = [ - 1, 2, 4, 7, 11, 16, 22, 29, 37, 46, 56, 67, 79, 92, 106, 121, 137, 154, 172, - 191, 211, 232, 254, 277, 301, 326, 352, 379, 407, 436, 466 /*block*/, - // line - 497, - 529, 562, 596, 631, 667, 704, 742, 781, 821, 862, 904, 947, 991, 1036, 1082, - 1129, 1177, 1226, - // line 2 - 1276, - 1327, 1379, -]; -``` - - From 9feed106c03f725e08800b2561cca7b67f0be304 Mon Sep 17 00:00:00 2001 From: Jon Egeland Date: Sat, 9 Dec 2023 21:54:26 +0000 Subject: [PATCH 2/3] remove prettier difference spec test now that the result matches Prettier --- .../fill-array-comments.js | 8 --- .../fill-array-comments.js.snap | 59 ------------------- 2 files changed, 67 deletions(-) delete mode 100644 crates/biome_js_formatter/tests/specs/js/module/prettier-differences/fill-array-comments.js delete mode 100644 crates/biome_js_formatter/tests/specs/js/module/prettier-differences/fill-array-comments.js.snap diff --git a/crates/biome_js_formatter/tests/specs/js/module/prettier-differences/fill-array-comments.js b/crates/biome_js_formatter/tests/specs/js/module/prettier-differences/fill-array-comments.js deleted file mode 100644 index 27b71ff1a238..000000000000 --- a/crates/biome_js_formatter/tests/specs/js/module/prettier-differences/fill-array-comments.js +++ /dev/null @@ -1,8 +0,0 @@ -[ - // Prettier prints the `-2` element on the same line as the `-3`. - // This is the case because Prettier doesn't add virtual groups around `fill` elements, making it return `true` when it - // encounters the first hard line break. As it happens, this line comment contains a hard line break, making - // Prettier believe that the `-3` with this leading comment all fits on the line, which, obviously, isn't the case. - -3, - -2 -] diff --git a/crates/biome_js_formatter/tests/specs/js/module/prettier-differences/fill-array-comments.js.snap b/crates/biome_js_formatter/tests/specs/js/module/prettier-differences/fill-array-comments.js.snap deleted file mode 100644 index 541f11d4e289..000000000000 --- a/crates/biome_js_formatter/tests/specs/js/module/prettier-differences/fill-array-comments.js.snap +++ /dev/null @@ -1,59 +0,0 @@ ---- -source: crates/biome_formatter_test/src/snapshot_builder.rs -info: js/module/prettier-differences/fill-array-comments.js ---- - -# Input - -```js -[ - // Prettier prints the `-2` element on the same line as the `-3`. - // This is the case because Prettier doesn't add virtual groups around `fill` elements, making it return `true` when it - // encounters the first hard line break. As it happens, this line comment contains a hard line break, making - // Prettier believe that the `-3` with this leading comment all fits on the line, which, obviously, isn't the case. - -3, - -2 -] - -``` - - -============================= - -# Outputs - -## Output 1 - ------ -Indent style: Tab -Indent width: 2 -Line ending: LF -Line width: 80 -Quote style: Double Quotes -JSX quote style: Double Quotes -Quote properties: As needed -Trailing comma: All -Semicolons: Always -Arrow parentheses: Always -Bracket spacing: true -Bracket same line: false ------ - -```js -[ - // Prettier prints the `-2` element on the same line as the `-3`. - // This is the case because Prettier doesn't add virtual groups around `fill` elements, making it return `true` when it - // encounters the first hard line break. As it happens, this line comment contains a hard line break, making - // Prettier believe that the `-3` with this leading comment all fits on the line, which, obviously, isn't the case. - -3, -2, -]; -``` - -# Lines exceeding max width of 80 characters -``` - 3: // This is the case because Prettier doesn't add virtual groups around `fill` elements, making it return `true` when it - 4: // encounters the first hard line break. As it happens, this line comment contains a hard line break, making - 5: // Prettier believe that the `-3` with this leading comment all fits on the line, which, obviously, isn't the case. -``` - - From 1ba3dcd89945b9939801ac317d074292f2c48f05 Mon Sep 17 00:00:00 2001 From: Jon Egeland Date: Sat, 9 Dec 2023 22:02:06 +0000 Subject: [PATCH 3/3] un-unignore quick_tet --- crates/biome_js_formatter/tests/quick_test.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/biome_js_formatter/tests/quick_test.rs b/crates/biome_js_formatter/tests/quick_test.rs index 1608925caaef..a6361694f7ed 100644 --- a/crates/biome_js_formatter/tests/quick_test.rs +++ b/crates/biome_js_formatter/tests/quick_test.rs @@ -9,7 +9,7 @@ mod language { include!("language.rs"); } -// #[ignore] +#[ignore] #[test] // use this test check if your snippet prints as you wish, without using a snapshot fn quick_test() {