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: incorrect semantic check #4999

Closed
Dunqing opened this issue Aug 20, 2024 · 5 comments · Fixed by #5035
Closed

transformer: incorrect semantic check #4999

Dunqing opened this issue Aug 20, 2024 · 5 comments · Fixed by #5035
Assignees
Labels
A-semantic Area - Semantic A-transformer Area - Transformer / Transpiler blocker C-bug Category - Bug

Comments

@Dunqing
Copy link
Member

Dunqing commented Aug 20, 2024

I saw nearly all tests have a Symbols mismatch after transform error. This error is thrown here

if self.previous_collect.symbol_ids.len() != current_collect.symbol_ids.len() {
self.errors.push(OxcDiagnostic::error("Symbols mismatch after transform"));
return;
}

After looking into it, what happened here, I found a bug in PostTransformChecker. From the code above, it can be seen that this is due to the unequal length of semantic symbols in the two different phases.

The previous_collect is collected in after_semantic and the current_collect is collected in after_transform. This is the cause of the problem. After the transformation, the Semantic will definitely change.

So we shouldn't use after_semantic's semantic to compare with after_transform's semantic. Instead, we should rebuild the semantic after the transformation, and compare it to after_transform's semantic

@Dunqing Dunqing added C-bug Category - Bug A-semantic Area - Semantic A-transformer Area - Transformer / Transpiler labels Aug 20, 2024
@Dunqing
Copy link
Member Author

Dunqing commented Aug 20, 2024

We may need to solve this problem as soon as possible. Since we can't catch valid any regression in semantic changes, I am a bit afraid to refactor the transformer 🥲

@overlookmotel
Copy link
Contributor

I'm going to look at this tomorrow. Will try to wrap it up as quickly as possible.

@Boshen
Copy link
Member

Boshen commented Aug 21, 2024

@Dunqing Can you give us a test case? Ideally what's wrong and the expected result?

@Boshen Boshen added the blocker label Aug 21, 2024
@Dunqing
Copy link
Member Author

Dunqing commented Aug 21, 2024

In the following test used nullish-coalescing-operators plugin

Input

// nullish-coalescing/transform-in-function/input.js
function foo(opts) {
  var foo = opts.foo ?? "default";
}

Output

// nullish-coalescing/transform-in-function/output.js
function foo(opts) {
  var _opts$foo;
  var foo = (_opts$foo = opts.foo) !== null && _opts$foo !== void 0 ? _opts$foo : "default";
}

The previous_collect.symbol_ids have 3 symbols, They are foo(function name), opts, and foo(var foo).
The current_collect.symbol_ids have 4 symbols, They are foo(function name), opts, _opts$foo, and foo(var foo).

Both of the different phases' symbols are correct. The root cause is we use after_semantic's Semantic to compare with after_transform's Semantic.

@overlookmotel
Copy link
Contributor

Fixed in #5035.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-semantic Area - Semantic A-transformer Area - Transformer / Transpiler blocker C-bug Category - Bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants