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: check symbols and scopes after transformation #4790

Closed
Boshen opened this issue Aug 7, 2024 · 5 comments · Fixed by #4917
Closed

semantic: check symbols and scopes after transformation #4790

Boshen opened this issue Aug 7, 2024 · 5 comments · Fixed by #4917
Assignees
Labels
A-transformer Area - Transformer / Transpiler

Comments

@Boshen
Copy link
Member

Boshen commented Aug 7, 2024

cc @overlookmotel how exactly do we want to check symbols and scopes?

@overlookmotel
Copy link
Collaborator

overlookmotel commented Aug 8, 2024

Process

I suggest the following process:

  1. Run parser on source to produce AST (AST v1).
  2. Run semantic on AST.
  3. Run transformer on AST.
  4. Store ScopeTree and SymbolTable from after transform (Semantic v1).
  5. Clone AST (making AST v2).
  6. Traverse AST v2 and set all scope_id, symbol_id and reference_id fields to None. i.e. AST v2 is now returned to virgin state like it would have been if it had come fresh out of the parser.
  7. Run semantic on AST v2. Store the new ScopeTree and SymbolTable (Semantic v2).
  8. Compare scope_id, symbol_id and reference_id fields between AST 1 and AST 2. They should match.
  9. Compare ScopeTree + SymbolTable v1 and v2. They should also match.

i.e. AST v2 and ScopeTree + SymbolTable v2 are how they should be. Make sure that state of both AST and Semantic after transform (v1) matches that.

The complication

The tricky part is what constitutes "matches".

For example if this is the input:

if (x) enum Foo {}
function f() {}

The output of transformer is:

if (x) {}
function f() {}

The scope IDs are:

Before transform:

// Scope ID 0
if (x) enum Foo { /* Scope ID 1 */ }
function f() { /* Scope ID 2 */ }

After transform:

// Scope ID 0
if (x) { /* Scope ID 3 */ } // <-- newly created scope
function f() { /* Scope ID 2 */ }

vs fresh semantic run on post-transform AST:

// Scope ID 0
if (x) { /* Scope ID 1 */ } // <-- numbered 1 as it's 2nd scope found in visitation order
function f() { /* Scope ID 2 */ }

Scope IDs of the if {} block are different in the 2 versions, because in the post-transform version, that scope was newly created in transformer.

But we don't want to throw an error on this because they are equivalent.

Likely same kind of thing will happen with SymbolIds and ReferenceIds.

Suggested solution

Traverse AST v1 and AST v2 in tandem and:

  1. Check that every node which has a ScopeId in AST v1 also has one in AST v2, and vice versa.
  2. Build a hash map mapping from v1 ScopeId to v2 ScopeId. So, in example above v1 Scope ID 3 -> v2 Scope ID 1.

When comparing v1 ScopeTree to v2 ScopeTree, use the mapping to translate IDs before checking the scope trees match.

Ditto for SymbolIds and ReferenceIds.

Conclusion

So this is all a bit of a pain! But, if we can build this test infra, it will give us extremely good test coverage and we can be very confident that transformer is keeping scopes/symbols in sync correctly.

Side note: I'd imagine running these checks as part of transformer conformance, so we get instant feedback on PRs if they foul up scopes.

@Boshen Boshen transferred this issue from oxc-project/monitor-oxc Aug 9, 2024
@Boshen Boshen self-assigned this Aug 9, 2024
@Boshen Boshen added the A-transformer Area - Transformer / Transpiler label Aug 9, 2024
@Boshen Boshen added this to the Transformer Milestone 2 milestone Aug 9, 2024
@overlookmotel
Copy link
Collaborator

overlookmotel commented Aug 10, 2024

I would be happy to take this is on if you like, Boshen.

@Dunqing
Copy link
Member

Dunqing commented Aug 10, 2024

Just a note, we may need to check duplicate AstNodeId, ScopeId, and SymbolId in AST. related to #4809

@Boshen
Copy link
Member Author

Boshen commented Aug 12, 2024

Borrowing the idea of the mangler, we can check whether symbols are the same in scope visitation order.

@Boshen Boshen changed the title check symbols and scopes after transformation semantic: check symbols and scopes after transformation Aug 12, 2024
@rzvxa
Copy link
Collaborator

rzvxa commented Aug 12, 2024

Just a note, we may need to check duplicate AstNodeId, ScopeId, and SymbolId in AST. related to #4809

This feels more related to #4804

Boshen added a commit that referenced this issue Aug 16, 2024
closes #4790

@overlookmotel enjoy ... take a look at the snapshots and probably nothing else.

The snapshots are minimal right now, but it's already showing symbols from import specifiers are not being removed. We can iterate on the snapshot representation to aid debugging later.

I'll extend this to `transformer_conformance` and `oxc-monitor` in an up coming PR.
@Boshen Boshen closed this as completed Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-transformer Area - Transformer / Transpiler
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants