Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(js_formatter): fix propagate_expand for interned elements with best_fitting #1141

Merged
merged 1 commit into from
Dec 11, 2023

Conversation

faultyserver
Copy link
Contributor

Summary

Playground Link

This one was fun! The problem:

// Biome
import("@somelongpath/enoughto/breaktheparent").then(
  ({ someLongFunctionName }) => someLongFunctionName({
      brokenObject,
    }),
);

// Prettier
import("@somelongpath/enoughto/breaktheparent").then(
  ({ someLongFunctionName }) =>
    someLongFunctionName({
      brokenObject,
    }),
);

When an expression breaks over multiple lines, it expands. That expansion is propagated up to all of the enclosing groups, forcing them to expand as well. This only stops when a best_fitting element is encountered in the enclosing ancesstry.

In this case, the { brokenObject } expression expands, meaning everything above it should also expand: the function call expression, the arrow expression, and the then call expression at the top. When this happens, the output matches Prettier.

However, Biome was not properly propagating the expansion in this case, specifically because of the presence of a best_fitting element and the use of interned elements. Specifically, when propagating expansion, BestFitting would set expands = false, causing the interned element to think it did not expand, and any references to it to lose the propagation.

The rest of this description is taken from the comment added inline in the code:

BestFitting acts as a boundary, meaning there is no need to continue processing this element and we can move onto the next. However, we don't set expands = false, because that ends up negating the expansion when processing Interned elements.

Only interned lists are affected, because they cache the expansion value based on the value of expands at the end of iterating the children. If a best_fitting element occurs after the last expanding element, and we end up setting expands = false here, then the interned element ends up thinking that its content doesn't expand, even though it might. Example:

  group(1,
    interned 1 [
      expand_parent,
      best_fitting,
    ]
  )
  group(2,
    [ref interned 1]
  )

Here, group(1) gets expanded directly by the expand_parent element. This happens immediately, and then expands = true is set. The interned element continues processing, and encounters the best_fitting. If we set expands = false there, then the interned element's result ends up being false, even though it does actually expand. Then, when group(2) checks for expansion, it looks at the ref to interned 1, which thinks it doesn't expand, and so group(2) stays flat.

By not setting expands = false, even though best_fitting is a boundary for expansion, we ensure that any references to the interned element will get the correct value for whether the contained content actually expands, regardless of the order of elements within it.

Instead, just returning false here enforces that best_fitting doesn't think it expands itself, but allows other sibling elements to still propagate their expansion.

Test Plan

Added an inline test to cover this case of propagation for interned best_fitting elements. No other test cases seemed to hit this, but I saw it come up a few times when running Biome on another large codebase.

Copy link

netlify bot commented Dec 10, 2023

Deploy Preview for biomejs ready!

Name Link
🔨 Latest commit c29ed59
🔍 Latest deploy log https://app.netlify.com/sites/biomejs/deploys/657641f075311300081308c0
😎 Deploy Preview https://deploy-preview-1141--biomejs.netlify.app/playground
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@github-actions github-actions bot added the A-Formatter Area: formatter label Dec 10, 2023
Copy link
Member

@Conaclos Conaclos left a comment

Choose a reason for hiding this comment

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

Oh, nice fix!!

@Conaclos Conaclos merged commit ae44459 into main Dec 11, 2023
18 checks passed
@Conaclos Conaclos deleted the faulty/interned-expand-propagate branch December 11, 2023 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Formatter Area: formatter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants