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): refactor indention for arrow chains #1136

Merged
merged 2 commits into from
Dec 12, 2023

Conversation

faultyserver
Copy link
Contributor

Summary

Playground Link with all the affected cases.

This PR reorganizes where indents are placed in ArrowChain to better match Prettier, and to avoid the need to use dedent. The primary issue this was made to resolve was parameter lists appearing overly-dedented when the parameters broke over multiple lines:

foo(
  (b) =>
    (
    c = "abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdef",
    d = "abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdef",
  ) =>
    (e) =>
      0,
);

Here, the b signature is appropriately indented, and the following signature is appropriately indented one additional level, but the parameters within that second signature are dedented outward, causing them to not line up with the e signature that follows. The correct print for this statement is:

a.b(
  (b) =>
    (
      c = "abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdef",
      d = "abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdef",
    ) =>
    (e) =>
      0,
);

The reason this happened was because of a dedent getting added around the parameters for each signature. That was originally added in #804 as an appropriate fix to parameters that were being overly indented two levels when breaking. But the root cause of the issue was further up the chain. Why were the parameters being indented twice at all?

It has to do with the difference between how Biome and Prettier print out arrow chains. Prettier prints them recursively, nesting each signature as the body of the previous expression, up until the tail of the chain, which is at the top-level. All signatures other than the first in a chain are also indented a second level in, and so in the body for the first signature, Prettier adds another indent, and then the rest of the chain continues recursively. A simplified IR for this could be:

indent([
  group("(", indent(firstSignatureParams), ")"),
  " =>",
  indent([
    line,
    group("(", indent(secondSignatureParams), ")"),
    " =>",
    line,
    group("(", indent(thirdSignatureParams), ")")
  ]),
  " =>", tailBody
])

The indent around each signatureParams then ensures they are indented one level further in if the parameter list breaks (see the first corrected example at the top). And this works exactly as expected.

Biome, however, prints the signature chain as a flat list, which is more efficient to process, but has different semantics. Instead of being able to rely on the first signature adding the indent and all of the subsequent ones using it implicitly, each signature has to manage its own indent level. A simplified IR here could be:

indent([
  group("#1", [
    indent([
      group([
        dedent([ "(", indent(first_signature_params), ")" ]),
        " =>",
        dedent([ "(", indent(second_signature_params), ")" ]),
        " =>",
        dedent([ "(", indent(third_signature_params), ")" ])
      ])
    ])
  ]),
  " =>", tail_body
])

This example is stripped down from a direct export from Biome before these changes. Importantly, notice that there are two indents around the entire signature chain. Somehow the first signature was appropriately not indenting a second time, but all of the following ones were. I'm not at all sure how, and I think that's part of why this was not functioning as expected. Unfortunately I do not have the capacity to figure that out now, but it isn't an issue anymore anyway since these changes refactor it entirely.

Anyway, this IR also shows that along with the two outer indents, each signature is also wrapped with both a dedent and another indent, effectively cancelling each other out. But something internally obviously doesn't fully cancel, since we encountered the original issue here as shown in the first example: the parameter list ends up overly dedented when it breaks over multiple lines. I'm also not entirely sure why here, but this combination of indent and dedent was clearly causing some of it.

So, this PR refactors all of the indention handling for arrow chains to more closely match Prettier. After the refactor, a simplified IR from Biome looks like this:

indent([
  group("#1", [
    group("(", indent(first_signature_params), ")"),
    " =>",
    indent(group("(", indent(second_signature_params), ")")),
    " =>",
    indent(group("(", indent(third_signature_params), ")")),
  ]),
  " =>", tail_body
]),

This matches much closer to Prettier's representation, removes the need for dedent at all, and also removes the mysteriously-correct behavior with behavior that follows very clearly from the IR. The entire chain is indented by the outer indent, then the signatures are written as a flat list. The first signature writes itself directly, with the parameters gaining an indent for if they break. All of the subsequent signatures then also indent themselves an additional level, as expected in the printed output.

Another nice consequence of this refactor is that some of the conditional logic that was correcting the previous odd behaviors is no longer necessary, like checking if the expression is within a call expression or not. That behavior now comes naturally from the existing IR, matching Prettier as well.

Note also that there are other permutations of arrow chains that are handled slightly differently, hence the reason that the => tokens are not part of the indent. Those cases are commented inline in the code.

Test Plan

There were somehow no tests that covered this! Like nothing about indention of expanded parameters in arrow chains. Not from Prettier and not in our own specs.....wild. So I added a new spec test with all of the permutations of this pattern I've seen, and the snapshot appears as expected, matching the output of Prettier.

Copy link

netlify bot commented Dec 10, 2023

Deploy Preview for biomejs ready!

Name Link
🔨 Latest commit 6e9c388
🔍 Latest deploy log https://app.netlify.com/sites/biomejs/deploys/6575557d063314000881a489
😎 Deploy Preview https://deploy-preview-1136--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 A-Formatter Area: formatter L-JavaScript Language: JavaScript and super languages labels Dec 10, 2023
@faultyserver faultyserver merged commit d1202dc into main Dec 12, 2023
18 checks passed
@faultyserver faultyserver deleted the faulty/curried-param-indention branch December 12, 2023 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Formatter Area: formatter L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant