fix(rome_js_formatter): print empty statements as empty blocks

[ematipico]

Summary

Fixes #2448

Test Plan

Added new cases, one for the if and one for do while

[github-actions]

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%

[github-actions]

Playground for commit b8c694d

[cloudflare-workers-and-pages]

Deploying with    Cloudflare Pages

Latest commit:	c7aa984
Status:	✅  Deploy successful!
Preview URL:	https://641e0373.tools-8rn.pages.dev

View logs

[NicholasLYang]

Nit: this pattern shows up a couple times. Should we have a function that gets the parent kind directly?

[leops]

When it comes to unifying the printing behavior of empty statements, I would suggest we go the opposite way and extend the behavior of if statements (always insert a block) to all other affected statements (for loops, for in loops, for of loops, while loops, do while loops and with blocks).
The reasoning behind this is that Rome JS had a useBlockStatements lint rule that emitted a fixable warning for these nodes if they were using a single statement as their body, but for this kind of "style fix" we might as well rewrite it directly in the formatter.

[ematipico]

When it comes to unifying the printing behavior of empty statements, I would suggest we go the opposite way and extend the behavior of if statements (always insert a block) to all other affected statements (for loops, for in loops, for of loops, while loops, do while loops and with blocks). The reasoning behind this is that Rome JS had a useBlockStatements lint rule that emitted a fixable warning for these nodes if they were using a single statement as their body, but for this kind of "style fix" we might as well rewrite it directly in the formatter.

Does that mean that we want to intentionally diverge from prettier?

[leops]

Does that mean that we want to intentionally diverge from prettier?

Well at least I think we should discuss whether we want to have code-style related lint rules in the analyzer or to enforce these decisions in the formatter directly. Ultimately the decision is probably about how disruptive the change is (how often are these single-statement syntax actually used) vs. how confusing it is for users to see fixable diagnostics in their code for things they'd expect the formatter to take care of.

[ematipico]

Does that mean that we want to intentionally diverge from prettier?

Well at least I think we should discuss whether we want to have code-style related lint rules in the analyzer or to enforce these decisions in the formatter directly. Ultimately the decision is probably about how disruptive the change is (how often are these single-statement syntax actually used) vs. how confusing it is for users to see fixable diagnostics in their code for things they'd expect the formatter to take care of.

I completely forgot about that lint rule. Yes, in Rome classic there was this idea where "coding style rules" should not exists because we own the formatter too.

This is the rule FYI: https://github.com/rome/tools/blob/archived-js/internal/compiler/lint/rules/js/useBlockStatements.test.md

At this point I think we should just diverge from prettier. I agree with you @leops

[yassere]

At this point I think we should just diverge from prettier. I agree with you @leops

I don't think we should. This is a style decision, and we should have a very high bar for where we diverge from Prettier purely for style reasons (rather than implementation complexity).

We don't necessarily need to add every lint rule that existed in Rome classic. But if we do end up with some "code style rules", I think that's ok because we've intentionally relinquished some of our control over the formatting output by targeting Prettier equivalence.

[ematipico]

@yassere Could let us know why though? Why you think we should not put blocks and why we don't need that lint rule anymore.

I think it makes sense to have blocks. Having empty statements can lead to errors, same for expressions without blocks.

[NicholasLYang]

Personally I view diverging from Prettier as a large enough reason. If we get far enough in linting that we create this rule again, we can consider diverging then. But until that time, we should do what's best for the formatter, and not what might be best for the linter. IMO, as long as we're not seriously inhibiting future tools, we should do what's best for the tools that currently exist. Each of our tools needs to gain adoption and the promise of a linter feature does not outweigh breaking prettier compatibility, at least for me.

[yassere]

@ematipico I think it makes sense to have blocks too. Maybe that's something we can have as a safe autofix for the linter. But for the formatter, I think we need to respect our commitment to match Prettier's styling decisions as much as possible, unless we have a very good reason not to.

Given our previous messaging, intentionally deviating from Prettier's style is not a decision we should make lightly. Each case should be discussed in its own issue so that we can make a strong case to justify that divergence and document it.