Skip to content

Support spread operator in call expressions #1931

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.

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 3 commits into from
Feb 10, 2015
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
265 changes: 134 additions & 131 deletions src/compiler/checker.ts

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions src/compiler/diagnosticInformationMap.generated.ts
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,7 @@ module ts {
this_cannot_be_referenced_in_a_computed_property_name: { code: 2465, category: DiagnosticCategory.Error, key: "'this' cannot be referenced in a computed property name." },
super_cannot_be_referenced_in_a_computed_property_name: { code: 2466, category: DiagnosticCategory.Error, key: "'super' cannot be referenced in a computed property name." },
A_computed_property_name_cannot_reference_a_type_parameter_from_its_containing_type: { code: 2466, category: DiagnosticCategory.Error, key: "A computed property name cannot reference a type parameter from its containing type." },
Spread_operator_in_new_expressions_is_only_available_when_targeting_ECMAScript_6_and_higher: { code: 2468, category: DiagnosticCategory.Error, key: "Spread operator in 'new' expressions is only available when targeting ECMAScript 6 and higher." },
Import_declaration_0_is_using_private_name_1: { code: 4000, category: DiagnosticCategory.Error, key: "Import declaration '{0}' is using private name '{1}'." },
Type_parameter_0_of_exported_class_has_or_is_using_private_name_1: { code: 4002, category: DiagnosticCategory.Error, key: "Type parameter '{0}' of exported class has or is using private name '{1}'." },
Type_parameter_0_of_exported_interface_has_or_is_using_private_name_1: { code: 4004, category: DiagnosticCategory.Error, key: "Type parameter '{0}' of exported interface has or is using private name '{1}'." },
Expand Down
4 changes: 4 additions & 0 deletions src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -1204,6 +1204,10 @@
"category": "Error",
"code": 2466
},
"Spread operator in 'new' expressions is only available when targeting ECMAScript 6 and higher.": {
"category": "Error",
"code": 2468
},

"Import declaration '{0}' is using private name '{1}'.": {
"category": "Error",
Expand Down
106 changes: 89 additions & 17 deletions src/compiler/emitter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1921,7 +1921,7 @@ module ts {
break;
}
// _a .. _h, _j ... _z, _0, _1, ...
name = "_" + (tempCount < 25 ? String.fromCharCode(tempCount + (tempCount < 8 ? 0: 1) + CharacterCodes.a) : tempCount - 25);
name = "_" + (tempCount < 25 ? String.fromCharCode(tempCount + (tempCount < 8 ? 0 : 1) + CharacterCodes.a) : tempCount - 25);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is pretty confusing. Can you put a comment explaining what this is doing? Also, I think it makes sense to replace the numbers with named constants. This can be addressed separately from this change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unrelated to this checkin, all that changed was formatting (guess VS must have formatted the file).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, just pointing out that it would be good to clarify it, but in a separate change.

tempCount++;
}
var result = <Identifier>createNode(SyntaxKind.Identifier);
Expand Down Expand Up @@ -2350,22 +2350,10 @@ module ts {
return true;
}

function emitArrayLiteral(node: ArrayLiteralExpression) {
var elements = node.elements;
var length = elements.length;
if (length === 0) {
write("[]");
return;
}
if (languageVersion >= ScriptTarget.ES6) {
write("[");
emitList(elements, 0, elements.length, /*multiLine*/(node.flags & NodeFlags.MultiLine) !== 0,
/*trailingComma*/ elements.hasTrailingComma);
write("]");
return;
}
function emitListWithSpread(elements: Expression[], multiLine: boolean, trailingComma: boolean) {
var pos = 0;
var group = 0;
var length = elements.length;
while (pos < length) {
// Emit using the pattern <group0>.concat(<group1>, <group2>, ...)
if (group === 1) {
Expand All @@ -2386,8 +2374,7 @@ module ts {
i++;
}
write("[");
emitList(elements, pos, i - pos, /*multiLine*/ (node.flags & NodeFlags.MultiLine) !== 0,
/*trailingComma*/ elements.hasTrailingComma);
emitList(elements, pos, i - pos, multiLine, trailingComma);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think passing trailingComma is not quite correct here. Passing it means that if the entire list has a trailing comma, then each non-spread group has a trailing comma, rather than just the last one. But I don't think it makes a difference semantically. At least, I cannot think of a case where it would.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right. I think I'll just get rid of the trailingComma parameter. Given we're substantially rewriting the original code here there's really no point in preserving this detail.

write("]");
pos = i;
}
Expand All @@ -2398,6 +2385,23 @@ module ts {
}
}

function emitArrayLiteral(node: ArrayLiteralExpression) {
var elements = node.elements;
if (elements.length === 0) {
write("[]");
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think for this function I prefer if...else if... else, instead of returning.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do.

}
if (languageVersion >= ScriptTarget.ES6) {
write("[");
emitList(elements, 0, elements.length, /*multiLine*/(node.flags & NodeFlags.MultiLine) !== 0,
/*trailingComma*/ elements.hasTrailingComma);
write("]");
return;
}
emitListWithSpread(elements, /*multiLine*/(node.flags & NodeFlags.MultiLine) !== 0,
/*trailingComma*/ elements.hasTrailingComma);
}

function emitObjectLiteral(node: ObjectLiteralExpression) {
write("{");
var properties = node.properties;
Expand Down Expand Up @@ -2492,7 +2496,75 @@ module ts {
write("]");
}

function hasSpreadElement(elements: Expression[]) {
return forEach(elements, e => e.kind === SyntaxKind.SpreadElementExpression);
}

function skipParentheses(node: Expression): Expression {
while (node.kind === SyntaxKind.ParenthesizedExpression || node.kind === SyntaxKind.TypeAssertionExpression) {
node = (<ParenthesizedExpression>node).expression;
Copy link
Contributor

Choose a reason for hiding this comment

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

This type assertion should be to ParenthesizedExpression | TypeAssertionExpression.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do.

}
return node;
}

function emitTarget(node: Expression): Expression {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe emitCallTarget?

if (node.kind === SyntaxKind.Identifier || node.kind === SyntaxKind.ThisKeyword || node.kind === SyntaxKind.SuperKeyword) {
emit(node);
return node;
}
var temp = createTempVariable(node);
recordTempDeclaration(temp);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can recordTempDeclaration be folded into createTempVariable?

Copy link
Member Author

Choose a reason for hiding this comment

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

It probably could, but I prefer keeping them separate instead of adding another boolean argument.

Copy link
Contributor

Choose a reason for hiding this comment

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

My concern is that you could do one without the other and forget. In general I don't really like void functions that are just called imperatively like this, because somebody will forget.

write("(");
emit(temp);
write(" = ");
emit(node);
write(")");
return temp;
}

function emitCallWithSpread(node: CallExpression) {
var target: Expression;
var expr = skipParentheses(node.expression);
if (expr.kind === SyntaxKind.PropertyAccessExpression) {
target = emitTarget((<PropertyAccessExpression>expr).expression);
write(".");
emit((<PropertyAccessExpression>expr).name);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please comment to explain why it is like this, and not simply emitTarget(expr). I realize that it's to make sure the temp receives the correct this value, but it took me a few minutes to understand.

}
else if (expr.kind === SyntaxKind.ElementAccessExpression) {
target = emitTarget((<PropertyAccessExpression>expr).expression);
write("[");
emit((<ElementAccessExpression>expr).argumentExpression);
write("]");
}
else if (expr.kind === SyntaxKind.SuperKeyword) {
target = expr;
write("_super");
}
else {
emit(node.expression);
}
write(".apply(");
if (target) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a sample call for each case to see what it corresponds to?

if (target.kind === SyntaxKind.SuperKeyword) {
emitThis(target);
}
else {
emit(target);
}
}
else {
write("void 0");
}
write(", ");
emitListWithSpread(node.arguments, /*multiLine*/ false, /*trailingComma*/ false);
write(")");
}

function emitCallExpression(node: CallExpression) {
if (languageVersion < ScriptTarget.ES6 && hasSpreadElement(node.arguments)) {
emitCallWithSpread(node);
return;
}
var superCall = false;
if (node.expression.kind === SyntaxKind.SuperKeyword) {
write("_super");
Expand Down
7 changes: 3 additions & 4 deletions src/compiler/parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1440,7 +1440,6 @@ module ts {
case ParsingContext.TypeParameters:
return isIdentifier();
case ParsingContext.ArgumentExpressions:
return token === SyntaxKind.CommaToken || isStartOfExpression();
case ParsingContext.ArrayLiteralMembers:
return token === SyntaxKind.CommaToken || token === SyntaxKind.DotDotDotToken || isStartOfExpression();
case ParsingContext.Parameters:
Expand Down Expand Up @@ -3544,19 +3543,19 @@ module ts {
return finishNode(node);
}

function parseArrayLiteralElement(): Expression {
function parseArgumentOrArrayLiteralElement(): Expression {
return token === SyntaxKind.DotDotDotToken ? parseSpreadElement() : parseAssignmentExpressionOrOmittedExpression();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is parseAssignmentExpressionOrOmittedExpression called from anywhere else? If not, it might make sense to just fold it into here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Isn't used anywhere else, I'll fold it in.

}

function parseArgumentExpression(): Expression {
return allowInAnd(parseAssignmentExpressionOrOmittedExpression);
return allowInAnd(parseArgumentOrArrayLiteralElement);
}

function parseArrayLiteralExpression(): ArrayLiteralExpression {
var node = <ArrayLiteralExpression>createNode(SyntaxKind.ArrayLiteralExpression);
parseExpected(SyntaxKind.OpenBracketToken);
if (scanner.hasPrecedingLineBreak()) node.flags |= NodeFlags.MultiLine;
node.elements = parseDelimitedList(ParsingContext.ArrayLiteralMembers, parseArrayLiteralElement);
node.elements = parseDelimitedList(ParsingContext.ArrayLiteralMembers, parseArgumentOrArrayLiteralElement);
parseExpected(SyntaxKind.CloseBracketToken);
return finishNode(node);
}
Expand Down
59 changes: 59 additions & 0 deletions tests/baselines/reference/callWithSpread.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
tests/cases/conformance/expressions/functionCalls/callWithSpread.ts(52,21): error TS2468: Spread operator in 'new' expressions is only available when targeting ECMAScript 6 and higher.


==== tests/cases/conformance/expressions/functionCalls/callWithSpread.ts (1 errors) ====
interface X {
foo(x: number, y: number, ...z: string[]);
}

function foo(x: number, y: number, ...z: string[]) {
}

var a: string[];
var z: number[];
var obj: X;
var xa: X[];

foo(1, 2, "abc");
foo(1, 2, ...a);
foo(1, 2, ...a, "abc");

obj.foo(1, 2, "abc");
obj.foo(1, 2, ...a);
obj.foo(1, 2, ...a, "abc");

(obj.foo)(1, 2, "abc");
(obj.foo)(1, 2, ...a);
(obj.foo)(1, 2, ...a, "abc");

xa[1].foo(1, 2, "abc");
xa[1].foo(1, 2, ...a);
xa[1].foo(1, 2, ...a, "abc");

(<Function>xa[1].foo)(...[1, 2, "abc"]);

class C {
constructor(x: number, y: number, ...z: string[]) {
this.foo(x, y);
this.foo(x, y, ...z);
}
foo(x: number, y: number, ...z: string[]) {
}
}

class D extends C {
constructor() {
super(1, 2);
super(1, 2, ...a);
}
foo() {
super.foo(1, 2);
super.foo(1, 2, ...a);
}
}

// Only supported in when target is ES6
var c = new C(1, 2, ...a);
~~~~
!!! error TS2468: Spread operator in 'new' expressions is only available when targeting ECMAScript 6 and higher.

117 changes: 117 additions & 0 deletions tests/baselines/reference/callWithSpread.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
//// [callWithSpread.ts]
interface X {
foo(x: number, y: number, ...z: string[]);
}

function foo(x: number, y: number, ...z: string[]) {
}

var a: string[];
var z: number[];
var obj: X;
var xa: X[];

foo(1, 2, "abc");
foo(1, 2, ...a);
foo(1, 2, ...a, "abc");

obj.foo(1, 2, "abc");
obj.foo(1, 2, ...a);
obj.foo(1, 2, ...a, "abc");

(obj.foo)(1, 2, "abc");
(obj.foo)(1, 2, ...a);
(obj.foo)(1, 2, ...a, "abc");

xa[1].foo(1, 2, "abc");
xa[1].foo(1, 2, ...a);
xa[1].foo(1, 2, ...a, "abc");

(<Function>xa[1].foo)(...[1, 2, "abc"]);

class C {
constructor(x: number, y: number, ...z: string[]) {
this.foo(x, y);
this.foo(x, y, ...z);
}
foo(x: number, y: number, ...z: string[]) {
}
}

class D extends C {
constructor() {
super(1, 2);
super(1, 2, ...a);
}
foo() {
super.foo(1, 2);
super.foo(1, 2, ...a);
}
}

// Only supported in when target is ES6
var c = new C(1, 2, ...a);


//// [callWithSpread.js]
var __extends = this.__extends || function (d, b) {
for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p];
function __() { this.constructor = d; }
__.prototype = b.prototype;
d.prototype = new __();
};
function foo(x, y) {
var z = [];
for (var _i = 2; _i < arguments.length; _i++) {
z[_i - 2] = arguments[_i];
}
}
var a;
var z;
var obj;
var xa;
foo(1, 2, "abc");
foo.apply(void 0, [1, 2].concat(a));
foo.apply(void 0, [1, 2].concat(a, ["abc"]));
obj.foo(1, 2, "abc");
obj.foo.apply(obj, [1, 2].concat(a));
obj.foo.apply(obj, [1, 2].concat(a, ["abc"]));
(obj.foo)(1, 2, "abc");
obj.foo.apply(obj, [1, 2].concat(a));
obj.foo.apply(obj, [1, 2].concat(a, ["abc"]));
xa[1].foo(1, 2, "abc");
(_a = xa[1]).foo.apply(_a, [1, 2].concat(a));
(_b = xa[1]).foo.apply(_b, [1, 2].concat(a, ["abc"]));
(_c = xa[1]).foo.apply(_c, [1, 2, "abc"]);
var C = (function () {
function C(x, y) {
var z = [];
for (var _i = 2; _i < arguments.length; _i++) {
z[_i - 2] = arguments[_i];
}
this.foo(x, y);
this.foo.apply(this, [x, y].concat(z));
}
C.prototype.foo = function (x, y) {
var z = [];
for (var _i = 2; _i < arguments.length; _i++) {
z[_i - 2] = arguments[_i];
}
};
return C;
})();
var D = (function (_super) {
__extends(D, _super);
function D() {
_super.call(this, 1, 2);
_super.apply(this, [1, 2].concat(a));
}
D.prototype.foo = function () {
_super.prototype.foo.call(this, 1, 2);
_super.prototype.foo.apply(this, [1, 2].concat(a));
};
return D;
})(C);
// Only supported in when target is ES6
var c = new C(1, 2, ...a);
var _a, _b, _c;
Loading