Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

feature(formatter): Add BestFitting IR element #2591

Merged
merged 3 commits into from
May 20, 2022

Conversation

MichaReiser
Copy link
Contributor

@MichaReiser MichaReiser commented May 17, 2022

This PR introduces the new IR element BestFitting. The IR matches Prettier's ConditionalGroupContent. This IR can be useful if the best representation depends on the available space. BestFitting defers the choice of how to print the element to the printer by providing multiple variants. The printer picks the element that best fits given the circumstances (that's where its name is coming from but I'm open to better names).

The printer picks the first variant that fits on the current line and falls back to print the last variant in expanded mode if no variant fits.

Part of #2579

Test Plan

I added an integration test as part of the documentation of the best_fitting function.

@MichaReiser MichaReiser mentioned this pull request May 17, 2022
10 tasks
@MichaReiser MichaReiser linked an issue May 17, 2022 that may be closed by this pull request
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented May 17, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: de4b35a
Status: ✅  Deploy successful!
Preview URL: https://395b6e35.tools-8rn.pages.dev

View logs

Base automatically changed from refactor/printer-try-flat-to-fits to main May 18, 2022 08:23
@MichaReiser MichaReiser force-pushed the feature/best-fitting-ir-element branch from 6b85f2f to 77c4385 Compare May 18, 2022 08:40
@MichaReiser MichaReiser temporarily deployed to aws May 18, 2022 08:40 Inactive
@github-actions
Copy link

Parser conformance results on ubuntu-latest

js/262

Test result main count This PR count Difference
Total 45878 45878 0
Passed 44938 44938 0
Failed 940 940 0
Panics 0 0 0
Coverage 97.95% 97.95% 0.00%

jsx/babel

Test result main count This PR count Difference
Total 39 39 0
Passed 36 36 0
Failed 3 3 0
Panics 0 0 0
Coverage 92.31% 92.31% 0.00%

ts/babel

Test result main count This PR count Difference
Total 588 588 0
Passed 519 519 0
Failed 69 69 0
Panics 0 0 0
Coverage 88.27% 88.27% 0.00%

ts/microsoft

Test result main count This PR count Difference
Total 16257 16257 0
Passed 12391 12391 0
Failed 3866 3866 0
Panics 0 0 0
Coverage 76.22% 76.22% 0.00%

@MichaReiser MichaReiser marked this pull request as ready for review May 18, 2022 08:45
@github-actions
Copy link

github-actions bot commented May 18, 2022

This PR introduces the new IR element `BestFitting`. The IR matches Prettier's `ConditionalGroupContent`. This IR can be useful if the best representation depends on the available space. `BestFitting` defers the choice of how to print the element to the printer by providing multiple variants. The printer picks the element that best fits given the circumstances (that's where its name is coming from but I'm open for better names).

The printer picks the first variant that fits on the current line and falls back to print the last variant in expanded mode if no variant fits.
@MichaReiser MichaReiser force-pushed the feature/best-fitting-ir-element branch from 77c4385 to b27630c Compare May 18, 2022 09:14
@MichaReiser MichaReiser temporarily deployed to aws May 18, 2022 09:14 Inactive

queue.enqueue(PrintElementCall::new(
variant,
args.with_print_mode(PrintMode::Expanded),
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to have the first variant printed in flat mode ? Or maybe it wouldn't make any difference

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's where I intentionally diverged from prettier as I outlined in the PR.

Prettier wraps each variant in an implicit Group which is why they try to fit each element with Flat first. We can do the same. The downside of this is that you often want to remove the implicit group from the second variant because you want to make that one break. Prettier does that by commonly wrapping all of the 1... variants in a group with shouldBreak: true.

We can also go a middle way and add an implicit group for the first variant but not for the other variants. But I'm not sure if that will be confusing.

@NicholasLYang
Copy link
Contributor

NicholasLYang commented May 18, 2022

Are we creating a conditionalGroup function too? If so, might be good to add documentation like Prettier's. Specifically, warning that it's exponential in time complexity and that the printings are expected to be ordered from least expanded to most expanded.

Nvm, saw comment. I think it's good for now, although I do think it's a little dangerous to have an IR element that relies on an invariant that we don't check.


impl BestFitting {
pub fn new(variants: Vec<FormatElement>) -> Self {
assert!(variants.len() > 1, "The variants collection must contain at least two variants where the first is the least expanded state and the last element the most expanded option.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it necessary to panic when someone tries to construct an invalid BestFitting rather than returning a result?
Would it be reasonable to expand FormatError to account for errors during IR construction?

If the intended use of best_fitting is that it's always passed a vec! literal rather than a dynamically built up vector, maybe best_fitting could be a macro that ensures there are always at least two variants at compile-time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All usages in prettier are indeed using a fixed set of variants. It's unfortunate that rust doesn't allow for var args without using macros.

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 introduced a best_fitting macro that checks the invariant and marked the from_slice_unchecked as unsafe.

@MichaReiser MichaReiser temporarily deployed to aws May 19, 2022 13:07 Inactive
@MichaReiser MichaReiser temporarily deployed to aws May 19, 2022 13:10 Inactive
@MichaReiser MichaReiser requested review from leops and yassere May 19, 2022 16:13
@MichaReiser MichaReiser merged commit 90429d9 into main May 20, 2022
@MichaReiser MichaReiser deleted the feature/best-fitting-ir-element branch May 20, 2022 07:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Conditional groups / Alternatives
4 participants