-
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
Conversation
Co-authored-by: andrewbranch <[email protected]>
Co-authored-by: andrewbranch <[email protected]>
Co-authored-by: andrewbranch <[email protected]>
tests/cases/fourslash/syntheticDefaultExportWithProjectReferences.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: andrewbranch <[email protected]>
Co-authored-by: andrewbranch <[email protected]>
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.
Co-authored-by: andrewbranch <[email protected]>
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 <[email protected]>
Replaced with |
|
@copilot I meant on line 3787 |
…n files Co-authored-by: andrewbranch <[email protected]>
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
| 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)
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.
Test coverage now proves this works the same whether you try to import the input .ts file or output .d.ts file of the referenced project, so current implementation seems good
Co-authored-by: andrewbranch <[email protected]>
|
@typescript-bot test top800 |
| app/src/index.ts(4,28): error TS1192: Module '"/home/src/workspaces/project/lib/dist/a"' has no default export. | ||
| app/src/index.ts(5,28): error TS1192: Module '"/home/src/workspaces/project/lib/dist/a"' has no default export. |
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 presence of these errors was actually always asserted in the test content (lines 66–67 in the baseline). Not sure if this used to work and regressed without noticing or what, but this baseline change matches the original intention.
|
@andrewbranch Here are the results of running the top 800 repos with tsc comparing Everything looks good! |
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.