Revise mapped tuple type instantiation logic #57031
Conversation
typescript-bot
added
Author: Team
For Milestone Bug
|
@typescript-bot test top100 |
|
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at 2980eae. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the diff-based user code test suite on this PR at 2980eae. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the diff-based top-repos suite on this PR at 2980eae. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the tsc-only perf test suite on this PR at 2980eae. You can monitor the build here. Update: The results are in! |
| // apply the mapped type itself to the variadic element type. For other elements in the variable part of the | ||
| // tuple, we surround the element type with an array type and apply the mapped type to that. This ensures | ||
| // that we get sequential property key types ("0", "1", "2", etc.) for the fixed part of the tuple, and | ||
| // property key type number for the remaining elements. |
There was a problem hiding this comment.
Can you keep something similar to the original example in place? Or is it too different now for some reason?
There was a problem hiding this comment.
and as you note the example with the keys, maybe include the example from your PR description as well?
type Keys<T> = { [K in keyof T]: K };
type Foo<T extends unknown[]> = Keys<[string, string, ...T, string]>; // ["0", "1", ...Keys<T>, number]There was a problem hiding this comment.
Problem is, the original transformation (as witnessed by that example) was inconsistent with regards to property keys, so not sure there's any value in keeping it.
There was a problem hiding this comment.
Yeah, I was thinking of trying to update the example, not keep it.
I guess the issue you might've run into is that we don't have great notation to describe this. The closest thing being something like:
We transform
M<[A, B?, ...T, ...C[]]
into
[M<{ 0: A }>[0], M<{ 1: B }>[1]?, ...M<T>, ...M<C[]>]
which is not too terrible.
But I think having an example still has value, so if we have to go concrete, the Keys example isn't bad.
There was a problem hiding this comment.
I'll add the Keys example. In general, we now process the fixed part of the tuple just like any other object. It's really just the elements in the variable part that get special processing.
|
@ahejlsberg Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@ahejlsberg Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
| const flags = elementFlags[i]; | ||
| return i < fixedLength ? instantiateMappedTypeTemplate(mappedType, getStringLiteralType("" + i), !!(flags & ElementFlags.Optional), fixedMapper) : | ||
| flags & ElementFlags.Variadic ? instantiateType(mappedType, prependTypeMapping(typeVariable, type, mapper)) : | ||
| getElementTypeOfArrayType(instantiateType(mappedType, prependTypeMapping(typeVariable, createArrayType(type), mapper))) || unknownType; |
There was a problem hiding this comment.
| getElementTypeOfArrayType(instantiateType(mappedType, prependTypeMapping(typeVariable, createArrayType(type), mapper))) || unknownType; | |
| getElementTypeOfArrayType(instantiateType(mappedType, prependTypeMapping(typeVariable, createArrayType(type), mapper))) ?? unknownType; |
| type KA = K<[string, string, boolean]>; // ["0", "1", "2"] | ||
| type KB = K<[string, string, ...string[], string]>; // ["0", "1", ...number[], number] | ||
| type KC = K<[...string[]]>; // number[] | ||
| type KD = K<string[]>; // number[] |
There was a problem hiding this comment.
I know it's not allowed to have non-generic variable elements follow a non-generic rest, but it would be good to make sure the checker tolerates states like that
| type KD = K<string[]>; // number[] | |
| type KD = K<string[]>; // number[] | |
| type KE = K<[string, ...boolean[], number?]>; | |
| type KE = K<[string, ...boolean[], ...number[]]>; |
There was a problem hiding this comment.
Since you bring this up here... this is killing me 😅
type A = [number, ...boolean[], ...string[]] // error
type B = [number, ...Array<boolean>, ...Array<string>] // okShouldn't those really behave the same way? IIRC I have a PR laying around somewhere that makes this error go away (by matching the behavior of the latter)
There was a problem hiding this comment.
Is the PR already open?
Not that it's related, but you inspired me to file something else I've put off mentioning for a while: #57034
There was a problem hiding this comment.
It is probably not the intention that we want to allow that code to be honest. But I'll let Anders weigh in.
There was a problem hiding this comment.
Is the PR already open?
sure thing :) #55446
and even more surprising to the user is that you can make the error go away when moving string[] to a type alias:
type Alias = string[]
type Ok2 = [...number[], ...Alias]So this whole rule prevents only a very narrow set of scenarios.
There was a problem hiding this comment.
@DanielRosenwasser Those extra examples wouldn't really check anything related to this PR since we normalize the tuple type before the instantiation logic even sees it.
There was a problem hiding this comment.
@Andarist I still think the error makes sense, but we should fix the error reporting logic to be consistent between the notations. Permitting [...string[], ...number[]] with no errors will just make users think we can actually check for a sequence of strings followed by a sequence of numbers, but that's not the case.
There was a problem hiding this comment.
I'd assume that people might already be relying on the fact that spreading like this might "normalize" the shape, so I thought that it is off the table as it would be considered a breaking change.
There was a problem hiding this comment.
Error reporting fixed with the latest commit.
There was a problem hiding this comment.
Error reporting fixed with the latest commit.
Thank you ❤️ I like that this will be more consistent now.
I found the old behavior (without the error) moderately useful. It's cool that it will still be possible - just somewhat unfortunate that to benefit from it we'll have to use an extra type wrapper (TS playground):
type A = [number, string?];
type B = [...boolean[], boolean];
// ok in 5.3, errors with the change
type C = [bigint, ...bigint[], ...B, ...A]; // [bigint, ...(string | number | bigint | boolean | undefined)[]]
type Indirect<A1 extends unknown[], A2 extends unknown[], A3 extends unknown[], A4 extends unknown[]> = [...A1, ...A2, ...A3, ...A4]
// ok in 5.3, ok with the change
type IndirectResult = Indirect<[bigint], bigint[], B, A> // [bigint, ...(string | number | bigint | boolean | undefined)[]]
|
Looks good, left some comments that should be quick to address. |
|
Hey @ahejlsberg, the results of running the DT tests are ready. |
| if (isTupleType(t)) { | ||
| return instantiateMappedTupleType(t, type, prependTypeMapping(typeVariable, t, mapper)); | ||
| return instantiateMappedTupleType(t, type, typeVariable, mapper); |
There was a problem hiding this comment.
An offtopic question if I may. I noticed (on several occasions already) that instantiating array/tuple mapped types eagerly~ leads to inconsistent behaviors with the object variants. The timing of instantiation is quite different and thus the availability of certain information tends to be different between them.
What are your thoughts on making this more consistent? How feasible that would even be? When it comes to objects each property stays deferred for as much as possible and I imagine that this would require deferring this instantiation per tuple element.
There was a problem hiding this comment.
In general, the advantage of eager instantiation is it is simpler to implement and it allows sharing of identical instantiations. The specific reason we eagerly instantiate mapped array and tuple types is that elsewhere in the compiler we detect arrays and tuples by looking for instantiations of Array<T> and the corresponding synthetic tuple classes. We'd have to extend those cases to also handle mapped types applied to arrays and tuples.
There was a problem hiding this comment.
Thank you very much for the answer. I might want to attempt unifying this at some point - it's good to know that you don't have strong objections 😉 Or at least I read that from your comment ;p I understand that this might introduce some new complexity but I think it might be inevitable (as long as the goal is to have consistent behavior). When trying to do this, I will gather the test cases backing up that it's a desired change.
|
@ahejlsberg Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
| type V15 = [...string[], ...number[]]; // Error | ||
| type V16 = [...string[], ...Array<number>]; // Error | ||
| type V17 = [...Array<string>, ...number[]]; // Error | ||
| type V18 = [...Array<string>, ...Array<number>]; // Error |
There was a problem hiding this comment.
I propose some extra test cases with type aliases:
| type V18 = [...Array<string>, ...Array<number>]; // Error | |
| type V18 = [...Array<string>, ...Array<number>]; // Error | |
| type Alias19 = number[]; | |
| type V19 = [...string[], ...Alias19]; // Error | |
| type Alias20 = Array<number>; | |
| type V20 = [...string[], ...Alias20]; // Error |
|
It might be worth rechecking if the recently merged in #55774 doesn't throw any wrinkles into this. |
|
I checked, and this PR still passes with main merged in (unless you were thinking of something other effect). |
|
Unfortunately, #55774 got reverted because it weirdly clashed with another PR that was merged recently ( #56727 ). However, I didn't think that a merge of those would fail any existing test cases but rather that maybe some new test cases should be written in light of this PR. I just had a hunch of sorts that maybe there is something to be re-checked but I didn't have the energy myself at the time to think about the actual test cases. |
This PR revises and simplifies our mapped tuple type instantiation logic. Previously, when a mapped type was applied to a variadic tuple type, we'd transform the operation into a sequence of operations on manufactured singleton tuple types. This would have surprising effects when observing property key types, as reported in #48856. For example:
With this PR, the property keys are more consistent:
The PR also fixes issues related to instantiation of tuples with required elements following a rest element, as reported in #56888, and it fixes the error reporting issue discussed here.
Finally, the PR reverts #53522. The fix in that PR is incorrect--it effectively leaks a type parameter. With this PR, the repro from #53458 now causes an excessive instantiation depth error, which should have been the behavior in the first place.
Fixes #48856.
Fixes #56888.