Improve signature assignability for argument lists of different lengths #49218
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…aram with tuple union
Andarist
commented
May 23, 2022
Andarist
commented
May 23, 2022
Andarist
commented
May 23, 2022
| Source has 2 element(s) but target allows only 1. | ||
| Type '[string] | [number]' is not assignable to type '[y: string]'. | ||
| Type '[number]' is not assignable to type '[y: string]'. | ||
| Type 'number' is not assignable to type 'string'. |
There was a problem hiding this comment.
To the best of my understanding, this change is correct and welcome under the implemented changes.
|
Actually, my simplistic "expand" idea wouldn't work with leading/middle rest elements. However, one could write a correct logic based on the code here: It is a little bit convoluted though, gonna continue looking into this. |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
That is definitely out of the scope of this PR. I don't intend to work on something like this - also, once you start involving generics in this then unions and overloads might stop being the same. Union means one of the members - any member, it's order-independent. OTOH, overloads are always matched from the top to the bottom so the first match "wins". |
This comment was marked as outdated.
This comment was marked as outdated.
|
I think I misunderstood the mechanism before as wrapping the arguments just affects the inference part. See #45972 (comment). I just look though the checker and wonder why |
…shorter-contextual-params
… using union of tuples as its rest
Andarist
force-pushed
the
fix/rest-tuple-union-shorter-contextual-params
branch
from
f7feca5 to
88027d5
Compare
Andarist
commented
Dec 28, 2022
Comment on lines
+80
to
+83
| !!! error TS2322: Type '[number, boolean] | [string, undefined]' is not assignable to type '[y: number, z: boolean]'. | ||
| !!! error TS2322: Type '[string, undefined]' is not assignable to type '[y: number, z: boolean]'. | ||
| !!! error TS2322: Type at position 0 in source is not compatible with type at position 0 in target. | ||
| !!! error TS2322: Type 'string' is not assignable to type 'number'. |
There was a problem hiding this comment.
This error change comes from the fact that I fill the slice of the target type with undefineds here (when it's shorter than the source's slice):
https://github.dev/microsoft/TypeScript/blob/6c5383d701417e8a98398b4171af0f201ee593c5/src/compiler/checker.ts#L19394-L19402
I think this is a good change. This will allow more patterns to be assignable, especially with target signatures using unions of tuples for their rest parameter. After all, the implementation is free to ignore any of the provided arguments etc. This is also already just fine in TS:
declare let f1: (a: string) => void
declare let f2: (a: string, b: number) => void
f2 = f1And since this is OK I think that this one should be too:
declare let f1: (x: string, ...args: [string] | [number, boolean]) => void;
// Type '(a: string, b: string | number, c: boolean | undefined) => void' is not assignable to type '(x: string, ...args: [string] | [number, boolean]) => void'.
// Types of parameters 'b' and 'args' are incompatible.
// Type '[string] | [number, boolean]' is not assignable to type '[b: string | number, c: boolean | undefined]'.
// Type '[string]' is not assignable to type '[b: string | number, c: boolean | undefined]'.
// Source has 1 element(s) but target requires 2.
f1 = (a, b, c) => {}So with this change that caused this particular error here to be reported differently we now allow the example above and I added a test case for it:
Andarist
commented
Dec 28, 2022
Comment on lines
+73
to
+75
| !!! error TS2322: Type '[string] | [number]' is not assignable to type '[y: string]'. | ||
| !!! error TS2322: Type '[number]' is not assignable to type '[y: string]'. | ||
| !!! error TS2322: Type 'number' is not assignable to type 'string'. |
There was a problem hiding this comment.
This is a change of the error for this case:
declare let f1: (x: string, ...args: [string] | [number, boolean]) => void;
declare let f2: (x: string, y: string) => void;
f1 = f2With the current changes in this PR, I create a slice from the target tuple and I cap it to the source's length~. I do that for similar reasons that I've outlined in the other comment. The source signature can freely ignore the "extra" arguments so we don't even need to check them here (we'd have to ignore them through some other mechanism anyway)
|
@jcalz this PR implements changes for the problem that you have noticed here. I pushed out tests for those cases in this commit |
|
Heya @gabritto, I've started to run the regular perf test suite on this PR at 608f7c3. You can monitor the build here. Update: The results are in! |
|
@gabritto 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
|
|
Hey @gabritto, the results of running the DT tests are ready. |
|
@gabritto Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
|
@gabritto Here they are:
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot perf test this |
|
Heya @gabritto, I've started to run the public perf test suite on this PR at 608f7c3. You can monitor the build here. Update: The results are in! |
|
Heya @gabritto, I've started to run the regular perf test suite on this PR at 608f7c3. You can monitor the build here. Update: The results are in! |
|
@gabritto Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@gabritto Here they are:
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot pack this |
|
Hey @gabritto, 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 |
…shorter-contextual-params
|
@typescript-bot test it |
|
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
|
@jakebailey Here are the results of running the user tests comparing Everything looks good! |
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
jakebailey
reviewed
Apr 19, 2024
Comment on lines
+20773
to
+20774
| const elementTypes: Type[] = []; | ||
| const elementFlags: ElementFlags[] = []; |
There was a problem hiding this comment.
Typically we use append rather than pushing onto an empty list directly; this is typically more efficient due to how the runtime constructs arrays.
Comment on lines
+24506
to
+24507
| !!(tupleStructureComparisonKind & TupleStructureComparisonKind.MatchFixed && f1 & ElementFlags.Fixed && f2 & ElementFlags.Fixed) || | ||
| !!(tupleStructureComparisonKind & TupleStructureComparisonKind.MatchVariable && f1 & ElementFlags.Variable && f2 & ElementFlags.Variable); |
There was a problem hiding this comment.
Why aren't these something like (f1 & ElementFlags.Fixed) === (f2 & ElementFlags.Fixed), as the original code did?
There was a problem hiding this comment.
Hm, I guess that breaks things.
| @@ -24440,9 +24498,14 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| return undefined; | |||
| } | |||
|
|
|||
| function isTupleTypeStructureMatching(t1: TupleTypeReference, t2: TupleTypeReference) { | |||
| function isTupleTypeStructureMatching(t1: TupleTypeReference, t2: TupleTypeReference, tupleStructureComparisonKind: TupleStructureComparisonKind) { | |||
There was a problem hiding this comment.
Curious if we could just accept ElementFlags as a mask here instead of coming up with a new enum.
|
@typescript-bot pack this |
gabritto
requested changes
Apr 19, 2024
There was a problem hiding this comment.
Ok I did review this a second time earlier this year, and I left a comment with a case that I think might be wrong with this PR, but forgot to "request changes": #49218 (comment)
Sorry about that.
|
Hey @gabritto, 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 top 400 repos comparing Everything looks good! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #48663
fixes #45972
fixes #56766