refactor(transformer): replace `&’a Trivias` with `Rc<Trivias>` by Dunqing · Pull Request #3580 · oxc-project/oxc

Dunqing · 2024-06-07T08:12:28Z

Transformer needs to borrow Trivias. https://github.com/rolldown/rolldown/pull/1364/files/8be1cc80520e1a466cd7b2d3fb8b637db9e529a9#r1630711060

graphite-app · 2024-06-07T08:12:32Z

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

Add the label “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.

Dunqing · 2024-06-07T08:12:41Z

refactor(transformer): replace &’a Trivias with Rc<Trivias> #3580 👈
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @Dunqing and the rest of your teammates on Graphite

codspeed-hq · 2024-06-07T08:18:05Z

CodSpeed Performance Report

Merging #3580 will not alter performance

_{Comparing 06-07-feat_ast_derive_clone_trait_for_trivias (34ea5fb) with main (d6370f1)}

Summary

✅ 22 untouched benchmarks

Boshen · 2024-06-07T09:36:17Z

No we should never clone this heavy data structure, we need to Rc or Arc it before hand.

overlookmotel · 2024-06-07T10:50:18Z

I also ran into problems with trying to share Trivias in another context. It's problematic that when you create a reference &Trivias, its lifetime doesn't live as long as Program<'a>.

In my view the best solution would be to store Trivias in the allocator, and parser could return a &'a mut Trivias with same lifetime as Program<'a>. Then it'd be easy to pass both around together.

Ideally the actual BTreeMap data would be stored in an arena, so it can be cheap to drop, but I can't find a BTreeMap implementation which supports arena allocators. So for now we could just add it as a field on Allocator. Allocator owns Trivias, and it'd get dropped when allocator is dropped.

Would that solve the problem that this PR is targeting?

Dunqing · 2024-06-07T13:23:53Z

Would that solve the problem that this PR is targeting?

I think would solve this problem. I will give it a try!

overlookmotel · 2024-06-07T15:26:05Z

OK great. We should also add a function to Allocator that resets both the bumpalo arena and Trivias:

impl Allocator {
  fn reset(&mut self) {
    self.bump.reset();
    self.trivias.reset();
  }
}

impl Trivias {
  fn reset(&mut self) {
    self.comments.clear();
    self.irregular_whitespaces.clear();
  }
}

Otherwise Trivias will keep growing each time the allocator is reset.

Dunqing · 2024-06-08T05:04:17Z

I tried to give Trivias the same lifetime as Program. However, this approach didn't solve this problem. The Transformer always needs to borrow the Trivias. So we cannot move out ast at the same time.

In my opinion, the only viable solution is to make Trivias cloneable. By using Rc, we can address this problem. I found the same solution here

I don't know much about lifetime, so maybe there's a better solution I don't know about.

overlookmotel · 2024-06-08T09:51:04Z

Oh dear! I'm not good with lifetimes either. I don't think any of us are!

If you can solve it with Rc, probably best to do that for now to unblock us. I don't think it's the optimal solution, but we can come back to it later if we want to try to remove Rc. Reasoning about the lifetimes in transformer is a brain-bender.

crates/oxc_wasm/src/lib.rs

crates/oxc_semantic/src/builder.rs

crates/oxc_wasm/src/lib.rs

overlookmotel · 2024-06-10T10:41:20Z

I'd suggest making the change above, but it's not so important.

Besides that, this change is motivated by a need in Rolldown, and I'm not familiar enough with Rolldown to say "LGTM", as I don't understand the context. But if it solves the problem, everything else looks good to me.

Dunqing · 2024-06-10T10:54:55Z

Besides that, this change is motivated by a need in Rolldown, and I'm not familiar enough with Rolldown to say "LGTM", as I don't understand the context. But if it solves the problem, everything else looks good to me.

Thank you for reviewing this! I tested this change in Rolldown and it worked.

overlookmotel · 2024-06-10T11:17:53Z

Thank you for reviewing this! I tested this change in Rolldown and it worked.

OK great. Merge it then I guess!

## [0.14.0] - 2024-06-12 ### Breaking * fix(codegen)!: remove the unecessary 4th argument from `Codegen::new` (#3640) * feat(ast)!: make `Trivias` clonable by adding `Arc` (#3638) ### Features - f6d9ca6 linter: Add `eslint/sort-imports` rule (#3568) (Wang Wenzhe) - 129f91e span: Port over more methods from TextRange (#3592) (Don Isaac) ### Bug Fixes - f8f6d33 ast: Correct `visited_node` attr for strict mode of arrow fns (#3635) (overlookmotel) - e6ad3fb diagnostics: Do not print ansi color codes in non-TTYs (#3624) (Boshen) - d65202d span: Correct doc comments (#3608) (overlookmotel) - 35e267b transformer: Arrow function transform use UIDs for `_this` vars (#3634) (overlookmotel) - 39bdebc transformer: Arrow func transform maintain scope ID (#3633) (overlookmotel) - 5cb7e6a transformer: Arrow func transform use correct spans (#3630) (overlookmotel) - 0c4ccb4 transformer: Arrow function transform alter `</this>` (#3627) (overlookmotel) - 8d237c4 transformer: JSX source calculate correct column when Unicode chars (#3615) (overlookmotel) - 9e8f4d6 transformer: Do not add `__source` for generated nodes (#3614) (overlookmotel) - 0fb4c35 transformer: Use UID for JSX source filename var (#3612) (overlookmotel) ### Performance - 3a59294 transformer: React display name transform reduce Atom allocations (#3616) (overlookmotel) - f4c1389 transformer: Create `Vec` with capacity (#3613) (overlookmotel) ### Refactor - 0f92521 ast: Replace recursion with loop (#3626) (overlookmotel) - 08f1010 ast: Make `AstBuilder` `Copy` (#3602) (overlookmotel) - 84304b4 linter: Add a `ctx.module_record()` method (#3637) (Boshen) - f98f777 linter: Add rule fixer (#3589) (Don Isaac) - e90e6a2 minifier: Make `Prepass` `Copy` (#3603) (overlookmotel) - 7d61832 semantic: Pass `Rc` by value (#3586) (overlookmotel) - 89bcbd5 transformer: Move `BoundIdentifier` into helpers (#3610) (overlookmotel) - 5793ff1 transformer: Replace `&’a Trivias` with `Rc<Trivias>` (#3580) (Dunqing) - 509871f transformer: Comment for unimplemented `spec` option in arrow fns transform (#3618) (overlookmotel) - 4b2e3a7 transformer: Fix indentation (#3617) (overlookmotel) - 3467e3d transformer: Remove outdated comment (#3606) (overlookmotel) - a799225 transformer: Flatten file structure for React transform (#3604) (overlookmotel) - 70f31a8 transformer: Reduce branching in JSX transform (#3596) (overlookmotel) - 3ae567d transformer: Remove dead code (#3588) (overlookmotel) - 60cbdec traverse: `generate_uid_in_root_scope` method (#3611) (overlookmotel) Co-authored-by: Boshen <[email protected]>

## [0.4.4] - 2024-06-14 ### Features - 8f5655d linter: Add eslint/no-useless-constructor (#3594) (Don Isaac) - 29c78db linter: Implement @typescript-eslint/explicit-function-return-type (#3455) (kaykdm) - 21d3425 linter: Typescript-eslint no-useless-empty-export (#3605) (keita hino) - 85c3b83 linter: Eslint-plugin-jest/max-nested-describes (#3585) (cinchen) - f6d9ca6 linter: Add `eslint/sort-imports` rule (#3568) (Wang Wenzhe) - 046ff3f linter/eslint: Add `no_unreachable` rule. (#3238) (rzvxa) - e32ce00 linter/jsdoc: Implement require-param-name rule (#3636) (Yuji Sugiura) - 110661c linter/jsdoc: Implement require-param-description (#3621) (Yuji Sugiura) - d6370f1 linter/jsdoc: Implement require-param-type rule (#3601) (Yuji Sugiura) - d9c5b33 semantic/cfg: Add `Condition` instruction. (#3567) (Ali Rezvani) - f2dfd66 semantic/cfg: Add iteration instructions. (#3566) (rzvxa) ### Bug Fixes - f0b689d linter: Panic in jsdoc/require-param (#3590) (Don Isaac) - e148a32 semantic/cfg: Correct unreachability propagation in try-finally. (#3667) (Ali Rezvani) ### Refactor - 84304b4 linter: Add a `ctx.module_record()` method (#3637) (Boshen) - f98f777 linter: Add rule fixer (#3589) (Don Isaac) - fa11644 linter: Pass `Rc` by value (#3587) (overlookmotel) - f702fb9 semantic/cfg: Cleanup control flow and it's builder. (#3650) (rzvxa) - 5793ff1 transformer: Replace `&’a Trivias` with `Rc<Trivias>` (#3580) (Dunqing) Co-authored-by: Boshen <[email protected]>

Dunqing marked this pull request as ready for review June 7, 2024 08:12

github-actions bot added the A-ast Area - AST label Jun 7, 2024

overlookmotel mentioned this pull request Jul 15, 2024

Convert TriviasMap (comments) into a Vec #4271

Closed

Dunqing force-pushed the 06-07-feat_ast_derive_clone_trait_for_trivias branch from e4cecfa to 7a90ad6 Compare June 8, 2024 05:05

github-actions bot added A-linter Area - Linter A-semantic Area - Semantic A-transformer Area - Transformer / Transpiler labels Jun 8, 2024

Dunqing changed the title ~~feat(ast): derive Clone trait for Trivias~~ refactor(transformer): replace &’a Trivias with Rc<Trivias> Jun 8, 2024

Dunqing force-pushed the 06-07-feat_ast_derive_clone_trait_for_trivias branch 2 times, most recently from d5e79d7 to 7225a41 Compare June 8, 2024 05:43

overlookmotel reviewed Jun 8, 2024

View reviewed changes

crates/oxc_wasm/src/lib.rs Outdated Show resolved Hide resolved

crates/oxc_semantic/src/builder.rs Outdated Show resolved Hide resolved

Dunqing force-pushed the 06-07-feat_ast_derive_clone_trait_for_trivias branch from 7225a41 to 44d313d Compare June 10, 2024 03:16

overlookmotel reviewed Jun 10, 2024

View reviewed changes

crates/oxc_wasm/src/lib.rs Outdated Show resolved Hide resolved

feat(ast): derive Clone trait for Trivias

34ea5fb

Dunqing force-pushed the 06-07-feat_ast_derive_clone_trait_for_trivias branch from d2421cb to 34ea5fb Compare June 10, 2024 10:50

Dunqing merged commit 5793ff1 into main Jun 11, 2024

Dunqing deleted the 06-07-feat_ast_derive_clone_trait_for_trivias branch June 11, 2024 05:23

Dunqing mentioned this pull request Jun 11, 2024

feat!: support jsx, ts and tsx via oxc transform by default rolldown/rolldown#1364

Merged

This was referenced Jun 12, 2024

Release crates v0.13.6 #3642

Closed

Release crates v0.14.0 #3643

Merged

github-actions bot mentioned this pull request Jun 14, 2024

Release oxlint v0.4.4 #3669

Merged

Uh oh!

Comments

Conversation

Dunqing commented Jun 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

graphite-app bot commented Jun 7, 2024

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

Uh oh!

Dunqing commented Jun 7, 2024

Uh oh!

codspeed-hq bot commented Jun 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CodSpeed Performance Report

Merging #3580 will not alter performance

Summary

Uh oh!

Boshen commented Jun 7, 2024

Uh oh!

overlookmotel commented Jun 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Dunqing commented Jun 7, 2024

Uh oh!

overlookmotel commented Jun 7, 2024

Uh oh!

Dunqing commented Jun 8, 2024

Uh oh!

overlookmotel commented Jun 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

overlookmotel commented Jun 10, 2024

Uh oh!

Dunqing commented Jun 10, 2024

Uh oh!

overlookmotel commented Jun 10, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Dunqing commented Jun 7, 2024 •

edited

Loading

codspeed-hq bot commented Jun 7, 2024 •

edited

Loading

overlookmotel commented Jun 7, 2024 •

edited

Loading

overlookmotel commented Jun 8, 2024 •

edited

Loading