Improve recursion identity for direct type instantiations #3445
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -839,12 +839,14 @@ func getRecursionIdentity(t *Type) RecursionId { | |
| // unique AST node. | ||
| return asRecursionId(t.AsTypeReference().node) | ||
| } | ||
| if t.symbol != nil && !(t.objectFlags&ObjectFlagsAnonymous != 0 && t.symbol.Flags&ast.SymbolFlagsClass != 0) && t.objectFlags&ObjectFlagsFromTypeNode == 0 { | ||
| // We track object types that have a symbol by that symbol (representing the origin of the type), but | ||
| // exclude the static sides of classes (since they share their symbols with the instance sides) and type | ||
| // references that originate in resolution of AST type nodes (since such type nodes cannot be the source | ||
| // of generative recursion without first being instantiated). | ||
|
Comment on lines
+842
to
+846
There was a problem hiding this comment. This change adjusts recursion identity handling to fix a checker soundness gap, but the PR doesn't add a regression test for #3426 (nested arrays from distinct namespaces should now produce TS2322). Please add a minimal compiler test case under |
||
| return asRecursionId(t.symbol) | ||
| } | ||
| if isTupleType(t) && t.objectFlags&ObjectFlagsFromTypeNode == 0 { | ||
| return asRecursionId(t.Target()) | ||
| } | ||
| } | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| deeplyNestedArrayTypes.ts(34,5): error TS2322: Type 'B.Outer' is not assignable to type 'A.Outer'. | ||
| Types of property 'inners' are incompatible. | ||
| Type 'B.Inner[]' is not assignable to type 'A.Inner[]'. | ||
| Type 'B.Inner' is not assignable to type 'A.Inner'. | ||
| Types of property 'mids' are incompatible. | ||
| Type 'B.Mid[]' is not assignable to type 'A.Mid[]'. | ||
| Type 'B.Mid' is not assignable to type 'A.Mid'. | ||
| Types of property 'leaves' are incompatible. | ||
| Type 'B.Leaf[]' is not assignable to type 'A.Leaf[]'. | ||
| Type 'B.Leaf' is not assignable to type 'A.Leaf'. | ||
| Types of property 'id' are incompatible. | ||
| Type 'number' is not assignable to type 'string'. | ||
|
|
||
|
|
||
| ==== deeplyNestedArrayTypes.ts (1 errors) ==== | ||
| // https://github.com/microsoft/typescript-go/issues/3426 | ||
|
|
||
| namespace A { | ||
| export type Outer = { | ||
| inners: Inner[]; | ||
| } | ||
| export type Inner = { | ||
| mids: Mid[]; | ||
| } | ||
| export type Mid = { | ||
| leaves: Leaf[]; | ||
| } | ||
| export type Leaf = { | ||
| id: string; | ||
| } | ||
| } | ||
|
|
||
| namespace B { | ||
| export type Outer = { | ||
| inners: Inner[]; | ||
| } | ||
| export type Inner = { | ||
| mids: Mid[]; | ||
| } | ||
| export type Mid = { | ||
| leaves: Leaf[]; | ||
| } | ||
| export type Leaf = { | ||
| id: number; | ||
| } | ||
| } | ||
|
|
||
| function test(a: A.Outer, b: B.Outer) { | ||
| a = b | ||
| ~ | ||
| !!! error TS2322: Type 'B.Outer' is not assignable to type 'A.Outer'. | ||
| !!! error TS2322: Types of property 'inners' are incompatible. | ||
| !!! error TS2322: Type 'B.Inner[]' is not assignable to type 'A.Inner[]'. | ||
| !!! error TS2322: Type 'B.Inner' is not assignable to type 'A.Inner'. | ||
| !!! error TS2322: Types of property 'mids' are incompatible. | ||
| !!! error TS2322: Type 'B.Mid[]' is not assignable to type 'A.Mid[]'. | ||
| !!! error TS2322: Type 'B.Mid' is not assignable to type 'A.Mid'. | ||
| !!! error TS2322: Types of property 'leaves' are incompatible. | ||
| !!! error TS2322: Type 'B.Leaf[]' is not assignable to type 'A.Leaf[]'. | ||
| !!! error TS2322: Type 'B.Leaf' is not assignable to type 'A.Leaf'. | ||
| !!! error TS2322: Types of property 'id' are incompatible. | ||
| !!! error TS2322: Type 'number' is not assignable to type 'string'. | ||
| } | ||
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,83 @@ | ||
| //// [tests/cases/compiler/deeplyNestedArrayTypes.ts] //// | ||
|
|
||
| === deeplyNestedArrayTypes.ts === | ||
| // https://github.com/microsoft/typescript-go/issues/3426 | ||
|
|
||
| namespace A { | ||
| >A : Symbol(A, Decl(deeplyNestedArrayTypes.ts, 0, 0)) | ||
|
|
||
| export type Outer = { | ||
| >Outer : Symbol(Outer, Decl(deeplyNestedArrayTypes.ts, 2, 13)) | ||
|
|
||
| inners: Inner[]; | ||
| >inners : Symbol(inners, Decl(deeplyNestedArrayTypes.ts, 3, 25)) | ||
| >Inner : Symbol(Inner, Decl(deeplyNestedArrayTypes.ts, 5, 5)) | ||
| } | ||
| export type Inner = { | ||
| >Inner : Symbol(Inner, Decl(deeplyNestedArrayTypes.ts, 5, 5)) | ||
|
|
||
| mids: Mid[]; | ||
| >mids : Symbol(mids, Decl(deeplyNestedArrayTypes.ts, 6, 25)) | ||
| >Mid : Symbol(Mid, Decl(deeplyNestedArrayTypes.ts, 8, 5)) | ||
| } | ||
| export type Mid = { | ||
| >Mid : Symbol(Mid, Decl(deeplyNestedArrayTypes.ts, 8, 5)) | ||
|
|
||
| leaves: Leaf[]; | ||
| >leaves : Symbol(leaves, Decl(deeplyNestedArrayTypes.ts, 9, 23)) | ||
| >Leaf : Symbol(Leaf, Decl(deeplyNestedArrayTypes.ts, 11, 5)) | ||
| } | ||
| export type Leaf = { | ||
| >Leaf : Symbol(Leaf, Decl(deeplyNestedArrayTypes.ts, 11, 5)) | ||
|
|
||
| id: string; | ||
| >id : Symbol(id, Decl(deeplyNestedArrayTypes.ts, 12, 24)) | ||
| } | ||
| } | ||
|
|
||
| namespace B { | ||
| >B : Symbol(B, Decl(deeplyNestedArrayTypes.ts, 15, 1)) | ||
|
|
||
| export type Outer = { | ||
| >Outer : Symbol(Outer, Decl(deeplyNestedArrayTypes.ts, 17, 13)) | ||
|
|
||
| inners: Inner[]; | ||
| >inners : Symbol(inners, Decl(deeplyNestedArrayTypes.ts, 18, 25)) | ||
| >Inner : Symbol(Inner, Decl(deeplyNestedArrayTypes.ts, 20, 5)) | ||
| } | ||
| export type Inner = { | ||
| >Inner : Symbol(Inner, Decl(deeplyNestedArrayTypes.ts, 20, 5)) | ||
|
|
||
| mids: Mid[]; | ||
| >mids : Symbol(mids, Decl(deeplyNestedArrayTypes.ts, 21, 25)) | ||
| >Mid : Symbol(Mid, Decl(deeplyNestedArrayTypes.ts, 23, 5)) | ||
| } | ||
| export type Mid = { | ||
| >Mid : Symbol(Mid, Decl(deeplyNestedArrayTypes.ts, 23, 5)) | ||
|
|
||
| leaves: Leaf[]; | ||
| >leaves : Symbol(leaves, Decl(deeplyNestedArrayTypes.ts, 24, 23)) | ||
| >Leaf : Symbol(Leaf, Decl(deeplyNestedArrayTypes.ts, 26, 5)) | ||
| } | ||
| export type Leaf = { | ||
| >Leaf : Symbol(Leaf, Decl(deeplyNestedArrayTypes.ts, 26, 5)) | ||
|
|
||
| id: number; | ||
| >id : Symbol(id, Decl(deeplyNestedArrayTypes.ts, 27, 24)) | ||
| } | ||
| } | ||
|
|
||
| function test(a: A.Outer, b: B.Outer) { | ||
| >test : Symbol(test, Decl(deeplyNestedArrayTypes.ts, 30, 1)) | ||
| >a : Symbol(a, Decl(deeplyNestedArrayTypes.ts, 32, 14)) | ||
| >A : Symbol(A, Decl(deeplyNestedArrayTypes.ts, 0, 0)) | ||
| >Outer : Symbol(A.Outer, Decl(deeplyNestedArrayTypes.ts, 2, 13)) | ||
| >b : Symbol(b, Decl(deeplyNestedArrayTypes.ts, 32, 25)) | ||
| >B : Symbol(B, Decl(deeplyNestedArrayTypes.ts, 15, 1)) | ||
| >Outer : Symbol(B.Outer, Decl(deeplyNestedArrayTypes.ts, 17, 13)) | ||
|
|
||
| a = b | ||
| >a : Symbol(a, Decl(deeplyNestedArrayTypes.ts, 32, 14)) | ||
| >b : Symbol(b, Decl(deeplyNestedArrayTypes.ts, 32, 25)) | ||
| } | ||
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| //// [tests/cases/compiler/deeplyNestedArrayTypes.ts] //// | ||
|
|
||
| === deeplyNestedArrayTypes.ts === | ||
| // https://github.com/microsoft/typescript-go/issues/3426 | ||
|
|
||
| namespace A { | ||
| export type Outer = { | ||
| >Outer : Outer | ||
|
|
||
| inners: Inner[]; | ||
| >inners : Inner[] | ||
| } | ||
| export type Inner = { | ||
| >Inner : Inner | ||
|
|
||
| mids: Mid[]; | ||
| >mids : Mid[] | ||
| } | ||
| export type Mid = { | ||
| >Mid : Mid | ||
|
|
||
| leaves: Leaf[]; | ||
| >leaves : Leaf[] | ||
| } | ||
| export type Leaf = { | ||
| >Leaf : Leaf | ||
|
|
||
| id: string; | ||
| >id : string | ||
| } | ||
| } | ||
|
|
||
| namespace B { | ||
| export type Outer = { | ||
| >Outer : Outer | ||
|
|
||
| inners: Inner[]; | ||
| >inners : Inner[] | ||
| } | ||
| export type Inner = { | ||
| >Inner : Inner | ||
|
|
||
| mids: Mid[]; | ||
| >mids : Mid[] | ||
| } | ||
| export type Mid = { | ||
| >Mid : Mid | ||
|
|
||
| leaves: Leaf[]; | ||
| >leaves : Leaf[] | ||
| } | ||
| export type Leaf = { | ||
| >Leaf : Leaf | ||
|
|
||
| id: number; | ||
| >id : number | ||
| } | ||
| } | ||
|
|
||
| function test(a: A.Outer, b: B.Outer) { | ||
| >test : (a: A.Outer, b: B.Outer) => void | ||
| >a : A.Outer | ||
| >A : any | ||
| >b : B.Outer | ||
| >B : any | ||
|
|
||
| a = b | ||
| >a = b : B.Outer | ||
| >a : A.Outer | ||
| >b : B.Outer | ||
| } | ||
|
|
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
createTypeReferenceExcaches instantiations only bytypeArguments(getTypeListKey). That ignores theobjectFlagsparameter, so a previously-cached instantiation withoutObjectFlagsFromTypeNodecan be returned to callers that request it (and vice versa). This makes the new recursion-identity behavior depend on call order and can cause both missed recursion detection and missed errors. Consider including the relevantobjectFlagsbits (at leastObjectFlagsFromTypeNode) in the cache key and/or maintaining a separate instantiation cache forFromTypeNodereferences so the flag is applied deterministically only where intended.Uh oh!
There was an error while loading. Please reload this page.
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.
This is a good observation and indeed something that was considered. The issue with tracking the flag in the cache key is that it creates distinct object identities for effectively identical types, which we then have to reconcile in multiple places elsewhere, for example in order to avoid strange types like
number[] | number[](two types that differ only by the flag). The flag is only used by a heuristic that ultimately also depends on type IDs (which similarly depend on declaration order), so it is a reasonable compromise to just record it in the first type instantiation.