Improve recursion identity for direct type instantiations#3445
Open
ahejlsberg wants to merge 3 commits intomainfrom
Open
Improve recursion identity for direct type instantiations#3445ahejlsberg wants to merge 3 commits intomainfrom
ahejlsberg wants to merge 3 commits intomainfrom
Conversation
Member
Author
|
@typescript-bot test it |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refines recursion identity selection in the checker so that type references produced directly from resolving AST type nodes don’t use their symbol as the recursion identity in isDeeplyNestedType, preventing false “generative recursion” detection that can mask real assignability errors (fixes #3426).
Changes:
- Introduces
ObjectFlagsFromTypeNodeto mark type references created directly from AST type node resolution. - Updates
getRecursionIdentityto avoid usingt.symbolas the recursion identity whenObjectFlagsFromTypeNodeis set. - Routes array/class/interface type-node resolution through a new
createTypeReferenceEx(..., ObjectFlagsFromTypeNode)helper.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
internal/checker/types.go |
Adds a new ObjectFlagsFromTypeNode bit to tag type references originating from AST type nodes. |
internal/checker/relater.go |
Excludes FromTypeNode references from using symbol recursion identity in getRecursionIdentity. |
internal/checker/checker.go |
Creates createTypeReferenceEx and uses it in type-node resolution to apply ObjectFlagsFromTypeNode. |
Comment on lines
24634
to
+24639
| id := getTypeListKey(typeArguments) | ||
| intf := target.AsInterfaceType() | ||
| if t, ok := intf.instantiations[id]; ok { | ||
| return t | ||
| } | ||
| t := c.newObjectType(ObjectFlagsReference, target.symbol) | ||
| t.objectFlags |= c.getPropagatingFlagsOfTypes(typeArguments, TypeFlagsNone) | ||
| t := c.newObjectType(ObjectFlagsReference|objectFlags|c.getPropagatingFlagsOfTypes(typeArguments, TypeFlagsNone), target.symbol) |
There was a problem hiding this comment.
createTypeReferenceEx caches instantiations only by typeArguments (getTypeListKey). That ignores the objectFlags parameter, so a previously-cached instantiation without ObjectFlagsFromTypeNode can 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 relevant objectFlags bits (at least ObjectFlagsFromTypeNode) in the cache key and/or maintaining a separate instantiation cache for FromTypeNode references so the flag is applied deterministically only where intended.
Member
Author
There was a problem hiding this comment.
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.
Comment on lines
+842
to
+846
| 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 side of a class since it shares its symbol with the instance side. | ||
| // 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). |
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 testdata/tests/cases/compiler/ that fails before this change and passes after, so we don’t regress this behavior again.
|
@ahejlsberg Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@ahejlsberg Here are the results of running the user tests with tsc comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
Member
|
Looks like this causes 30% more allocs in mui. |
|
@ahejlsberg Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
Member
Author
There's something really odd about that test run. Check time (which is the only thing that would be affected) is slightly down, but parse time is way up by 42%. |
Member
Author
|
@typescript-bot perf test this faster |
|
@ahejlsberg Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Member
Author
|
microsoft/TypeScript#63415 implements this same change in the Strada code base. Interestingly, there is one failure in top 400 repos in Strada. It appears to be caused by relating almost identical copies of |
Andarist
reviewed
Apr 21, 2026
| } | ||
| links.resolvedType = c.createNormalizedTypeReference(target, elementTypes) | ||
| if target.objectFlags&ObjectFlagsTuple != 0 { | ||
| links.resolvedType = c.createNormalizedTupleType(target, elementTypes) |
Contributor
There was a problem hiding this comment.
Shouldn't this also accept and propagate ObjectFlagsFromTypeNode? FWIW, while testing this PR locally I ended up implementing this: https://github.com/microsoft/typescript-go/compare/fix-3426...Andarist:fix/deeply-nested-tuples-from-type-nodes?expand=1
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
With this PR, we exclude type references that originate directly from resolution of AST type nodes from using their symbol as their recursion identity in
isDeeplyNestedType. Infinite recursion in type relationships is ultimately fuelled by types produced byinstantiateType, and we know that type references that originate directly from resolution of AST type nodes have not been subject to such instantiation. Therefore, it is safe to use themselves as their recursion identity.Previously, code such as the following would not error because
isDeeplyNestedTypewould see three nested instantiations ofArray<T>with increasing type IDs and mistake that for generative recursion. The example now errors because the array instantiations all originate direcly in resolution of AST type nodes.Fixes #3426.