fix(rome_js_formatter): in JSX, some spaces are removed #3211

[denbezrukov]

Summary

Fix #3211

Prettier removes softline, hardline before and after jsxWhitespace.

Open Question

There is another one difference between Rome and Prettier. Prettier adds jsxWhitespace and break a line after Text. But IR the same. Can it be a problem with fill builder?

Rome:

                 ...
		Text <a data-very-long-prop-breakline-rome-playground data-other>
			some link
		</a>
		....

Prettier:

                 ...
		Text{" "}
		<a data-very-long-prop-breakline-rome-playground data-other>
			some link
		</a>
		 ...

Playground

Test Plan

Add new test cases.
cargo test -p rome_js_formatter

This PR:
Average compatibility: 88.44
Compatible lines: 89.60

Main:
Average compatibility: 88.42
Compatible lines: 89.30

[netlify]

✅ Deploy Preview for docs-rometools ready!

Name	Link
🔨 Latest commit	5831a07
🔍 Latest deploy log	https://app.netlify.com/sites/docs-rometools/deploys/6343c34cd8e72b00084d6756
😎 Deploy Preview	https://deploy-preview-3251--docs-rometools.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[MichaReiser]

Regarding the open question, there's one difference that may be of importance:

// Prettier
"Text",
ifBreak(['{" "}', softline], " "),

// Rome 
fill([
  "Text",
  [],
  [
    if_group_breaks(["{" "}", soft_line_break]),
    if_group_fits_on_line([" "])
  ],
[],

Prettier prints the ifBreak into the separator slot of fill (fill is a sequence of item, separator, item, separator....) and a separator gets printed in expanded mode if:

• previous item, separator, next item dont fit on the line

For items, the item is only printed if it doesn't fit on the line, regardless of the next item or separator.

[denbezrukov]

Regarding the open question, there's one difference that may be of importance:

// Prettier
"Text",
ifBreak(['{" "}', softline], " "),

// Rome 
fill([
  "Text",
  [],
  [
    if_group_breaks(["{" "}", soft_line_break]),
    if_group_fits_on_line([" "])
  ],
[],

Prettier prints the ifBreak into the separator slot of fill (fill is a sequence of item, separator, item, separator....) and a separator gets printed in expanded mode if:

• previous item, separator, next item dont fit on the line

For items, the item is only printed if it doesn't fit on the line, regardless of the next item or separator.

Could you please explain more? What is the difference between Prettier ifBreak and Rome if_group_breaks + if_group_fits_on_line inside fill? :)

[MichaReiser]

As a small note. I noticed an instability with JSX formatting when adding the prettier tests and the fix is now included in the comments formatting. It required to move the best fitting element to the FormatJsxElement. I don't think this will effect this PR much.

https://github.com/rome/tools/blob/refactor/js-syntax-comments/crates/rome_js_formatter/src/jsx/tag/element.rs

[MichaReiser]

Could you please explain more? What is the difference between Prettier ifBreak and Rome if_group_breaks + if_group_fits_on_line inside fill? :)

The difference isn't around if_group_fits_on_line and if_group_breaks but the around the fill. I should have added the fill to prettier's example too

// Prettier
fill([
"Text",
ifBreak(['{" "}', softline], " "),
...

// Rome 
fill([
  "Text",
  [],
  [
    if_group_breaks(["{" "}", soft_line_break]),
    if_group_fits_on_line([" "])
  ],
[],
...

Fill, in Rome and prettier, is a sequence of item, separator, item, separator. As you can see, Prettier puts the softline into the separator "slot" whereas Rome adds an empty separator and writes the ifBreak into an item slot.

The printing of item and separator slots differ

tools/crates/rome_formatter/src/printer/mod.rs

Lines 360 to 482 in a124d36

	/// Tries to fit as much content as possible on a single line.
	/// Each item forms a virtual group that is either printed in flat or expanded mode.
	/// It handles three different cases:
	///
	/// * The first and second content fit on a single line. It prints the content and separator in flat mode.
	/// * The first content fits on a single line, but the second doesn't. It prints the content in flat and the separator in expanded mode.
	/// * Neither the first nor the second content fit on the line. It brings the first content and the separator in expanded mode.
	fn print_fill(
	&mut self,
	queue: &mut ElementCallQueue<'a>,
	content: &'a [FormatElement],
	args: PrintElementArgs,
	) {
	let empty_rest = ElementCallQueue::default();
	let mut items = content.iter();
	let current_content = match items.next() {
	None => {
	// Empty list
	return;
	}
	Some(item) => item,
	};
	let mut current_fits = fits_on_line(
	[current_content],
	args.with_print_mode(PrintMode::Flat),
	&empty_rest,
	self,
	);
	self.print_all(
	queue,
	&[current_content],
	args.with_print_mode(if current_fits {
	PrintMode::Flat
	} else {
	PrintMode::Expanded
	}),
	);
	// Process remaining items
	loop {
	match (items.next(), items.next()) {
	(Some(separator), Some(next_item)) => {
	// A line break in expanded mode is always necessary if the current item didn't fit.
	// otherwise see if both contents fit on the line.
	let current_and_next_fit = current_fits
	&& fits_on_line(
	[separator, next_item],
	args.with_print_mode(PrintMode::Flat),
	&empty_rest,
	self,
	);
	if current_and_next_fit {
	// Print Space and next item on the same line
	self.print_all(
	queue,
	&[separator, next_item],
	args.with_print_mode(PrintMode::Flat),
	);
	} else {
	// Print the separator and then check again if the next item fits on the line now
	self.print_all(
	queue,
	&[separator],
	args.with_print_mode(PrintMode::Expanded),
	);
	let next_fits = fits_on_line(
	[next_item],
	args.with_print_mode(PrintMode::Flat),
	&empty_rest,
	self,
	);
	if next_fits {
	self.print_all(
	queue,
	&[next_item],
	args.with_print_mode(PrintMode::Flat),
	);
	} else {
	self.print_all(
	queue,
	&[next_item],
	args.with_print_mode(PrintMode::Expanded),
	);
	}
	current_fits = next_fits;
	}
	}
	// Trailing separator
	(Some(separator), _) => {
	let print_mode = if current_fits
	&& fits_on_line(
	[separator],
	args.with_print_mode(PrintMode::Flat),
	&empty_rest,
	self,
	) {
	PrintMode::Flat
	} else {
	PrintMode::Expanded
	};
	self.print_all(queue, &[separator], args.with_print_mode(print_mode));
	}
	(None, None) => {
	break;
	}
	(None, Some(_)) => {
	// Unreachable for iterators implementing [FusedIterator] which slice.iter implements.
	// Reaching this means that the first `iter.next()` returned `None` but calling `iter.next()`
	// again returns `Some`
	unreachable!()
	}
	}
	}
	}

The logic is as follow:

Print the first item in flat if it fits on the line, otherwise print it in expanded mode. For all remaining separator & item pairs do:

• Test if separator and item fit on the same line (as the preceding item): If so, print both in flat
• Otherwise:
  • Print the separator in expanded mode
  • Test again if the item now fits on the line. If so, print in flat mode, otherwise print in expanded mode

That's why we should move the softline break into the separator slot.

[MichaReiser]

This is looking good to me. I added a few comments that would be good to address before merging.

I further recommend rebasing on main to get the prettier snapshot tests.

[MichaReiser]

Looking good. Would you mind adding the prettier compatibility metrics to the test plan.

And there seems to be an issue with the codegen CI job.

Edit: rebasing on main should fix the CI job failures

[denbezrukov]

After 1022663 the bug started reproducing again, need to figure out (:

[MichaReiser]

After 1022663 the bug started reproducing again, need to figure out (:

Hmm that's odd. You aren't introspecting any IR, aren't you? Let me know if you need any help.

[denbezrukov]

Hmm that's odd. You aren't introspecting any IR, aren't you? Let me know if you need any help.

Hm, previous version of print_fill had a condition:

 let current_and_next_fit = current_fits 
     && fits_on_line( 
         [separator, next_item], 
         args.with_print_mode(PrintMode::Flat), 
         &empty_rest, 
         self, 
     ); 

which tested a separator and a next_item. If elements couldn't be printed on one line the separator would be printed in an expanded mode.

I'm try to find the same condition in current version. But it seems that now print_fill_entries tests only a single element.

[MichaReiser]

Hmm that's odd. You aren't introspecting any IR, aren't you? Let me know if you need any help.

Hm, previous version of print_fill had a condition:

 let current_and_next_fit = current_fits 
     && fits_on_line( 
         [separator, next_item], 
         args.with_print_mode(PrintMode::Flat), 
         &empty_rest, 
         self, 
     ); 

which tested a separator and a next_item. If elements couldn't be printed on one line the separator would be printed in an expanded mode.

I'm try to find the same condition in current version. But it seems that now print_fill_entries tests only a single element.

The corresponding code now is covered by

tools/crates/rome_formatter/src/printer/mod.rs

Lines 459 to 462 in f3079b4

	let all_fits = if current_fits {
	self.fits_fill_entry(SeparatorItemPairPredicate::default(), queue, stack)?
	} else {
	false

The SeparatorItemPairPredicate takes the elements from the queue until it:

• either reaches the end of the fill (separator only, should be rare)
• it processed the separator and the next item

[MichaReiser]

I checked out your branch, generated the IR, and then rebased it on main and again, looked at the IR for:

before_break1 =
  <span>
    <span barbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbar /> foo
  </span>

Not rebased

fill([
  group([
    "<",
    "span",
    line_suffix_boundary,
    indent([
      soft_line_break_or_space,
      "barbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbar"
    ]),
    line_suffix_boundary,
    soft_line_break_or_space,
    "/",
    ">"
  ]),
  [
    if_group_breaks(["{" "}", soft_line_break]),
    if_group_fits_on_line([" "])
  ],
  [],
  "foo",
  []
])

Rebased

fill([
    <interned 1> [
      [
        group([
          "<span",
          line_suffix_boundary,
          indent([
            soft_line_break_or_space,
            "barbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbar"
          ]),
          line_suffix_boundary,
          soft_line_break_or_space,
          "/>"
        ])
      ],
      [],
      [
        if_group_breaks(["{" "}", soft_line_break]),
        if_group_fits_on_line([" "])
      ],
      [],
      ["foo"],
      []
    ]
  ])

Notice how the

[
  if_group_breaks(["{" "}", soft_line_break]),
  if_group_fits_on_line([" "])
],

Is the second child of fill for the not rebased version but becomes the third entry after rebasing on main.

[MichaReiser]

I think I managed to narrow it down. The issue is with

tools/crates/rome_js_formatter/src/jsx/lists/child_list.rs

Lines 177 to 180 in d03f026

	let format_separator = format_with(|f| match line_mode {
	Some(mode) => f.write_element(FormatElement::Line(mode)),
	None => Ok(()),
	});

This used to work because

tools/crates/rome_js_formatter/src/jsx/lists/child_list.rs

Lines 503 to 505 in d03f026

	write!(buffer, [separator])?;
	buffer.into_vec()
	}

assumed that separator only writes exactly one element without enforcing it. That means, the fact that the snipped didn't write any separator before "just worked" because the write implementation didn't correctly enforce it.

The rebased version that uses StartEntry and EndEntry now guarantees that a separator is written even if separator happens to not write anything.

I'm not entirely sure what's the best fix. The easy fix is to simply add a method to the multiline builder that let's you write a single entry without caring that item/separator are balanced. I don't mind that overly much but it eases that item/separator position can be "accidentally" flipped.

[denbezrukov]

I'm not entirely sure what's the best fix. The easy fix is to simply add a method to the multiline builder that let's you write a single entry without caring that item/separator are balanced. I don't mind that overly much but it eases that item/separator position can be "accidentally" flipped.

Yes, you're right! Thank you so much. I've just tried your suggestion and it worked. I'm going to push it.
What is another way to fix it? Try to take next element in SeparatorItemPairPredicate if current one is empty? Is it possible/better?

[MichaReiser]

Yes, you're right! Thank you so much. I've just tried your suggestion and it worked. I'm going to push it.
What is another way to fix it? Try to take next element in SeparatorItemPairPredicate if current one is empty? Is it possible/better?

I don't think this is something that should be fixed inside of the printer because the printer can't know what entry is intended to be the item or separator. Correctly writing the pairs is the responsibility of the formatting code.

Edit: That's why I think fixing it in JsxChildList is the way to go and we probably have to accept the fact that it won't be possible to fully express the guarantee that any child item always writes a item + separator. What I think would be useful though is to add comments for the cases where we only write one, explaining why it is safe to e.g. omit the separator in that case because we always know that, in this case, it must e.g. have reached the end of the iterator or the next element is guaranteed to be a space/newline that should be written into the separator "slot".

[denbezrukov]

This PR:
Average compatibility: 88.44
Compatible lines: 89.60

Main:
Average compatibility: 88.42
Compatible lines: 89.30

[denbezrukov]

@MichaReiser
I was thinking about how we can keep only one method with separator.

• https://github.com/denbezrukov/tools/blob/3880b2d1e39c128448b847535c3a9185697bcd65/crates/rome_js_formatter/src/jsx/lists/child_list.rs#L111-L115
• https://github.com/denbezrukov/tools/blob/3880b2d1e39c128448b847535c3a9185697bcd65/crates/rome_js_formatter/src/jsx/lists/child_list.rs#L204-L208

Here we can call next on the iterator for JsxChild::Newline , JsxChild::Whitespace, JsxChild::EmptyLine.
For None we can return empty format_with, because None means that we reach the end of the iteration.

• https://github.com/denbezrukov/tools/blob/3880b2d1e39c128448b847535c3a9185697bcd65/crates/rome_js_formatter/src/jsx/lists/child_list.rs#L166-L177
  I'm not sure that we can remove write_without_separator here. We can do it only if JsxChild::Newline and JsxChild::EmptyLine always go after JsxChild::Word, JsxChild::NonText.

[MichaReiser]

Nit: Here, we're writing the separator and we obviously don't want to write a separator with a separator :D

That's why I think it would be good to add a multiline.write_separator(...) method that has the same behaviour as write_without_separator but only exists to document that we differentiate between items that normally should be written with a separator and separators that... are separators and don't need a separator.

[denbezrukov]

Done (:

[MichaReiser]

Thank you so much for working on this. I have a small recommendation to introduce a new method on the builder to make it more clear that it sometimes writes an item without a separator (requires documentation), a separator (fine to not write a second separator), or an item with a separator.

@ematipico are you OK merging this change during my PTO in case @denbezrukov updates the PR after today?

[ematipico]

@ematipico are you OK merging this change during my PTO in case @denbezrukov updates the PR after today?

Sure, no problem 😃

[denbezrukov]

There is another problem =(

Let's say that we have:

 (
  <div>
    <br />
    texttexttexttexttexttexttexttexttexxxxxxxxxxttextxxxtexttext{typeeeggggee}{" "}
  </div>
);

Rome output in #3251:

<div>
	<br />
	texttexttexttexttexttexttexttexttexxxxxxxxxxttextxxxtexttext{typeeeggggee}{" "}
</div>;

Prettier output:

<div>
	<br />
	texttexttexttexttexttexttexttexttexxxxxxxxxxttextxxxtexttext{
		typeeeggggee
	}{" "}
</div>;

It has the Rome IR in #3251:

[
  <interned 0> [group(["<div", line_suffix_boundary, ">"])],
  indent([
    hard_line_break,
    fill([
      <interned 1> [
        ["<br", line_suffix_boundary, " />"],
        [hard_line_break],
        ["texttexttexttexttexttexttexttexttexxxxxxxxxxttextxxxtexttext"],
        [soft_line_break],
        [
          group([
            "{",
            indent([soft_line_break, "typeeeggggee"]),
            soft_line_break,
            line_suffix_boundary,
            "}"
          ])
        ],
        ["{" "}"]
      ]
    ])
  ]),
  hard_line_break,
  "</div",
  line_suffix_boundary,
  ">;",
  hard_line_break
]

which is close to the Prettier one:

[
  group([
    group(["<", "div", indent([]), ">"]),
    indent([
      hardline,
      fill([
        ["<", "br", " />"],
        hardline,
        "texttexttexttexttexttexttexttexttexxxxxxxxxxttextxxxtexttext",
        softline,
        group([
          "{",
          indent([softline, "typeeeggggee"]),
          softline,
          lineSuffixBoundary,
          "}",
        ]),
        '{" "}',
      ]),
    ]),
    hardline,
    "</",
    "div",
    ">",
  ]),
  ";",
  hardline,
]

It matched Prettier output before #3307 because when the printer checks

group([
  "{",
  indent([softline, "typeeeggggee"]),
  softline,
  lineSuffixBoundary,
  "}",
]),

tools/crates/rome_formatter/src/printer/mod.rs

Lines 198 to 233 in 1fd9b7b

	FormatElement::Tag(StartGroup(group)) => {
	let group_mode = if !group.mode().is_flat() {
	PrintMode::Expanded
	} else {
	match args.mode() {
	PrintMode::Flat if self.state.measured_group_fits => {
	// A parent group has already verified that this group fits on a single line
	// Thus, just continue in flat mode
	PrintMode::Flat
	}
	// The printer is either in expanded mode or it's necessary to re-measure if the group fits
	// because the printer printed a line break
	_ => {
	self.state.measured_group_fits = true;
	// Measure to see if the group fits up on a single line. If that's the case,
	// print the group in "flat" mode, otherwise continue in expanded mode
	stack.push(TagKind::Group, args.with_print_mode(PrintMode::Flat));
	let fits = fits_on_line(AllPredicate, queue, stack, self)?;
	stack.pop(TagKind::Group)?;
	if fits {
	PrintMode::Flat
	} else {
	PrintMode::Expanded
	}
	}
	}
	};
	stack.push(TagKind::Group, args.with_print_mode(group_mode));
	if let Some(id) = group.id() {
	self.state.group_modes.insert_print_mode(id, group_mode);
	}
	}

it went to the second branch and used theAllPredicate to check fits_on_line which included the {" "} element and used expand mode. After #3307 it doesn't include {" "} to check fits_on_line for the group.

[MichaReiser]

Yeah... I looked at it today and haven't been able to come up with a good solution.

The reason why this works for prettier is that they schedule the item and separator with the corresponding modes and then simply continue printing. That means if the item fits but the separator doesn't, then the print queue looks like this:

[ item, mode: flat]
[separator: mode expand]
... other filling entries

That means that if the item contains a group or best fitting and then measures if it fits, it ends at the separator that now gets printed in expanded mode (making any soft line break a hard line break). The expanding of the separator guarantees that any inner measuring "terminates".

I don't think this is entirely ideal because it requires that any group or best_fitting inside a fill item has to re-measure and the same can be mimicked by:

• Measuring if the item and separator fit.
  • Yes -> print the item in flat mode
  • No: Measure if the item with the expanded separator fits
  • Yes -> print the item in flat mode
  • No -> print the item and separator in expanded mode. -> continue to the next item
• Measure if the separator and the next item fit
  • Yes -> Print the separator in flat mode -> continue at the top
  • No -> Print the separator in expanded mode

How's this different from what we do today. Today, our implementation measures if the separator and next item fit, but never if the item + next separator do. This is important for the issue we're seeing now because we must expand the item if neither the flat nor expanded separator fit (there's no real alternative).

The challenge I'm facing is how to measure if the item + separator in expanded mode fit. The approach I have in mind is to create a FitsOnLine struct that keeps the internal fits queue, fits stack, and fits state across multiple fits_on_line calls. But I haven't had the time to entirely investigate how to implement it in detail

[MichaReiser]

The code for this could be similar to this:

while (not at end) {
	let mut fits_on_line = FitsOnLine::new(queue, stack, self); 
	
	let item_fits = fits_on_line.fits_fill_entry(PrintMode::Flat); 
	
	// Not even the item fits, print item and separator in expanded mode.
	if !item_fits {
	    drop(fits_on_line);
		self.print_entry(queue, stack , PrintMode::Expanded),
	    self.print_entry(queue, stack, PrintMode:Expanded),
	    continue;
	}

	let separator_fits = fits_on_line.fits_fill_entry(PrintMode::Flat);

	// The item fits, but now the separator doesn't...
    if !separator_fits {
       drop(fits_on_line);
       
		// Now test if the separator fits in expanded mode. 
       let mut fits_on_line = FitsOnLine::new(queue, stack, self); 
	   // skip item, we know it fits
       queue.skip_content(TagKind::Entry); 
	   let separator_fits = fits_on_line.fits_fill_entry(PrintMode::Expanded);
	   drop(fits_on_line);      

		// If the separator fits, then print the item in flat mode, but expand the separator
       if separator_fits {
		self.measured_fits_on_line = true;
        self.print_entry(queue, stack, PrintMode::Flat);
		self.measured_fits_on_line = false;
        self.print_entry(queue, stack, PrintMode::Expanded);
       } else {
		// If not, then print both in expanded mode (the best we can do in this situation).
		self.print_entry(queue, stack , PrintMode::Expanded),
	    self.print_entry(queue, stack, PrintMode:Expanded),
       }
 		continue;
    }

	// we know here that the item + separator fit. Now the question is if the next item fits as well.

	// Test if the next item fits too
	let next_fits = fits_on_line.fits_fill_entry(PrintMode::Flat);

	drop(fits_on_line);

	// Print the item in flat mode because we know the flat or expanded separator always fit.
	self.measured_fits_on_line = true;
	self.print_entry(queue, stack, PrintMode::Flat);
	
	// Print the separator in flat or expanded mode, depending if the next item also fits on the line
	self.measured_fits_on_line = next_fits;
	self.print_entry(queue, stack, if next_fits { PrintMode::Flat } else { PrintMode::Expanded })
	self.measured_fits_on_line = false;

	// Now continue with the next item. We may be clever here and avoid recomputing if the item fits but that might be difficult to accomplish
}

self.measured_fits_on_line = false;

The alternative would be to change Fill to a FormatElement instead of a Tag that stores all its items in a Box<[]>

[denbezrukov]

• Measuring if the item and separator fit.

  • Yes -> print the item in flat mode
  • No: Measure if the item with the expanded separator fits
  • Yes -> print the item in flat mode
  • No -> print the item and separator in expanded mode. -> continue to the next item

• Measure if the separator and the next item fit

  • Yes -> Print the separator in flat mode -> continue at the top
  • No -> Print the separator in expanded mode

I'm going to try to implement it in another PR.

[ematipico]

I suppose we have to delay this fix to the next release

[MichaReiser]

@denbezrukov How do you feel about updating the failing snapshot test? This would allow us to merge this PR and address the printer issue in a follow up PR.

[denbezrukov]

@denbezrukov How do you feel about updating the failing snapshot test? This would allow us to merge this PR and address the printer issue in a follow up PR.

Done.