Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

refactor(Formatter): Move Format, Formatter, to rome_formatter #2559

Merged
merged 15 commits into from
May 11, 2022

Conversation

MichaReiser
Copy link
Contributor

@MichaReiser MichaReiser commented May 9, 2022

Summary

This PR moves the shared traits Format, Formatter, IntoFormatElement, and the extension traits to rome_formatter.

I introduce two new traits to work around Rust's orphan rule.

  • AsFormat: Has a single format() method that returns an object that knows how to format self (by reference)
  • IntoFormat: Has a single into_format() that returns an object that knows how to format self (takes ownership)

An object that isn't able to implement Format itself because of Rust's orphan rule can implement AsFormat and IntoFormat.

This PR changes the node formatting by making them implement AsFormat and IntoFormat instead of implementing Format. The formatting is implemented by a Format* new-type that has a single static ::format method. This new type has been necessary to have a way to share the formatting logic between AsFormat and IntoFormat.

Usage

The syntax for formatting a node doesn't differ much from how it was before the refactor:

// Before
node.format(formatter)?;

// After
formatted![formatter, node.format()]?`

The benefit of returning a Format object by AsFormat and IntoFormat is that any extension trait or helper functions continue to work as before:

// Before
if_stmt.else_clause().with(|else_clause| ... )?

// After
formatted![formatter, if_stmt.else_clause.format().with(|else_clause| ... )]?

Note: It would be possible to have a trait providing the format_with and other helpers but I decided against it for the time being to not have even more indirection.

Open Question

It can currently be a bit confusing that Format and AsFormat provide a format method. Should we:

  • Rename AsFormat.format to as_format? I decided to use format() to match Path.display.
  • Rename Format.format to fmt (similar to Display/Debug)

Test Plan

cargo test

Deleted a

  • Separted list
  • node list
  • node
  • unknown node
  • union

And ran the code gen. Verified that the code compiles correctly

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented May 9, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 1a8b1fb
Status: ✅  Deploy successful!
Preview URL: https://d11a19b8.tools-8rn.pages.dev

View logs

@MichaReiser MichaReiser force-pushed the refactor/into-format-element branch from af2b8a7 to 0c81af3 Compare May 9, 2022 13:59
@MichaReiser
Copy link
Contributor Author

!bench_formatter

@github-actions
Copy link

github-actions bot commented May 9, 2022

Formatter Benchmark Results

group                                    main                                   pr
-----                                    ----                                   --
formatter/checker.ts                     1.00   498.4±16.14ms     5.2 MB/sec    1.03   515.0±14.23ms     5.0 MB/sec
formatter/compiler.js                    1.00   307.7±11.90ms     3.4 MB/sec    1.02   313.3±10.19ms     3.3 MB/sec
formatter/d3.min.js                      1.00   233.6±10.24ms  1148.8 KB/sec    1.04    243.3±9.57ms  1103.3 KB/sec
formatter/dojo.js                        1.00     16.2±0.71ms     4.2 MB/sec    1.02     16.4±0.74ms     4.2 MB/sec
formatter/ios.d.ts                       1.00   340.9±11.64ms     5.5 MB/sec    1.05   358.6±14.23ms     5.2 MB/sec
formatter/jquery.min.js                  1.00     64.5±4.01ms  1312.1 KB/sec    1.07     69.0±3.00ms  1226.0 KB/sec
formatter/math.js                        1.00   460.4±16.30ms  1440.3 KB/sec    1.06   486.4±14.22ms  1363.3 KB/sec
formatter/parser.ts                      1.00     10.9±0.47ms     4.4 MB/sec    1.04     11.4±0.48ms     4.3 MB/sec
formatter/pixi.min.js                    1.00   264.8±10.48ms  1697.2 KB/sec    1.03    272.0±8.92ms  1652.2 KB/sec
formatter/react-dom.production.min.js    1.00     79.6±2.46ms  1480.0 KB/sec    1.00     79.4±4.10ms  1484.2 KB/sec
formatter/react.production.min.js        1.00      3.7±0.19ms  1689.9 KB/sec    1.04      3.9±0.14ms  1629.5 KB/sec
formatter/router.ts                      1.00      8.3±0.34ms     7.3 MB/sec    1.04      8.6±0.42ms     7.1 MB/sec
formatter/tex-chtml-full.js              1.00   582.2±14.97ms  1602.8 KB/sec    1.04   607.5±15.23ms  1536.1 KB/sec
formatter/three.min.js                   1.00   299.9±10.04ms  2004.5 KB/sec    1.01   303.5±11.68ms  1980.7 KB/sec
formatter/typescript.js                  1.00  1949.0±42.11ms     4.9 MB/sec    1.02  1992.4±37.90ms     4.8 MB/sec
formatter/vue.global.prod.js             1.00    105.4±4.79ms  1170.5 KB/sec    1.00    105.3±5.40ms  1172.2 KB/sec

@MichaReiser
Copy link
Contributor Author

!bench_formatter

@MichaReiser MichaReiser requested a review from leops May 9, 2022 15:19
@MichaReiser MichaReiser marked this pull request as ready for review May 9, 2022 15:23
@github-actions
Copy link

github-actions bot commented May 9, 2022

Formatter Benchmark Results

group                                    main                                   pr
-----                                    ----                                   --
formatter/checker.ts                     1.00    406.4±1.62ms     6.4 MB/sec    1.00    406.0±3.60ms     6.4 MB/sec
formatter/compiler.js                    1.00    250.9±1.65ms     4.2 MB/sec    1.00    249.9±0.54ms     4.2 MB/sec
formatter/d3.min.js                      1.00    191.4±2.32ms  1402.0 KB/sec    1.01    193.8±0.46ms  1385.0 KB/sec
formatter/dojo.js                        1.00     13.7±0.19ms     5.0 MB/sec    1.00     13.7±0.11ms     5.0 MB/sec
formatter/ios.d.ts                       1.00    293.1±1.11ms     6.4 MB/sec    1.05    307.5±1.20ms     6.1 MB/sec
formatter/jquery.min.js                  1.00     55.1±0.37ms  1536.4 KB/sec    1.01     55.8±0.31ms  1516.8 KB/sec
formatter/math.js                        1.00    382.5±1.40ms  1733.5 KB/sec    1.00    382.8±2.48ms  1732.1 KB/sec
formatter/parser.ts                      1.00      9.4±0.15ms     5.1 MB/sec    1.02      9.6±0.14ms     5.0 MB/sec
formatter/pixi.min.js                    1.00    212.9±1.51ms     2.1 MB/sec    1.03    218.6±2.37ms     2.0 MB/sec
formatter/react-dom.production.min.js    1.00     66.0±0.35ms  1786.6 KB/sec    1.01     66.8±1.11ms  1763.2 KB/sec
formatter/react.production.min.js        1.00      3.1±0.01ms  2019.1 KB/sec    1.02      3.2±0.01ms  1970.6 KB/sec
formatter/router.ts                      1.00      7.2±0.02ms     8.5 MB/sec    1.02      7.3±0.02ms     8.3 MB/sec
formatter/tex-chtml-full.js              1.00    473.8±3.55ms  1969.5 KB/sec    1.02    481.6±2.01ms  1937.4 KB/sec
formatter/three.min.js                   1.00    245.7±0.63ms     2.4 MB/sec    1.02    249.4±2.38ms     2.4 MB/sec
formatter/typescript.js                  1.01   1583.7±3.75ms     6.0 MB/sec    1.00   1573.1±5.72ms     6.0 MB/sec
formatter/vue.global.prod.js             1.00     86.7±0.34ms  1423.5 KB/sec    1.01     87.6±0.94ms  1407.8 KB/sec

@MichaReiser MichaReiser force-pushed the refactor/into-format-element branch from 5c81163 to 16abd60 Compare May 9, 2022 16:04
@MichaReiser MichaReiser force-pushed the refactor/into-format-element branch from 2b38bf3 to c344075 Compare May 10, 2022 11:51
@MichaReiser MichaReiser force-pushed the refactor/into-format-element branch from 637f06b to 60a18b9 Compare May 10, 2022 12:34
Base automatically changed from refactor/formatted to main May 11, 2022 07:39
@MichaReiser MichaReiser force-pushed the refactor/into-format-element branch from 60a18b9 to 94adc68 Compare May 11, 2022 07:54
@MichaReiser MichaReiser temporarily deployed to aws May 11, 2022 07:54 Inactive
@github-actions
Copy link

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
Copy link

github-actions bot commented May 11, 2022

@MichaReiser MichaReiser temporarily deployed to aws May 11, 2022 10:20 Inactive
@MichaReiser MichaReiser temporarily deployed to aws May 11, 2022 10:33 Inactive
@MichaReiser MichaReiser temporarily deployed to aws May 11, 2022 12:26 Inactive
@MichaReiser MichaReiser temporarily deployed to aws May 11, 2022 15:54 Inactive
@MichaReiser MichaReiser temporarily deployed to aws May 11, 2022 15:59 Inactive
use rome_formatter::{FormatOwnedWithRule, FormatRefWithRule};

use crate::{AsFormat, FormatNodeRule, IntoFormat};
impl<'a> AsFormat<'a> for rome_js_syntax::JsScript {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to have a

trait HasFormatRule: Sized {
  type Rule: FormatRule<Self>;
}

trait and then have generic AsFormat and IntoFormat implementations. That worked well except that formatting any Option<&HasFormatRule> didn't work :( So I went forward with just generating that code.

@MichaReiser MichaReiser merged commit e6409b8 into main May 11, 2022
@MichaReiser MichaReiser deleted the refactor/into-format-element branch May 11, 2022 16:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants