feat(traverse): support clone_identifier_reference method in TraverseCtx

[Dunqing]

related: #4804

needs from: #4876

The clone_identifier_reference method is used to clone an IdentifierReference and create a Reference and insert it to SymbolTable's resolved_references.

The reason we need this is because we need to make sure that IdentifierReference's reference_id is unique

[Dunqing]

• feat(traverse): support clone_identifier_reference method in TraverseCtx #4880 👈
• 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]

CodSpeed Performance Report

Merging #4880 will not alter performance

_{Comparing 08-13-feat_traverse_support_clone_identifier_reference_method_in_traversectx (72a37fc) with main (47c9552)}

Summary

✅ 29 untouched benchmarks

[graphite-app]

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.

[rzvxa]

Sorry if I'm a little out of the loop here, Out of interest I was wondering where we need 2 copies of the exact same node.
Since it is related to #4804 I'm interested to know about our use cases.
Doesn't it make more sense to have a generic API that would accept both BindingIdentifier and IdentifierReference and can output an IdentifierReference? Something like ctx.create_reference_to(it).

Edit:

Is there a reason we have something like this in our code:

            let ident = ctx.create_reference_id(
                param.span,
                param.name.clone(),
                param.symbol_id.get(),
                ReferenceFlag::Read,
            );

instead of passing param directly?

[Dunqing]

Sorry if I'm a little out of the loop here, Out of interest I was wondering where we need 2 copies of the exact same node. Since it is related to #4804 I'm interested to know about our use cases.

For example:

// In
a.b **= 0

// Out
var _a;
(_a = a), (_a["b"] = Math.pow(_a["b"], 0));

The final result, in this case, is the _a at least needs to clone three times, and a need to clone once

Doesn't it make more sense to have a generic API that would accept both BindingIdentifier and IdentifierReference and can output an IdentifierReference? Something like ctx.create_reference_to(it).

It would be better if we have an API to accept both BindingIdentifier and IdentifierReference and return anIdentifierReference which has the same symbol_id.

Edit:

Is there a reason we have something like this in our code:

            let ident = ctx.create_reference_id(
                param.span,
                param.name.clone(),
                param.symbol_id.get(),
                ReferenceFlag::Read,
            );

instead of passing param directly?

IdentifierReference only has reference_id. If we want to get a symbol_id through reference_id, so we need to get a reference by reference_id first, and then we can get symbol_id through reference.symbol_id()

[Dunqing]

Sorry if I'm a little out of the loop here, Out of interest I was wondering where we need 2 copies of the exact same node. Since it is related to #4804 I'm interested to know about our use cases.

For example:

// In
a.b **= 0

// Out
var _a;
(_a = a), (_a["b"] = Math.pow(_a["b"], 0));

The final result, in this case, is the _a at least needs to clone three times, and a need to clone once

Doesn't it make more sense to have a generic API that would accept both BindingIdentifier and IdentifierReference and can output an IdentifierReference? Something like ctx.create_reference_to(it).

It would be better if we have an API to accept both BindingIdentifier and IdentifierReference and return anIdentifierReference which has the same symbol_id. But different parameters have different implementation details

Edit:

Is there a reason we have something like this in our code:

            let ident = ctx.create_reference_id(
                param.span,
                param.name.clone(),
                param.symbol_id.get(),
                ReferenceFlag::Read,
            );

instead of passing param directly?

IdentifierReference only has reference_id. If we want to get a symbol_id through reference_id, so we need to get a reference by reference_id first, and then we can get symbol_id through reference.symbol_id()

[Dunqing]

According to use case #4876, only clone_identifier_reference is not enough. clone_expression can be quite common.

All of these Expressions include IdentifierReference

a.b // MemberExpression
a[b] // MemberExpression
`123 ${variable} 321` // TemplateLiteralExpression

Currently, I use move_expression instead of clone_in, but when an Expression may used to multi-place, then we must clone it

The API is best provided by Traverse, so we have to pass ScopeTree and SymbolTable

[Dunqing]

I have written this code inside plugins, so does not block any yet

[overlookmotel]

The problem with cloning an IdentifierReference like this is that it require an unnecessary lookup - you have to get the Reference from the reference_id to then get the SymbolId.

When you're generating multiple IdentifierReferences for the same symbol, it's better to look up the SymbolId only once, and then reuse it. Or, in this case, _a is a UID, so we have the SymbolId from the start.

Another API

There is an API hidden in helpers which is good for this use case:

oxc/crates/oxc_transformer/src/helpers/bindings.rs

Lines 12 to 17 in ea1e64a

	/// Store for a created binding identifier
	#[derive(Clone)]
	pub struct BoundIdentifier<'a> {
	pub name: Atom<'a>,
	pub symbol_id: SymbolId,
	}

// In
a.b **= 0

// Out
var _a;
(_a = a), (_a["b"] = Math.pow(_a["b"], 0));

In this case you could use it as follows:

// NB: `BindingIdentifier` + `IdentifierReference` type annotations are not required
// - just for clarity in this example

// Create `_a`
let binding = BoundIdentifier::new_uid(
    "a", // Name *without* `_` prefix
    ctx.current_scope_id(),
    SymbolFlags::FunctionScopedVariable,
    ctx
);
let id: BindingIdentifier = binding.create_binding_identifier();

// Then create your `IdentifierReference`s
let ref1: IdentifierReference = binding.create_read_reference(ctx);
let ref2: IdentifierReference = binding.create_read_reference(ctx);
let ref3: IdentifierReference = binding.create_read_reference(ctx);

// Now use them: `var <id>; (<ref1> = a), (<ref2>["b"] = Math.pow(<ref3>["b"], 0))`

Actually, this is not correct - ref1 should have ReferenceFlag::Write flag not Read. We should add a BoundIdentifier::create_write_reference method. I'll make a PR for that now (edit: here it is: #4897).

A couple of notes:

• The reason for having a BoundIdentifier type rather than just using BindingIdentifier is that:
  • BoundIdentifier is smaller as it doesn't include Span.
  • BoundIdentifier can have symbol_id stored as pure SymbolId, not Cell<Option<SymbolId>>.
• This BoundIdentifier API could probably be better - it could be methods on TraverseCtx instead - but I'm not sure if we should change it right now when we're in the middle of intense work on the transformer. What do you think?

Should we merge this PR?

clone_identifier_reference is good if you only need to make a single clone, and you don't know the SymbolId already.

But the danger of having this API is that it's then tempting to use it in places where it'd be better to use a more efficient method like BoundIdentifier.

Side note

The final result, in this case, is the _a at least needs to clone three times, and a need to clone once.

I don't think a should be cloned at all. It should just be moved.

[rzvxa]

@Dunqing Thanks for your explanation, Now it makes perfect sense to me.

I have an idea to introduce an Identifier trait that can unify these.
We can pass in an Identifier and get a BoundIdentifier from it, That way should be able to use it wherever we please.

To build upon @overlookmotel snippet it would look something like this:

// NB: `BindingIdentifier` + `IdentifierReference` type annotations are not required
// - just for clarity in this example

// where we already have `_a` binding identifier as `ident`:

let a_prime: IdentifierReference = ident.scoped_ref(ctx.scoping());

// when we want multiple copies from the same identifier
let binding: BoundIdentifier = ident.scoped_bound(ctx.scoping());

let ref1: IdentifierReference = binding.create_read_reference(ctx);
let ref2: IdentifierReference = binding.create_read_reference(ctx);
let ref3: IdentifierReference = binding.create_read_reference(ctx);


// Similarly if you have access to `_a` as `IdentfierReference` we can still write the exact same code:

let a_double_prime: IdentifierReference = ident_ref.scoped_ref(ctx.scoping());

// when we want multiple copies from the same identifier
let binding: BoundIdentifier = ident_ref.scoped_bound(ctx.scoping());

let ref1: IdentifierReference = binding.create_read_reference(ctx);
let ref2: IdentifierReference = binding.create_read_reference(ctx);
let ref3: IdentifierReference = binding.create_read_reference(ctx);

I'm not sure if it is the right time to do such a thing but I can't help but feel like having an Identifier trait can simplify a lot of hassles.

[overlookmotel]

@rzvxa Your proposed trait is very elegant and flexible. But... my only worry about it is that it's maybe too flexible. Same as what I said above about clone_identifier_reference method - it makes it too easy to do the wrong (expensive) thing and create a BoundIdentifier from an IdentifierReference when you already have the SymbolId.

I actually think use cases for cloning existing IdentifierReferences are quite rare. The transform we've been talking about here is quite typical:

• The original IdentifierReference is just moved, and doesn't need to be cloned.
• The only binding that we do need to create multiple IdentifierReferences for is a UID which we have newly created. So BoundIdentifier::new_uid covers us for this.

Here's another transform which follows same pattern (only UIDs needs cloning):

// In
const [x, y, z] = arr;

// Out
var _arr = _slicedToArray(arr, 3); // Helper call
const x = _arr[0], _arr[1], _arr[2];

Therefore, adding an API which makes it easy to do the wrong thing, and has few use cases where its the right thing may not on balance be a benefit overall.

The background context is that once we've laid the groundwork, we hope to open up the transformer to contributions (similar to the linter). People who are less familiar with Oxc will tend to just grab whatever API they find that makes the transform work, so there's an argument for limiting our API surface in order to guide people towards the "good path".

I could be wrong that there aren't many places where we'll need to clone existing IdentifierReferences. But I suggest we wait and see if we need this API, and only add it if we do.

[overlookmotel]

@Dunqing Conversation here has broadened out beyond the content of this specific PR. Just to bring it back to the specific:

Do you think BoundIdentifier (especially once #4897 is merged) can cover this, so maybe we don't need clone_identifier_reference too?

Sorry I hid BoundIdentifier away in a silly place and didn't let anyone know it was there. I got lazy!

[overlookmotel]

I was wrong! The exponent operator transform does requiring cloning IdentifierReferences in some cases. So we probably should add this clone_identifier_reference method for that.

Concerning the Ident trait idea: Personally I think it's better for this to be an explicitly-named method just for IdentifierReferences, rather than a cover-all trait, so potentially bad usages of this method are easier to spot.

[graphite-app]

Merge activity

• Aug 15, 8:02 AM EDT: The merge label 'merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
• Aug 15, 8:02 AM EDT: overlookmotel added this pull request to the Graphite merge queue.
• Aug 15, 8:05 AM EDT: overlookmotel merged this pull request with the Graphite merge queue.