refactor(ast)!: change 'raw' from &str to Option<Atom> #7547

ShuiRuTian · 2024-11-29T15:13:33Z

Changed all "raw" propperty of literal types(if it has this property) to Option

graphite-app · 2024-11-29T15:13:41Z

Your org has enabled the Graphite merge queue for merging into main

Add the label “0-merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

overlookmotel

Thanks loads for tackling this.

Turns out this is more complicated than I thought! Sorry for the litany of comments below.

First off, could you please rebase on main and fix the merge conflicts so CI can run?

Most of my comments are the same - they relate to not unwrapping when can't be sure that raw is Some.

In lots of places we need a fallback for converting NumericLiteral's value to a string if raw is None. Probably it'd be good to add a method NumericLiteral::raw_str which can be used in all those places (which I guess will need to return a Cow<str>).

crates/oxc_ast/src/ast_builder_impl.rs

crates/oxc_ast/src/ast_impl/js.rs

crates/oxc_ast/src/ast_impl/literal.rs

crates/oxc_ast/src/ast/literal.rs

crates/oxc_codegen/src/gen.rs

crates/oxc_minifier/src/ast_passes/peephole_substitute_alternate_syntax.rs

crates/oxc_minifier/src/node_util/mod.rs

crates/oxc_prettier/src/format/mod.rs

crates/oxc_semantic/src/checker/javascript.rs

crates/oxc_traverse/src/ast_operations/gather_node_parts.rs

ShuiRuTian · 2024-11-30T17:48:04Z

Thanks a lot for the detailed comments!

Sorry, I was indeed abusing unwrap...

Add a new method called raw_str as suggested.

Most problems should already be fixed.

codspeed-hq · 2024-11-30T17:53:51Z

CodSpeed Performance Report

Merging #7547 will improve performances by 4.67%

_{Comparing ShuiRuTian:update-raw-for-literal-types (7eb3802) with main (e5145b0)}

Summary

⚡ 2 improvements
✅ 27 untouched benchmarks

Benchmarks breakdown

	Benchmark	`main`	`ShuiRuTian:update-raw-for-literal-types`	Change
⚡	`semantic[cal.com.tsx]`	29 ms	28.1 ms	+3.1%
⚡	`transformer[RadixUIAdoptionSection.jsx]`	154.6 µs	147.7 µs	+4.67%

overlookmotel · 2024-12-02T11:59:57Z

Thanks a lot for the detailed comments!

Glad you found them useful and not annoying!

Nice speed bump on transformer benchmarks.

Could you please try and fix the failing test, and fix clippy, and then I'll review again?

A couple of quick things in meantime:

When formatting NumericLiteral's value, I don't think there's any need to look at what base is. Just Cow::Owned(self.value.to_string()) should do it.
Please make sure all changes related to BigIntLiteral are removed. I can see a few remaining in the diff.

Side note: I think we can also remove NumericLiteral::base field later on, or make it an Option. Like raw, it only has meaning where the NumericLiteral came from source code. A NumericLiteral which is generated is just a value. It has no "base". But please don't do that now! We should look at doing that in a separate PR, if we want to do it at all.

overlookmotel · 2024-12-04T01:48:59Z

I believe the CI failure on "Test NAPI" CI job is unrelated to content of this PR. I've also seen it on one of my recent PRs #7618.

Please let me know when ready for me to review again.

ShuiRuTian · 2024-12-04T03:55:35Z

I believe the CI failure on "Test NAPI" CI job is unrelated to content of this PR. I've also seen it on one of my recent PRs #7618.

Cool, if so, the PR should be ready to be reviewed now.

There might also be some comment issues, like whether I should add "Safety: xxx" above unwrap or some comments is not clearly enough. but the logic part should be done.

…anges

overlookmotel

Branch got out of date with main. I've rebased on main and fixed conflicts. I've also taken the liberty of pushing a few small commits to fix a few things, rather than going through another round of review - I hope you don't mind.

Am going to merge now. @ShuiRuTian please take a look at the extra commits I pushed if you're interested.

Thanks loads for your work on this!

ShuiRuTian · 2024-12-05T07:27:49Z

Thanks much for the detailed guide! It really helps a lot

overlookmotel · 2024-12-05T11:13:48Z

To be honest, I don't think this one is correct: 4874660

If raw is None then it returns None, which it shouldn't. But how RegExpLiteral stores raw needs changes anyway. At present it stores it in 2 places.

ShuiRuTian requested review from overlookmotel, Dunqing, Boshen and leaysgur as code owners November 29, 2024 15:13

overlookmotel changed the title ~~refactor: change 'raw' from &str to Option<Atom>~~ refactor(ast)!: change 'raw' from &str to Option<Atom> Nov 30, 2024

github-actions bot added the C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior label Nov 30, 2024

overlookmotel removed the request for review from leaysgur November 30, 2024 12:04

overlookmotel requested changes Nov 30, 2024

View reviewed changes

ShuiRuTian force-pushed the update-raw-for-literal-types branch 2 times, most recently from cc5c3a2 to 6604759 Compare November 30, 2024 17:45

Boshen assigned overlookmotel Dec 2, 2024

ShuiRuTian force-pushed the update-raw-for-literal-types branch 2 times, most recently from 8c93101 to 9b6952f Compare December 3, 2024 17:17

ShuiRuTian added 2 commits December 5, 2024 00:02

refactor: change 'raw' from &str to Option<Atom>

55e6101

fix comments

4e518b9

ShuiRuTian and others added 6 commits December 5, 2024 00:02

fix clippy

96b2fdb

Tweak comments

ce590de

Avoid unwrapping twice

bb6430b

Don't unwrap NumericLiteral::raw in prettier

aaaff61

Update ComputedMemberExpression::static_property_name for recent ch…

4874660

…anges

Update linter for recent changes

7eb3802

overlookmotel force-pushed the update-raw-for-literal-types branch from 9b6952f to 7eb3802 Compare December 5, 2024 00:26

overlookmotel approved these changes Dec 5, 2024

View reviewed changes

overlookmotel merged commit ebc80f6 into oxc-project:main Dec 5, 2024
26 checks passed

This was referenced Dec 6, 2024

release(oxlint): v0.14.1 #7692

Merged

release(crates): v0.40.0 #7775

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(ast)!: change 'raw' from &str to Option<Atom> #7547

refactor(ast)!: change 'raw' from &str to Option<Atom> #7547

ShuiRuTian commented Nov 29, 2024

graphite-app bot commented Nov 29, 2024

overlookmotel left a comment

ShuiRuTian commented Nov 30, 2024

codspeed-hq bot commented Nov 30, 2024 •

edited

Loading

overlookmotel commented Dec 2, 2024 •

edited

Loading

overlookmotel commented Dec 4, 2024 •

edited

Loading

ShuiRuTian commented Dec 4, 2024

overlookmotel left a comment •

edited

Loading

ShuiRuTian commented Dec 5, 2024

overlookmotel commented Dec 5, 2024

refactor(ast)!: change 'raw' from &str to Option<Atom> #7547

refactor(ast)!: change 'raw' from &str to Option<Atom> #7547

Conversation

ShuiRuTian commented Nov 29, 2024

graphite-app bot commented Nov 29, 2024

Your org has enabled the Graphite merge queue for merging into main

overlookmotel left a comment

Choose a reason for hiding this comment

ShuiRuTian commented Nov 30, 2024

codspeed-hq bot commented Nov 30, 2024 • edited Loading

CodSpeed Performance Report

Merging #7547 will improve performances by 4.67%

Summary

Benchmarks breakdown

overlookmotel commented Dec 2, 2024 • edited Loading

overlookmotel commented Dec 4, 2024 • edited Loading

ShuiRuTian commented Dec 4, 2024

overlookmotel left a comment • edited Loading

Choose a reason for hiding this comment

ShuiRuTian commented Dec 5, 2024

overlookmotel commented Dec 5, 2024

codspeed-hq bot commented Nov 30, 2024 •

edited

Loading

overlookmotel commented Dec 2, 2024 •

edited

Loading

overlookmotel commented Dec 4, 2024 •

edited

Loading

overlookmotel left a comment •

edited

Loading