feat(traverse): add generate_uid_in_current_hoist_scope method

[Dunqing]

Add an API to handle variable hoisting.

[graphite-app]

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.

[Dunqing]

• fix(transformer/async-to-generator): correct the SymbolFlags of function id in module #7470
• fix(transformer/object-rest-spread): generate catch variable binding with correct SymbolFlags #7469
• feat(traverse): add generate_uid_in_current_hoist_scope method #7423 👈 (View in Graphite)
• main

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

[codspeed-hq]

CodSpeed Performance Report

Merging #7423 will not alter performance

_{Comparing 11-22-feat_traverse_support_automatically_variable_hoisting_during_generating_bindings (9c9deae) with main (3a1ef6a)}

Summary

✅ 30 untouched benchmarks

[overlookmotel]

I've squashed the auto-fix commit and updated conformance results, so can better see what's going on here. Reviewing now...

[overlookmotel]

Some suggestions:

1. Add current_var_hoisting_scope_id method to TraverseCtx too.
2. Instead of set_current_scope_id taking 2 params, add a 2nd function set_current_hoist_scope_id instead.

• Only generate calls to set_current_hoist_scope_id for the scopes which are "hoist scopes" (functions etc).
• This should be faster for the majority of scopes which are not "hoist scopes".

3. Rather than all binding creation going via add_binding which has the cost of looking up scope flags and a branch, should we have generate_uid_in_current_scope and generate_uid_in_current_hoist_scope?

• When generating a let binding, there's no reason to do any work checking flags.

These could be done in follow-up PRs if you prefer.

Longer-term we should probably (as you suggested previously) integrate creating the binding into VarDeclarationsStore::insert_var and insert_let. So then those 2 functions will already know what scope the binding should be in, as they know if it's a var or let.

[Dunqing]

3. Rather than all binding creation going via add_binding which has the cost of looking up scope flags and a branch, should we have generate_uid_in_current_scope and generate_uid_in_current_hoist_scope?

Yes, we even do not need to pass SymbolFlags in this API

[overlookmotel]

Yes, we even do not need to pass SymbolFlags in this API

Oh yes! I didn't even think of that.

[Dunqing]

Yes, we even do not need to pass SymbolFlags in this API

Oh yes! I didn't even think of that.

Haha. I am trying to avoid adding new APIs (because we have many generate binding APIs), but it is worth doing it.

[Dunqing]

1. Add current_var_hoisting_scope_id method to TraverseCtx too.

We have no real case that needs to be inserted to var hoisting scope id manually, so I think it doesn't need to be added to TraverseCtx for now. What are your thoughts on this? Please let me know.

[overlookmotel]

We currently alias all methods of TraverseScoping (and TraverseAncestry) on to TraverseCtx, so personally I think it's preferable to be consistent. Generated code for walk_* functions can also use the shorter version.

[Dunqing]

Seems we still need an API to automatically check whether to use pass-in scope_id or current_hoist_scope_id. Please see #7469. Now it looks good

[overlookmotel]

I pushed a few commits. Just nits, really.

Merging this now. @Dunqing if you don't like any of my changes, feel free to alter them in follow-ups.

[overlookmotel]

Merge activity

• Nov 26, 6:17 AM EST: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
• Nov 26, 6:17 AM EST: A user added this pull request to the Graphite merge queue.
• Nov 26, 6:27 AM EST: A user merged this pull request with the Graphite merge queue.