fix(oxc_transform): overlap replacement

[IWANABETHATGUY]

closed #7594

Reference

https://github.com/oxc-project/oxc/pull/7343/files

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

[IWANABETHATGUY]

• feat(oxc_transformer): share object define reference for ReplaceGlobalDefines #7698
• fix(oxc_transform): overlap replacement #7621 👈 (View in Graphite)
• main

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

[IWANABETHATGUY]

Since TraverseCtxt has no functionality to skip visit sub tree(Fix me if I am wrong, I don't find the usage in codebase).
Need to lock the sub tree if one ast node is replaced by some value.
TODO:
if overlap appears in AssignmentExpr the output should still be wrong, since we can't get address of AssignmentExpression for now.

@overlookmotel any chance to implement ToAddress for AssignmentExpression ?

[codspeed-hq]

CodSpeed Performance Report

Merging #7621 will not alter performance

_{Comparing 12-04-fix_overlap_replacement (f7d41dd) with main (ac910ee)}

Summary

✅ 29 untouched benchmarks

[overlookmotel]

Since TraverseCtxt has no functionality to skip visit sub tree(Fix me if I am wrong, I don't find the usage in codebase).

Correct. Traverse has no facility to skip visiting sub tree at present. We could look at adding that, but it might cause quite a performance regression, as it'd introduce an extra check and branch after every single AST node.

The way you are handling it here seems like a good option in meantime, especially as the subtree is likely small, so not a great cost to visiting it.

TODO: if overlap appears in AssignmentExpr the output should still be wrong, since we can't get address of AssignmentExpression for now.

@overlookmotel any chance to implement ToAddress for AssignmentExpression ?

I'm not familiar with the define plugin, so not sure what you mean by "overlap". Can you give an example showing the problem with AssignmentExpression?

[IWANABETHATGUY]

Since TraverseCtxt has no functionality to skip visit sub tree(Fix me if I am wrong, I don't find the usage in codebase).

Correct. Traverse has no facility to skip visiting sub tree at present. We could look at adding that, but it might cause quite a performance regression, as it'd introduce an extra check and branch after every single AST node.

The way you are handling it here seems like a good option in meantime, especially as the subtree is likely small, so not a great cost to visiting it.

TODO: if overlap appears in AssignmentExpr the output should still be wrong, since we can't get address of AssignmentExpression for now.
@overlookmotel any chance to implement ToAddress for AssignmentExpression ?

I'm not familiar with the define plugin, so not sure what you mean by "overlap". Can you give an example showing the problem with AssignmentExpression?

Here is one, https://esbuild.github.io/try/#dAAwLjI0LjAAewogIGRlZmluZTogewogICAgJ2ltcG9ydC5tZXRhLmVudi5GT08nOiAnaW1wb3J0Lm1ldGEuZW52LkZPTycsCiAgICAnaW1wb3J0Lm1ldGEuZW52JzogJ19fZm9vX18nLAogIH0sCn0ACmNvbnNvbGUubG9nKAoJW2ltcG9ydC5tZXRhLmVudi5GT08gPSAwXQopCg, you could see
both
import.meta.env.FOO and import.meta.env matched part of import.meta.env.FOO
for esbuild it will replace import.meta.env.FOO to import.meta.env.FOO and skip visit sub tree.

but for us,
we first replace import.meta.env.FOO to import.meta.env.FOO, then visit the child import.meta.env, and replace it to
__foo__, so the final output should be __foo__.FOO

[overlookmotel]

Apart from the comments I've made, looks good.

But could you please add some comments about what ast_node_lock is and how it works?

[overlookmotel]

Merge activity

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