Fixed the issue with some longer variadic tuples with any rest being incorrectly assignable to shorter variadic tuples
#50218
Conversation
…g incorrectly assignable to shorter variadic tuples
typescript-bot
added
the
For Uncommitted Bug
| /** Number of required or variadic elements */ | ||
| minLength: number; | ||
| /** Number of initial required or optional elements */ | ||
| fixedLength: number; | ||
| /** True if tuple has any rest or variadic elements */ | ||
| hasRestElement: boolean; |
There was a problem hiding this comment.
By moving those to JSDocs we improve slightly the IDE experience as this information gets displayed in the tooltips when hovering over those properties
| const sourceIndex = i < targetArity - endCount ? i : i + sourceArity - targetArity; | ||
| const sourceFlags = isTupleType(source) && (i < startCount || i >= targetArity - endCount) ? source.target.elementFlags[sourceIndex] : ElementFlags.Rest; | ||
| const targetFlags = target.target.elementFlags[i]; | ||
| for (let sourcePosition = 0; sourcePosition < sourceArity; sourcePosition++) { |
There was a problem hiding this comment.
The core of the fix is in the iteration "style". Previously we were iterating over targetArity and "slicing" a source in the position of the target rest element.
When we consider the added test case:
declare let tt3: [number, string, ...any[]]
let tt4: [number, ...number[]] = tt3It means that for the target's ...number[] we were creating a slice from those elements: string, ...any[]. This, in turn, was indexed~/unionized and thus became string | any which is just an equivalent of any. And that was used as a source type for the target, introducing a bug.
I've flipped this and now this loop iterates over sourceArity. This ensures that a specific position in the source is assignable, in "isolation", to the respective target position.
There was a problem hiding this comment.
From testing this code with some examples and carefully reading through it, I think it's at least as correct as before (and more, now that it fixes the bug). I still need to review the error messages though.
There was a problem hiding this comment.
Thank you very much for the review! I know that there are some changes in the error messages and stuff - just want to let you know that I would happily make any required adjustments to this PR, but when it comes to those error messages I need to know what would be the expected results. IIRC the current messages are not incorrect - but perhaps you'd like to have them improved somehow.
| @@ -18,12 +18,10 @@ tests/cases/conformance/types/tuple/restTupleElements1.ts(33,31): error TS2344: | |||
| Type 'string' is not assignable to type 'number'. | |||
| tests/cases/conformance/types/tuple/restTupleElements1.ts(34,31): error TS2344: Type '[number, number, string]' does not satisfy the constraint '[number, ...number[]]'. | |||
| Type at positions 1 through 2 in source is not compatible with type at position 1 in target. | |||
| Type 'string | number' is not assignable to type 'number'. | |||
| Type 'string' is not assignable to type 'number'. | |||
| Type 'string' is not assignable to type 'number'. | |||
There was a problem hiding this comment.
A change like this is correct with the new logic. As mentioned in the other comment - I no longer create unions from "tuple slices". That's why the reported source is just a "single" type here and not a union.
I think that perhaps this error could be somehow improved/rephrased cause it mentions a position span in the source where in fact the type mismatch happens only on a given position (or only on some positions of this span). The mentioned span covers all of the positions in the source that must match a single position of the rest in the target.
Note though that reporting a single type here is not completely new, this could always happen in cases like this:
declare let tt3: [number, string, ...string[]];
let tt4: [number, ...number[]] = tt3;
// Type '[number, string, ...string[]]' is not assignable to type '[number, ...number[]]'.
// Type at positions 1 through 2 in source is not compatible with type at position 1 in target.
// Type 'string' is not assignable to type 'number'.(2322) I think there is a potential of just dropping this specific error:
https://github.com/microsoft/TypeScript/pull/50218/files#diff-d9ab6589e714c71e657f601cf30ff51dfc607fc98419bf72e04f6b0fa92cc4b8R20329
in favor of the existing simpler one:
https://github.com/microsoft/TypeScript/pull/50218/files#diff-d9ab6589e714c71e657f601cf30ff51dfc607fc98419bf72e04f6b0fa92cc4b8R20332
I've decided not to introduce this change here to limit the scope of the PR and to minimize the initial diff to make it easier to review it.
| !!! error TS2322: Type 'T' is not assignable to type 'number[]'. | ||
| !!! error TS2322: Type 'unknown[]' is not assignable to type 'number[]'. | ||
| !!! error TS2322: Type 'unknown' is not assignable to type 'number'. |
There was a problem hiding this comment.
As far as I can tell this is still correct, the reported error is now slightly different cause of the change in the logic (position by position comparison instead of comparing an indexed slice of the source to the target type)
typescript-bot
added
For Backlog Bug
| @@ -94,14 +92,12 @@ tests/cases/conformance/types/tuple/restTupleElements1.ts(59,4): error TS2345: A | |||
| ~~~~~~~~~~~~~~~~~~~~~~~~ | |||
| !!! error TS2344: Type '[number, number, string]' does not satisfy the constraint '[number, ...number[]]'. | |||
| !!! error TS2344: Type at positions 1 through 2 in source is not compatible with type at position 1 in target. | |||
| !!! error TS2344: Type 'string | number' is not assignable to type 'number'. | |||
| !!! error TS2344: Type 'string' is not assignable to type 'number'. | |||
| !!! error TS2344: Type 'string' is not assignable to type 'number'. | |||
There was a problem hiding this comment.
I'm ok with that change. The message could be better, because what ultimately fails is the type at position 2 in source, so we could be more specific. But considering we had the same problem before this PR, it seems out of scope for this PR.
There was a problem hiding this comment.
I could improve on this error message here or in a followup PR - I would need a confirmation on how this should be reported though. I guess that in this case, I should drop the "positions X through Y" bit and just report this error for the fixed~ position. IIRC this would mean that TS2344 would no longer be reported anywhere - and it's the primary reason why I've left it like this. I wasn't sure how I should approach removing an existing error code.
| assign<[number, ...number[]], [number, number, number, string]>(); // Error | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
| !!! error TS2344: Type '[number, number, number, string]' does not satisfy the constraint '[number, ...number[]]'. | ||
| !!! error TS2344: Type at positions 1 through 3 in source is not compatible with type at position 1 in target. | ||
| !!! error TS2344: Type 'string | number' is not assignable to type 'number'. | ||
| !!! error TS2344: Type 'string' is not assignable to type 'number'. | ||
| !!! error TS2344: Type 'string' is not assignable to type 'number'. |
There was a problem hiding this comment.
Same here, given what we've had before, I think this is good. It actually might be more clear than the previous message, because before we had type 'string | number' mentioned, but the original code doesn't really have that union type, so that could be confusing.
There was a problem hiding this comment.
Yeah, I also found the union type being reported here kinda unintuitive.
|
@ahejlsberg I believe you were the last one to work on the tuple length assignability rules. Can you take a look at this too? |
|
@typescript-bot test this |
|
Heya @gabritto, I've started to run the perf test suite on this PR at ab0eb5c. You can monitor the build here. Update: The results are in! |
|
Heya @gabritto, I've started to run the diff-based user code test suite on this PR at ab0eb5c. You can monitor the build here. Update: The results are in! |
|
Heya @gabritto, I've started to run the diff-based top-repos suite on this PR at ab0eb5c. You can monitor the build here. Update: The results are in! |
|
@gabritto Here are the results of running the user test suite comparing Everything looks good! |
|
@gabritto Here they are:
CompilerComparison Report - main..50218
System
Hosts
Scenarios
TSServerComparison Report - main..50218
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot run dt |
|
@gabritto Here are the results of running the top-repos suite comparing Everything looks good! |
|
Ok, all the tests look clean, so I think we're good to merge this once we snap for 5.0. Thanks! |
fixes #50216