Skip to content

Fixed a relationship check when apparent type of mapped type expands to a union #56027

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Andarist
Copy link
Contributor

@Andarist Andarist commented Oct 8, 2023

fixes #56018

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Oct 8, 2023
source = getApparentType(source);
if (!(originalSource.flags & TypeFlags.Union) && source.flags & TypeFlags.Union && (result = unionOrIntersectionRelatedTo(source, target, reportErrors, intersectionState))) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not fond of how this check is so targeted. I also tried to make the least amount of work required here so I chosen to use unionOrIntersectionRelatedTo but perhaps I should go full circle and use isRelatedTo?

I think it's pretty unlikely for getApparentType to end up with some other type of a type that would require us to go full circle but I'm definitely not 100% sure of that.

Maybe a better fix would be to obtain the getApparentTypeOfMappedType sooner in this function. The only thing that it does is that it instantiates mapped types with everyType(constraint, isArrayOrTupleType). And that's the thing that might "expand" it to a union.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unionOrIntersectionRelatedTo is going to skip the cache (it basically only exists to skip the cache for small unions and intersections), so yeah, probably go full circle and reenter isRelatedTo, so it can make that cache or no-cache call. Honestly, you can probably generalize this to const oldSource = source; source = getApparentType(source); if (oldSource !== source) return isRelatedTo(source, target, reportErrors, intersectionState) - probably. There may be a perf reason to only limit the reentry to union apparent types, but until that's shown, the even more general approach is probably preferable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of those diffs worked:

diff 1
diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts
index 3573d5b466..e63d2810cb 100644
--- a/src/compiler/checker.ts
+++ b/src/compiler/checker.ts
@@ -22447,12 +22447,10 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
                 }
                 const sourceIsPrimitive = !!(sourceFlags & TypeFlags.Primitive);
                 if (relation !== identityRelation) {
-                    const originalSource = source;
-                    source = getApparentType(source);
-                    if (!(originalSource.flags & TypeFlags.Union) && source.flags & TypeFlags.Union && (result = unionOrIntersectionRelatedTo(source, target, reportErrors, intersectionState))) {
-                        return result;
+                    const apparentSource = getApparentType(source);
+                    if (apparentSource !== source) {
+                        return isRelatedTo(apparentSource, target, RecursionFlags.Both, reportErrors);
                     }
-                    sourceFlags = source.flags;
                 }
                 else if (isGenericMappedType(source)) {
                     return Ternary.False;
diff 2
diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts
index 3573d5b466..8670df0345 100644
--- a/src/compiler/checker.ts
+++ b/src/compiler/checker.ts
@@ -22447,12 +22447,10 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
                 }
                 const sourceIsPrimitive = !!(sourceFlags & TypeFlags.Primitive);
                 if (relation !== identityRelation) {
-                    const originalSource = source;
-                    source = getApparentType(source);
-                    if (!(originalSource.flags & TypeFlags.Union) && source.flags & TypeFlags.Union && (result = unionOrIntersectionRelatedTo(source, target, reportErrors, intersectionState))) {
+                    const apparentSource = getApparentType(source);
+                    if (apparentSource !== source && (result = isRelatedTo(apparentSource, target, RecursionFlags.Both, reportErrors))) {
                         return result;
                     }
-                    sourceFlags = source.flags;
                 }
                 else if (isGenericMappedType(source)) {
                     return Ternary.False;
diff 3
diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts
index 3573d5b466..db0c7abbc6 100644
--- a/src/compiler/checker.ts
+++ b/src/compiler/checker.ts
@@ -22447,11 +22447,11 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
                 }
                 const sourceIsPrimitive = !!(sourceFlags & TypeFlags.Primitive);
                 if (relation !== identityRelation) {
-                    const originalSource = source;
-                    source = getApparentType(source);
-                    if (!(originalSource.flags & TypeFlags.Union) && source.flags & TypeFlags.Union && (result = unionOrIntersectionRelatedTo(source, target, reportErrors, intersectionState))) {
+                    const apparentSource = getApparentType(source);
+                    if (apparentSource !== source && (result = isRelatedTo(apparentSource, target, RecursionFlags.Both, reportErrors))) {
                         return result;
                     }
+                    source = apparentSource;
                     sourceFlags = source.flags;
                 }
                 else if (isGenericMappedType(source)) {

I'll have to look later at what actually happens when isRelatedTo gets called here though since I just blindly tried to call it here ;p

@typescript-bot typescript-bot added For Backlog Bug PRs that fix a backlog bug and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Nov 3, 2023
source = getApparentType(source);
if (!(originalSource.flags & TypeFlags.Union) && source.flags & TypeFlags.Union && (result = unionOrIntersectionRelatedTo(source, target, reportErrors, intersectionState))) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unionOrIntersectionRelatedTo is going to skip the cache (it basically only exists to skip the cache for small unions and intersections), so yeah, probably go full circle and reenter isRelatedTo, so it can make that cache or no-cache call. Honestly, you can probably generalize this to const oldSource = source; source = getApparentType(source); if (oldSource !== source) return isRelatedTo(source, target, reportErrors, intersectionState) - probably. There may be a perf reason to only limit the reentry to union apparent types, but until that's shown, the even more general approach is probably preferable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
Status: Waiting on author
Development

Successfully merging this pull request may close these issues.

Incorrect inference of mapped substitution type in conditional involving a union of arrays (regression)
3 participants