-
Notifications
You must be signed in to change notification settings - Fork 13.2k
Fix: Consult referenced project options for synthetic default export eligibility #63038
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: andrewbranch <3277153+andrewbranch@users.noreply.github.com>
Co-authored-by: andrewbranch <3277153+andrewbranch@users.noreply.github.com>
Co-authored-by: andrewbranch <3277153+andrewbranch@users.noreply.github.com>
|
|
||
| // Test case for synthetic default imports from declaration files. | ||
| // Without project references, .d.ts files allow synthetic defaults due to ambiguity. | ||
| // With project references (tested manually), the referenced project's options determine |
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.
Manual testing is absolutely not sufficient. Delete this test and make one that actually reproduces the bug.
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.
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.
Replaced the fourslash test with a proper tsc test that reproduces the bug through project references. The new test referenced project with esnext module disallows synthetic default imports creates a minimal reproduction with a lib project (module: esnext, named exports only) and an app project that references it and attempts a default import. The baseline correctly shows error TS1192. (commit 4e968bb)
Co-authored-by: andrewbranch <3277153+andrewbranch@users.noreply.github.com>
Co-authored-by: andrewbranch <3277153+andrewbranch@users.noreply.github.com>
andrewbranch
left a comment
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.
At the top of the function, there's
const usageMode = file && getEmitSyntaxForModuleSpecifierExpression(usage);
if (file && usageMode !== undefined) {
const targetMode = host.getImpliedNodeFormatForEmit(file);The host methods called from there should already be looking up the project reference redirect via getCompilerOptionsForFile in program.ts, so I'm actually wondering why this wasn't already doing the trick. I think your added condition of referencedModuleKind >= ModuleKind.ES2015 is insufficient because in --module nodenext there are more complex conditions that determine the output format of files, which made me realize this possibly should have already been handled above. Please debug and investigate what's happening in those calls when we're processing the referenced project's declaration file.
src/compiler/checker.ts
Outdated
| } | ||
| // If the referenced project's module format is ESM (ES2015 or later), | ||
| // it cannot have a synthetic default | ||
| const referencedModuleKind = referencedOptions.module ?? ModuleKind.CommonJS; |
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.
@copilot getEmitModuleKind(referencedOptions)
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.
Changed to use getEmitModuleKind(referencedOptions) instead of directly accessing referencedOptions.module. This properly computes the effective module kind accounting for defaults and edge cases. (commit d392ebd)
Co-authored-by: andrewbranch <3277153+andrewbranch@users.noreply.github.com>
Investigated the early-return logic at lines 3786-3796. The issue is that |
|
@copilot there's a related function getEmitModuleFormatOfFile which is similar to getImpliedNodeFormatForEmit but falls back to getEmitModuleKind rather than returning undefined/none outside of node16/nodenext. I don't recall if that function is currently on CheckerHost but it should be easy to wire up if not. If we replace getImpliedNodeFormatForEmit with that function, perhaps the block of code you added will be superfluous? |
Co-authored-by: andrewbranch <3277153+andrewbranch@users.noreply.github.com>
Replaced with |
|
@copilot I meant on line 3787 |
…n files Co-authored-by: andrewbranch <3277153+andrewbranch@users.noreply.github.com>
Kept |
|
@copilot what happens when you replace line 3787 and delete the rest of your added code? If that's wrong explain to me what's going wrong and why |
Replacing line 3787 with |
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
This PR fixes an issue where TypeScript doesn't properly check default imports from project reference declaration files. When importing from a project reference with ESM module format, TypeScript should disallow synthetic default imports if the imported module only has named exports.
Changes:
- Added logic in
canHaveSyntheticDefaultto check project reference options when determining module format for declaration files - Added a new test case to verify the fix works for project references with ESNext module
- Updated baselines to reflect the new errors for project reference imports
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/compiler/checker.ts | Added lines 3797-3809 to check referenced project options and disallow synthetic defaults for ESM project references |
| src/testRunner/unittests/tsc/projectReferences.ts | Added test case "referenced project with esnext module disallows synthetic default imports" |
| tests/baselines/reference/tsc/projectReferences/referenced-project-with-esnext-module-disallows-synthetic-default-imports.js | New baseline showing the expected error for default import from ESM project reference |
| tests/baselines/reference/tsc/projectReferences/default-import-interop-uses-referenced-project-settings.js | Updated baseline to include additional errors for project reference imports (lines 89-90) |
src/compiler/checker.ts
Outdated
| // This is a declaration file from a project reference, so we can determine | ||
| // its module format from the referenced project's options | ||
| const targetModuleKind = host.getEmitModuleFormatOfFile(file); | ||
| if (usageMode === ModuleKind.ESNext && targetModuleKind >= ModuleKind.ES2015) { |
Copilot
AI
Jan 22, 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 condition should use a range check to only include ES module kinds (ES2015, ES2020, ES2022, ESNext) and explicitly exclude Node16+ and Preserve. The current check targetModuleKind >= ModuleKind.ES2015 would incorrectly include Node16 (100), Node18 (101), Node20 (102), NodeNext (199), and Preserve (200).
The condition should be changed to:
if (usageMode === ModuleKind.ESNext && ModuleKind.ES2015 <= targetModuleKind && targetModuleKind <= ModuleKind.ESNext)This pattern is used elsewhere in the codebase (e.g., line 48656 of checker.ts) to specifically check for ES module kinds.
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.
Changed the condition to use the proper range check: ModuleKind.ES2015 <= targetModuleKind && targetModuleKind <= ModuleKind.ESNext. This excludes Node16+ (100-199) and Preserve (200) which have higher numeric values but aren't pure ES modules. (commit d7ac3ad)
src/compiler/checker.ts
Outdated
| const redirect = host.getRedirectFromSourceFile(file.path); | ||
| if (redirect) { |
Copilot
AI
Jan 22, 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.
For declaration files from project references, consider using getRedirectFromOutput instead of getRedirectFromSourceFile. The function getRedirectFromSourceFile maps source file paths to project references, while getRedirectFromOutput maps output file paths to project references. Since this code block handles declaration files (outputs), using getRedirectFromOutput(file.path)?.resolvedRef would be more semantically correct and consistent with the pattern in getRedirectReferenceForResolution (program.ts:2112).
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.
@copilot investigate whether this suggestion is correct, and apply the other suggestion above (#63038 (comment))
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.
Investigated and found that getRedirectFromOutput alone doesn't work for all test cases. Using the pattern from getRedirectReferenceForResolution, I now try both getRedirectFromSourceFile || getRedirectFromOutput since declaration files can be mapped either way depending on resolution context. This ensures the check works regardless of how the file was resolved. (commit d7ac3ad)
Co-authored-by: andrewbranch <3277153+andrewbranch@users.noreply.github.com>
Fix for Referenced Project Options Not Consulted for Synthetic Default Export Eligibility
✅ All steps completed:
getRedirectFromSourceFileandgetRedirectFromOutputfor robustnessSummary
This PR fixes an issue where TypeScript doesn't check default imports from project reference
.d.tsfiles.Changes
Modified
canHaveSyntheticDefaultinsrc/compiler/checker.ts:getImpliedNodeFormatForEmitlogic for node16/nodenext (lines 3787-3795)getImpliedNodeFormatForEmitreturns undefinedgetRedirectFromSourceFileandgetRedirectFromOutputto find project referenceshost.getEmitModuleFormatOfFile(file)to get the referenced project's module formatES2015 <= kind <= ESNext)Key improvements:
Correct ES module range check: Changed from
>= ModuleKind.ES2015toModuleKind.ES2015 <= targetModuleKind && targetModuleKind <= ModuleKind.ESNextto exclude Node16+ (100-199) and Preserve (200) which have higher numeric values but aren't pure ES modules.Robust redirect lookup: Uses
getRedirectFromSourceFile || getRedirectFromOutputsince declaration files can be mapped either way depending on resolution context.Added test:
referenced project with esnext module disallows synthetic default importsvalidates the fix.Updated baselines: Tests correctly show errors for project references while keeping node_modules permissive.
Impact
Project references with ES modules get correct diagnostics. Node_modules packages remain permissive.
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.