feat(oxc_allocator): Add new method `clone_in_with_semantic_ids` for `CloneIn` trait #8608

IWANABETHATGUY · 2025-01-20T02:49:35Z

Unlike clone_in the new method also clone semantic fields, this is used for scenario like caching ast and semantic data at the same time.

graphite-app · 2025-01-20T02:49:41Z

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

0-merge - adds this PR to the back of the merge queue
hotfix - for urgent hot fixes, skip the queue and merge this PR next

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

_{An organization admin has enabled the Graphite Merge Queue in this repository.} _{Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.}

IWANABETHATGUY · 2025-01-20T02:50:03Z

crates/oxc_ast/src/ast/js.rs

@@ -41,7 +41,6 @@ pub struct Program<'a> {
    pub directives: Vec<'a, Directive<'a>>,
    pub body: Vec<'a, Statement<'a>>,
    #[estree(skip)]
-    #[clone_in(default)]


This is not right, should use cfg(attribete)

Boshen · 2025-01-20T05:36:53Z

Can you provide some context and link your Rolldown prototype for other reviewers?

IWANABETHATGUY · 2025-01-20T05:52:46Z

Can you provide some context and link your Rolldown prototype for other reviewers?

OK, I will link it once the new poc is finished.

overlookmotel · 2025-01-20T10:38:12Z

I'm not quite sure what you're trying to achieve here, but this may be relevant: oxc-project/backlog#127

CloneIn is a bit of a nightmare. I tend to think we should not have introduced it in the first place, because it's a footgun.

At present, it unsets all scope_id etc fields to None, which is not good because a post-semantic AST should always have these fields set with an ID.

But if we go the other way (as in your PR), then it will have ID fields set, but they're likely to be the wrong IDs.

So either way is wrong! CloneIn cannot set the IDs correctly without context about where the cloned node is going to be inserted.

See also: oxc-project/backlog#96

IWANABETHATGUY · 2025-01-20T11:03:30Z

I'm not quite sure what you're trying to achieve here, but this may be relevant: oxc-project/backlog#127

CloneIn is a bit of a nightmare. I tend to think we should not have introduced it in the first place, because it's a footgun.

At present, it unsets all scope_id etc fields to None, which is not good because a post-semantic AST should always have these fields set with an ID.

But if we go the other way (as in your PR), then it will have ID fields set, but they're likely to be the wrong IDs.

So either way is wrong! CloneIn cannot set the IDs correctly without context about where the cloned node is going to be inserted.

See also: oxc-project/backlog#96

incremental build scenario, since we need to modify the ast after parsing, we clone Ast to avoid parse it next round,
I assume if the source code is not changed, the AST and semantic data should also not be changed. In this scenario, the semantic data(like ScopeId, SymbolId) could be cloned safely

overlookmotel · 2025-01-20T14:13:26Z

Ah I see.

If you clone both the AST and also the matching ScopeTree and SymbolTable, then yes the IDs will all remain valid. Ditto if you don't clone ScopeTree or SymbolTable, but you don't modify them in any way either.

But I suggest we introduce a new trait for this use case CloneIntoWithIds, and document that it's only valid to use in these specific circumstances. Other code currently relies on the current implementation of CloneIn, so I don't think we should change it.

CloneIn is codegen-ed, so we can easily codegen CloneIntoWithIds too.

Note on naming: I suggest CloneIntoWithIds not CloneInWithIds to indicate that you're cloning into a different allocator, instead of in the same allocator (see oxc-project/backlog#96).

We need Allocator::with_capacity method for other purposes, so I've broken that out into a separate PR #8620.

I assume you'll also need a way to get the capacity required for the allocator you're cloning into, so I've also added that in #8621. Please take a look and confirm that covers what you need.

overlookmotel · 2025-01-21T11:16:05Z

Actually maybe we don't need a new trait. We could just add a 2nd method to CloneIn: clone_in_with_semantic_ids.

...and document the pitfalls of using this method!

@IWANABETHATGUY Are you comfortable enough with oxc_ast_tools to make that change yourself?

IWANABETHATGUY · 2025-01-21T17:07:18Z

Actually maybe we don't need a new trait. We could just add a 2nd method to CloneIn: clone_in_with_semantic_ids.

...and document the pitfalls of using this method!

@IWANABETHATGUY Are you comfortable enough with oxc_ast_tools to make that change yourself?

Thanks for your guiding, I will try it tomorrow. I am working on other thing today.

IWANABETHATGUY · 2025-01-22T09:23:53Z

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

0-merge - adds this PR to the back of the merge queue
hotfix - for urgent hot fixes, skip the queue and merge this PR next

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

_{An organization admin has enabled the Graphite Merge Queue in this repository.} _{Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.}

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

tasks/ast_tools/src/derives/clone_in.rs

codspeed-hq · 2025-01-22T09:33:11Z

CodSpeed Performance Report

Merging #8608 will not alter performance

_{Comparing feat/arena-clone (ad3ef33) with main (ba201a6)}

Summary

✅ 32 untouched benchmarks

crates/oxc_syntax/src/reference.rs

overlookmotel · 2025-01-22T15:23:26Z

crates/oxc_allocator/src/clone_in.rs

+    /// Almost same as `clone_in`, but for some special type, it will also clone the semantic ids.
+    #[inline]
+    fn clone_in_with_semantic_ids(&self, allocator: &'new_alloc Allocator) -> Self::Cloned {
+        self.clone_in(allocator)
+    }


Please expand this comment to explain why it's generally a bad idea to use this method.

tasks/ast_tools/src/derives/clone_in.rs

overlookmotel · 2025-01-22T15:36:35Z

tasks/ast_tools/src/derives/clone_in.rs

@@ -60,52 +62,92 @@ fn derive_enum(def: &EnumDef) -> TokenStream {
        }
    };

-    impl_clone_in(&ty_ident, def.has_lifetime, &alloc_ident, &body)
+    impl_clone_in(&ty_ident, def.has_lifetime, &alloc_ident, &body, &quote!())


You will need a clone_in_with_semantic_ids impl for most enums too. If you call clone_in_with_semantic_ids on a &Program you want it to be using clone_in_with_semantic_ids all the way through the AST, not diverting to clone_in as soon as it hits a Statement.

Otherwise Program::clone_in_with_semantic_ids will do exactly the same as Program::clone_in - cloned AST won't have any IDs.

github-actions bot added A-semantic Area - Semantic A-ast Area - AST C-enhancement Category - New feature or request labels Jan 20, 2025

IWANABETHATGUY commented Jan 20, 2025

View reviewed changes

IWANABETHATGUY changed the title ~~feat(oxc_allocator): make data in arena cloneable~~ TEST feat(oxc_allocator): make data in arena cloneable Jan 20, 2025

overlookmotel mentioned this pull request Jan 20, 2025

feat(allocator): add Allocator::capacity and used_bytes methods #8621

Merged

github-actions bot added the A-ast-tools Area - AST tools label Jan 22, 2025

IWANABETHATGUY force-pushed the feat/arena-clone branch from 091b93a to e5c451e Compare January 22, 2025 09:23

IWANABETHATGUY force-pushed the feat/arena-clone branch 3 times, most recently from 73768c1 to a519e6e Compare January 22, 2025 09:26

IWANABETHATGUY changed the title ~~TEST feat(oxc_allocator): make data in arena cloneable~~ feat(oxc_allocator): Add new method clone_in_with_semantic_ids for CloneIn trait Jan 22, 2025

IWANABETHATGUY commented Jan 22, 2025

View reviewed changes

tasks/ast_tools/src/derives/clone_in.rs Show resolved Hide resolved

IWANABETHATGUY marked this pull request as ready for review January 22, 2025 09:32

IWANABETHATGUY requested review from overlookmotel, Boshen and Dunqing as code owners January 22, 2025 09:32

overlookmotel requested changes Jan 22, 2025

View reviewed changes

overlookmotel removed request for Boshen and Dunqing January 22, 2025 15:38

IWANABETHATGUY force-pushed the feat/arena-clone branch 4 times, most recently from 1dd4c8e to c479a19 Compare January 22, 2025 16:27

IWANABETHATGUY requested a review from leaysgur as a code owner January 22, 2025 16:27

IWANABETHATGUY force-pushed the feat/arena-clone branch 2 times, most recently from 7f1144f to bd3e390 Compare January 23, 2025 03:41

refactor(minifier): use NonEmptyStack for function stack (#8661)

ad3ef33

IWANABETHATGUY force-pushed the feat/arena-clone branch from bd3e390 to ad3ef33 Compare January 23, 2025 03:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(oxc_allocator): Add new method `clone_in_with_semantic_ids` for `CloneIn` trait #8608

feat(oxc_allocator): Add new method `clone_in_with_semantic_ids` for `CloneIn` trait #8608

IWANABETHATGUY commented Jan 20, 2025 •

edited

Loading

graphite-app bot commented Jan 20, 2025

IWANABETHATGUY Jan 20, 2025

Boshen commented Jan 20, 2025

IWANABETHATGUY commented Jan 20, 2025

overlookmotel commented Jan 20, 2025 •

edited

Loading

IWANABETHATGUY commented Jan 20, 2025 •

edited

Loading

overlookmotel commented Jan 20, 2025

overlookmotel commented Jan 21, 2025 •

edited

Loading

IWANABETHATGUY commented Jan 21, 2025

IWANABETHATGUY commented Jan 22, 2025

codspeed-hq bot commented Jan 22, 2025 •

edited

Loading

overlookmotel Jan 22, 2025

overlookmotel Jan 22, 2025

feat(oxc_allocator): Add new method clone_in_with_semantic_ids for CloneIn trait #8608

Are you sure you want to change the base?

feat(oxc_allocator): Add new method clone_in_with_semantic_ids for CloneIn trait #8608

Conversation

IWANABETHATGUY commented Jan 20, 2025 • edited Loading

graphite-app bot commented Jan 20, 2025

How to use the Graphite Merge Queue

IWANABETHATGUY Jan 20, 2025

Choose a reason for hiding this comment

Boshen commented Jan 20, 2025

IWANABETHATGUY commented Jan 20, 2025

overlookmotel commented Jan 20, 2025 • edited Loading

IWANABETHATGUY commented Jan 20, 2025 • edited Loading

overlookmotel commented Jan 20, 2025

overlookmotel commented Jan 21, 2025 • edited Loading

IWANABETHATGUY commented Jan 21, 2025

IWANABETHATGUY commented Jan 22, 2025

How to use the Graphite Merge Queue

codspeed-hq bot commented Jan 22, 2025 • edited Loading

CodSpeed Performance Report

Merging #8608 will not alter performance

Summary

overlookmotel Jan 22, 2025

Choose a reason for hiding this comment

overlookmotel Jan 22, 2025

Choose a reason for hiding this comment

feat(oxc_allocator): Add new method `clone_in_with_semantic_ids` for `CloneIn` trait #8608

feat(oxc_allocator): Add new method `clone_in_with_semantic_ids` for `CloneIn` trait #8608

IWANABETHATGUY commented Jan 20, 2025 •

edited

Loading

overlookmotel commented Jan 20, 2025 •

edited

Loading

IWANABETHATGUY commented Jan 20, 2025 •

edited

Loading

overlookmotel commented Jan 21, 2025 •

edited

Loading

codspeed-hq bot commented Jan 22, 2025 •

edited

Loading