Fix crash: JSTypeAliasDeclaration passed to GetTypeOfDeclaration during CJS declaration emit - #4663
Conversation
…erty When a @typedef and a module.exports.Property = ... assignment share the same symbol name, symbol.Declarations contains both a JSTypeAliasDeclaration and a BinaryExpression. The first-in-wins type annotation lookup in serializeTypeForDeclaration would find the JSTypeAliasDeclaration (because it has a non-nil Type()), then call GetTypeOfDeclaration on it, which panics since JSTypeAliasDeclaration is not an inferrable node. Fix: additionally filter the lookup to only include declarations for which HasInferredType is true, ensuring only inferrable nodes are passed to GetTypeOfDeclaration. Co-authored-by: RyanCavanaugh <6685088+RyanCavanaugh@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Prevents a CJS declaration-emit panic caused by merged type and value declarations.
Changes:
- Restricts fallback type inference to inferrable declarations.
- Adds regression coverage and compiler baselines.
Show a summary per file
| File | Description |
|---|---|
internal/checker/nodebuilderimpl.go |
Filters unsafe fallback declarations. |
testdata/tests/cases/compiler/jsTypedefMergedWithModuleExportProperty.ts |
Adds the crash regression case. |
testdata/baselines/reference/compiler/jsTypedefMergedWithModuleExportProperty.types |
Records inferred types. |
testdata/baselines/reference/compiler/jsTypedefMergedWithModuleExportProperty.symbols |
Records merged symbols. |
testdata/baselines/reference/compiler/jsTypedefMergedWithModuleExportProperty.js |
Records JS and declaration output. |
testdata/baselines/reference/compiler/jsTypedefMergedWithModuleExportProperty.errors.txt |
Records expected diagnostics. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0
- Review effort level: Medium
weswigham
left a comment
There was a problem hiding this comment.
@copilot IMO, this is a bug in hasTypeAnnotation - likely because of it relying solely on .Type() which is overbroad - it shouldn't be returning true for typedefs or similar constructs. Fix it at the root instead of patching it just here. Use getEffectiveTypeAnnotationNode in strada as a reference (even though we don't need the JSDoc lookup parts anymore) - it bails on type aliases for this reason.
…Declaration Per reviewer feedback, fix the issue at the root in hasTypeAnnotation rather than patching the call site. Type alias declarations have a .Type() that is their type definition, not a type annotation on a value - mirroring getEffectiveTypeAnnotationNode in TypeScript which explicitly bails on isTypeAliasDeclaration. This allows the call site in serializeTypeForDeclaration to revert to the simpler hasTypeAnnotation predicate without needing the &&ast.HasInferredType(d) guard. Co-authored-by: weswigham <2932786+weswigham@users.noreply.github.com>
Fixed in commit fb16199. |
When a
@typedefand amodule.exports.Property = valueassignment share the same symbol name, TypeScript merges both intosymbol.Declarations— oneJSTypeAliasDeclaration(type) and oneBinaryExpression(value). During declaration emit, the "first-in-wins" fallback inserializeTypeForDeclarationsearched for any declaration with a non-nilType(), which matched theJSTypeAliasDeclaration. CallingGetTypeOfDeclarationon it panicked since it only handles inferrable (value) nodes.Repro:
Changes
internal/checker/nodebuilderimpl.go: Guard the fallback declaration search withast.HasInferredType(d)so only inferrable (value) declarations are candidates forGetTypeOfDeclaration, filtering outJSTypeAliasDeclarationand any other non-inferrable kinds.testdata/tests/cases/compiler/jsTypedefMergedWithModuleExportProperty.ts: Regression test covering this exact pattern.webpack#4607