🐛 loop/if with empty statement body

[jer3m01]

Environment information

WSL (W11/Ubuntu)
Yarn 3.2.0

What happened?

1. Create project using yarn 3.2.0
2. Install Rome
3. Run yarn format (rome format .)
4. Rerun yarn

project$ yarn format
Formatted x files in xms

project$ yarn
project/.yarn/releases/yarn-3.2.0.cjs:37184
                                                !e && !this.buffer.length && !this[sA] && this.emit("drain");
                                                ^

SyntaxError: Unexpected token '!'
    at Object.compileFunction (node:vm:352:18)
    at wrapSafe (node:internal/modules/cjs/loader:1031:15)
    at Module._compile (node:internal/modules/cjs/loader:1065:27)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1153:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:79:12)
    at node:internal/main/run_main_module:17:47

Expected result

Rome should correctly format cjs without breaking it or not touch cjs at all.

Code of Conduct

• I agree to follow Rome's Code of Conduct

[MichaReiser]

Playground

This is probably a common issue in all places where an EmptyStatement is the body of a loop/if condition. The empty statement must be printed if it's the direct child of an if/while to print valid syntax.

[leops]

Additionally .yarn should be added to the list of directories ignored by default in the formatting CLI

[ematipico]

We added .yarn to the list of ignored folders. It will be available at 0.5.* version

[ematipico]

@yassere @NicholasLYang

Following the conversation in #2504

So you'd prefer to stick with compatibility, then implement the lint rule and then in that case to change the formatter to accommodate the lint rule?

In case what I said would happen (it might not), how should we justify the change? Why you'd prefer to accomodate the formatter now and then revert it to a accomodate the linter later? Wouldn't this be seen as a change of opinion?

[yassere]

change the formatter to accommodate the lint rule?

What about the formatter would need to change? I don't think that Prettier prevents you from always using blocks. It just allows you to omit them in certain cases.

I think our formatter should do what Prettier does. If you run the format command or send a textDocument/formatting request, the output should "embrace Prettier’s styling decisions as much as possible" because that's what we announced.

We can still have opinionated autofixes as part of our linter (e.g. the check command) as long as they're not fundamentally incompatible with the formatter output. Am I misunderstanding the issue here and there's actually an incompatibility?

[ematipico]

Am I misunderstanding the issue here and there's actually an incompatibility?

Yes, there's an incompatibility and I would like to understand how we should move. I will show everything so everyone has a clear picture of things will be. The incompatibility is how our linter will react to this code.

Example of code:

do; while(true);

Then it's formatted, prettier's style:

do;
while (true);

Then the our linter will execute and it will show error because of the rule js/useBlockStatements. The autofix will be:

do {
} while (true);

Now, I don't see anything wrong in this flow, it's just that it seems weird that we have to provide an autofix inside the linter while we can do that inside the formatter. At the end, the code we will have inside Rome is the last example because everything will be passed under the rome check, which will do everything.

So wouldn't be faster to just change the style inside the formatter instead of having a lint rule too? Do you think I am overthinking this? Is this flow normal?

[yassere]

do while(true);

Did you mean do; while(true);? Because Prettier doesn't format do while(true); and I don't think our parser currently recovers that into a single do...while statement.

What I meant by "incompatibility" is if the formatter changes code from A to B and then the linter changes it back from B to A. That would make it impossible for code to be in a state that's stable for both the formatter and the linter, and that's definitely something we should avoid.

But the code do {} while (true); would be stable if the formatter matches Prettier. The formatter itself wouldn't replace an empty statement with an empty block. But if the linter adds an empty block, the formatter won't try to remove it.

So wouldn't be faster to just change the style inside the formatter instead of having a lint rule too?

It might be a little faster, but I don't think that warrants violating our commitment to embracing Prettier's styling decisions for our formatter. We could consider having a more opinionated option in the future, but I don't think that's a priority.

because everything will be passed under the rome check, which will do everything.

That's only when they choose to use rome check. It should be possible for people to use rome format and get the Prettier-like formatting they expect.

[ematipico]

@leops what's your opinion on this one?

I don't mind both approaches.

[leops]

I think since we aim to have an integrated formatter + linter experience we can actually do both at the same time: if users want to use the formatter in Prettier-compatible mode they get a fixable diagnostic in their editor, but if they opt-in to use our own configuration for the formatter we can disable the corresponding lint rule in the analyzer and just rewrite their code with braces when the file gets formatted

[ematipico]

Given the discussion, we could re-open #2504 and apply the first solution.