-
Notifications
You must be signed in to change notification settings - Fork 824
Fix import elision with type-only usage #2646
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
Conversation
Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
Found that tsgo incorrectly emits require() when:
- Import contains both 'type' and non-type specifiers
- Non-type specifier is only used in type positions
Minimal repro: import { Value, type ValueData } from './provider';
where Value is only used as a type annotation.
Identified that the bug occurs specifically when: - Import statement contains BOTH type-only and non-type specifiers - The non-type specifier is only used in type positions Most likely cause: Import processing logic doesn't properly distinguish between type-level and value-level references when marking aliases as referenced. The presence of a type-only import in the same statement seems to trigger incorrect reference marking for non-type imports. Compared with TypeScript source and found that tsgo is missing logic in markSymbolOfAliasDeclarationIfTypeOnly to check if alias targets resolve to type-only declarations.
Created testdata/repro-import-elision-bug/ with:
- provider.ts: Exports Value class and ValueData type
- broken.ts: Demonstrates bug with mixed type/non-type imports
- working.ts: Shows correct behavior without type import
- README.md: Instructions for reproducing the bug
The broken.ts file imports { Value, type ValueData } where Value
is only used in type positions, and incorrectly emits require().
Concise summary of the import elision bug investigation including: - Clear reproduction steps - Root cause analysis - Key findings and trigger conditions - Next steps for implementing a fix - References to relevant code and test files
Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
|
@copilot as you said, this is a partial fix. Think harder and figure out how to fully fix it. Cross compare against |
…ormers When type-only import specifiers (like `type ValueData`) are present, the TypeEraserTransformer modifies the import clause before the ImportElisionTransformer runs. This caused isElisionBlocked to return true, preventing proper elision of the import declaration. The fix ensures that: 1. The import clause is always visited first 2. If the import clause becomes nil (all bindings elided), the import declaration is elided regardless of isElisionBlocked 3. If the import clause changed, the import declaration is updated This makes tsgo match TypeScript's behavior, where imports only used in type positions are properly elided. Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
Fixed in e624128. The root cause was that |
The test case is covered by other existing submodule tests like importSpecifiers1.ts and nodeModulesImportAttributesModeDeclarationEmitErrors. Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
…check The conditional checks for isElisionBlocked and importClause comparison were unnecessary since we always want to return the UpdateImportDeclaration result after the importClause nil check. Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Fixes incorrect import elision where tsgo could emit runtime require(...) calls for imports that are only used in type positions, aligning emitted output more closely with the TypeScript reference baselines.
Changes:
- Update
ImportElisionTransformerto dropImportDeclarations when the visitedImportClauseis fully elided, preventing conversion into side-effect-only imports. - Elide
type-onlyImportSpecifiers directly inImportElisionTransformer. - Update reference baselines and remove now-unnecessary
.diffbaselines wheretsgooutput converged with the reference behavior.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/transformers/tstransforms/importelision.go | Changes import elision behavior for ImportDeclaration and ImportSpecifier. |
| testdata/baselines/reference/submodule/conformance/nodeModulesImportModeDeclarationEmitErrors1(module=nodenext).js.diff | Removed baseline-diff file now that output matches reference. |
| testdata/baselines/reference/submodule/conformance/nodeModulesImportModeDeclarationEmitErrors1(module=nodenext).js | Updates expected emitted JS to no longer include a require("pkg"). |
| testdata/baselines/reference/submodule/conformance/nodeModulesImportModeDeclarationEmitErrors1(module=node20).js.diff | Removed baseline-diff file now that output matches reference. |
| testdata/baselines/reference/submodule/conformance/nodeModulesImportModeDeclarationEmitErrors1(module=node20).js | Updates expected emitted JS to no longer include a require("pkg"). |
| testdata/baselines/reference/submodule/conformance/nodeModulesImportModeDeclarationEmitErrors1(module=node18).js.diff | Removed baseline-diff file now that output matches reference. |
| testdata/baselines/reference/submodule/conformance/nodeModulesImportModeDeclarationEmitErrors1(module=node18).js | Updates expected emitted JS to no longer include a require("pkg"). |
| testdata/baselines/reference/submodule/conformance/nodeModulesImportModeDeclarationEmitErrors1(module=node16).js.diff | Removed baseline-diff file now that output matches reference. |
| testdata/baselines/reference/submodule/conformance/nodeModulesImportModeDeclarationEmitErrors1(module=node16).js | Updates expected emitted JS to no longer include a require("pkg"). |
| testdata/baselines/reference/submodule/conformance/nodeModulesImportAttributesModeDeclarationEmitErrors(module=nodenext).js.diff | Removed baseline-diff file now that output matches reference. |
| testdata/baselines/reference/submodule/conformance/nodeModulesImportAttributesModeDeclarationEmitErrors(module=nodenext).js | Updates expected emitted JS to no longer include a require("pkg"). |
| testdata/baselines/reference/submodule/conformance/nodeModulesImportAttributesModeDeclarationEmitErrors(module=node20).js.diff | Removed baseline-diff file now that output matches reference. |
| testdata/baselines/reference/submodule/conformance/nodeModulesImportAttributesModeDeclarationEmitErrors(module=node20).js | Updates expected emitted JS to no longer include a require("pkg"). |
| testdata/baselines/reference/submodule/conformance/nodeModulesImportAttributesModeDeclarationEmitErrors(module=node18).js.diff | Removed baseline-diff file now that output matches reference. |
| testdata/baselines/reference/submodule/conformance/nodeModulesImportAttributesModeDeclarationEmitErrors(module=node18).js | Updates expected emitted JS to no longer include a require("pkg"). |
| testdata/baselines/reference/submodule/conformance/nodeModulesImportAttributesModeDeclarationEmitErrors(module=node16).js.diff | Removed baseline-diff file now that output matches reference. |
| testdata/baselines/reference/submodule/conformance/nodeModulesImportAttributesModeDeclarationEmitErrors(module=node16).js | Updates expected emitted JS to no longer include a require("pkg"). |
| testdata/baselines/reference/submodule/conformance/importSpecifiers1.js.diff | Removed baseline-diff file now that output matches reference. |
| testdata/baselines/reference/submodule/conformance/importSpecifiers1.js | Updates expected emitted JS for d.js to export {};. |
| case ast.KindImportDeclaration: | ||
| if !tx.isElisionBlocked(node) { | ||
| n := node.AsImportDeclaration() | ||
| // Do not elide a side-effect only import declaration. | ||
| // import "foo"; | ||
| if n.ImportClause != nil { | ||
| importClause := tx.Visitor().VisitNode(n.ImportClause) | ||
| if importClause == nil { | ||
| return nil | ||
| } | ||
| return tx.Factory().UpdateImportDeclaration(n, n.Modifiers(), importClause, n.ModuleSpecifier, tx.Visitor().VisitNode(n.Attributes)) | ||
| n := node.AsImportDeclaration() | ||
| // Do not elide a side-effect only import declaration. | ||
| // import "foo"; | ||
| if n.ImportClause != nil { | ||
| importClause := tx.Visitor().VisitNode(n.ImportClause) | ||
| if importClause == nil { | ||
| return nil | ||
| } | ||
| return tx.Factory().UpdateImportDeclaration(n, n.Modifiers(), importClause, n.ModuleSpecifier, tx.Visitor().VisitNode(n.Attributes)) | ||
| } | ||
| return tx.Visitor().VisitEachChild(node) |
Copilot
AI
Feb 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR description says the fix is to elide imports even when isElisionBlocked is true only when the visited ImportClause becomes nil, but the implementation removes the isElisionBlocked gate for all ImportDeclarations. This changes behavior for any import declaration that was modified earlier in the pipeline (including by custom before transforms) and may re-enable elision in cases the isElisionBlocked contract is meant to prevent. Consider restoring the isElisionBlocked check for the general case and only bypassing it for the specific scenario where the visited ImportClause becomes nil (or adjust isElisionBlocked to treat TypeEraser updates as non-blocking).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR description was not updated to reflect the later changes, unfortunately
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isElisionBlocked seems to only be needed for custom transformers, which we don't have, too.
|
I investigated this more and figured out that the problem is that the old TS transform has been split into a type eraser and an import elider. And so, the "fix" is actually the removal of the The extra bit is that both contain basically the same code for erasing imports, which makes me think that we shouldn't have a separate import elider at all. |
|
Sent #2673 instead |
tsgoemits arequirestatement when a value is imported without thetypekeyword but is only used in type-level positionsisElisionBlockedreturns true when TypeEraserTransformer modifies import clauseimportelision.goto elide imports even when isElisionBlocked is true if import clause becomes nilOriginal prompt
requirestatement when a value is imported without thetypekeyword but only used in type-level positions #2636💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.