Fixed issues with inference from nested tuple types of the same shape

[Andarist]

fixes #48524
fixes #52722

[Andarist]

I'm not overly confident that it's the correct shape of this fix but all tests pass.

The problem was that for the inner tuple the same recursion identity was computed here as the type.target was shared between those tuples (as they have the same shape). And thus invokeOnce was skipping over action(source, target) call there while inferring for the inner tuple.

This check is done within this if statement:
https://github.dev/microsoft/TypeScript/blob/942d2a91c115f22857513a9d3f4bde2fb1521f50/src/compiler/checker.ts#L20918-L20919

However, this still goes into this if statement for a tuple because it doesn't satisfy the isObjectOrArrayLiteralType(type) check. I think that perhaps a better fix here would be to make a change to this if condition. I've tried this alternative patch and all tests seem to be passing to:

Alternative patch

diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts
index 7bcc6f5bdc..efce2901b4 100644
--- a/src/compiler/checker.ts
+++ b/src/compiler/checker.ts
@@ -20916,7 +20916,7 @@ namespace ts {
         // reference the type have a recursion identity that differs from the object identity.
         function getRecursionIdentity(type: Type): object {
             // Object and array literals are known not to contain recursive references and don't need a recursion identity.
-            if (type.flags & TypeFlags.Object && !isObjectOrArrayLiteralType(type)) {
+            if (type.flags & TypeFlags.Object && !isObjectOrArrayLiteralType(type) && !isTupleType(type)) {
                 if (getObjectFlags(type) && ObjectFlags.Reference && (type as TypeReference).node) {
                     // Deferred type references are tracked through their associated AST node. This gives us finer
                     // granularity than using their associated target because each manifest type reference has a
@@ -20928,10 +20928,6 @@ namespace ts {
                     // exclude the static side of classes from this check since it shares its symbol with the instance side.
                     return type.symbol;
                 }
-                if (isTupleType(type)) {
-                    // Tuple types are tracked through their target type
-                    return type.target;
-                }
             }
             if (type.flags & TypeFlags.TypeParameter) {
                 return type.symbol;

The comment above the isTupleType suggests that it's checking if the type was written as literal tuple type:
https://github.dev/microsoft/TypeScript/blob/942d2a91c115f22857513a9d3f4bde2fb1521f50/src/compiler/checker.ts#L21278-L21282
and the comment above the mentioned condition suggests that we don't want to go into this block if a type was written as literal (it only mentions arrays and objects though):
https://github.dev/microsoft/TypeScript/blob/942d2a91c115f22857513a9d3f4bde2fb1521f50/src/compiler/checker.ts#L20918

So perhaps this isTupleType check shouldn't be inside this if block from the start?

[ycmjason]

❤️

[RyanCavanaugh]

@ahejlsberg LGTY? This has shown up with a few manifestations

[Andarist]

I was thinking about it more and I think it's the correct change semantically. A tuple can't really reference itself directly as far as I know - so it always has to go through an alias. If it goes through an alias then that alias won't be entered again thanks to the recursion identity check and the tuple in it won't even be reached.

Would there be a chance for a playground build of this PR?

[jakebailey]

@typescript-bot pack this

[typescript-bot]

Heya @jakebailey, I've started to run the tarball bundle task on this PR at a7d5826. You can monitor the build here.

[typescript-bot]

Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/147477/artifacts?artifactName=tgz&fileId=89112C4EB6A21A56181946173B2D8362BF190655060485720433E0707072C2A902&fileName=/typescript-5.0.0-insiders.20230225.tgz"
    }
}

and then running npm install.

---

There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@5.0.0-pr-49226-5".;

[weswigham]

Honestly, the logic presented here, that tuple structure can only be self-recursive in the presence of aliases, makes sense to me, and makes using the tuple type itself as the recursion identity seem fine (since somewhere in the stack will be a repeating alias symbol).

[ahejlsberg]

Just noticed this was merged, but it causes an issue in this example:

type T1<T> = [number, T1<{ x: T }>];
type T2<T> = [42, T2<{ x: T }>];

function qq<U>(x: T1<U>, y: T2<U>) {
    x = y;  // Excessive stack depth comparing types 'T2<U>' and 'T1<U>'
    y = x;  // Error as expected
}

The change in the PR effectively disables isDeeplyNestedType checks for tuples and we end up hitting the excessive stack depth panic backstop.

[Andarist]

@ahejlsberg do u have a suggested fix for this in mind or should I investigate this?

[weswigham]

You could probably change it from return type to return type.aliasSymbol || type - that way tuples with associated aliases appropriately backstop deep recursive structures. I thought aliases were already handled like this in getRecursionIdentity, hence my comment above on merge, but apparently not. Still, shouldn't be a large change and I think should fix @ahejlsberg 's example.

[Andarist]

That doesn't seem to cut it. I'm gonna investigate why sometime later today.

[weswigham]

Ah, probably because we call getNormalizedType on tuples which replaces them with their normalized form, removing the alias data.

[ahejlsberg]

I have a fix for this. We solved a similar problem in isDeeplyNestedType by tracking whether type IDs are increasing as we move up the recursion tracking stack. Increasing type IDs indicate newly generated types, and those we want to count, whereas decreasing type IDs indicate manually written instantiations that we don't want to count. However, the circularity checking in invokeOnce doesn't use isDeeplyNestedType, so it doesn't benefit from this. But with this change it can:

        function invokeOnce<Source extends Type, Target extends Type>(source: Source, target: Target, action: (source: Source, target: Target) => void) {
            const key = source.id + "," + target.id;
            const status = visited && visited.get(key);
            if (status !== undefined) {
                inferencePriority = Math.min(inferencePriority, status);
                return;
            }
            (visited || (visited = new Map<string, number>())).set(key, InferencePriority.Circularity);
            const saveInferencePriority = inferencePriority;
            inferencePriority = InferencePriority.MaxValue;
            // We stop inferring and report a circularity if we encounter duplicate recursion identities on both
            // the source side and the target side.
            const saveExpandingFlags = expandingFlags;
            (sourceStack ??= []).push(source);
            (targetStack ??= []).push(target);
            if (isDeeplyNestedType(source, sourceStack, sourceStack.length, 2)) expandingFlags |= ExpandingFlags.Source;
            if (isDeeplyNestedType(target, targetStack, targetStack.length, 2)) expandingFlags |= ExpandingFlags.Target;
            if (expandingFlags !== ExpandingFlags.Both) {
                action(source, target);
            }
            else {
                inferencePriority = InferencePriority.Circularity;
            }
            targetStack.pop();
            sourceStack.pop();
            expandingFlags = saveExpandingFlags;
            visited.set(key, inferencePriority);
            inferencePriority = Math.min(inferencePriority, saveInferencePriority);
        }

Plus change the declarations of sourceStack and targetStack to have type Type[].

Best I can tell this takes care of the problem.

[ahejlsberg]

I can put up a PR with these changes. Alternatively, @Andarist you can put up a new PR.

[Andarist]

@ahejlsberg if u already have the fix then go for it :)