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

challenge(formatter): edge case with expression statement and type cast #754

Merged
merged 2 commits into from Nov 20, 2023

Conversation

Conaclos
Copy link
Member

Summary

This fixes the parse error for typescript/as/expression-statement.ts.
Unfortunately we have to diverge from prettier here. Thus, this does not improve the compatibility score. However, this should not make it worse.

The following code is valid in TypeScript:

(type) as unknown;

Prettier and Biome currently format this code into an invalid code:

type as unknown;

I added an edge case check in the formatting code of as expressions.
It adds parens when the parent node is a statement and the expression is the identifier type.
Alternatively, this logic could be implemented in JsIdnetifierExpression::needs_parentheses. However, tis could incur a perf regression because identifier expression are more common than as expressions.

Test Plan

Updated tests.

@github-actions github-actions bot added A-Formatter Area: formatter L-JavaScript Language: JavaScript and super languages labels Nov 17, 2023
@Conaclos Conaclos changed the title challenge(formatter): edge case wit hexpression statement and type cast challenge(formatter): edge case with expression statement and type cast Nov 17, 2023
@Conaclos Conaclos requested review from a team November 17, 2023 16:24
Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix looks good to me. Although could you try something? Try to commit the suggested fix with JsIdentifierExpression::needs_parentheses_with_parent, and bench the result. And then bench this solution. If there aren't big differences, we could use the needs_parentheses_with_parent solution, which is more idiomatic against our architecture.

Remember to add a comment to #739 once this is merged

@Conaclos
Copy link
Member Author

!bench_formatter

Copy link
Contributor

Formatter Benchmark Results

group                                    main                                   pr
-----                                    ----                                   --
formatter/big5-added.json                1.00    373.8±2.69µs    45.2 MB/sec    1.04    390.0±2.11µs    43.3 MB/sec
formatter/canada.json                    1.00    173.2±4.95ms    12.4 MB/sec    1.01    175.2±4.12ms    12.3 MB/sec
formatter/checker.ts                     1.04    283.2±3.52ms     9.2 MB/sec    1.00    273.3±3.40ms     9.5 MB/sec
formatter/compiler.js                    1.06    160.8±1.88ms     6.5 MB/sec    1.00    151.5±1.97ms     6.9 MB/sec
formatter/d3.min.js                      1.06    124.6±2.35ms     2.1 MB/sec    1.00    117.6±2.56ms     2.2 MB/sec
formatter/db.json                        1.00     10.5±0.15ms    17.4 MB/sec    1.08     11.4±0.22ms    16.1 MB/sec
formatter/dojo.js                        1.05      8.8±0.06ms     7.8 MB/sec    1.00      8.4±0.03ms     8.2 MB/sec
formatter/eucjp.json                     1.00    635.9±4.83µs    61.6 MB/sec    1.05    670.2±2.22µs    58.4 MB/sec
formatter/ios.d.ts                       1.09    189.2±4.61ms     9.9 MB/sec    1.00    173.1±2.72ms    10.8 MB/sec
formatter/jquery.min.js                  1.06     35.7±0.27ms     2.3 MB/sec    1.00     33.7±0.31ms     2.4 MB/sec
formatter/math.js                        1.04    249.7±3.41ms     2.6 MB/sec    1.00    239.3±2.40ms     2.7 MB/sec
formatter/package-lock.json              1.00      4.5±0.02ms    30.6 MB/sec    1.08      4.9±0.07ms    28.3 MB/sec
formatter/parser.ts                      1.06      6.1±0.09ms     8.0 MB/sec    1.00      5.7±0.02ms     8.5 MB/sec
formatter/pixi.min.js                    1.05    135.4±1.46ms     3.2 MB/sec    1.00    128.8±1.65ms     3.4 MB/sec
formatter/react-dom.production.min.js    1.06     40.6±0.46ms     2.8 MB/sec    1.00     38.3±0.41ms     3.0 MB/sec
formatter/react.production.min.js        1.05      2.1±0.03ms     3.0 MB/sec    1.00  1960.2±36.46µs     3.1 MB/sec
formatter/router.ts                      1.06      2.2±0.03ms    10.8 MB/sec    1.00      2.0±0.03ms    11.5 MB/sec
formatter/tex-chtml-full.js              1.05    323.1±4.87ms     2.8 MB/sec    1.00    306.7±3.97ms     3.0 MB/sec
formatter/three.min.js                   1.07    163.1±3.52ms     3.6 MB/sec    1.00    152.4±2.46ms     3.9 MB/sec
formatter/typescript.js                  1.00   1084.4±8.50ms     8.8 MB/sec  
formatter/vue.global.prod.js             1.07     54.7±0.95ms     2.2 MB/sec    1.00     51.2±0.58ms     2.4 MB/sec

@Conaclos Conaclos force-pushed the conaclos/challenge/type-cast-expression-statement branch from c08ea3f to bb5f62b Compare November 20, 2023 10:33
@Conaclos
Copy link
Member Author

Good news: we no longer diverge from prettier. This was addressed in prettier#15514.

@Conaclos Conaclos marked this pull request as draft November 20, 2023 10:36
@Conaclos
Copy link
Member Author

!bench_formatter

@Conaclos Conaclos force-pushed the conaclos/challenge/type-cast-expression-statement branch from bb5f62b to c1626f5 Compare November 20, 2023 11:11
Copy link
Contributor

Formatter Benchmark Results

group                                    main                                   pr
-----                                    ----                                   --
formatter/big5-added.json                1.00    365.5±4.05µs    46.2 MB/sec    1.01    370.2±2.50µs    45.6 MB/sec
formatter/canada.json                    1.05    173.9±4.14ms    12.3 MB/sec    1.00    165.2±3.39ms    13.0 MB/sec
formatter/checker.ts                     1.00    289.8±3.46ms     9.0 MB/sec    1.00    290.8±4.49ms     8.9 MB/sec
formatter/compiler.js                    1.00    168.2±2.48ms     6.2 MB/sec    1.00    168.4±3.26ms     6.2 MB/sec
formatter/d3.min.js                      1.00    128.3±2.25ms     2.0 MB/sec    1.00    128.8±2.67ms     2.0 MB/sec
formatter/db.json                        1.00     10.6±0.44ms    17.2 MB/sec    1.02     10.8±0.10ms    16.9 MB/sec
formatter/dojo.js                        1.00      9.0±0.08ms     7.6 MB/sec    1.00      9.0±0.05ms     7.6 MB/sec
formatter/eucjp.json                     1.00    630.3±5.20µs    62.1 MB/sec    1.02    640.9±6.37µs    61.1 MB/sec
formatter/ios.d.ts                       1.00    192.9±2.11ms     9.7 MB/sec    1.01    194.0±4.25ms     9.6 MB/sec
formatter/jquery.min.js                  1.00     37.3±1.27ms     2.2 MB/sec    1.00     37.1±0.61ms     2.2 MB/sec
formatter/math.js                        1.00    258.8±2.78ms     2.5 MB/sec    1.01    260.9±3.86ms     2.5 MB/sec
formatter/package-lock.json              1.00      4.5±0.03ms    30.4 MB/sec    1.04      4.7±0.01ms    29.3 MB/sec
formatter/parser.ts                      1.00      6.3±0.06ms     7.8 MB/sec    1.01      6.3±0.03ms     7.7 MB/sec
formatter/pixi.min.js                    1.00    141.0±2.98ms     3.1 MB/sec    1.00    141.6±2.40ms     3.1 MB/sec
formatter/react-dom.production.min.js    1.00     41.7±0.87ms     2.8 MB/sec    1.02     42.5±0.83ms     2.7 MB/sec
formatter/react.production.min.js        1.00      2.1±0.03ms     2.9 MB/sec    1.02      2.1±0.04ms     2.9 MB/sec
formatter/router.ts                      1.01      2.2±0.01ms    10.6 MB/sec    1.00      2.2±0.02ms    10.6 MB/sec
formatter/tex-chtml-full.js              1.00    331.4±5.37ms     2.7 MB/sec    1.02    337.4±4.56ms     2.7 MB/sec
formatter/three.min.js                   1.00    168.4±4.37ms     3.5 MB/sec    1.00    168.1±3.04ms     3.5 MB/sec
formatter/typescript.js                  1.02  1130.3±12.78ms     8.4 MB/sec    1.00  1113.4±11.92ms     8.5 MB/sec
formatter/vue.global.prod.js             1.00     56.9±1.05ms     2.1 MB/sec    1.00     56.6±1.41ms     2.1 MB/sec

@Conaclos
Copy link
Member Author

!bench_formatter

Copy link
Contributor

Formatter Benchmark Results

group                                    main                                   pr
-----                                    ----                                   --
formatter/big5-added.json                1.05    381.6±8.85µs    44.3 MB/sec    1.00    362.5±3.73µs    46.6 MB/sec
formatter/canada.json                    1.05    170.8±3.52ms    12.6 MB/sec    1.00    162.7±3.19ms    13.2 MB/sec
formatter/checker.ts                     1.00    286.8±3.58ms     9.1 MB/sec    1.03    295.8±3.89ms     8.8 MB/sec
formatter/compiler.js                    1.00    166.2±2.24ms     6.3 MB/sec    1.00    165.7±4.43ms     6.3 MB/sec
formatter/d3.min.js                      1.00    127.2±2.44ms     2.1 MB/sec    1.00    127.5±2.29ms     2.1 MB/sec
formatter/db.json                        1.05     10.9±0.10ms    16.8 MB/sec    1.00     10.3±0.09ms    17.7 MB/sec
formatter/dojo.js                        1.00      9.0±0.04ms     7.6 MB/sec    1.01      9.0±0.02ms     7.6 MB/sec
formatter/eucjp.json                     1.05    657.7±6.35µs    59.5 MB/sec    1.00    626.1±3.88µs    62.5 MB/sec
formatter/ios.d.ts                       1.02    197.1±2.08ms     9.5 MB/sec    1.00    193.2±2.31ms     9.7 MB/sec
formatter/jquery.min.js                  1.00     36.5±0.25ms     2.3 MB/sec    1.01     36.7±0.56ms     2.3 MB/sec
formatter/math.js                        1.00    257.4±3.36ms     2.5 MB/sec    1.00    257.5±2.55ms     2.5 MB/sec
formatter/package-lock.json              1.03      4.6±0.02ms    30.1 MB/sec    1.00      4.4±0.02ms    31.0 MB/sec
formatter/parser.ts                      1.00      6.3±0.02ms     7.8 MB/sec    1.00      6.3±0.02ms     7.8 MB/sec
formatter/pixi.min.js                    1.00    139.5±2.17ms     3.1 MB/sec    1.00    139.6±1.82ms     3.1 MB/sec
formatter/react-dom.production.min.js    1.00     41.4±0.35ms     2.8 MB/sec    1.01     41.7±0.38ms     2.8 MB/sec
formatter/react.production.min.js        1.00      2.1±0.03ms     2.9 MB/sec    1.00      2.1±0.03ms     2.9 MB/sec
formatter/router.ts                      1.01      2.2±0.02ms    10.6 MB/sec    1.00      2.2±0.03ms    10.7 MB/sec
formatter/tex-chtml-full.js              1.00    331.1±4.52ms     2.8 MB/sec    1.00    331.2±4.41ms     2.8 MB/sec
formatter/three.min.js                   1.01    166.2±2.64ms     3.5 MB/sec    1.00    165.4±3.93ms     3.6 MB/sec
formatter/typescript.js                  1.00   1106.6±8.60ms     8.6 MB/sec    1.00   1109.5±9.94ms     8.6 MB/sec
formatter/vue.global.prod.js             1.00     55.1±0.50ms     2.2 MB/sec    1.01     55.9±0.67ms     2.2 MB/sec

@Conaclos Conaclos force-pushed the conaclos/challenge/type-cast-expression-statement branch from f7a7693 to 26b67cb Compare November 20, 2023 15:46
@Conaclos Conaclos marked this pull request as ready for review November 20, 2023 15:46
@Conaclos Conaclos merged commit 7bac8fd into main Nov 20, 2023
15 checks passed
@Conaclos Conaclos deleted the conaclos/challenge/type-cast-expression-statement branch November 20, 2023 15:53
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.

None yet

2 participants