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

transformer: Sync AST nodes and scopes #3503

Closed
overlookmotel opened this issue Jun 2, 2024 · 4 comments
Closed

transformer: Sync AST nodes and scopes #3503

overlookmotel opened this issue Jun 2, 2024 · 4 comments
Assignees
Labels
C-enhancement Category - New feature or request

Comments

@overlookmotel
Copy link
Collaborator

It's common in transformer that you need to:

  1. Remove a statement from AST.
  2. Replace a statement with multiple statements.
  3. Insert a statement at top of a statement block (e.g. import React from 'react').

All of these cause us problems because the contents of Vec<Statement> needs to be shuffled up/down, which can be expensive, especially at top level where there may be many statements.

We currently have various workarounds for this, including delaying inserting statements until later in transform, or rebuilding the Vec<Statement>, copying all the contents. Multiple transforms may repeat this "shuffle" on the same set of statements.

Could we add a variant Statement::CompositeStatement?

struct CompositeStatement<'a> {
  stmts: Vec<'a, Statement<'a>>
}

During transform, you can perform any insert/replace/delete action by replacing a Statement with a CompositeStatement. Then when exiting the statement block, if it contains any CompositeStatements, the Vec<Statement> would be shuffled to flatten all the CompositeStatements. This could happen in place and in a single pass, enacting all the changes made by multiple transforms all in one go.

i.e. CompositeStatement would only be used temporarily in the transformer, and they'd be gone in final AST, when transformation is complete.

The other advantage is that we can keep AST and scopes tree in sync at all times. Currently we add bindings to scope tree for new insertions before the actual AST nodes which produce those bindings are inserted, so there's a period where the 2 are out of sync. This may cause us problems further down the line.

The downside of this idea is that whenever iterating over a Vec<Statement> in transformer, you now need to handle CompositeStatements and step into them.

@overlookmotel overlookmotel added the C-enhancement Category - New feature or request label Jun 2, 2024
@Boshen Boshen changed the title Composite statement type for transformer? Sync AST nodes and scopes Jun 3, 2024
@Boshen Boshen assigned overlookmotel and unassigned Boshen Jun 3, 2024
@Boshen Boshen added the P-high Priority - High label Jun 3, 2024
@Boshen
Copy link
Member

Boshen commented Jun 3, 2024

We need a way to detect broken scopes in transform-ci.

@overlookmotel
Copy link
Collaborator Author

overlookmotel commented Jun 8, 2024

Replacing Vec<Statement> with a chunked linked list would be another way to solve these problems. Probably better than the CompositeStatement solution suggested above.

@Boshen Boshen added this to the Transformer Milestone 2 milestone Jun 21, 2024
@Boshen Boshen changed the title Sync AST nodes and scopes transformer: Sync AST nodes and scopes Jun 26, 2024
@Boshen Boshen removed the P-high Priority - High label Jul 7, 2024
@Dunqing
Copy link
Member

Dunqing commented Jul 31, 2024

I created a #4581 to add a test to verify that ScopeTree and SymbolTable are correct after the transformation

@overlookmotel
Copy link
Collaborator Author

Closing in favour of oxc-project/backlog#35.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category - New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants