fix(transformer): use UIDs in TS namespace transforms

[overlookmotel]

Use the TraverseCtx::generate_uid method introduced in #3395 to fix some of the TS namespace test cases.

But... I honestly have no idea what I'm doing here!

I am running up against a combination of 3 different areas where I know very little:

1. I am unfamiliar with Semantic.
2. I am unfamiliar with Oxc's conventions for writing transforms.
3. I don't "speak" Typescript!

This PR should not be merged as is. I'm pretty sure it's wrong. I've done it mostly just to test out generate_uid.

In particular:

1. Is SymbolFlags::FunctionScopedVariable the right flags to use in all cases here?
2. generate_uid creates a symbol, and registers a binding for it in the scope tree. So if there are any branches in this logic where name doesn't actually get used after generate_uid is called, then it's not right.
3. Identifiers which are added to AST should also have corresponding ReferenceIds created for them (but I imagine this is also missing in many other places in the transformer).

On point 2: We could split up generate_uid into 2 functions. One function would find a valid UID name but not register a binding for it. If you want to actually use that name, you'd call a 2nd function to generate the binding. Would that be a better API?

Could someone help me out to progress this please?

(Sorry my lack of knowledge is a bit useless here. I will learn all these things in time, but just trying to be honest about where I'm at right now. I'm sure I could figure it out myself, but it would take me hours, whereas others will probably look at it and know what to do in about 5 mins.)

[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.

[overlookmotel]

• fix(transformer): use UIDs in TS namespace transforms #3395 👈
• feat(transformer): add TraverseCtx::generate_uid #3394
• main

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

Join @overlookmotel and the rest of your teammates on Graphite

[codspeed-hq]

CodSpeed Performance Report

Merging #3395 will not alter performance

_{Comparing 05-23-fix_transformer_use_uids_in_ts_namespace_transforms (2fcf871) with main (d80e9b4)}

Summary

✅ 22 untouched benchmarks

[Boshen]

The code was setup this way so you can just replace it, and it's working 😁

[Dunqing]

In particular:

1. Is SymbolFlags::FunctionScopedVariable the right flags to use in all cases here?

Correct!

2. generate_uid creates a symbol, and registers a binding for it in the scope tree. So if there are any branches in this logic where name doesn't actually get used after generate_uid is called, then it's not right.

Yes.

3. Identifiers which are added to AST should also have corresponding ReferenceIds created for them (but I imagine this is also missing in many other places in the transformer).

Yes, we need to bind all unresolved references for the new binding.

On point 2: We could split up generate_uid into 2 functions. One function would find a valid UID name but not register a binding for it. If you want to actually use that name, you'd call a 2nd function to generate the binding. Would that be a better API?

I think we can have all three API's at the same time, but not now, not until we really need it!

Haha, I think this PR we can merge. You did the right thing.

[Dunqing]

The remaining test cases are for me to take over, and I'm eager to start using these APIs!

[overlookmotel]

I've been through this more thoroughly now. Fixed one problem with a scope binding being registered where no actual binding is created.

But found another problem:

oxc/crates/oxc_transformer/src/typescript/namespace.rs

Lines 136 to 140 in 2fcf871

	// TODO: This binding is created in wrong scope.
	// Needs to be created in scope of function which `transform_namespace` creates below.
	let name = self.ctx.ast.new_atom(
	&ctx.generate_uid_in_current_scope(real_name, SymbolFlags::FunctionScopedVariable),
	);

I'm not quite sure best way to tackle that. If you guys are happy to merge this in its partially-broken state, with the "TODO" comment, then please do so.

The downside of having scopes in the transformer is that we now have to keep them in sync with changes to the AST. I imagine we'll have to do a 2nd pass through all the transforms implemented so far and find all the places where that's not happening.

[Dunqing]

I'm not quite sure best way to tackle that. If you guys are happy to merge this in its partially-broken state, with the "TODO" comment, then please do so.

Let's merge this, and I will take a try at this problem