fix(rome_formatter): measure first variant of BestFitting in flat mode

[yassere]

Summary

This is a minimal fix for the BestFitting element that was broken by #2645. It now attempts to fit the first variant in flat mode on a single line. If that doesn't fit, it continues attempting the remaining variants in PrintMode::Expanded before defaulting to the most_expanded variant. This seems to match the original intention based on the doc comments for the best_fitting macro. @MichaReiser?

I don't know if this will be sufficient for our needs, but I believe that matching the exact behavior of Prettier's conditionalGroup could involve heavy enough changes to our "fits" measurement and printing that it may be worth trying this out first.

Happy to close this, though, if we'd rather go with a different approach.

Test Plan

Removed the ignore from the broken best_fitting doc test.

[github-actions]

Playground for commit 2488026

[MichaReiser]

This may fix the test bit I don't think it works as expected because any group inside of the second or later variant always get printed in expanded mode (same as for rest elements). My understanding is that the printing of a variant should work the same as any other content outside of the group where the first group changes the print mode to flat to all its enclosing group and the mode only changes back for the groups from the rest queue

[yassere]

This may fix the test bit I don't think it works as expected because any group inside of the second or later variant always get printed in expanded mode (same as for rest elements). My understanding is that the printing of a variant should work the same as any other content outside of the group where the first group changes the print mode to flat to all its enclosing group and the mode only changes back for the groups from the rest queue

Ah, yeah that makes sense.

[cloudflare-workers-and-pages]

Deploying with    Cloudflare Pages

Latest commit:	5f9b437
Status:	✅  Deploy successful!
Preview URL:	https://e6dd693b.tools-8rn.pages.dev
Branch Preview URL:	https://fix-best-fitting.tools-8rn.pages.dev

View logs

[yassere]

My understanding is that the printing of a variant should work the same as any other content outside of the group

This seems to be the case, unless I'm misunderstanding what you're saying here. I added a test where the best fitting second variant is printed the same as it would be if it were printed directly. Inner groups are still printed flat when possible. Do you have an example where the current implementation doesn't work as intended? Do you think the problem is with how it measures, how it prints, or both?

Also, I'm not sure about this line:

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

Line 281 in f687573

self.state.measured_group_fits = true;

What's the purpose of setting that as true when measuring in expanded mode?

[MichaReiser]

My understanding is that the printing of a variant should work the same as any other content outside of the group

This seems to be the case, unless I'm misunderstanding what you're saying here. I added a test where the best fitting second variant is printed the same as it would be if it were printed directly. Inner groups are still printed flat when possible. Do you have an example where the current implementation doesn't work as intended? Do you think the problem is with how it measures, how it prints, or both?

It could be but let's say we have the following example:

// variant2: 
token("("), group_elements(
  token("["), soft_line_break(), 
  indent(token("abcdef")), 
  soft_line_break(), 
  token("]"))
)

My understanding is that the printer calls fits_on_line with print_mode: expanded for variant 2. The printer then enqueues the group 2 with the print mode it's currently in, the same as for the variant, which is expanded here

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

Line 726 in 941faf9

FormatElement::Group(group) => queue.enqueue(PrintElementCall::new(&group.content, args)),

Which will ultimately cause the soft_line_breaks to break because the print_mode is Expanded when the printer measures the first soft_line_break.

You can change the second variant of the best_fitting test to

///             format_args!(
///                 token("("),
///                 group_elements(&format_args![
///                     token("["),
///                     soft_block_indent(&token("1, 2, 3")),
///                     token("]"),
///                 ]),
///                 token(")"),
///             ),

This should not fit because the printer should try to print the inner group as flat and not as expanded but the printer breaks at the soft_line_breaks and, therefore claims that this variant fits.

aVeryLongIdentifier([\n\t1, 2, 3\n])

Also, I'm not sure about this line:

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

Line 281 in f687573

self.state.measured_group_fits = true;

What's the purpose of setting that as true when measuring in expanded mode?

The purpose on that line is to cache the fact that everything following now up to the first line break will fit on the line. So that the printer doesn't need to re-measure if the picked group contains an inner group:

// variant2: 
token("("), group_elements(token("["), token("abcdef"), token("]"))

There's no point in re-calculating if the group containing the array fits because the printer already ensured that this is the case when measuring the variant content.

The boolean gets set to false when printing the next line break and any group following then must be re-measured.

[yassere]

Which will ultimately cause the soft_line_breaks to break because the print_mode is Expanded when the printer measures the first soft_line_break.

They won't necessarily break during printing if that group fits on one line. The second variant will be selected if it fits in expanded mode, but a group that's enqueued for printing in expanded mode will still be printed flat if it fits.

If we have:

best_fitting!(
    format_args!(token(
        "Something that will not fit on a 30 character print width."
    )),
    format_args![
        token("("),
        group_elements(&format_args![
            token("["),
            soft_line_break(),
            indent(&token("abcdef")),
            soft_line_break(),
            token("]")
        ])
    ],
    format_args!(token("Most"), hard_line_break(), token("Expanded"))
)

Formatting that at width 80 prints the first variant.
Formatting that at width 30 prints the second variant as:

([abcdef]

Formatting that at width 8 prints the second variant as:

([
abcdef
]

Formatting that at width 1 defaults to printing the most expanded variant.

Does that address your concern or am I still misunderstanding?

This should not fit because the printer should try to print the inner group as flat and not as expanded but the printer breaks at the soft_line_breaks and, therefore claims that this variant fits.

That variant would be selected because it fits in expanded mode, but inner groups would still be printed flat if they fit.
If we only want a variant to be selected if it fits entirely in flat mode, we can change that. But it seems less useful to me because if all of the content could fit on a single line, then shouldn't the first variant fit? In real use cases, I assume you won't generally have tokens in the first variant that can be omitted in later variants.

[MichaReiser]

token("("),

That makes sense. I think where this solution differs from what I thought it does is that inner groups "fit" if the content up to the first line break fits (including soft) whereas what prettier does (I think) is that the group only fits if the whole group in flat mode fits.

We can go forward with your solution and see if it suits our needs. Please add some documentation explaining how this behaviour differs from prettiers

[yassere]

I think where this solution differs from what I thought it does is that inner groups "fit" if the content up to the first line break fits (including soft) whereas what prettier does (I think) is that the group only fits if the whole group in flat mode fits.

Yeah, that's a good point. In this implementation, there's no way to enforce that a specific group inside a variant must be flat when measuring if that variant fits. If that ends up being important, we can follow up with a new approach. I think we'd need to add something like should_break to our Group to enable more fine-tuned control of the breaking behavior of variants.

[ematipico]

Thank you for addressing my nitpicks! Let's 🚢 it!!