biomejs / biome

A toolchain for web projects, aimed to provide functionalities to maintain them. Biome offers formatter and linter, usable via CLI and LSP.
https://biomejs.dev
Apache License 2.0
14.95k stars 464 forks source link

☂️ Prettier Divergence collection from the Prettier challenge #739

Closed faultyserver closed 10 months ago

faultyserver commented 11 months ago

_NOTE: This issue thread is for explaining intentional divergences from Prettier. It is not intended as a place to seek clarification about formatting rules or point out discrepancies with Prettier. For those, please open a new issue or discussion. Once a decision has been made, a new entry will be added to this thread explaining the rationale and purpose behind choosing to diverge from Prettier's rules._

This Issue will function as a hub for collecting divergence from Prettier. When you want to comment, please use the following template:

## Divergence

<! -- Add a diff and the name of the file from `report.md` -->

## Reason

<! -- Reason why we should diverge -->
ematipico commented 11 months ago

Divergence

Bogus or duplicate modifiers on class properties

Input

// Multiple accessibility modifiers
class Foo {
  private public a  = 1;
}

// Declare function with body
declare function foo ( ) {  }

// Invalid use of abstract
class Bar {
  abstract  foo  ;
}

// Duplicate Readonly
class Read {
  readonly readonly   x: number;
}

Diff

// Multiple accessibility modifiers
class Foo {
-  private a = 1;
+  private public a  = 1;
}

// Declare function with body
-declare function foo() {};
+declare function foo ( ) {  }

// Invalid use of abstract
class Bar {
-  abstract foo;
+  abstract  foo  ;
}

// Duplicate Readonly
class Read {
-  readonly x: number;
+  readonly readonly   x: number;
}

Related Prettier Tests

Reason

Prettier's Babel-based parsing for JS/TS is very loose and allows multiple errors to be ignored. Biome's parser intentionally has stricter rules than Prettier and correctly asserts that the statements in these tests are invalid. Duplicate modifiers for functions are not semantically valid, function declarations are not allowed to have bodies, non-abstract classes cannot have abstract properties, etc.

In Prettier, these errors aren't considered parse errors, and the AST is still built "correctly" with the appropriate nodes. When formatting, Prettier then just treats these nodes as normal and formats them accordingly.

In Biome, the parsing errors result in Bogus nodes, which may contain any number of valid nodes, invalid nodes, and/or raw tokens. When formatting, Biome treats bogus nodes as effectively plain text, printing them out verbatim into the resulting code without any formatting, since attempting to format could be incorrect and cause semantic changes.

For class properties, Prettier's current parsing strategy also uses boolean fields for modifiers, meaning only one of each kind of modifier can ever be present (accessibility modifiers are stored as a single string). When printing, Prettier just looks at the list of booleans and decides which modifiers to print out again. Biome instead keeps a list of modifiers, meaning duplicates are kept around and able to be analyzed (hence the parsing error messages about duplicate modifiers and ordering). When printing out the bogus nodes, this list is still kept intact, and printing out the unformatted text results in those modifiers continuing to exist.

There are ways that Biome can address this. One possibility is to try to interpret the Bogus nodes when formatting and construct valid nodes out of them. If a valid node can be built, then it would just format that node like normal, otherwise it prints the bogus text verbatim as it does currently. However, this is messy and introduces a form of parsing logic into the formatter that is really not meaningful.

Another option instead is to introduce some form of "syntactically-valid bogus node" into the parser, which accepts these kinds of errors that are purely semantic (duplicate modifiers, abstract properties in non-abstract classes). It would continue to build the nodes like normal (effectively matching the behavior in Prettier), but store them inside of a the new kind of bogus node, including the diagnostics along with it. When formatting, these special bogus nodes would just attempt to format the inner node, and then fallback if there's an error (the existing format_or_verbatim utility would do this already). This keeps the parsing and formatting logic separate from each other, but introduce more complexity to the parser, allowing invalid states to be considered semi-valid.

After a little bit of discussion in Discord, the decision for now is to not worry about this divergence unless it's needed to reach 95% compatibility with Prettier.

Playground link

https://biomejs.dev/playground/?indentStyle=space&quoteStyle=single&trailingComma=none&code=LwAvACAATQB1AGwAdABpAHAAbABlACAAYQBjAGMAZQBzAHMAaQBiAGkAbABpAHQAeQAgAG0AbwBkAGkAZgBpAGUAcgBzAAoAYwBsAGEAcwBzACAARgBvAG8AIAB7AAoAIAAgAHAAcgBpAHYAYQB0AGUAIABwAHUAYgBsAGkAYwAgAGEAIAAgAD0AIAAxADsACgB9AAoACgAvAC8AIABEAGUAYwBsAGEAcgBlACAAZgB1AG4AYwB0AGkAbwBuACAAdwBpAHQAaAAgAGIAbwBkAHkACgBkAGUAYwBsAGEAcgBlACAAZgB1AG4AYwB0AGkAbwBuACAAZgBvAG8AIAAoACAAKQAgAHsAIAAgAH0ACgAKAC8ALwAgAEkAbgB2AGEAbABpAGQAIAB1AHMAZQAgAG8AZgAgAGEAYgBzAHQAcgBhAGMAdAAKAGMAbABhAHMAcwAgAEIAYQByACAAewAKACAAIABhAGIAcwB0AHIAYQBjAHQAIAAgAGYAbwBvACAAIAA7AAoAfQAKAAoALwAvACAARAB1AHAAbABpAGMAYQB0AGUAIABSAGUAYQBkAG8AbgBsAHkACgBjAGwAYQBzAHMAIABSAGUAYQBkACAAewAKACAAIAByAGUAYQBkAG8AbgBsAHkAIAByAGUAYQBkAG8AbgBsAHkAIAAgACAAeAA6ACAAbgB1AG0AYgBlAHIAOwAKAH0A

Conaclos commented 11 months ago

Divergence

Prettier doesn't unquote some object properties that are valid JavaScript identifiers.

Input

const obj = {
 'a': true,
 b: true,
 "𐊧": true,
}

Diff

 const obj = {
   a: true,
   b: true,
-  "𐊧": true,
+  𐊧: true,
 };

Playground link

Prettier test

Reason

Prettier and Biome unquote object and class properties that are valid JavaScript identifiers. Prettier unquotes only valid ES5 identifiers.

In an ecosystem where ES2015 is now widespread, this seems to be a legacy restriction. Thus, we decided to diverge here by unquoting all valid JavaScript identifiers in ES2015+.

A possible workaround would be to introduce a configuration to set the ECMAScript version a project uses. We could then adjust the unquoting behaviour based on that version. Setting the ECMAScript version to ES5 could match the behaviour of Prettier.

Conaclos commented 11 months ago

Divergence

Prettier has an inconsistent behavior for assignment in computed keys.

Input

a = {
  [this.resource = resource]: 1,
}

Diff

 a = {
-  [(this.resource = resource)]: 1,
+  [this.resource = resource]: 1,
 };

Playground link

Prettier Test

Reason

Prettier and Biome enclose some assignment expressions between parentheses, in particular in conditionals. This allows to identify potentially an expression that should be a comparison.

Prettier has an inconsistent behavior, because it adds parentheses for an assignment in a computed key of an object property and doesn't for a computed key of a class property, as demonstrated by the following example:

Input

a = {
  [x = 0]: 1,
}

class C {
  [x = 0] = 1
}

Output

a = {
  [(x = 0)]: 1,
};

class C {
  [x = 0] = 1;
}

Playground link

To be consistent, we decide to diverge and omit the parentheses. Alternatively, we could enclose any assignment in a computed key of an object or of a class.

Conaclos commented 11 months ago

Divergence

Prettier accepts an incorrect modifier for the type parameters of an interface.

Input

interface L<in const T> {}

Diff

-interface L<const in T> {}
+interface L<in const T> {}

Playground link

Prettier Test

Reason

As mentioned in a previous divergence, Prettier's Babel-based parsing for JS/TS is very loose and allows multiple errors to be ignored. Biome's parser intentionally has stricter rules than Prettier and correctly asserts that the const modifier is not allowed for type parameters of interfaces.

In Prettier, this error isn't considered a parse error, and the AST is still built "correctly" with the appropriate nodes. When formatting, Prettier then just treats these nodes as normal and formats them accordingly.

In Biome, the parsing errors result in a Bogus node, which may contain any number of valid nodes, invalid nodes, and/or raw tokens. When formatting, Biome treats bogus nodes as effectively plain text, printing them out verbatim into the resulting code without any formatting, since attempting to format could be incorrect and cause semantic changes.

We could adopt the same strategy that discussed in a previous comment to fix the divergence.

Conaclos commented 11 months ago

[!IMPORTANT] This divergence is now fixed

Divergence

Prettier keeps parentheses around optional chains.

PrettierTests

Reason

~Prettier encloses every optional chain between parentheses. As discussed on Discord, we think this reduces readability. For the time being, we decide not to fix this divergence.~

(a?.b).c and a?.b.c are not semantically identical. The first code evaluates a?.b and then calls .c on the result. In the second case, .c is only called if a is not undefined/null. Although the first code is probably a bug (it is also flagged as incorrect by TypeScript), we decide to keep the brackets to preserve the original semantics – which is what Prettier does.

Conaclos commented 11 months ago

[!IMPORTANT] This divergence is now fixed

Divergence

Prettier adds a trailing comma to some type parameter lists of arrow functions to distinguish them from JSX elements.

Prettier Tests

Reason

~Prettier outputs an inconsistent behavior, because it adds a trailing comma to type parameter lists of arrow functions, and removes any trailing comma to type parameter lists of classes and interfaces. To be consistent we decided to diverge and to not add this trailing comma.~

In some cases, a trailing comma is required to distinguish an arrow function from a JSX element. We fixed the divergence by making the trailing comma mandatory in these specific cases.

thorn0 commented 11 months ago

Prettier encloses optional chains between parentheses

Removing the parentheses here changes the meaning of the code. The call or member expression becomes part of the optional chain.

Prettier adds a trailing comma to type parameters of arrow functions.

Prettier does this only in TSX to disambiguate generics from JSX elements.

fisker commented 11 months ago

Prettier does this only in TSX to disambiguate generics from JSX elements.

That has been updated, changelog

Conaclos commented 11 months ago

Prettier does this only in TSX to disambiguate generics from JSX elements.

That has been updated, changelog

You are right. In fact, Biome already keeps the trailing comma. However, Biome seems not able to parse the const modifier for the first type parameter of an arrow function. This is an issue on our side. Thanks for bringing it to light :)

Removing the parentheses here changes the meaning of the code. The call or member expression becomes part of the optional chain.

Thanks for highlighting this.

Conaclos commented 10 months ago

Divergence

Prettier adds a trailing comma to type parameters of arrow functions even when it is not required.

Input (tsx)

<T = unknown>() => {};

Diff

- <T = unknown,>() => {};
+ <T = unknown>() => {};

Reason

In some specific cases, a type parameter list of an arrow function requires a trailing comma to distinguish it from a JSX element. When a default type is provided, this trailing comma is not required. Here, we diverge from Prettier, because we think it better respects the original intent of Prettier, which was to add a trailing comma only when required.

ghost commented 10 months ago

Divergence (off-topic, suggested)

Input (ts)

const A = {
  x: "x",
  y: "y",
  z: "z",
};

const {
  x,
  y,
  z,
} = A;

Diff

const A = {
  x: "x",
  y: "y",
  z: "z",
};

- const {x, y, z} = A;
+ const {
+  x,
+  y,
+  z,
+ };

Test https://github.com/biomejs/biome/blob/main/crates/biome_js_formatter/report.md#jsassignmentdestructuringjs or https://github.com/biomejs/biome/blob/main/crates/biome_js_formatter/report.md#jsassignmentdestructuring-heuristicjs

Reason

There was never any good reason given by Prettier why these should be treated differently. In an object literal, if the developer enters a newline then Prettier formats it with all newlines. The same should be done for destructuring.

GabenGar commented 10 months ago

Pretty sure the last case depends on the line length of the destructuring statement, past certain length it newlines all the keys. IMO Prettier is in the right there because newlining in all cases feels not so much different from doing const keyName = A.keyName for each key, for which destructuring was supposed to be a sugar in the first place. But long single-line desructuring statement can become obnoxious to read (especially when paired with nested destructuring).

ghost commented 10 months ago

No, you missed the part where the developer has a choice with object literals, but not with destructuring.

Prettier does wrap destructured assignments and object literals when the go past the line wrap limit. In an object literal though, when the developer places a newline their themselves then Prettier puts all props on newlines regardless of the line wrap limit.

So what is the reasoning to not do the same thing for destructuring?

I'd love to hear a reason why developers should have this option in one spot, but not the other. Thanks.

thorn0 commented 10 months ago

@waynebloss This issue isn't the right place for this discussion, but see https://prettier.io/docs/en/rationale#%EF%B8%8F-a-note-on-formatting-reversibility

ghost commented 10 months ago

@thorn0 Thanks, I didn't start a discussion. I left a Divergence and then someone responded to that, which I had no control over. Perhaps your message was meant for @GabenGar ? (Thanks for the link, but I find the rationale there to be severely lacking.)

faultyserver commented 10 months ago

This thread is specifically to list out cases where Biome has already intentionally decided to deviate from Prettier's behavior and explain the rationale for that as an easily accessible source of truth for any differences that people might encounter. It's not really intended as a place to point out discrepancies between Biome or Prettier or seek clarification about them.

If you'd like to ask why Biome is different than Prettier in a specific case, then opening an issue about it would be the best place to start (or asking in Discord, or a Discussion). If it is then decided to not be remedied by changing Biome, then a solidified, well-established answer will be added to this thread.

If you'd like to ask why Prettier decides to format things in a certain way (i.e., Biome does this because Prettier does, but why does Prettier do it), then the Prettier repository would be the appropriate place to ask.

In this situation, Biome is just matching Prettier's behavior, so the question should be asked to the Prettier team, or asked in a new issue for Biome to consider deviating.

ghost commented 10 months ago

This thread is specifically to list out cases where Biome has already intentionally decided to deviate from Prettier's behavior...

Ahhh, thanks @faultyserver that was not clear.

ematipico commented 10 months ago

Thank you everyone, I'm going to close the issue, as it solved its purpose.