Fix typedef binding with CJS exports=
#826
Conversation
With this PR, top-level typedefs in file with commonjs exports
(`module.exports=x` or `exports=x`) are no longer added as exports of
the file. They're added as exports of whatever is `export=`.
The big change is that, in Strada, non-top-level typedefs are also
exported. But it never made sense for these two scoped type aliases to
be exported, let alone *both*:
```js
function one() {
/** @typedef {string} T */
/** @type {T} */
var s = 's'
}
function two() {
/** @typedef {number} T */
/** @type {T} */
var n = 1
}
/* @type {T} */
var error = "I AM ERROR"
```
I'm not sure that I declared the local and export='d symbols in the
right way, so I'd appreciate expert opinions there.
There was a problem hiding this comment.
Pull Request Overview
This pull request updates the typedef binding behavior in CommonJS modules so that only top-level typedefs are exported via the export= assignment, correcting unintended exports of scoped typedefs. Key changes include updating baseline error files to reflect the new behavior and modifying binder.go to delay binding of JSTypeAliasDeclarations when the file is a CommonJS module.
Reviewed Changes
Copilot reviewed 44 out of 55 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| testdata/baselines/reference/submodule/...errors.txt | Updated error messages and counts reflecting removal of extra export assignment issues |
| internal/binder/binder.go | Introduced delayed binding for JSTypeAliasDeclarations and added delayedBindJSDocTypedefTag to bind typedefs conditionally |
Files not reviewed (11)
- testdata/baselines/reference/submodule/compiler/checkJsTypeDefNoUnusedLocalMarked.errors.txt: Language not supported
- testdata/baselines/reference/submodule/conformance/importingExportingTypes.symbols: Language not supported
- testdata/baselines/reference/submodule/conformance/importingExportingTypes.symbols.diff: Language not supported
- testdata/baselines/reference/submodule/conformance/jsDeclarationsImportAliasExposedWithinNamespace.symbols: Language not supported
- testdata/baselines/reference/submodule/conformance/jsDeclarationsImportAliasExposedWithinNamespace.symbols.diff: Language not supported
- testdata/baselines/reference/submodule/conformance/jsDeclarationsImportAliasExposedWithinNamespaceCjs.symbols: Language not supported
- testdata/baselines/reference/submodule/conformance/jsDeclarationsImportAliasExposedWithinNamespaceCjs.symbols.diff: Language not supported
- testdata/baselines/reference/submodule/conformance/jsDeclarationsImportTypeBundled.errors.txt: Language not supported
- testdata/baselines/reference/submodule/conformance/jsDeclarationsTypeAliases.errors.txt: Language not supported
- testdata/baselines/reference/submodule/conformance/jsDeclarationsTypedefAndLatebound.errors.txt: Language not supported
- testdata/baselines/reference/submodule/conformance/jsDeclarationsTypedefPropertyAndExportAssignment.types: Language not supported
| if b.file.Symbol != nil { | ||
| if exportEq := b.file.Symbol.Exports[ast.InternalSymbolNameExportEquals]; exportEq != nil && b.file.CommonJSModuleIndicator != nil { | ||
| for _, node := range b.delayedTypeAliases { | ||
| b.declareSymbol(ast.GetSymbolTable(&exportEq.Exports), exportEq /*parent*/, node, ast.SymbolFlagsTypeAlias, ast.SymbolFlagsTypeAliasExcludes) |
There was a problem hiding this comment.
This works for one level of indirection to a local namespace-y thing, but what about an aliased namespacey thing? Eg
const mod = require("./mod.js")
/**
* @typedef {string} T
*/
module.exports = modor, abusing a ts construct to do it locally,
namespace ns {
export namespace inner {}
}
import mod = ns.inner
/**
* @typedef {string} T
*/
module.exports = modmod's symbol will be an alias, and this will just patch the typedefs onto the alias symbol, and not the alias symbol's ultimate target, which, dollars to donuts, means it's going to effectively go missing without extra lookup logic in the checker, similar to what we used to have. 🥹
There was a problem hiding this comment.
Even simpler, this also reproduces the problem.
function f() { }
/** @typedef {string} T */
module.exports = f
| if namespace == nil || ast.NodeIsMissing(right) { | ||
| return nil | ||
| var immediate *ast.Symbol | ||
| alias := c.resolveEntityName(left, ast.SymbolFlagsAlias, true /*ignoreErrors*/, true /*dontResolveAlias*/, location) |
There was a problem hiding this comment.
Hm. So if the resolution of the LHS of a qualified name fails, we try to look it up with only one level of aliasing? This seems... weird? While this might work in some scenarios, if getExportsOfSymbol doesn't return the merged in aliases, declaration emit is going to struggle to figure out how to name these typedefs, as traversing the exports is how symbols are named. Basically, we'd like to interpret
/**
* @typedef {number} Foo
*/
module.exports = name
as
export * from name // even though this isn't a thing in ESM (yet)
export type Foo = numberI think if we want to handle this in a non-jank way, we need to bind a module.exports = name in a file with typedefs not as an alias but instead as a (new?) namespacey symbol kind that has logic in getExportsOfSymbol to merge together the typedef set and the resolved, late-bound alias's set. (Once again, a callback to the .cjsSymbolMerged logic we used to have)
I'm also honestly a lil unsure what a syntactically driven declaration emit for this is.
There was a problem hiding this comment.
Maybe we just add support for parsing/checking export * from name in declaration files and actually do that transform on reparse? In theory it'll be added to esm alongside the module declarations proposal, probably.
With this PR, top-level typedefs in file with commonjs exports (
module.exports=xorexports=x) are no longer added as exports of the file. They're added as exports of whatever isexport=. I'm not sure that I declared the local and export='d symbols in the right way, so I'd appreciate expert opinions there.The big change compared to Strada is that only top-level typedefs are exported. But it never made sense for these two scoped type aliases to be exported, let alone both:
It's especially weird that a scoped alias would be inaccessible at the top-level, but then accessible in a different file.