fix(transformer): clean up semantics for stripped TypeScript syntax#24180
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
d443afc to
189bc84
Compare
Merging this PR will degrade performance by 4.81%
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing Footnotes
|
f801b6c to
29a6c00
Compare
|
This should be ready to go. There will be some followups needed, but those are out of scope for now. @Dunqing do you mind having a look? I'd like to get this in today. In regards to the perf regression, I think it's acceptable - we're inching towards correctness here, and it'll allow rolldown to avoid a semantic rebuild in future. |
There was a problem hiding this comment.
Pull request overview
Cleans up semantic data produced/kept by the transformer when stripping TypeScript-only syntax, so transformer “after transform” semantics stay aligned with semantics rebuilt from the transformed AST (reducing snapshot noise ahead of upcoming AST/scope work).
Changes:
- Remove type-space-only resolved/unresolved references and delete bindings for stripped TS-only declarations/imports.
- Preserve runtime bindings when a type import is merged with a value declaration by removing just the import’s redeclaration and promoting surviving symbol metadata.
- Add coverage for type-import + value redeclaration, and refresh transformer/semantic conformance snapshots.
Reviewed changes
Copilot reviewed 4 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
crates/oxc_transformer/src/typescript/annotations.rs |
Removes bindings when erasing TS imports/specifiers; preserves bindings for merged runtime declarations. |
crates/oxc_semantic/src/scoping.rs |
Adds remove_symbol_declaration and expands delete_typescript_bindings to prune TS-only references as well as bindings. |
tasks/coverage/misc/pass/type-import-value-redeclaration.ts |
New coverage case for type-import merged with runtime function declarations. |
tasks/transform_conformance/snapshots/oxc.snap.md |
Updated transformer conformance snapshot after semantic cleanup. |
tasks/transform_conformance/snapshots/babel.snap.md |
Updated Babel transformer conformance snapshot after semantic cleanup. |
tasks/coverage/snapshots/transformer_misc.snap |
Snapshot refresh due to added misc coverage case. |
tasks/coverage/snapshots/semantic_misc.snap |
Snapshot refresh reflecting reduced semantic mismatches after TS reference cleanup. |
tasks/coverage/snapshots/semantic_babel.snap |
Snapshot refresh reflecting reduced semantic mismatches after TS reference cleanup. |
tasks/coverage/snapshots/parser_misc.snap |
Snapshot refresh due to added misc coverage case. |
tasks/coverage/snapshots/formatter_misc.snap |
Snapshot refresh due to added misc coverage case. |
tasks/coverage/snapshots/codegen_misc.snap |
Snapshot refresh due to added misc coverage case. |
Dunqing
left a comment
There was a problem hiding this comment.
Overall, it looks good, and the performance regression is expected. Just a small improvement we could make is in the follow-up.
29a6c00 to
e06692c
Compare
Merge activity
|
…24180) ## Summary Keep transformer semantic data in sync when TypeScript-only syntax is removed. This change: - removes resolved and unresolved references belonging only to stripped TypeScript syntax - removes bindings for erased type-only and unused imports - preserves runtime bindings when a type import is merged with a value declaration - promotes the surviving declaration’s symbol flags, span, and redeclaration metadata - adds coverage for type imports merged with runtime declarations - refreshes semantic and transformer conformance snapshots ## Motivation We are preparing to change the AST shape, including adding and rearranging scopes. Currently, transformer semantic snapshots contain thousands of lines of mismatches caused by stale bindings and references from TypeScript syntax that has already been removed from the AST. That creates a large amount of unrelated snapshot noise. With that noise present, snapshot changes caused by the upcoming AST work would be difficult to review: a real scope or semantic regression could easily be hidden among mechanical churn. This PR fixes the underlying semantic inconsistencies first, giving us a much cleaner baseline for reviewing future AST and scope changes. It is not intended to change emitted JavaScript.
5df8cc9 to
e8b50ee
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5df8cc9eda
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
### 🚀 Features - 260425f semantic/examples: Include unresolved references (#24214) (camc314) - 2d9b0b3 minifier: Fold boolean-literal ternary branches in value contexts (#24110) (Dunqing) - 61fbf10 ast: Implement `ReplaceWith` on all AST types (#24013) (overlookmotel) - 7db7a29 allocator: Add `ReplaceWith` trait (#24012) (overlookmotel) - 4eb074e mangler: Add `reserved` option for names that must not be mangled (#24041) (Dunqing) - 2e62012 data_structures: Add `StringExt` trait (#24006) (overlookmotel) - 60e7160 minifier: Drop side-effect-free IIFEs whose result is unused (#23967) (Dunqing) - 26dd9e2 ast: Add method to widen inherited enum ref to parent ref (#23961) (overlookmotel) ### 🐛 Bug Fixes - e8b50ee transformer: Clean up semantics for stripped TypeScript syntax (#24180) (camc314) - d966d0b react_compiler: Remove clippy allows (#24168) (Boshen) - 854ef8d react_compiler: Compile generic functions instead of over-bailing on type-param hoisting (#24158) (Boshen) - 093586c react_compiler: Align memoization cache-slot allocation with Babel (#24157) (Boshen) - 09c8f59 react_compiler: Normalize snapshot fixture paths (#24142) (camc314) - f13df97 react_compiler: Drop stray empty statement from catch bindings (#24133) (Boshen) - cb2a505 react_compiler: Codegen destructuring reassignment targets (#24131) (Boshen) - b82c394 react_compiler: Propagate codegen invariants instead of emitting empty bodies (#24128) (Boshen) - 5771982 react_compiler: Render unchanged programs as source in fixture snapshots (#24129) (Boshen) - 4b16e1a transformer/async-to-generator: Preserve direct eval scope flags (#24136) (camc314) - 4e9194f react_compiler: Lower `delete obj.prop` to Property/ComputedDelete (#24123) (Boshen) - 0b25582 ast: Type binding node `typeAnnotation` as `TSTypeAnnotation | null` (#23113) (Boshen) - 018c0e5 transformer: Hoist lowered async declarations (#22770) (camc314) - 652fbaf mangler: Keep names of destructured exported bindings (#24036) (Dunqing) - e274415 minifier: Don't drop global calls that throw despite pure arguments (#23917) (Dunqing) - 59abb30 minifier: Only merge string literals in `try_fold_add` when the inner operator is `+` (#23622) (Jerry Zhao) ### ⚡ Performance - c5ca77b transformer: Avoid cloning refresh options (#24191) (camc314) - bf1a151 react_compiler: Compile out debug printers (#24184) (Boshen) - abb44a0 transformer: Build fixed object-rest arguments (#24190) (camc314) - a4db731 isolated_declarations: Use `ReplaceWith` instead of `TakeIn` (#24016) (overlookmotel) - ff10855 transformer: Use `ReplaceWith` instead of `TakeIn` (#24015) (overlookmotel) - bd49aff ecmascript: Avoid heap-allocating Math.min/max/imul operands (#23941) (Lawrence Lin) - e4b708b react_compiler: Skip compiled files before prefilters (#24171) (Boshen) - c59f2fe rust: Return impl ExactSizeIterator from slice-backed accessors (#24144) (Boshen) - 5d6d04a codegen: SWAR-skip boring byte runs in sourcemap line/column scan (#24023) (Boshen) - a55e0be traverse: Reduce string operations in `get_var_name_from_node` (#24007) (overlookmotel) - e6d48e1 transformer/nullish_coalescing: Move cold path into separate function (#23989) (overlookmotel) - c4e35b5 transformer/object_rest_spread: Pre-allocate capacity in `Vec` (#23988) (overlookmotel) - 527b8e5 transformer/decorators: Narrow type earlier (#23987) (overlookmotel) ### 📚 Documentation - 30d17f5 allocator: Clarify docs for `TakeIn::take_in_box` (#24093) (overlookmotel) - 675e6a8 ast: Correct doc comment for `PrivateFieldExpression` (#24008) (overlookmotel) - e4c30e6 minifier: Explain what `dce` mode means (#23994) (Dunqing) - 37cbf88 ast_macros: Document fields of `StructDetails` (#23959) (overlookmotel) - 4de3e54 ast: Correct doc comment (#23948) (overlookmotel) Co-authored-by: Boshen <[email protected]>
Summary
Keep transformer semantic data in sync when TypeScript-only syntax is removed.
This change:
Motivation
We are preparing to change the AST shape, including adding and rearranging scopes.
Currently, transformer semantic snapshots contain thousands of lines of mismatches caused by stale bindings and references from TypeScript syntax that has already been removed from the AST. That creates a large amount of unrelated snapshot noise.
With that noise present, snapshot changes caused by the upcoming AST work would be difficult to review: a real scope or semantic regression could easily be hidden among mechanical churn.
This PR fixes the underlying semantic inconsistencies first, giving us a much cleaner baseline for reviewing future AST and scope changes. It is not intended to change emitted JavaScript.