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

Support spread operator in call expressions #1931

Merged
merged 3 commits into from Feb 10, 2015

Conversation

Projects
None yet
5 participants
@ahejlsberg
Member

ahejlsberg commented Feb 5, 2015

This PR adds support for the spread (...) operator in call expressions with down-level code generation for ES3/5. For example:

function drawText(x: number, y: number, ...strings: string[]) {
    // draw text
}

drawText(10, 20, "hello");
drawText(10, 20, "hello", "world");
var a = ["one", "two"];
drawText(10, 20, ...a);
drawText(10, 20, "zero", ...a, "three");

The function calls are emitted unchanged for ES6, but when the target is ES3 or ES5, the calls are rewritten as follows:

drawText(10, 20, "hello");
drawText(10, 20, "hello", "world");
var a = ["one", "two"];
drawText.apply(void 0, [10, 20].concat(a));
drawText.apply(void 0, [10, 20, "zero"].concat(a, ["three"]));

Type checking requires spread elements to match up with a rest parameter. Thus, in a typed function call it is not possible to use a spread element for non-rest parameters. For example:

var pos = [10, 20];
drawText(...pos, "hello", "world");  // Error
(<any>drawText)(...pos, "hello", "world");  // But ok in untyped call

A temporary variable may be injected to ensure the this argument is only evaluated once. For example:

interface Context {
    drawText(x: number, y: number, ...strings: string[]);
}

declare function getContext(): Context;

var text = ["hello", "world"];
getContext().drawText(10, 20, ...text);

This generates the code:

var text = ["hello", "world"];
(_a = getContext()).drawText.apply(_a, [10, 20].concat(text));
var _a;
(<Function>xa[1].foo)(...[1, 2, "abc"]);
class C {

This comment has been minimized.

@yuit

yuit Feb 5, 2015

Contributor

This class C/D can be pull out into its own test cases

@yuit

yuit Feb 5, 2015

Contributor

This class C/D can be pull out into its own test cases

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

This comment has been minimized.

@yuit

yuit Feb 5, 2015

Contributor

This one can also be separated out into its own test file.

@yuit

yuit Feb 5, 2015

Contributor

This one can also be separated out into its own test file.

@@ -0,0 +1,54 @@
// @target: ES6
interface X {

This comment has been minimized.

@yuit

yuit Feb 5, 2015

Contributor

This one can be separated into its own test file. Separated test files will make it much easier to check

@yuit

yuit Feb 5, 2015

Contributor

This one can be separated into its own test file. Separated test files will make it much easier to check

Show outdated Hide outdated src/compiler/parser.ts Outdated
@@ -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);

This comment has been minimized.

@JsonFreeman

JsonFreeman Feb 6, 2015

Contributor

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.

@JsonFreeman

JsonFreeman Feb 6, 2015

Contributor

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.

This comment has been minimized.

@ahejlsberg

ahejlsberg Feb 6, 2015

Member

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

@ahejlsberg

ahejlsberg Feb 6, 2015

Member

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

This comment has been minimized.

@JsonFreeman

JsonFreeman Feb 6, 2015

Contributor

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

@JsonFreeman

JsonFreeman Feb 6, 2015

Contributor

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

Show outdated Hide outdated src/compiler/emitter.ts Outdated
Show outdated Hide outdated src/compiler/emitter.ts Outdated
Show outdated Hide outdated src/compiler/emitter.ts Outdated
return node;
}
var temp = createTempVariable(node);
recordTempDeclaration(temp);

This comment has been minimized.

@JsonFreeman

JsonFreeman Feb 6, 2015

Contributor

Can recordTempDeclaration be folded into createTempVariable?

@JsonFreeman

JsonFreeman Feb 6, 2015

Contributor

Can recordTempDeclaration be folded into createTempVariable?

This comment has been minimized.

@ahejlsberg

ahejlsberg Feb 6, 2015

Member

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

@ahejlsberg

ahejlsberg Feb 6, 2015

Member

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

This comment has been minimized.

@JsonFreeman

JsonFreeman Feb 6, 2015

Contributor

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.

@JsonFreeman

JsonFreeman Feb 6, 2015

Contributor

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.

Show outdated Hide outdated src/compiler/emitter.ts Outdated
Show outdated Hide outdated src/compiler/emitter.ts Outdated
emit(node.expression);
}
write(".apply(");
if (target) {

This comment has been minimized.

@JsonFreeman

JsonFreeman Feb 6, 2015

Contributor

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

@JsonFreeman

JsonFreeman Feb 6, 2015

Contributor

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

// interface B extends A { (x: 'foo'): string }
// var b: B;
// b('foo') // <- here overloads should be processed as [(x:'foo'): string, (x: string): void]
function reorderCandidates(signatures: Signature[], result: Signature[]): void {

This comment has been minimized.

@JsonFreeman

JsonFreeman Feb 6, 2015

Contributor

I'm assuming nothing actually changed in this function, correct?

@JsonFreeman

JsonFreeman Feb 6, 2015

Contributor

I'm assuming nothing actually changed in this function, correct?

This comment has been minimized.

@ahejlsberg

ahejlsberg Feb 6, 2015

Member

Correct, I just changed it to top-level, added parameters, and gave it a more indicative name.

@ahejlsberg

ahejlsberg Feb 6, 2015

Member

Correct, I just changed it to top-level, added parameters, and gave it a more indicative name.

if (arg.kind !== SyntaxKind.OmittedExpression) {
var paramType = getTypeAtPosition(signature, arg.kind === SyntaxKind.SpreadElementExpression ? -1 : i);
if (i === 0 && args[i].parent.kind === SyntaxKind.TaggedTemplateExpression) {
var argType = globalTemplateStringsArrayType;

This comment has been minimized.

@JsonFreeman

JsonFreeman Feb 6, 2015

Contributor

Declare argType outside

@JsonFreeman

JsonFreeman Feb 6, 2015

Contributor

Declare argType outside

continue;
var arg = args[i];
if (arg.kind !== SyntaxKind.OmittedExpression) {
var paramType = getTypeAtPosition(signature, arg.kind === SyntaxKind.SpreadElementExpression ? -1 : i);

This comment has been minimized.

@JsonFreeman

JsonFreeman Feb 6, 2015

Contributor

Why -1?

@JsonFreeman

JsonFreeman Feb 6, 2015

Contributor

Why -1?

This comment has been minimized.

@ahejlsberg

ahejlsberg Feb 6, 2015

Member

I use that as an indicator to return the rest parameter type of the signature (as an array type as opposed to as the element type of the array).

@ahejlsberg

ahejlsberg Feb 6, 2015

Member

I use that as an indicator to return the rest parameter type of the signature (as an array type as opposed to as the element type of the array).

This comment has been minimized.

@JsonFreeman

JsonFreeman Feb 6, 2015

Contributor

Yeah I definitely prefer separating getTypeAtPosition into 2 functions, like you said you had earlier.

@JsonFreeman

JsonFreeman Feb 6, 2015

Contributor

Yeah I definitely prefer separating getTypeAtPosition into 2 functions, like you said you had earlier.

continue;
}
// No need to special-case tagged templates; their excludeArgument value will be 'undefined'.
// No need to check for omitted args and template expressions, their exlusion value is always undefined

This comment has been minimized.

@JsonFreeman

JsonFreeman Feb 6, 2015

Contributor

True

@JsonFreeman

JsonFreeman Feb 6, 2015

Contributor

True

This comment has been minimized.

@robertpenner

robertpenner Feb 24, 2015

Typo: "exlusion"

@robertpenner

robertpenner Feb 24, 2015

Typo: "exlusion"

var parameterType = getTypeAtPosition(signature, i);
inferTypes(context, checkExpressionWithContextualType(args[i], parameterType, inferenceMapper), parameterType);
var arg = args[i];
var paramType = getTypeAtPosition(signature, arg.kind === SyntaxKind.SpreadElementExpression ? -1 : i);

This comment has been minimized.

@JsonFreeman

JsonFreeman Feb 6, 2015

Contributor

Again, I don't understand the -1.

@JsonFreeman

JsonFreeman Feb 6, 2015

Contributor

Again, I don't understand the -1.

// unless we're reporting errors
var argType = i === 0 && node.kind === SyntaxKind.TaggedTemplateExpression ? globalTemplateStringsArrayType :
arg.kind === SyntaxKind.StringLiteral && !reportErrors ? getStringLiteralType(<LiteralExpression>arg) :
checkExpressionWithContextualType(arg, paramType, excludeArgument && excludeArgument[i] ? identityMapper : undefined);

This comment has been minimized.

@JsonFreeman

JsonFreeman Feb 6, 2015

Contributor

This is kind of a mouthful

@JsonFreeman

JsonFreeman Feb 6, 2015

Contributor

This is kind of a mouthful

This comment has been minimized.

@ahejlsberg

ahejlsberg Feb 6, 2015

Member

Honestly, I find it a lot easier to understand than the 25 lines of imperative code it replaces.

@ahejlsberg

ahejlsberg Feb 6, 2015

Member

Honestly, I find it a lot easier to understand than the 25 lines of imperative code it replaces.

@@ -6532,9 +6530,14 @@ module ts {
}
function getTypeAtPosition(signature: Signature, pos: number): Type {

This comment has been minimized.

@JsonFreeman

JsonFreeman Feb 6, 2015

Contributor

I would far prefer to have two separate functions, one for spread, and one for normal arguments. I noticed above that you use the conditional operator to switch the pos value you pass in, so instead you can just switch the function that you call, and pass the pos as is.

@JsonFreeman

JsonFreeman Feb 6, 2015

Contributor

I would far prefer to have two separate functions, one for spread, and one for normal arguments. I noticed above that you use the conditional operator to switch the pos value you pass in, so instead you can just switch the function that you call, and pass the pos as is.

This comment has been minimized.

@ahejlsberg

ahejlsberg Feb 6, 2015

Member

I had it that way, but this actually seemed better (and shorter). Anyway, I can change it back.

@ahejlsberg

ahejlsberg Feb 6, 2015

Member

I had it that way, but this actually seemed better (and shorter). Anyway, I can change it back.

return signature.hasRestParameter ?
pos < signature.parameters.length - 1 ? getTypeOfSymbol(signature.parameters[pos]) : getRestTypeOfSignature(signature) :
pos < signature.parameters.length ? getTypeOfSymbol(signature.parameters[pos]) : anyType;
getTypeOfSymbol(signature.parameters[signature.parameters.length - 1]) :

This comment has been minimized.

@JsonFreeman

JsonFreeman Feb 6, 2015

Contributor

I think that if the spread argument is not in the rest range, we should return anyArrayType, just like the case where there is no rest param.

@JsonFreeman

JsonFreeman Feb 6, 2015

Contributor

I think that if the spread argument is not in the rest range, we should return anyArrayType, just like the case where there is no rest param.

This comment has been minimized.

@ahejlsberg

ahejlsberg Feb 6, 2015

Member

I don't understand what you mean.

@ahejlsberg

ahejlsberg Feb 6, 2015

Member

I don't understand what you mean.

This comment has been minimized.

@JsonFreeman

JsonFreeman Feb 6, 2015

Contributor

Like in the following case:

function foo(x: string, y: string, ...rest: number[]) {}
foo(...["hi", "bye"]);

This is an error, but getTypeOfSymbol returns number[]. But I think it should return any[], just like it does if the rest param were not there.

@JsonFreeman

JsonFreeman Feb 6, 2015

Contributor

Like in the following case:

function foo(x: string, y: string, ...rest: number[]) {}
foo(...["hi", "bye"]);

This is an error, but getTypeOfSymbol returns number[]. But I think it should return any[], just like it does if the rest param were not there.

This comment has been minimized.

@JsonFreeman

JsonFreeman Feb 6, 2015

Contributor

Or it could return a union of all the param types starting from the current index in the param list, all the way to the end, but I think that's not necessary, since it is an error case.

@JsonFreeman

JsonFreeman Feb 6, 2015

Contributor

Or it could return a union of all the param types starting from the current index in the param list, all the way to the end, but I think that's not necessary, since it is an error case.

@JsonFreeman

This comment has been minimized.

Show comment
Hide comment
@JsonFreeman

JsonFreeman Feb 6, 2015

Contributor

Oh interesting. So this does preserve the trailing comma on the last one still, but only if it's not a spread.

Contributor

JsonFreeman commented on src/compiler/emitter.ts in 0819ca8 Feb 6, 2015

Oh interesting. So this does preserve the trailing comma on the last one still, but only if it's not a spread.

ahejlsberg added a commit that referenced this pull request Feb 10, 2015

Merge pull request #1931 from Microsoft/spreadCall
Support spread operator in call expressions

@ahejlsberg ahejlsberg merged commit 4b92e42 into master Feb 10, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@ahejlsberg ahejlsberg deleted the spreadCall branch Feb 10, 2015

@DickvdBrink DickvdBrink referenced this pull request Mar 8, 2015

Closed

Spread arguments #238

@tinganho tinganho referenced this pull request May 7, 2015

Merged

New with spread #3066

@basarat basarat referenced this pull request Feb 28, 2016

Open

Spread parameters #79

@Microsoft Microsoft locked and limited conversation to collaborators Jun 18, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.