feat(rome_js_formatter): variable declarator formatting

[denbezrukov]

Summary

This PR partially implements #2423

This PR adds

• OnlyLeft layout to the list of layouts. This layout is hit, usually, when variable declarator doesn't have initializer.
• New conditional branch for should_break_left_hand_side when variable declaration has_complex_type_annotation. Prettier hasComplexTypeAnnotation.
• New conditional branch for should_not_indent_if_parent_indents. Don't indent if great_parent is JS_VARIABLE_DECLARATOR.

Open questions

There are problems with BindExpression and RecordExpression. I described them in comments.

Test Plan

Add new tests specs

[denbezrukov]

Will be ready for review after #2729

[denbezrukov]

Need to check because Rome and Prettier have different IR for these cases:
const _id1 = data.createTestMessageWithAReallyLongName.someVeryLongProperty.thisIsAlsoALongProperty._id;
const { imStore, showChat, customerServiceAccount } = store[config.reduxStoreName];
const pendingIndicators = shield.alarmGeneratorConfiguration.getPendingVersionColumnValues;
const pendingIndicatorz = shield.alarmGeneratorConfiguration.getPendingVersionColumnValues();
const bifornCringerMoshedPerplexSawderGlyphsHa = someBigFunctionName("foo")( "bar", );

https://gist.github.com/MichaReiser/77799fe4363bcc48838a159e581d8229#jsassignmentunaryjs

• I guess that it's because of isPoorlyBreakableMemberOrCallChain https://github.com/prettier/prettier/blob/be9d63e543b6ac28aa6eee7ab8dc45849cd8f0b3/src/language-js/print/assignment.js#L215

[denbezrukov]

https://github.com/prettier/prettier/blob/fde0b49d7866e203ca748c306808a87b7c15548f/src/language-js/print/assignment.js#L278

[ematipico]

Is there a particular reason why we changed this into an Option? The reason why we want SyntaxResult, is to usually bubble up any syntax error and prevent formatting nodes that are syntactically wrong.

And if the right hand side is missing, it's usually a syntax error.

[denbezrukov]

Yes, you're right. But for JsVariableDeclarator variant a right part is initializer and it has Option<JsInitializerClause> type. We can try to keep SyntaxResult:

• make fn right(&self) -> SyntaxResult<Option<RightAssignmentLike>>. I have concerns that it will be more difficult to use.
• unwrap Option<JsInitializerClause> here and introduce should_only_left function which will be a guarantee of safety.

[ematipico]

Yeah, I agree that having SyntaxResult<Option<RightAssignmentLike> would add a lot of branches/indirection/safety comments. Let's try your second suggested solution. Documenting should_only_left should help developers understand why we're doing this

[denbezrukov]

12ada03

[ematipico]

Would you mind adding a test case with a suppression comment? We should make sure that the layout formatting is not applied to the node that is suppressed

[denbezrukov]

c5223b9
Is it correct?
Could you please explain why we need this test? How could we by-pass a suppression comment?

[ematipico]

We could by-pass it by not calling .format() on a node. Because this union is now taking charge of many nodes (left and right are now unions), we have to be careful and make sure that each node of left and right call format()

[MichaReiser]

This shouldn't be in here :O

[denbezrukov]

This PR is closed :( I created another one and it didn't have this line :) #2762

[MichaReiser]

Too bad, I hoped that the performance regression comes from the dbg expression 😞

[denbezrukov]

Do we have tools to diagnostic the performance issue?
How do you usually debug this?🙏🏼

[MichaReiser]

You can run cargo bench_formatter to profile the formatter. The command has two useful options

• --save-baseline=<name>: Stores the result under the given name. Useful to e.g. run the benchmarks on main first so that you can then compare against these base results. To compare benchmark, use critcmp <name1> <name2> (requires critcmp)
• --filter=filters the files to benchmarks. I often go withtex-chtml-full` because it seems to be big enough that performance improvements are visible but doesn't take as long to run as e.g. typescript.

Finding bottlenecks inside the formatter can be difficult and the available tools vary depending on the platform you use:

• Mac: I very much like cargo-instruments.
• CLion has a built-in profiler for Linux and I believe MacOS.

The rust book lists some more available tools as well as how you can enable debug information in release builts (you want that to be able to make sense of the output)

[MichaReiser]

I just found Firefox Profiler. You can do profiling with (assuming linux and that perf is installed)

cargo build --release
perf record -F999 --call-graph dwarf cargo bench_formatter --filter tex --criterion=false
perf script -F +pid > /tmp/test.perf

Then upload the file on the website. They also have great documentation

[denbezrukov]

Thank you so much for your answers!
I'll try to debug it.

[MichaReiser]

Small note. This PR seems to have added a roughly 10% performance regression. I would say this is kind of expected as it has to do many checks but I'm wondering if there are any hotspots that could be improved. We need to be careful to not slowly regress performance by a large margin.

Before

formatter/tex-chtml-full.js
                        time:   [300.11 ms 300.58 ms 301.08 ms]
                        thrpt:  [3.0267 MiB/s 3.0317 MiB/s 3.0365 MiB/s]

After

formatter/tex-chtml-full.js
                        time:   [275.64 ms 276.25 ms 276.91 ms]
                        thrpt:  [3.2908 MiB/s 3.2987 MiB/s 3.3060 MiB/s]
                 change:
                        time:   [-8.8836% -6.5148% -4.4963%] (p = 0.00 < 0.05)
                        thrpt:  [+4.7080% +6.9688% +9.7497%]