Skip to content

Fixed spreading iterables into arguments list directly into rest-only signatures #52838

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 2, 2023
Merged

Fixed spreading iterables into arguments list directly into rest-only signatures #52838

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9f68c1c
Fixed spreading iterables into arguments list directly into rest-only…
Andarist Feb 18, 2023
a570cdf
add extra test cases
Andarist Feb 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32200,19 +32200,26 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
}

function getSpreadArgumentType(args: readonly Expression[], index: number, argCount: number, restType: Type, context: InferenceContext | undefined, checkMode: CheckMode) {
const inConstContext = isConstTypeVariable(restType);

if (index >= argCount - 1) {
const arg = args[argCount - 1];
if (isSpreadArgument(arg)) {
// We are inferring from a spread expression in the last argument position, i.e. both the parameter
// and the argument are ...x forms.
return getMutableArrayOrTupleType(arg.kind === SyntaxKind.SyntheticExpression ? (arg as SyntheticExpression).type :
checkExpressionWithContextualType((arg as SpreadElement).expression, restType, context, checkMode));
const spreadType = arg.kind === SyntaxKind.SyntheticExpression ? (arg as SyntheticExpression).type :
checkExpressionWithContextualType((arg as SpreadElement).expression, restType, context, checkMode);

if (isArrayLikeType(spreadType)) {
return getMutableArrayOrTupleType(spreadType);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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']); // OK

I 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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

I think the latter?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The justification in my mind being that it should match what you get if you write foo(1, 'bar').

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(But do tell me if I'm right or wrong here)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see your memory is as stale as mine is on a regular basis

}

return createArrayType(checkIteratedTypeOrElementType(IterationUse.Spread, spreadType, undefinedType, arg.kind === SyntaxKind.SpreadElement ? (arg as SpreadElement).expression : arg), inConstContext);
}
}
const types = [];
const flags = [];
const names = [];
const inConstContext = isConstTypeVariable(restType);
for (let i = index; i < argCount; i++) {
const arg = args[i];
if (isSpreadArgument(arg)) {
Expand Down
37 changes: 37 additions & 0 deletions tests/baselines/reference/argumentsSpreadRestIterables(target=es5).errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
tests/cases/compiler/argumentsSpreadRestIterables.tsx(1,22): error TS2304: Cannot find name 'Iterable'.
tests/cases/compiler/argumentsSpreadRestIterables.tsx(8,21): error TS2461: Type '"hello"' is not an array type.
tests/cases/compiler/argumentsSpreadRestIterables.tsx(10,27): error TS2461: Type '"hello"' is not an array type.
tests/cases/compiler/argumentsSpreadRestIterables.tsx(15,19): error TS2461: Type '"hello"' is not an array type.
tests/cases/compiler/argumentsSpreadRestIterables.tsx(17,25): error TS2461: Type '"hello"' is not an array type.


==== tests/cases/compiler/argumentsSpreadRestIterables.tsx (5 errors) ====
declare const itNum: Iterable<number>
~~~~~~~~
!!! error TS2304: Cannot find name 'Iterable'.

;(function(...rest) {})(...itNum)
;(function(a, ...rest) {})('', ...itNum)
;(function(a, ...rest) {})('', true, ...itNum)

declare function fn1<const T extends readonly unknown[]>(...args: T): T;
const res1 = fn1(..."hello");
~~~~~~~
!!! error TS2461: Type '"hello"' is not an array type.
const res2 = fn1(...itNum);
const res3 = fn1(true, ..."hello");
~~~~~~~
!!! error TS2461: Type '"hello"' is not an array type.
const res4 = fn1(true, ...itNum);

// repro from #52781
declare function foo<T extends unknown[]>(...args: T): T;
const p1 = foo(..."hello");
~~~~~~~
!!! error TS2461: Type '"hello"' is not an array type.
const p2 = foo(...itNum);
const p3 = foo(true, ..."hello");
~~~~~~~
!!! error TS2461: Type '"hello"' is not an array type.
const p4 = foo(true, ...itNum);

70 changes: 70 additions & 0 deletions tests/baselines/reference/argumentsSpreadRestIterables(target=es5).symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
=== tests/cases/compiler/argumentsSpreadRestIterables.tsx ===
declare const itNum: Iterable<number>
>itNum : Symbol(itNum, Decl(argumentsSpreadRestIterables.tsx, 0, 13))
>Iterable : Symbol(Iterable)

;(function(...rest) {})(...itNum)
>rest : Symbol(rest, Decl(argumentsSpreadRestIterables.tsx, 2, 11))
>itNum : Symbol(itNum, Decl(argumentsSpreadRestIterables.tsx, 0, 13))

;(function(a, ...rest) {})('', ...itNum)
>a : Symbol(a, Decl(argumentsSpreadRestIterables.tsx, 3, 11))
>rest : Symbol(rest, Decl(argumentsSpreadRestIterables.tsx, 3, 13))
>itNum : Symbol(itNum, Decl(argumentsSpreadRestIterables.tsx, 0, 13))

;(function(a, ...rest) {})('', true, ...itNum)
>a : Symbol(a, Decl(argumentsSpreadRestIterables.tsx, 4, 11))
>rest : Symbol(rest, Decl(argumentsSpreadRestIterables.tsx, 4, 13))
>itNum : Symbol(itNum, Decl(argumentsSpreadRestIterables.tsx, 0, 13))

declare function fn1<const T extends readonly unknown[]>(...args: T): T;
>fn1 : Symbol(fn1, Decl(argumentsSpreadRestIterables.tsx, 4, 46))
>T : Symbol(T, Decl(argumentsSpreadRestIterables.tsx, 6, 21))
>args : Symbol(args, Decl(argumentsSpreadRestIterables.tsx, 6, 57))
>T : Symbol(T, Decl(argumentsSpreadRestIterables.tsx, 6, 21))
>T : Symbol(T, Decl(argumentsSpreadRestIterables.tsx, 6, 21))

const res1 = fn1(..."hello");
>res1 : Symbol(res1, Decl(argumentsSpreadRestIterables.tsx, 7, 5))
>fn1 : Symbol(fn1, Decl(argumentsSpreadRestIterables.tsx, 4, 46))

const res2 = fn1(...itNum);
>res2 : Symbol(res2, Decl(argumentsSpreadRestIterables.tsx, 8, 5))
>fn1 : Symbol(fn1, Decl(argumentsSpreadRestIterables.tsx, 4, 46))
>itNum : Symbol(itNum, Decl(argumentsSpreadRestIterables.tsx, 0, 13))

const res3 = fn1(true, ..."hello");
>res3 : Symbol(res3, Decl(argumentsSpreadRestIterables.tsx, 9, 5))
>fn1 : Symbol(fn1, Decl(argumentsSpreadRestIterables.tsx, 4, 46))

const res4 = fn1(true, ...itNum);
>res4 : Symbol(res4, Decl(argumentsSpreadRestIterables.tsx, 10, 5))
>fn1 : Symbol(fn1, Decl(argumentsSpreadRestIterables.tsx, 4, 46))
>itNum : Symbol(itNum, Decl(argumentsSpreadRestIterables.tsx, 0, 13))

// repro from #52781
declare function foo<T extends unknown[]>(...args: T): T;
>foo : Symbol(foo, Decl(argumentsSpreadRestIterables.tsx, 10, 33))
>T : Symbol(T, Decl(argumentsSpreadRestIterables.tsx, 13, 21))
>args : Symbol(args, Decl(argumentsSpreadRestIterables.tsx, 13, 42))
>T : Symbol(T, Decl(argumentsSpreadRestIterables.tsx, 13, 21))
>T : Symbol(T, Decl(argumentsSpreadRestIterables.tsx, 13, 21))

const p1 = foo(..."hello");
>p1 : Symbol(p1, Decl(argumentsSpreadRestIterables.tsx, 14, 5))
>foo : Symbol(foo, Decl(argumentsSpreadRestIterables.tsx, 10, 33))

const p2 = foo(...itNum);
>p2 : Symbol(p2, Decl(argumentsSpreadRestIterables.tsx, 15, 5))
>foo : Symbol(foo, Decl(argumentsSpreadRestIterables.tsx, 10, 33))
>itNum : Symbol(itNum, Decl(argumentsSpreadRestIterables.tsx, 0, 13))

const p3 = foo(true, ..."hello");
>p3 : Symbol(p3, Decl(argumentsSpreadRestIterables.tsx, 16, 5))
>foo : Symbol(foo, Decl(argumentsSpreadRestIterables.tsx, 10, 33))

const p4 = foo(true, ...itNum);
>p4 : Symbol(p4, Decl(argumentsSpreadRestIterables.tsx, 17, 5))
>foo : Symbol(foo, Decl(argumentsSpreadRestIterables.tsx, 10, 33))
>itNum : Symbol(itNum, Decl(argumentsSpreadRestIterables.tsx, 0, 13))

102 changes: 102 additions & 0 deletions tests/baselines/reference/argumentsSpreadRestIterables(target=es5).types
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
=== tests/cases/compiler/argumentsSpreadRestIterables.tsx ===
declare const itNum: Iterable<number>
>itNum : Iterable<number>

;(function(...rest) {})(...itNum)
>(function(...rest) {})(...itNum) : void
>(function(...rest) {}) : (...rest: Iterable<number>) => void
>function(...rest) {} : (...rest: Iterable<number>) => void
>rest : Iterable<number>
>...itNum : Iterable<number>
>itNum : Iterable<number>

;(function(a, ...rest) {})('', ...itNum)
>(function(a, ...rest) {})('', ...itNum) : void
>(function(a, ...rest) {}) : (a: string, ...rest: Iterable<number>) => void
>function(a, ...rest) {} : (a: string, ...rest: Iterable<number>) => void
>a : string
>rest : Iterable<number>
>'' : ""
>...itNum : Iterable<number>
>itNum : Iterable<number>

;(function(a, ...rest) {})('', true, ...itNum)
>(function(a, ...rest) {})('', true, ...itNum) : void
>(function(a, ...rest) {}) : (a: string, rest_0: boolean, ...rest_1: any[]) => void
>function(a, ...rest) {} : (a: string, rest_0: boolean, ...rest_1: any[]) => void
>a : string
>rest : [boolean, ...any[]]
>'' : ""
>true : true
>...itNum : Iterable<number>
>itNum : Iterable<number>

declare function fn1<const T extends readonly unknown[]>(...args: T): T;
>fn1 : <const T extends readonly unknown[]>(...args: T) => T
>args : T

const res1 = fn1(..."hello");
>res1 : readonly any[]
>fn1(..."hello") : readonly any[]
>fn1 : <const T extends readonly unknown[]>(...args: T) => T
>..."hello" : any
>"hello" : "hello"

const res2 = fn1(...itNum);
>res2 : Iterable<number>
>fn1(...itNum) : Iterable<number>
>fn1 : <const T extends readonly unknown[]>(...args: T) => T
>...itNum : Iterable<number>
>itNum : Iterable<number>

const res3 = fn1(true, ..."hello");
>res3 : readonly [true, ...any[]]
>fn1(true, ..."hello") : readonly [true, ...any[]]
>fn1 : <const T extends readonly unknown[]>(...args: T) => T
>true : true
>..."hello" : any
>"hello" : "hello"

const res4 = fn1(true, ...itNum);
>res4 : readonly [true, ...any[]]
>fn1(true, ...itNum) : readonly [true, ...any[]]
>fn1 : <const T extends readonly unknown[]>(...args: T) => T
>true : true
>...itNum : Iterable<number>
>itNum : Iterable<number>

// repro from #52781
declare function foo<T extends unknown[]>(...args: T): T;
>foo : <T extends unknown[]>(...args: T) => T
>args : T

const p1 = foo(..."hello");
>p1 : any[]
>foo(..."hello") : any[]
>foo : <T extends unknown[]>(...args: T) => T
>..."hello" : any
>"hello" : "hello"

const p2 = foo(...itNum);
>p2 : Iterable<number>
>foo(...itNum) : Iterable<number>
>foo : <T extends unknown[]>(...args: T) => T
>...itNum : Iterable<number>
>itNum : Iterable<number>

const p3 = foo(true, ..."hello");
>p3 : [boolean, ...any[]]
>foo(true, ..."hello") : [boolean, ...any[]]
>foo : <T extends unknown[]>(...args: T) => T
>true : true
>..."hello" : any
>"hello" : "hello"

const p4 = foo(true, ...itNum);
>p4 : [boolean, ...any[]]
>foo(true, ...itNum) : [boolean, ...any[]]
>foo : <T extends unknown[]>(...args: T) => T
>true : true
>...itNum : Iterable<number>
>itNum : Iterable<number>

70 changes: 70 additions & 0 deletions tests/baselines/reference/argumentsSpreadRestIterables(target=esnext).symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
=== tests/cases/compiler/argumentsSpreadRestIterables.tsx ===
declare const itNum: Iterable<number>
>itNum : Symbol(itNum, Decl(argumentsSpreadRestIterables.tsx, 0, 13))
>Iterable : Symbol(Iterable, Decl(lib.es2015.iterable.d.ts, --, --))

;(function(...rest) {})(...itNum)
>rest : Symbol(rest, Decl(argumentsSpreadRestIterables.tsx, 2, 11))
>itNum : Symbol(itNum, Decl(argumentsSpreadRestIterables.tsx, 0, 13))

;(function(a, ...rest) {})('', ...itNum)
>a : Symbol(a, Decl(argumentsSpreadRestIterables.tsx, 3, 11))
>rest : Symbol(rest, Decl(argumentsSpreadRestIterables.tsx, 3, 13))
>itNum : Symbol(itNum, Decl(argumentsSpreadRestIterables.tsx, 0, 13))

;(function(a, ...rest) {})('', true, ...itNum)
>a : Symbol(a, Decl(argumentsSpreadRestIterables.tsx, 4, 11))
>rest : Symbol(rest, Decl(argumentsSpreadRestIterables.tsx, 4, 13))
>itNum : Symbol(itNum, Decl(argumentsSpreadRestIterables.tsx, 0, 13))

declare function fn1<const T extends readonly unknown[]>(...args: T): T;
>fn1 : Symbol(fn1, Decl(argumentsSpreadRestIterables.tsx, 4, 46))
>T : Symbol(T, Decl(argumentsSpreadRestIterables.tsx, 6, 21))
>args : Symbol(args, Decl(argumentsSpreadRestIterables.tsx, 6, 57))
>T : Symbol(T, Decl(argumentsSpreadRestIterables.tsx, 6, 21))
>T : Symbol(T, Decl(argumentsSpreadRestIterables.tsx, 6, 21))

const res1 = fn1(..."hello");
>res1 : Symbol(res1, Decl(argumentsSpreadRestIterables.tsx, 7, 5))
>fn1 : Symbol(fn1, Decl(argumentsSpreadRestIterables.tsx, 4, 46))

const res2 = fn1(...itNum);
>res2 : Symbol(res2, Decl(argumentsSpreadRestIterables.tsx, 8, 5))
>fn1 : Symbol(fn1, Decl(argumentsSpreadRestIterables.tsx, 4, 46))
>itNum : Symbol(itNum, Decl(argumentsSpreadRestIterables.tsx, 0, 13))

const res3 = fn1(true, ..."hello");
>res3 : Symbol(res3, Decl(argumentsSpreadRestIterables.tsx, 9, 5))
>fn1 : Symbol(fn1, Decl(argumentsSpreadRestIterables.tsx, 4, 46))

const res4 = fn1(true, ...itNum);
>res4 : Symbol(res4, Decl(argumentsSpreadRestIterables.tsx, 10, 5))
>fn1 : Symbol(fn1, Decl(argumentsSpreadRestIterables.tsx, 4, 46))
>itNum : Symbol(itNum, Decl(argumentsSpreadRestIterables.tsx, 0, 13))

// repro from #52781
declare function foo<T extends unknown[]>(...args: T): T;
>foo : Symbol(foo, Decl(argumentsSpreadRestIterables.tsx, 10, 33))
>T : Symbol(T, Decl(argumentsSpreadRestIterables.tsx, 13, 21))
>args : Symbol(args, Decl(argumentsSpreadRestIterables.tsx, 13, 42))
>T : Symbol(T, Decl(argumentsSpreadRestIterables.tsx, 13, 21))
>T : Symbol(T, Decl(argumentsSpreadRestIterables.tsx, 13, 21))

const p1 = foo(..."hello");
>p1 : Symbol(p1, Decl(argumentsSpreadRestIterables.tsx, 14, 5))
>foo : Symbol(foo, Decl(argumentsSpreadRestIterables.tsx, 10, 33))

const p2 = foo(...itNum);
>p2 : Symbol(p2, Decl(argumentsSpreadRestIterables.tsx, 15, 5))
>foo : Symbol(foo, Decl(argumentsSpreadRestIterables.tsx, 10, 33))
>itNum : Symbol(itNum, Decl(argumentsSpreadRestIterables.tsx, 0, 13))

const p3 = foo(true, ..."hello");
>p3 : Symbol(p3, Decl(argumentsSpreadRestIterables.tsx, 16, 5))
>foo : Symbol(foo, Decl(argumentsSpreadRestIterables.tsx, 10, 33))

const p4 = foo(true, ...itNum);
>p4 : Symbol(p4, Decl(argumentsSpreadRestIterables.tsx, 17, 5))
>foo : Symbol(foo, Decl(argumentsSpreadRestIterables.tsx, 10, 33))
>itNum : Symbol(itNum, Decl(argumentsSpreadRestIterables.tsx, 0, 13))

Loading