Only export @typedef type aliases in modules
#1999
Conversation
ahejlsberg
requested review from
Copilot,
jakebailey and
sandersn
and removed request for
Copilot
| // === /b.js === | ||
| // /** @type {import('./a').[|Foo|]} */ | ||
| // const x = 0; | ||
|
|
There was a problem hiding this comment.
This seems like a regression, maybe? a.js theoretically is a module because it has module.exports in it, I think, but now we are losing that export and therefore the reference.
There was a problem hiding this comment.
Yeah, that's an oversight. We need to handle the implied export of @typedef in find-all-references.
| @@ -10,13 +10,12 @@ | |||
| function funky(declaration) { | |||
| return false; | |||
| } | |||
| +export = donkey; | |||
| module.exports = donkey; | |||
| +export var funky = funky; | |||
There was a problem hiding this comment.
Not your bug but definitely an interesting one.
There was a problem hiding this comment.
This is actually a bug fix I made part of this PR. We now mark reparsed export assignment nodes as a TypeScript construct such that they're properly omitted from the emitted .js.
There was a problem hiding this comment.
Yes, right, I was just commenting on the remaining diff in the file.
There was a problem hiding this comment.
Got it. Definitely funky!
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the handling of JSDoc @typedef declarations in CommonJS/external modules. Previously, @typedef declarations were being marked as exported during parsing. The fix moves this logic to later stages (declaration emit and binder), ensuring proper scoping and type visibility.
Key Changes
- Removed premature
exportmodifier addition during parsing of@typedeftags - Added logic in declaration transformer to mark
@typedefas exported when in external modules - Updated binder, checker, and language server to recognize
@typedefas implicitly exported in modules - Fixed export assignment to properly flag TypeScript subtree facts
Reviewed Changes
Copilot reviewed 155 out of 155 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/parser/reparser.go | Removes explicit export modifier from @typedef parsing |
| internal/transformers/declarations/transform.go | Adds export flag for @typedef in external modules during emission |
| internal/binder/binder.go | Treats @typedef as having export modifier in modules |
| internal/checker/emitresolver.go | Makes @typedef visible in external modules |
| internal/checker/checker.go | Adds @typedef to allowed class member kinds |
| internal/ls/importTracker.go | Updates export detection to handle @typedef |
| internal/ast/ast.go | Fixes ExportAssignment subtree facts and adds @typedef to write access declarations |
| testdata/baselines/* | Updated test baselines showing corrected behavior |
| modifiers := p.newModifierList(export.Loc, p.nodeSlicePool.NewSlice1(export)) | ||
|
|
||
| typeAlias := p.factory.NewJSTypeAliasDeclaration(modifiers, p.factory.DeepCloneReparse(tag.AsJSDocTypedefTag().Name()), nil, nil) | ||
| typeAlias := p.factory.NewJSTypeAliasDeclaration(nil, p.factory.DeepCloneReparse(tag.AsJSDocTypedefTag().Name()), nil, nil) |
There was a problem hiding this comment.
According to custom coding guideline 1000000, the comment on line 73 states '!!! Don't mark typedefs as exported if they are not in a module'. However, the fix now relies on later stages to determine if the typedef is in a module. Consider updating this comment to clarify that export modifiers are added during declaration transformation based on module context, not during parsing.
| if ast.IsJSTypeAliasDeclaration(node) && parentIsFile && ast.IsExternalOrCommonJSModule(node.Parent.AsSourceFile()) { | ||
| additions |= ast.ModifierFlagsExport | ||
| } | ||
| return maskModifierFlags(tx.host, node, mask, additions) | ||
| } | ||
|
|
There was a problem hiding this comment.
This logic for determining whether a JSTypeAliasDeclaration should be exported is duplicated in multiple locations (binder.go line 383, emitresolver.go line 164, importTracker.go line 575). Consider extracting this into a shared helper function to improve maintainability and ensure consistency.
| if ast.IsJSTypeAliasDeclaration(node) && parentIsFile && ast.IsExternalOrCommonJSModule(node.Parent.AsSourceFile()) { | |
| additions |= ast.ModifierFlagsExport | |
| } | |
| return maskModifierFlags(tx.host, node, mask, additions) | |
| } | |
| if shouldExportJSTypeAliasDeclaration(node) { | |
| additions |= ast.ModifierFlagsExport | |
| } | |
| return maskModifierFlags(tx.host, node, mask, additions) | |
| } |
There was a problem hiding this comment.
Is this worth addressing if we are actually copy and pasting this code?
testdata/baselines/reference/submodule/conformance/jsDeclarationsTypedefFunction.js
Show resolved
Hide resolved
testdata/baselines/reference/submodule/conformance/jsdocTemplateTagDefault.js
Show resolved
Hide resolved
testdata/baselines/reference/submodule/conformance/jsdocTemplateTagNameResolution.js
Show resolved
Hide resolved
In this PR:
@typedeftype aliases when they occur in modules (i.e. don't export in script files).ast.KindJSTypeAliasDeclaration.Most of the baseline changes result from not marking script files as modules and no longer including invalid
export =in emitted .js files.Fixes #1988.