Fixed spreading iterables into arguments list directly into rest-only signatures #52838
Conversation
| checkExpressionWithContextualType((arg as SpreadElement).expression, restType, context, checkMode); | ||
|
|
||
| if (isArrayLikeType(spreadType)) { | ||
| return getMutableArrayOrTupleType(spreadType); |
There was a problem hiding this comment.
likely this shouldn't always produce mutable types but there are other bugs here, like this one:
declare function foo<const T extends readonly unknown[]>(...args: T): T;
const p = foo(...[1, 'bar']); // A spread argument must either have a tuple type or be passed to a rest parameter.(2556)
const p2 = foo(true, ...[1, 'bar']); // OKI would prefer to fix this in a followup PR as the fix for this largely lies elsewhere - then I could revisit this piece of code to produce mutable/readonly array/tuple types based on inConstContext.
Sideways q - should T in the broken case be inferred as readonly (number | string)[] or should it be readonly [1, 'bar']? In other words - should the spread array expression inherit the const context?
There was a problem hiding this comment.
Sideways q - should
Tin the broken case be inferred asreadonly (number | string)[]or should it bereadonly [1, 'bar']? In other words - should the spread array expression inherit the const context?
I think the latter?
There was a problem hiding this comment.
The justification in my mind being that it should match what you get if you write foo(1, 'bar').
There was a problem hiding this comment.
I also think that the latter is better but currently it's complicated to implement it. IIRC the easiest way to achieve this would be through getEffectiveCallArguments but it creates a cycle for spreads. getContextualTypeForArgument/getContextualTypeForArgumentAtIndex call back into getEffectiveCallArguments. I think that I've even seen a comment somewhere about spread arguments never needing the contextual type - so this would be the first time when that would actually have to become supported.
There was a problem hiding this comment.
Maybe I'm missing something, but is it possible to yank the required code out of getEffectiveCallArguments into a helper so that it's not directly used?
There was a problem hiding this comment.
Yeah, maybe. I can't say that I explored this in full yet. I might have also missed some additional things from my initial investigation, it's a little bit stale in my memory ;p
I could try to give this a shot in a followup PR unless you strongly think that it should go in together.
There was a problem hiding this comment.
Unless I'm mistaken, your code sample behaves the same between main and this PR, so, I feel like it's fine to leave as-is and then report a bug for further refinement.
There was a problem hiding this comment.
(But do tell me if I'm right or wrong here)
There was a problem hiding this comment.
Unless I'm mistaken, your code sample behaves the same between main and this PR,
Ye, the behavior is the same.
I feel like it's fine to leave as-is and then report a bug for further refinement.
It seems that I already started to work on further improvements related to this here (who could have guessed that? 😱 ) and I even mentioned some more details around that issue with getEffectiveCallArguments in the comment here
There was a problem hiding this comment.
Ah, I see your memory is as stale as mine is on a regular basis
| ;(function(a, ...rest) {})('', true, ...itNum) | ||
|
|
||
| declare function fn1<const T extends readonly unknown[]>(...args: T): T; | ||
| const res1 = fn1(..."hello"); |
There was a problem hiding this comment.
I would also prefer to handle this in a follow-up PR - but one additional issue comes to mind here. Should this produce readonly string[] (like now) or should it actually be readonly ['h', 'e', 'l', 'l', 'o']? :P
The existing behavior is the current one, since in the case that works the readonly string[] gets inferred:
declare function foo<const T extends readonly unknown[]>(...args: T): T;
const p2 = foo(true, ...'hello');
// ^? readonly [true, ...string[]]There was a problem hiding this comment.
It seems complicated to do that, so I'd probably say to start with the easier option.
There was a problem hiding this comment.
Ye, I definitely want to start with the easier option (what is implemented today). Spreading into characters wouldn't have to be that complicated though since we could just expand the string into an array using JS itself:
[...'hello'].map(char => /* create string literal types */)This solution has a nice advantage... it would work correctly with emojis 👨🎤
There is one potential problem with this though and I will touch on it in a different comment.
| @@ -0,0 +1,22 @@ | |||
| // @strict: true | |||
| // @noEmit: true | |||
| // @target: es5, esnext | |||
There was a problem hiding this comment.
Any reason to be testing es5 here?
There was a problem hiding this comment.
Basically to ensure you can't spread strings in es5 and that errors are expected to be reported for that. There is IterationUse.AllowsStringInputFlag that is used by for-of that allows strings to be iterated over in es5 for that purpose. I'm not sure why it's like that (and IIRC it's only allowed since es5 so it's not allowed in es3, I'm also not sure why that is. So overall, this just makes sure that this flag is not used in es5 context.
|
@typescript-bot test this |
|
Heya @jakebailey, I've started to run the perf test suite on this PR at a570cdf. 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 a570cdf. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the extended test suite on this PR at a570cdf. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at a570cdf. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at a570cdf. 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 a570cdf. You can monitor the build here. |
|
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! |
|
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
|
@jakebailey Here they are:
CompilerComparison Report - main..52838
System
Hosts
Scenarios
TSServerComparison Report - main..52838
System
Hosts
Scenarios
StartupComparison Report - main..52838
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
This comment was marked as outdated.
This comment was marked as outdated.
fixes #52781 , cc @RyanCavanaugh