Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

semantic: traverse scope tree #5386

Closed
Boshen opened this issue Sep 1, 2024 · 4 comments
Closed

semantic: traverse scope tree #5386

Boshen opened this issue Sep 1, 2024 · 4 comments
Assignees
Labels
blocker C-bug Category - Bug

Comments

@Boshen
Copy link
Member

Boshen commented Sep 1, 2024

#5232 removed traversing the scope tree, we need it back or have a way to traverse scopes.

This blocks us from making a release for Rolldown.

https://github.com/rolldown/rolldown/blob/6b5d7dc97b47895bd8280d29c2a815e159fa6b8c/crates/rolldown/src/utils/renamer.rs#L162-L173

@Boshen
Copy link
Member Author

Boshen commented Sep 1, 2024

It seems like we can make this optional, it's only used after rolldown recreates the scope tree https://github.com/rolldown/rolldown/blob/c916947780f036779725d45729e7de65e4745abb/crates/rolldown/src/utils/pre_process_ecma_ast.rs#L101-L102

@overlookmotel
Copy link
Contributor

Maybe we can move ChildScopeCollector into Semantic. That might satisfy Rolldown's requirements.

I'll take a look tomorrow.

@overlookmotel
Copy link
Contributor

I looked at Rolldown's rename_non_top_level_symbol function. To be honest, I find it hard to understand why it's doing it the way it is. It strikes me that it shouldn't be necessary to walk the scopes tree, renaming loads of bindings at different levels of the tree. Could instead:

  1. Find any top-level vars which are used in more than one module in same chunk. These need renaming.
  2. If none found, exit.
  3. Iterate through symbols + unresolved references for each module in the chunk and build a list of var names which are in use anywhere in the ASTs of the modules. Any names not in this list are available.
  4. Rename the duplicate top-level vars identified in step (1) to names which are not in the set built in step (3).

i.e. the only bindings which need to be renamed are top-level bindings.

I'd imagine this would be faster than the current implementation, and would not require child_scope_ids.

But... I imagine making that change would break loads of Rolldown's tests which rely on the current naming scheme.

We don't have time to figure that out, so yes let's revert #5232, so we can get a release out that doesn't break Rolldown.

BUT... can we put it behind a compile-time feature, not a runtime one? If we use a runtime check, the repeated if options.child_scopes checks will cost perf even when it's disabled. Oxc doesn't need child_scope_ids, and it's a shame we have to pay for it in transformer etc when we're only including it for Rolldown.

Or we revert with a runtime flag first, and then switch over to a compile-time feature and measure the impact on Oxc's benchmarks.

@Boshen Boshen assigned Boshen and unassigned overlookmotel Sep 2, 2024
@overlookmotel
Copy link
Contributor

Closed in #5403.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocker C-bug Category - Bug
Projects
None yet
Development

No branches or pull requests

2 participants