Fix keyof deferred for non-generic substitution types (#2186)#3728
Fix keyof deferred for non-generic substitution types (#2186)#3728adavila0703 wants to merge 3 commits intomicrosoft:mainfrom
Conversation
When getIndexTypeEx sees a substitution type whose baseType and constraint are both non-generic, compute keyof as the union of keyof on each branch instead of deferring via shouldDeferIndexType (substitution types are instantiable non-primitives). Ports the behavior from microsoft/TypeScript#61999 (microsoft/TypeScript#61728). Adds compiler baseline tests substitutionTypeNonGenericIndexType1 and substitutionTypeNonGenericIndexType2 under testdata/tests/cases/compiler/.
@microsoft-github-policy-service agree |
There was a problem hiding this comment.
Pull request overview
Fixes keyof/index-type computation for substitution types in the Go port of the TypeScript checker. Previously, getIndexTypeEx would incorrectly defer keyof for some substitution types (even when both baseType and constraint are non-generic), which broke indexed-access assignability and produced unsimplified quick info; this aligns behavior with upstream TypeScript’s fix for microsoft/TypeScript#61999 / #61728.
Changes:
- Resolve
keyofimmediately for non-generic substitution types by returning the union of index types from the substitution’sbaseTypeandconstraint. - Add two local compiler test cases covering the regression scenarios (quick info simplification and assignability).
- Commit new
.typesand.symbolsbaselines for the added tests.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/checker/checker.go | Updates getIndexTypeEx to avoid deferring keyof for non-generic substitution types and compute a concrete index-type union. |
| testdata/tests/cases/compiler/substitutionTypeNonGenericIndexType1.ts | Regression test for quick info / simplification behavior involving substitution types. |
| testdata/tests/cases/compiler/substitutionTypeNonGenericIndexType2.ts | Regression test for assignability of indexed access through a conditional/substitution scenario. |
| testdata/baselines/reference/compiler/substitutionTypeNonGenericIndexType1.types | Baseline for type display output for test case 1. |
| testdata/baselines/reference/compiler/substitutionTypeNonGenericIndexType1.symbols | Baseline for symbol output for test case 1. |
| testdata/baselines/reference/compiler/substitutionTypeNonGenericIndexType2.types | Baseline for type display output for test case 2. |
| testdata/baselines/reference/compiler/substitutionTypeNonGenericIndexType2.symbols | Baseline for symbol output for test case 2. |
| if t.flags&TypeFlagsSubstitution != 0 { | ||
| st := t.AsSubstitutionType() | ||
| if !c.isGenericType(st.baseType) && !c.isGenericType(st.constraint) { | ||
| return c.getUnionType([]*Type{c.getIndexTypeEx(st.baseType, indexFlags), c.getIndexTypeEx(st.constraint, indexFlags)}) | ||
| } |
Summary
getIndexTypeExwas deferringkeyoffor substitution types whenshouldDeferIndexTypeapplied. For substitution types whosebaseTypeandconstraintare both non-generic, that deferral is incorrect, it breaks assignability for indexed access types likeTest["rejectClose"]and leaves quick info unsimplified.This change resolves
keyofimmediately for non-generic substitution types by returning the union ofgetIndexTypeExon the substitution'sbaseTypeandconstraint.This matches the behavior from microsoft/TypeScript#61999 for microsoft/TypeScript#61728.
NoInfersubstitution types are still handled first, unchanged.Tests
substitutionTypeNonGenericIndexType1.tsandsubstitutionTypeNonGenericIndexType2.tsundertestdata/tests/cases/compiler/with committed.symbolsand.typesbaselines.go test ./internal/testrunner/ -run "TestLocal/substitutionTypeNonGenericIndexType" -count=1Notes
I noticed #2186 is currently in the Post-7.0 milestone. I am submitting this as a checker bug fix / parity change with upstream TypeScript behavior rather than a new feature, but I am happy to defer or retarget if you prefer to keep this for after 7.0.