Skip to content

Fixed parenthesized array literal expressions spread in calls not being tupleized #54623

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 1 commit into from
Jun 29, 2023
Merged

Fixed parenthesized array literal expressions spread in calls not being tupleized #54623

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 7 additions & 3 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29937,17 +29937,21 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
(node.kind === SyntaxKind.BinaryExpression && (node as BinaryExpression).operatorToken.kind === SyntaxKind.EqualsToken);
}

function isSpreadIntoCallOrNew(node: ArrayLiteralExpression) {
const parent = walkUpParenthesizedExpressions(node.parent);
return isSpreadElement(parent) && isCallOrNewExpression(parent.parent);
}

function checkArrayLiteral(node: ArrayLiteralExpression, checkMode: CheckMode | undefined, forceTuple: boolean | undefined): Type {
const elements = node.elements;
const elementCount = elements.length;
const elementTypes: Type[] = [];
const elementFlags: ElementFlags[] = [];
pushCachedContextualType(node);
const inDestructuringPattern = isAssignmentTarget(node);
const isSpreadIntoCallOrNew = isSpreadElement(node.parent) && isCallOrNewExpression(node.parent.parent);
const inConstContext = isSpreadIntoCallOrNew || isConstContext(node);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

While reading the source code today I noticed that isSpreadIntoCallOrNew here (as part of inConstContext).

I think it was semantically incorrect (and I added it here). It didn't exactly create any issues because isConstContext here is mainly used to decide if the checked type should be readonly or not.

Since I removed part of the condition we can now observe removal of the readonly modifier from some snapshot tests. I think it's fine... or at least, I wasn't yet able to prove that those tuples should be readonly. Even in actual const contexts etc a mutable array is still fine. Even if it turns out that the readonly modifier is needed for some case, I think that it should be added independently, based on isSpreadIntoCallOrNew.

As a bonus, I noticed that parenthesized expressions were not handled here so I improved this.

Copy link
Member

Choose a reason for hiding this comment

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

The changed baselines that didn’t strictly need to change make this hard to approve. I’d rather not find out that the readonly was important in an untested edge case because of a regression.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I split this into 2 PRs then? Note that the change that allowed those and introduced those readonly modifiers in some cases were only merged into 5.1 so removing it really shouldn't affect many people.

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 missed that the same PR originally made them readonly. Probably fine then

const inConstContext = isConstContext(node);
const contextualType = getApparentTypeOfContextualType(node, /*contextFlags*/ undefined);
const inTupleContext = isSpreadIntoCallOrNew || !!contextualType && someType(contextualType, isTupleLikeType);
const inTupleContext = isSpreadIntoCallOrNew(node) || !!contextualType && someType(contextualType, isTupleLikeType);
let hasOmittedExpression = false;
for (let i = 0; i < elementCount; i++) {
const e = elements[i];
Expand Down
13 changes: 12 additions & 1 deletion tests/baselines/reference/arraySpreadInCall.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
arraySpreadInCall.ts(21,12): error TS2554: Expected 0-1 arguments, but got 2.
arraySpreadInCall.ts(32,12): error TS2554: Expected 0-1 arguments, but got 2.


==== arraySpreadInCall.ts (1 errors) ====
Expand All @@ -8,14 +8,25 @@ arraySpreadInCall.ts(21,12): error TS2554: Expected 0-1 arguments, but got 2.
f1(1, 2, ...[3, 4], 5, 6);
f1(1, 2, ...[3], 4, ...[5, 6]);
f1(...[1, 2], ...[3, 4], ...[5, 6]);
f1(...(([1, 2])), ...(((([3, 4])))), ...([5, 6]));

declare function f2<T extends unknown[]>(...args: T): T;
const x21 = f2(...[1, 'foo'])
const x22 = f2(true, ...[1, 'foo'])
const x23 = f2(...([1, 'foo']))
const x24 = f2(true, ...([1, 'foo']))

declare function f3<T extends readonly unknown[]>(...args: T): T;
const x31 = f3(...[1, 'foo'])
const x32 = f3(true, ...[1, 'foo'])
const x33 = f3(...([1, 'foo']))
const x34 = f3(true, ...([1, 'foo']))

declare function f4<const T extends readonly unknown[]>(...args: T): T;
const x41 = f4(...[1, 'foo'])
const x42 = f4(true, ...[1, 'foo'])
const x43 = f4(...([1, 'foo']))
const x44 = f4(true, ...([1, 'foo']))

// dicovered in #52845#issuecomment-1459132562
interface IAction {
Expand Down
94 changes: 68 additions & 26 deletions tests/baselines/reference/arraySpreadInCall.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,51 +25,93 @@ f1(1, 2, ...[3], 4, ...[5, 6]);
f1(...[1, 2], ...[3, 4], ...[5, 6]);
>f1 : Symbol(f1, Decl(arraySpreadInCall.ts, 0, 0))

f1(...(([1, 2])), ...(((([3, 4])))), ...([5, 6]));
>f1 : Symbol(f1, Decl(arraySpreadInCall.ts, 0, 0))

declare function f2<T extends unknown[]>(...args: T): T;
>f2 : Symbol(f2, Decl(arraySpreadInCall.ts, 5, 36))
>T : Symbol(T, Decl(arraySpreadInCall.ts, 7, 20))
>args : Symbol(args, Decl(arraySpreadInCall.ts, 7, 41))
>T : Symbol(T, Decl(arraySpreadInCall.ts, 7, 20))
>T : Symbol(T, Decl(arraySpreadInCall.ts, 7, 20))
>f2 : Symbol(f2, Decl(arraySpreadInCall.ts, 6, 50))
>T : Symbol(T, Decl(arraySpreadInCall.ts, 8, 20))
>args : Symbol(args, Decl(arraySpreadInCall.ts, 8, 41))
>T : Symbol(T, Decl(arraySpreadInCall.ts, 8, 20))
>T : Symbol(T, Decl(arraySpreadInCall.ts, 8, 20))

const x21 = f2(...[1, 'foo'])
>x21 : Symbol(x21, Decl(arraySpreadInCall.ts, 8, 5))
>f2 : Symbol(f2, Decl(arraySpreadInCall.ts, 5, 36))
>x21 : Symbol(x21, Decl(arraySpreadInCall.ts, 9, 5))
>f2 : Symbol(f2, Decl(arraySpreadInCall.ts, 6, 50))

const x22 = f2(true, ...[1, 'foo'])
>x22 : Symbol(x22, Decl(arraySpreadInCall.ts, 9, 5))
>f2 : Symbol(f2, Decl(arraySpreadInCall.ts, 5, 36))
>x22 : Symbol(x22, Decl(arraySpreadInCall.ts, 10, 5))
>f2 : Symbol(f2, Decl(arraySpreadInCall.ts, 6, 50))

const x23 = f2(...([1, 'foo']))
>x23 : Symbol(x23, Decl(arraySpreadInCall.ts, 11, 5))
>f2 : Symbol(f2, Decl(arraySpreadInCall.ts, 6, 50))

const x24 = f2(true, ...([1, 'foo']))
>x24 : Symbol(x24, Decl(arraySpreadInCall.ts, 12, 5))
>f2 : Symbol(f2, Decl(arraySpreadInCall.ts, 6, 50))

declare function f3<T extends readonly unknown[]>(...args: T): T;
>f3 : Symbol(f3, Decl(arraySpreadInCall.ts, 9, 35))
>T : Symbol(T, Decl(arraySpreadInCall.ts, 11, 20))
>args : Symbol(args, Decl(arraySpreadInCall.ts, 11, 50))
>T : Symbol(T, Decl(arraySpreadInCall.ts, 11, 20))
>T : Symbol(T, Decl(arraySpreadInCall.ts, 11, 20))
>f3 : Symbol(f3, Decl(arraySpreadInCall.ts, 12, 37))
>T : Symbol(T, Decl(arraySpreadInCall.ts, 14, 20))
>args : Symbol(args, Decl(arraySpreadInCall.ts, 14, 50))
>T : Symbol(T, Decl(arraySpreadInCall.ts, 14, 20))
>T : Symbol(T, Decl(arraySpreadInCall.ts, 14, 20))

const x31 = f3(...[1, 'foo'])
>x31 : Symbol(x31, Decl(arraySpreadInCall.ts, 12, 5))
>f3 : Symbol(f3, Decl(arraySpreadInCall.ts, 9, 35))
>x31 : Symbol(x31, Decl(arraySpreadInCall.ts, 15, 5))
>f3 : Symbol(f3, Decl(arraySpreadInCall.ts, 12, 37))

const x32 = f3(true, ...[1, 'foo'])
>x32 : Symbol(x32, Decl(arraySpreadInCall.ts, 13, 5))
>f3 : Symbol(f3, Decl(arraySpreadInCall.ts, 9, 35))
>x32 : Symbol(x32, Decl(arraySpreadInCall.ts, 16, 5))
>f3 : Symbol(f3, Decl(arraySpreadInCall.ts, 12, 37))

const x33 = f3(...([1, 'foo']))
>x33 : Symbol(x33, Decl(arraySpreadInCall.ts, 17, 5))
>f3 : Symbol(f3, Decl(arraySpreadInCall.ts, 12, 37))

const x34 = f3(true, ...([1, 'foo']))
>x34 : Symbol(x34, Decl(arraySpreadInCall.ts, 18, 5))
>f3 : Symbol(f3, Decl(arraySpreadInCall.ts, 12, 37))

declare function f4<const T extends readonly unknown[]>(...args: T): T;
>f4 : Symbol(f4, Decl(arraySpreadInCall.ts, 18, 37))
>T : Symbol(T, Decl(arraySpreadInCall.ts, 20, 20))
>args : Symbol(args, Decl(arraySpreadInCall.ts, 20, 56))
>T : Symbol(T, Decl(arraySpreadInCall.ts, 20, 20))
>T : Symbol(T, Decl(arraySpreadInCall.ts, 20, 20))

const x41 = f4(...[1, 'foo'])
>x41 : Symbol(x41, Decl(arraySpreadInCall.ts, 21, 5))
>f4 : Symbol(f4, Decl(arraySpreadInCall.ts, 18, 37))

const x42 = f4(true, ...[1, 'foo'])
>x42 : Symbol(x42, Decl(arraySpreadInCall.ts, 22, 5))
>f4 : Symbol(f4, Decl(arraySpreadInCall.ts, 18, 37))

const x43 = f4(...([1, 'foo']))
>x43 : Symbol(x43, Decl(arraySpreadInCall.ts, 23, 5))
>f4 : Symbol(f4, Decl(arraySpreadInCall.ts, 18, 37))

const x44 = f4(true, ...([1, 'foo']))
>x44 : Symbol(x44, Decl(arraySpreadInCall.ts, 24, 5))
>f4 : Symbol(f4, Decl(arraySpreadInCall.ts, 18, 37))

// dicovered in #52845#issuecomment-1459132562
interface IAction {
>IAction : Symbol(IAction, Decl(arraySpreadInCall.ts, 13, 35))
>IAction : Symbol(IAction, Decl(arraySpreadInCall.ts, 24, 37))

run(event?: unknown): unknown;
>run : Symbol(IAction.run, Decl(arraySpreadInCall.ts, 16, 19))
>event : Symbol(event, Decl(arraySpreadInCall.ts, 17, 8))
>run : Symbol(IAction.run, Decl(arraySpreadInCall.ts, 27, 19))
>event : Symbol(event, Decl(arraySpreadInCall.ts, 28, 8))
}
declare const action: IAction
>action : Symbol(action, Decl(arraySpreadInCall.ts, 19, 13))
>IAction : Symbol(IAction, Decl(arraySpreadInCall.ts, 13, 35))
>action : Symbol(action, Decl(arraySpreadInCall.ts, 30, 13))
>IAction : Symbol(IAction, Decl(arraySpreadInCall.ts, 24, 37))

action.run(...[100, 'foo']) // error
>action.run : Symbol(IAction.run, Decl(arraySpreadInCall.ts, 16, 19))
>action : Symbol(action, Decl(arraySpreadInCall.ts, 19, 13))
>run : Symbol(IAction.run, Decl(arraySpreadInCall.ts, 16, 19))
>action.run : Symbol(IAction.run, Decl(arraySpreadInCall.ts, 27, 19))
>action : Symbol(action, Decl(arraySpreadInCall.ts, 30, 13))
>run : Symbol(IAction.run, Decl(arraySpreadInCall.ts, 27, 19))


135 changes: 122 additions & 13 deletions tests/baselines/reference/arraySpreadInCall.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,15 @@ f1(1, 2, 3, 4, ...[5, 6]);
>3 : 3
>4 : 4
>...[5, 6] : number
>[5, 6] : readonly [number, number]
>[5, 6] : [number, number]
>5 : 5
>6 : 6

f1(...[1], 2, 3, 4, 5, 6);
>f1(...[1], 2, 3, 4, 5, 6) : void
>f1 : (a: number, b: number, c: number, d: number, e: number, f: number) => void
>...[1] : number
>[1] : readonly [number]
>[1] : [number]
>1 : 1
>2 : 2
>3 : 3
Expand All @@ -40,7 +40,7 @@ f1(1, 2, ...[3, 4], 5, 6);
>1 : 1
>2 : 2
>...[3, 4] : number
>[3, 4] : readonly [number, number]
>[3, 4] : [number, number]
>3 : 3
>4 : 4
>5 : 5
Expand All @@ -52,27 +52,50 @@ f1(1, 2, ...[3], 4, ...[5, 6]);
>1 : 1
>2 : 2
>...[3] : number
>[3] : readonly [number]
>[3] : [number]
>3 : 3
>4 : 4
>...[5, 6] : number
>[5, 6] : readonly [number, number]
>[5, 6] : [number, number]
>5 : 5
>6 : 6

f1(...[1, 2], ...[3, 4], ...[5, 6]);
>f1(...[1, 2], ...[3, 4], ...[5, 6]) : void
>f1 : (a: number, b: number, c: number, d: number, e: number, f: number) => void
>...[1, 2] : number
>[1, 2] : readonly [number, number]
>[1, 2] : [number, number]
>1 : 1
>2 : 2
>...[3, 4] : number
>[3, 4] : readonly [number, number]
>[3, 4] : [number, number]
>3 : 3
>4 : 4
>...[5, 6] : number
>[5, 6] : readonly [number, number]
>[5, 6] : [number, number]
>5 : 5
>6 : 6

f1(...(([1, 2])), ...(((([3, 4])))), ...([5, 6]));
>f1(...(([1, 2])), ...(((([3, 4])))), ...([5, 6])) : void
>f1 : (a: number, b: number, c: number, d: number, e: number, f: number) => void
>...(([1, 2])) : number
>(([1, 2])) : [number, number]
>([1, 2]) : [number, number]
>[1, 2] : [number, number]
>1 : 1
>2 : 2
>...(((([3, 4])))) : number
>(((([3, 4])))) : [number, number]
>((([3, 4]))) : [number, number]
>(([3, 4])) : [number, number]
>([3, 4]) : [number, number]
>[3, 4] : [number, number]
>3 : 3
>4 : 4
>...([5, 6]) : number
>([5, 6]) : [number, number]
>[5, 6] : [number, number]
>5 : 5
>6 : 6

Expand All @@ -85,7 +108,7 @@ const x21 = f2(...[1, 'foo'])
>f2(...[1, 'foo']) : [number, string]
>f2 : <T extends unknown[]>(...args: T) => T
>...[1, 'foo'] : string | number
>[1, 'foo'] : readonly [number, string]
>[1, 'foo'] : [number, string]
>1 : 1
>'foo' : "foo"

Expand All @@ -95,7 +118,28 @@ const x22 = f2(true, ...[1, 'foo'])
>f2 : <T extends unknown[]>(...args: T) => T
>true : true
>...[1, 'foo'] : string | number
>[1, 'foo'] : readonly [number, string]
>[1, 'foo'] : [number, string]
>1 : 1
>'foo' : "foo"

const x23 = f2(...([1, 'foo']))
>x23 : [number, string]
>f2(...([1, 'foo'])) : [number, string]
>f2 : <T extends unknown[]>(...args: T) => T
>...([1, 'foo']) : string | number
>([1, 'foo']) : [number, string]
>[1, 'foo'] : [number, string]
>1 : 1
>'foo' : "foo"

const x24 = f2(true, ...([1, 'foo']))
>x24 : [boolean, number, string]
>f2(true, ...([1, 'foo'])) : [boolean, number, string]
>f2 : <T extends unknown[]>(...args: T) => T
>true : true
>...([1, 'foo']) : string | number
>([1, 'foo']) : [number, string]
>[1, 'foo'] : [number, string]
>1 : 1
>'foo' : "foo"

Expand All @@ -108,7 +152,7 @@ const x31 = f3(...[1, 'foo'])
>f3(...[1, 'foo']) : [number, string]
>f3 : <T extends readonly unknown[]>(...args: T) => T
>...[1, 'foo'] : string | number
>[1, 'foo'] : readonly [number, string]
>[1, 'foo'] : [number, string]
>1 : 1
>'foo' : "foo"

Expand All @@ -118,7 +162,72 @@ const x32 = f3(true, ...[1, 'foo'])
>f3 : <T extends readonly unknown[]>(...args: T) => T
>true : true
>...[1, 'foo'] : string | number
>[1, 'foo'] : readonly [number, string]
>[1, 'foo'] : [number, string]
>1 : 1
>'foo' : "foo"

const x33 = f3(...([1, 'foo']))
>x33 : [number, string]
>f3(...([1, 'foo'])) : [number, string]
>f3 : <T extends readonly unknown[]>(...args: T) => T
>...([1, 'foo']) : string | number
>([1, 'foo']) : [number, string]
>[1, 'foo'] : [number, string]
>1 : 1
>'foo' : "foo"

const x34 = f3(true, ...([1, 'foo']))
>x34 : [boolean, number, string]
>f3(true, ...([1, 'foo'])) : [boolean, number, string]
>f3 : <T extends readonly unknown[]>(...args: T) => T
>true : true
>...([1, 'foo']) : string | number
>([1, 'foo']) : [number, string]
>[1, 'foo'] : [number, string]
>1 : 1
>'foo' : "foo"

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

const x41 = f4(...[1, 'foo'])
>x41 : readonly [number, string]
>f4(...[1, 'foo']) : readonly [number, string]
>f4 : <const T extends readonly unknown[]>(...args: T) => T
>...[1, 'foo'] : string | number
>[1, 'foo'] : [number, string]
>1 : 1
>'foo' : "foo"

const x42 = f4(true, ...[1, 'foo'])
>x42 : readonly [true, number, string]
>f4(true, ...[1, 'foo']) : readonly [true, number, string]
>f4 : <const T extends readonly unknown[]>(...args: T) => T
>true : true
>...[1, 'foo'] : string | number
>[1, 'foo'] : [number, string]
>1 : 1
>'foo' : "foo"

const x43 = f4(...([1, 'foo']))
>x43 : readonly [number, string]
>f4(...([1, 'foo'])) : readonly [number, string]
>f4 : <const T extends readonly unknown[]>(...args: T) => T
>...([1, 'foo']) : string | number
>([1, 'foo']) : [number, string]
>[1, 'foo'] : [number, string]
>1 : 1
>'foo' : "foo"

const x44 = f4(true, ...([1, 'foo']))
>x44 : readonly [true, number, string]
>f4(true, ...([1, 'foo'])) : readonly [true, number, string]
>f4 : <const T extends readonly unknown[]>(...args: T) => T
>true : true
>...([1, 'foo']) : string | number
>([1, 'foo']) : [number, string]
>[1, 'foo'] : [number, string]
>1 : 1
>'foo' : "foo"

Expand All @@ -137,7 +246,7 @@ action.run(...[100, 'foo']) // error
>action : IAction
>run : (event?: unknown) => unknown
>...[100, 'foo'] : string | number
>[100, 'foo'] : readonly [number, string]
>[100, 'foo'] : [number, string]
>100 : 100
>'foo' : "foo"

Expand Down
Loading