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

feat(rome_js_formatter): number literals, number literal types, bigint literal types #3554

Merged
merged 5 commits into from
Nov 3, 2022

Conversation

jeysal
Copy link
Contributor

@jeysal jeysal commented Nov 3, 2022

Summary

Resolves #3294.

Also formats TS number literal types in the same way.
Also formats TS bigint literal types in the same way bigint literals are formatted (unrelated to the other changes in the PR, just something I noticed).

Establishes 100% Prettier compatibility for

  • js/literal-numeric-separator/test.js
  • js/literal/number.js
  • typescript/webhost/webtsc.ts
  • typescript/typeparams/consistent/simple-types.ts

Performance

I have not benchmarked this e.g. against a primitive implementation that uses replace and manual string insertions/deletions.
But it should be rather fast because it

  • only writes a new string if the number literal actually needs changing,
  • only ever iterates through the number literal chars once, and
  • only ever appends to the end of the string, no deletions, no insertions in the middle, etc.

Test Plan

Includes unit tests for format_trimmed_number().
Existing prettier diff tests cover the integration.

@netlify
Copy link

netlify bot commented Nov 3, 2022

Deploy Preview for docs-rometools ready!

Name Link
🔨 Latest commit 9ad278e
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/6363c34c7cd5a400092fdc6c
😎 Deploy Preview https://deploy-preview-3554--docs-rometools.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Contributor

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Awesome. Thanks for this contribution. Overall looking good. I have a few remarks. Feel free to disagree ;)

@@ -21,7 +23,7 @@ foo = 1.0000;bar = 1.0000;baz=1.0000;
-foo = 1.0;
-bar = 1.0;
-baz = 1.0;
+foo = 1.0000;bar = 1.0000;baz=1.0000;
+foo = 1.0000;bar = 1.0;baz=1.0000;
Copy link
Contributor

Choose a reason for hiding this comment

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

Uhm, it is strange that Rome prints all assignments on a single line but this is unrelated to your changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. Hey, at least the middle one was recognized as a number literal and improved by my formatting :P

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I know why. It is because of range formatting.

foo = 1.0000;<<<PRETTIER_RANGE_START>>>bar = 1.0000;<<<PRETTIER_RANGE_END>>>baz=1.0000;
// The range will be 13~26
// `foo` ends at 13, should not format
// `bar` ends at 26, should format

crates/rome_js_formatter/src/utils/number_utils.rs Outdated Show resolved Hide resolved
crates/rome_js_formatter/src/utils/number_utils.rs Outdated Show resolved Hide resolved
crates/rome_js_formatter/src/utils/number_utils.rs Outdated Show resolved Hide resolved
crates/rome_js_formatter/src/utils/number_utils.rs Outdated Show resolved Hide resolved

impl Format<JsFormatContext> for CleanedNumberLiteralText<'_> {
fn fmt(&self, f: &mut JsFormatter) -> FormatResult<()> {
format_replaced(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One thing I just noticed: We always format_replaced here, even if we have a Cow::Borrowed original token text. Probably we should reuse the original token in that case?
I'm just a litte irritated because string_utils does not appear to do that, it looks just like this. Does using text_trimmed on its own already require us to do a replaced token?
@MichaReiser

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it using format_replaced here?

impl Format<JsFormatContext> for CleanedStringLiteralText<'_> {
fn fmt(&self, f: &mut Formatter<JsFormatContext>) -> FormatResult<()> {
format_replaced(
self.token,
&syntax_token_cow_slice(
self.text.clone(),
self.token,
self.token.text_trimmed_range().start(),
),
)
.fmt(f)
}
}

Using format_replaced even if the token hasn't been replaced is OK, both yield the same formatting.

Rome's AST supports skipped token trivia. Skipped token trivia are invalid tokens that get parsed as trivia to not invalidate the AST to get better error recovery.

That's why it is important to format the skipped token trivia with every token to not drop some content that has been present in the source code. That's what the Format implementation for the token and format_replaced both do.

Skipped trivia isn't used widely , but one use case is:

<div className={asdf asdf} />;

The second asdf is invalid but parsing it as a regular token invalidates the whole statement. That's why our parser attaches the second asdf to the } and continues parsing as if nothing has happened, yielding better results overall.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it is, sorry, I wasn't clear enough. I was irritated that string_utils always uses format_replaced and never just formats even if the text needs no changes and the original token can be preserved. So I wasn't sure if there is perhaps a reason to always use format_replaced.

But if you say it's the same anyway, no performance difference or anything, I suppose I don't need to worry about it and can just keep it the same as what string_utils does, always format_replaced.

@jeysal
Copy link
Contributor Author

jeysal commented Nov 3, 2022

@MichaReiser all comments addressed!

@jeysal jeysal requested review from MichaReiser and removed request for ematipico November 3, 2022 12:24
Copy link
Contributor

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Nice work!

@MichaReiser
Copy link
Contributor

MichaReiser commented Nov 3, 2022

@jeysal, there's one remaining lint issue that needs fixing before I can merge the PR

@jeysal
Copy link
Contributor Author

jeysal commented Nov 3, 2022

Yup, I hadn't run lint locally. Was already on it, should be green now.

@MichaReiser MichaReiser merged commit 175662b into rome:main Nov 3, 2022
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.

📎 Formatter: Number literal separators
2 participants