Fix contextual typing for post-spread tuple elements #52769
Conversation
typescript-bot
added
Author: Team
For Milestone Bug
# Conflicts: # src/compiler/types.ts
|
@typescript-bot test this |
|
Heya @ahejlsberg, I've started to run the extended test suite on this PR at 54f49e9. You can monitor the build here. |
|
Heya @ahejlsberg, I've started to run the diff-based top-repos suite on this PR at 54f49e9. You can monitor the build here. |
|
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at 54f49e9. You can monitor the build here. |
|
Heya @ahejlsberg, I've started to run the diff-based user code test suite on this PR at 54f49e9. You can monitor the build here. |
|
Heya @ahejlsberg, I've started to run the abridged perf test suite on this PR at 54f49e9. You can monitor the build here. |
|
Heya @ahejlsberg, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here. |
|
|
||
| const d1 = foo([...staticPath1Level, randomID, 'folder', 'subfolder', 'another-subfolder', 'doc.pdf']); | ||
| const d2 = foo([...staticPath2Level, randomID, 'folder', 'subfolder', 'another-subfolder', 'doc.pdf']); | ||
| const d3 = foo([...staticPath3Level, randomID, 'folder', 'subfolder', 'another-subfolder', 'doc.pdf']); |
There was a problem hiding this comment.
I thought that this PR might also fix this issue but it doesn't. Perhaps it's something to work looking into here? The problem is similar but it's kinda a reverse one. This PR fixes contextual types for post-spread elements at the expression/runtime level and this other case is about post-spread elements at the type level.
The problem is that getTypeOfPropertyOfContextualType is still used here without any modifications as the primary source for the contextual type. The logic was only adjusted in the "else" branch here. The contextual type for this case is successfully retrieved through those lines and since this doesn't take the position in the array expression into account - it's just string.
| const d3 = foo([...staticPath3Level, randomID, 'folder', 'subfolder', 'another-subfolder', 'doc.pdf']); | |
| const d3 = foo([...staticPath3Level, randomID, 'folder', 'subfolder', 'another-subfolder', 'doc.pdf']); | |
| // repro from #52760 | |
| const e1: [...string[], '!'] = ['a', '!']; | |
There was a problem hiding this comment.
Yeah, we need to fix that too.
There was a problem hiding this comment.
When mentioning this case I was also thinking beyond union reductions, it would make sense for this to work too:
declare function test(
...args: [...((arg: number) => void)[], (arg: string) => void]
): void;
test(
(a) => {
a
// ^? actual: any; expected: number
},
(b) => {
b
// ^? actual: any; expected: number
},
(c) => {
c
// ^? actual: any; expected: string
},
);There was a problem hiding this comment.
This would require us to reason about elements at a fixed offset from the end of a tuple. We currently only reason about elements at a fixed offset from the start. It adds some more complexity, but it is certainly possible to do.
| || mapType( | ||
| arrayContextualType, | ||
| t => getIteratedTypeOrElementType(IterationUse.Element, t, undefinedType, /*errorNode*/ undefined, /*checkAssignability*/ false), | ||
| index >= 0 && getTypeOfPropertyOfContextualType(arrayContextualType, "" + index as __String) || |
There was a problem hiding this comment.
I know that it's not exactly 100% related to this case - but I was touching this function just recently here. It would be cool if that improvement (if correct) could also go into 5.0 - both would make for a nice "batch" of improvements for contextual types for element expressions. 😉
|
@typescript-bot test this |
|
Heya @jakebailey, I've started to run the extended test suite on this PR at 2631e2f. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at 2631e2f. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 2631e2f. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the abridged perf test suite on this PR at 2631e2f. 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 2631e2f. You can monitor the build here. |
|
@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 Everything looks good! |
|
@jakebailey Here they are:Comparison Report - main..52769
System
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Tests and performance all look good. |
| @@ -465,7 +465,7 @@ fn1([1, 'abc']); // [number, string] | |||
| fn1([1, 'abc', true]); // [string, boolean] | |||
| >fn1([1, 'abc', true]) : [string, boolean] | |||
| >fn1 : <T, U>(t: [...unknown[], T, U]) => [T, U] | |||
| >[1, 'abc', true] : [number, string, boolean] | |||
| >[1, 'abc', true] : [number, string, true] | |||
There was a problem hiding this comment.
I guess it doesn't seem to have any observable effect here, but, why is the last thing here true while the first two are widened?
There was a problem hiding this comment.
Following inference and instantiation, the contextual type for the array literal is [...unknown[], string, boolean]. The contextual type for the true literal is the is computed from unknown | string | boolean which previously would reduce to just unknown. With this PR we don't perform subtype reduction, so boolean survives in the contextual union type and causes the true literal to stay true. But the change doesn't really matter since we've already inferred boolean for U.
Fixes #52684.
Fixes #52760.