Get apparent type for calls used directly at argument positions to aid nested inferences #52866
Conversation
…d nested inferences
typescript-bot
added
the
For Uncommitted Bug
src/compiler/checker.ts
Outdated
| const argumentApparentType = outerContext && isCallOrNewExpression(node.parent) && getDiscriminatedApparentType(node, contextualType); | ||
| const instantiatedType = instantiateType(argumentApparentType && argumentApparentType !== unknownType ? argumentApparentType : contextualType, outerMapper); |
There was a problem hiding this comment.
This is a different take on the problem that the one in #52865 .
The argumentApparentType !== unknownType is really meant to be isNotDefaultConstraintOfUnconstrainedTypeParamere(argumentApparentType). I thought that something like this would already exist but I couldn't find it. it's likely that without the strict null checks, we'd have to check against emptyObjectType. That's also why I temporarily added @strict here:
https://github.com/microsoft/TypeScript/pull/52866/files#diff-0ed2dcde3e5999198b7eebb67d0c508dbd4497536f956829f74706f33eca470dR3
|
@typescript-bot test this |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at 5d63a08. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at 5d63a08. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at 5d63a08. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the extended test suite on this PR at 5d63a08. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 5d63a08. You can monitor the build here. Update: The results are in! |
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
@jakebailey Here are the results of running the user test suite comparing Everything looks good! |
|
Heya @jakebailey, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here. |
|
@jakebailey Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
typescript-bot
added
For Backlog Bug
There was a problem hiding this comment.
This is probably closer to a good fix than its' competitor PR, since our contextual type instantiation choice is somewhat heuristic in nature, and skipping said instantiation is supposed to be a perf optimization according to nearby comments, not something with visible impact (even though it definitely has visible impact in some cases, like the linked issue, and I know of others). The interplay of contextual type (instantiation) and return type inference is particularly complex, so I'd understand that adjusting something there is the way to go.
However, that said, what I don't get is the change to do discrimination of the contextual type, when, to what I can see, the motivating example doesn't have any discrimination in it. Is that an untested change that's also here for some reason? Is the key part of this change really just
const instantiatedType = instantiateType(getApparentType(contextualType) === unknownType ? contextualType : getApparentType(contextualType), outerMapper);?
Because that feels kinda weird - like we should always get getting the apparent type of the contextual type or not, not this result-type-swapped middleground. Moreover, switching on unknownType in this context feels odd, too - what's privileged about a T extends unknown instead of a T extends number here (for example, if you swapped all your matcher examples to use a T extends string instead of an unconstrained T, would they still work)? I'm struggling to see the guiding principle behind the change.
|
I would have to refresh my memory on this one but I mentioned my intention behind this |
…nstantiated-from-the-outer-one-2 # Conflicts: # src/compiler/checker.ts
This part doesn't have a test right now. The main idea behind this fix is to mimic what
Not quite, the whole baselines diff with the quoted versiondiff --git a/tests/baselines/reference/callChain.3.errors.txt b/tests/baselines/reference/callChain.3.errors.txt
index 6deb1db1b4..2165107e1d 100644
--- a/tests/baselines/reference/callChain.3.errors.txt
+++ b/tests/baselines/reference/callChain.3.errors.txt
@@ -1,6 +1,6 @@
tests/cases/conformance/expressions/optionalChaining/callChain/callChain.3.ts(3,7): error TS2322: Type 'number | undefined' is not assignable to type 'number'.
Type 'undefined' is not assignable to type 'number'.
-tests/cases/conformance/expressions/optionalChaining/callChain/callChain.3.ts(4,7): error TS2322: Type 'number | undefined' is not assignable to type 'number'.
+tests/cases/conformance/expressions/optionalChaining/callChain/callChain.3.ts(4,7): error TS2322: Type 'Number | undefined' is not assignable to type 'number'.
Type 'undefined' is not assignable to type 'number'.
@@ -13,7 +13,7 @@ tests/cases/conformance/expressions/optionalChaining/callChain/callChain.3.ts(4,
!!! error TS2322: Type 'undefined' is not assignable to type 'number'.
const n2: number = a?.m?.({x: absorb()}); // likewise
~~
-!!! error TS2322: Type 'number | undefined' is not assignable to type 'number'.
+!!! error TS2322: Type 'Number | undefined' is not assignable to type 'number'.
!!! error TS2322: Type 'undefined' is not assignable to type 'number'.
const n3: number | undefined = a?.m?.({x: 12}); // should be ok
const n4: number | undefined = a?.m?.({x: absorb()}); // likewise
diff --git a/tests/baselines/reference/callChain.3.types b/tests/baselines/reference/callChain.3.types
index c4afc572ab..c1ce3b91d5 100644
--- a/tests/baselines/reference/callChain.3.types
+++ b/tests/baselines/reference/callChain.3.types
@@ -20,13 +20,13 @@ const n1: number = a?.m?.({x: 12 }); // should be an error (`undefined` is not a
const n2: number = a?.m?.({x: absorb()}); // likewise
>n2 : number
->a?.m?.({x: absorb()}) : number | undefined
+>a?.m?.({x: absorb()}) : Number | undefined
>a?.m : (<T>(obj: { x: T; }) => T) | undefined
>a : { m?<T>(obj: { x: T; }): T; } | undefined
>m : (<T>(obj: { x: T; }) => T) | undefined
->{x: absorb()} : { x: number; }
->x : number
->absorb() : number
+>{x: absorb()} : { x: Number; }
+>x : Number
+>absorb() : Number
>absorb : <T>() => T
const n3: number | undefined = a?.m?.({x: 12}); // should be okOn top of that, we also get an Basically, it seems that for nested properties~ this call to
If we'd just replace this with: const instantiatedType = instantiateType(getApparentType(contextualType), outerMapper)then we'd end up with the same quick info failure and those changed baselines (similar but not the same as the previous ones): baselines diff with non-conditional `getApparentType` calldiff --git a/tests/baselines/reference/callChain.3.errors.txt b/tests/baselines/reference/callChain.3.errors.txt
index 6deb1db1b4..e93c130238 100644
--- a/tests/baselines/reference/callChain.3.errors.txt
+++ b/tests/baselines/reference/callChain.3.errors.txt
@@ -1,10 +1,10 @@
tests/cases/conformance/expressions/optionalChaining/callChain/callChain.3.ts(3,7): error TS2322: Type 'number | undefined' is not assignable to type 'number'.
Type 'undefined' is not assignable to type 'number'.
-tests/cases/conformance/expressions/optionalChaining/callChain/callChain.3.ts(4,7): error TS2322: Type 'number | undefined' is not assignable to type 'number'.
- Type 'undefined' is not assignable to type 'number'.
+tests/cases/conformance/expressions/optionalChaining/callChain/callChain.3.ts(4,7): error TS2322: Type 'unknown' is not assignable to type 'number'.
+tests/cases/conformance/expressions/optionalChaining/callChain/callChain.3.ts(6,7): error TS2322: Type 'unknown' is not assignable to type 'number | undefined'.
-==== tests/cases/conformance/expressions/optionalChaining/callChain/callChain.3.ts (2 errors) ====
+==== tests/cases/conformance/expressions/optionalChaining/callChain/callChain.3.ts (3 errors) ====
declare function absorb<T>(): T;
declare const a: { m?<T>(obj: {x: T}): T } | undefined;
const n1: number = a?.m?.({x: 12 }); // should be an error (`undefined` is not assignable to `number`)
@@ -13,10 +13,11 @@ tests/cases/conformance/expressions/optionalChaining/callChain/callChain.3.ts(4,
!!! error TS2322: Type 'undefined' is not assignable to type 'number'.
const n2: number = a?.m?.({x: absorb()}); // likewise
~~
-!!! error TS2322: Type 'number | undefined' is not assignable to type 'number'.
-!!! error TS2322: Type 'undefined' is not assignable to type 'number'.
+!!! error TS2322: Type 'unknown' is not assignable to type 'number'.
const n3: number | undefined = a?.m?.({x: 12}); // should be ok
const n4: number | undefined = a?.m?.({x: absorb()}); // likewise
+ ~~
+!!! error TS2322: Type 'unknown' is not assignable to type 'number | undefined'.
// Also a test showing `!` vs `?` for good measure
let t1 = a?.m?.({x: 12});
diff --git a/tests/baselines/reference/callChain.3.types b/tests/baselines/reference/callChain.3.types
index c4afc572ab..0167fb3e9f 100644
--- a/tests/baselines/reference/callChain.3.types
+++ b/tests/baselines/reference/callChain.3.types
@@ -20,13 +20,13 @@ const n1: number = a?.m?.({x: 12 }); // should be an error (`undefined` is not a
const n2: number = a?.m?.({x: absorb()}); // likewise
>n2 : number
->a?.m?.({x: absorb()}) : number | undefined
+>a?.m?.({x: absorb()}) : unknown
>a?.m : (<T>(obj: { x: T; }) => T) | undefined
>a : { m?<T>(obj: { x: T; }): T; } | undefined
>m : (<T>(obj: { x: T; }) => T) | undefined
->{x: absorb()} : { x: number; }
->x : number
->absorb() : number
+>{x: absorb()} : { x: unknown; }
+>x : unknown
+>absorb() : unknown
>absorb : <T>() => T
const n3: number | undefined = a?.m?.({x: 12}); // should be ok
@@ -41,13 +41,13 @@ const n3: number | undefined = a?.m?.({x: 12}); // should be ok
const n4: number | undefined = a?.m?.({x: absorb()}); // likewise
>n4 : number | undefined
->a?.m?.({x: absorb()}) : number | undefined
+>a?.m?.({x: absorb()}) : unknown
>a?.m : (<T>(obj: { x: T; }) => T) | undefined
>a : { m?<T>(obj: { x: T; }): T; } | undefined
>m : (<T>(obj: { x: T; }) => T) | undefined
->{x: absorb()} : { x: number; }
->x : number
->absorb() : number
+>{x: absorb()} : { x: unknown; }
+>x : unknown
+>absorb() : unknown
>absorb : <T>() => T
// Also a test showing `!` vs `?` for good measure
diff --git a/tests/baselines/reference/inferFromBindingPattern.types b/tests/baselines/reference/inferFromBindingPattern.types
index ee217fafa8..b5f8e0790f 100644
--- a/tests/baselines/reference/inferFromBindingPattern.types
+++ b/tests/baselines/reference/inferFromBindingPattern.types
@@ -95,16 +95,16 @@ declare function stringy<T = string>(arg?: T): T;
>arg : T | undefined
const isStringTuple = makeTuple(stringy()); // [string]
->isStringTuple : [string]
->makeTuple(stringy()) : [string]
+>isStringTuple : [unknown]
+>makeTuple(stringy()) : [unknown]
>makeTuple : <T1>(arg: T1) => [T1]
->stringy() : string
+>stringy() : unknown
>stringy : <T = string>(arg?: T | undefined) => T
const [isAny] = makeTuple(stringy()); // [string]
->isAny : string
->makeTuple(stringy()) : [string]
+>isAny : unknown
+>makeTuple(stringy()) : [unknown]
>makeTuple : <T1>(arg: T1) => [T1]
->stringy() : string
+>stringy() : unknown
>stringy : <T = string>(arg?: T | undefined) => T
It's not that
Yes.
It's just a product of my experiments with this code, primarily done by looking at other, similar, cases, comparing them and fine-tuning this call site. It's not exactly that I have any "guiding principle" behind this change 😉 it just tries to mimick Note that currently this PR breaks VS Code's codebase, I managed to extract a few failing test cases based on the bot's results: TS playground. Every other caught case looks pretty much the same so I think that those 3 are representative - gonna investigate this soon. |
|
I've done further debugging to understand what's happening with the breaks in VS Code's codebase:
Perhaps part of the fix should be to somehow determine the correct behavior based on the existence of the outer |
…nstantiated-from-the-outer-one-2
…nstantiated-from-the-outer-one-2 # Conflicts: # src/compiler/checker.ts
|
Would love to see this merged to fix #57978 |
fixes #52864
fixes #57978
competes with #52865