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.

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
ahejlsberg merged 3 commits into from
Feb 10, 2015
Merged

Support spread operator in call expressions #1931

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2494b2d
Support spread operator in call expressions
ahejlsberg Feb 4, 2015
bbe51cf
Adding tests
ahejlsberg Feb 4, 2015
0819ca8
Addressing CR feedback
ahejlsberg Feb 6, 2015
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
Next Next commit
Support spread operator in call expressions
  • Loading branch information
@ahejlsberg
ahejlsberg committed Feb 4, 2015
commit 2494b2d90fd73bc657321a53520f3d5be41b94ba
265 changes: 134 additions & 131 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5818,10 +5818,74 @@ module ts {
return unknownSignature;
}

// Re-order candidate signatures into the result array. Assumes the result array to be empty.
// The candidate list orders groups in reverse, but within a group signatures are kept in declaration order
// A nit here is that we reorder only signatures that belong to the same symbol,
// so order how inherited signatures are processed is still preserved.
// interface A { (x: string): void }
// 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 {
Copy link
Contributor

@JsonFreeman JsonFreeman Feb 6, 2015

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@ahejlsberg ahejlsberg Feb 6, 2015

Choose a reason for hiding this comment

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

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

var lastParent: Node;
var lastSymbol: Symbol;
var cutoffIndex: number = 0;
var index: number;
var specializedIndex: number = -1;
var spliceIndex: number;
Debug.assert(!result.length);
for (var i = 0; i < signatures.length; i++) {
var signature = signatures[i];
var symbol = signature.declaration && getSymbolOfNode(signature.declaration);
var parent = signature.declaration && signature.declaration.parent;
if (!lastSymbol || symbol === lastSymbol) {
if (lastParent && parent === lastParent) {
index++;
}
else {
lastParent = parent;
index = cutoffIndex;
}
}
else {
// current declaration belongs to a different symbol
// set cutoffIndex so re-orderings in the future won't change result set from 0 to cutoffIndex
index = cutoffIndex = result.length;
lastParent = parent;
}
lastSymbol = symbol;

// specialized signatures always need to be placed before non-specialized signatures regardless
// of the cutoff position; see GH#1133
if (signature.hasStringLiterals) {
specializedIndex++;
spliceIndex = specializedIndex;
// The cutoff index always needs to be greater than or equal to the specialized signature index
// in order to prevent non-specialized signatures from being added before a specialized
// signature.
cutoffIndex++;
}
else {
spliceIndex = index;
}

result.splice(spliceIndex, 0, signature);
}
}

function getSpreadArgumentIndex(args: Expression[]): number {
for (var i = 0; i < args.length; i++) {
if (args[i].kind === SyntaxKind.SpreadElementExpression) {
return i;
}
}
return -1;
}

function hasCorrectArity(node: CallLikeExpression, args: Expression[], signature: Signature) {
var adjustedArgCount: number;
var typeArguments: NodeArray<TypeNode>;
var callIsIncomplete: boolean;
var adjustedArgCount: number; // Apparent number of arguments we will have in this call
var typeArguments: NodeArray<TypeNode>; // Type arguments (undefined if none)
var callIsIncomplete: boolean; // In incomplete call we want to be lenient when we have too few arguments

if (node.kind === SyntaxKind.TaggedTemplateExpression) {
var tagExpression = <TaggedTemplateExpression>node;
Expand Down Expand Up @@ -5866,35 +5930,29 @@ module ts {
typeArguments = callExpression.typeArguments;
}

Debug.assert(adjustedArgCount !== undefined, "'adjustedArgCount' undefined");
Debug.assert(callIsIncomplete !== undefined, "'callIsIncomplete' undefined");

return checkArity(adjustedArgCount, typeArguments, callIsIncomplete, signature);

/**
* @param adjustedArgCount The "apparent" number of arguments that we will have in this call.
* @param typeArguments Type arguments node of the call if it exists; undefined otherwise.
* @param callIsIncomplete Whether or not a call is unfinished, and we should be "lenient" when we have too few arguments.
* @param signature The signature whose arity we are comparing.
*/
function checkArity(adjustedArgCount: number, typeArguments: NodeArray<TypeNode>, callIsIncomplete: boolean, signature: Signature): boolean {
// Too many arguments implies incorrect arity.
if (!signature.hasRestParameter && adjustedArgCount > signature.parameters.length) {
return false;
}
// If the user supplied type arguments, but the number of type arguments does not match
// the declared number of type parameters, the call has an incorrect arity.
var hasRightNumberOfTypeArgs = !typeArguments ||
(signature.typeParameters && typeArguments.length === signature.typeParameters.length);
if (!hasRightNumberOfTypeArgs) {
return false;
}

// If the user supplied type arguments, but the number of type arguments does not match
// the declared number of type parameters, the call has an incorrect arity.
var hasRightNumberOfTypeArgs = !typeArguments ||
(signature.typeParameters && typeArguments.length === signature.typeParameters.length);
if (!hasRightNumberOfTypeArgs) {
return false;
}
// If spread arguments are present, check that they correspond to a rest parameter. If so, no
// further checking is necessary.
var spreadArgIndex = getSpreadArgumentIndex(args);
if (spreadArgIndex >= 0) {
return signature.hasRestParameter && spreadArgIndex >= signature.parameters.length - 1;
}

// If the call is incomplete, we should skip the lower bound check.
var hasEnoughArguments = adjustedArgCount >= signature.minArgumentCount;
return callIsIncomplete || hasEnoughArguments;
// Too many arguments implies incorrect arity.
if (!signature.hasRestParameter && adjustedArgCount > signature.parameters.length) {
return false;
}

// If the call is incomplete, we should skip the lower bound check.
var hasEnoughArguments = adjustedArgCount >= signature.minArgumentCount;
return callIsIncomplete || hasEnoughArguments;
}

// If type has a single call signature and no other members, return that signature. Otherwise, return undefined.
Expand Down Expand Up @@ -5927,32 +5985,32 @@ module ts {
// We perform two passes over the arguments. In the first pass we infer from all arguments, but use
// wildcards for all context sensitive function expressions.
for (var i = 0; i < args.length; i++) {
if (args[i].kind === SyntaxKind.OmittedExpression) {
continue;
}
var parameterType = getTypeAtPosition(signature, i);
if (i === 0 && args[i].parent.kind === SyntaxKind.TaggedTemplateExpression) {
inferTypes(context, globalTemplateStringsArrayType, parameterType);
continue;
var arg = args[i];
if (arg.kind !== SyntaxKind.OmittedExpression) {
var paramType = getTypeAtPosition(signature, arg.kind === SyntaxKind.SpreadElementExpression ? -1 : i);
Copy link
Contributor

@JsonFreeman JsonFreeman Feb 6, 2015

Choose a reason for hiding this comment

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

Why -1?

Copy link
Member Author

@ahejlsberg ahejlsberg Feb 6, 2015

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

@JsonFreeman JsonFreeman Feb 6, 2015

Choose a reason for hiding this comment

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

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

if (i === 0 && args[i].parent.kind === SyntaxKind.TaggedTemplateExpression) {
var argType = globalTemplateStringsArrayType;
Copy link
Contributor

@JsonFreeman JsonFreeman Feb 6, 2015

Choose a reason for hiding this comment

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

Declare argType outside

}
else {
// For context sensitive arguments we pass the identityMapper, which is a signal to treat all
// context sensitive function expressions as wildcards
var mapper = excludeArgument && excludeArgument[i] !== undefined ? identityMapper : inferenceMapper;
var argType = checkExpressionWithContextualType(arg, paramType, mapper);
}
inferTypes(context, argType, paramType);
}
// For context sensitive arguments we pass the identityMapper, which is a signal to treat all
// context sensitive function expressions as wildcards
var mapper = excludeArgument && excludeArgument[i] !== undefined ? identityMapper : inferenceMapper;
inferTypes(context, checkExpressionWithContextualType(args[i], parameterType, mapper), parameterType);
}

// In the second pass we visit only context sensitive arguments, and only those that aren't excluded, this
// time treating function expressions normally (which may cause previously inferred type arguments to be fixed
// as we construct types for contextually typed parameters)
if (excludeArgument) {
for (var i = 0; i < args.length; i++) {
if (args[i].kind === SyntaxKind.OmittedExpression) {
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
Copy link
Contributor

@JsonFreeman JsonFreeman Feb 6, 2015

Choose a reason for hiding this comment

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

True

Copy link

@robertpenner robertpenner Feb 24, 2015

Choose a reason for hiding this comment

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

Typo: "exlusion"

if (excludeArgument[i] === false) {
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);
Copy link
Contributor

@JsonFreeman JsonFreeman Feb 6, 2015

Choose a reason for hiding this comment

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

Again, I don't understand the -1.

inferTypes(context, checkExpressionWithContextualType(arg, paramType, inferenceMapper), paramType);
}
}
}
Expand Down Expand Up @@ -5990,37 +6048,24 @@ module ts {
return typeArgumentsAreAssignable;
}

function checkApplicableSignature(node: CallLikeExpression, args: Node[], signature: Signature, relation: Map<RelationComparisonResult>, excludeArgument: boolean[], reportErrors: boolean) {
function checkApplicableSignature(node: CallLikeExpression, args: Expression[], signature: Signature, relation: Map<RelationComparisonResult>, excludeArgument: boolean[], reportErrors: boolean) {
for (var i = 0; i < args.length; i++) {
var arg = args[i];
var argType: Type;

if (arg.kind === SyntaxKind.OmittedExpression) {
continue;
}

var paramType = getTypeAtPosition(signature, i);

if (i === 0 && node.kind === SyntaxKind.TaggedTemplateExpression) {
// A tagged template expression has something of a
// "virtual" parameter with the "cooked" strings array type.
argType = globalTemplateStringsArrayType;
}
else {
// String literals get string literal types unless we're reporting errors
argType = arg.kind === SyntaxKind.StringLiteral && !reportErrors
? getStringLiteralType(<LiteralExpression>arg)
: checkExpressionWithContextualType(<LiteralExpression>arg, paramType, excludeArgument && excludeArgument[i] ? identityMapper : undefined);
}

// Use argument expression as error location when reporting errors
var isValidArgument = checkTypeRelatedTo(argType, paramType, relation, reportErrors ? arg : undefined,
Diagnostics.Argument_of_type_0_is_not_assignable_to_parameter_of_type_1);
if (!isValidArgument) {
return false;
if (arg.kind !== SyntaxKind.OmittedExpression) {
// Check spread elements against rest type (from arity check we know spread argument corresponds to a rest parameter)
var paramType = getTypeAtPosition(signature, arg.kind === SyntaxKind.SpreadElementExpression ? -1 : i);
// A tagged template expression provides a special first argument, and string literals get string literal types
// 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);
Copy link
Contributor

@JsonFreeman JsonFreeman Feb 6, 2015

Choose a reason for hiding this comment

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

This is kind of a mouthful

Copy link
Member Author

@ahejlsberg ahejlsberg Feb 6, 2015

Choose a reason for hiding this comment

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

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

// Use argument expression as error location when reporting errors
if (!checkTypeRelatedTo(argType, paramType, relation, reportErrors ? arg : undefined,
Diagnostics.Argument_of_type_0_is_not_assignable_to_parameter_of_type_1)) {
return false;
}
}
}

return true;
}

Expand Down Expand Up @@ -6087,8 +6132,8 @@ module ts {
}

var candidates = candidatesOutArray || [];
// collectCandidates fills up the candidates array directly
collectCandidates();
// reorderCandidates fills up the candidates array directly
reorderCandidates(signatures, candidates);
if (!candidates.length) {
error(node, Diagnostics.Supplied_parameters_do_not_match_any_signature_of_call_target);
return resolveErrorCall(node);
Expand Down Expand Up @@ -6278,60 +6323,6 @@ module ts {
return undefined;
}

// The candidate list orders groups in reverse, but within a group signatures are kept in declaration order
// A nit here is that we reorder only signatures that belong to the same symbol,
// so order how inherited signatures are processed is still preserved.
// interface A { (x: string): void }
// 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 collectCandidates(): void {
var result = candidates;
var lastParent: Node;
var lastSymbol: Symbol;
var cutoffIndex: number = 0;
var index: number;
var specializedIndex: number = -1;
var spliceIndex: number;
Debug.assert(!result.length);
for (var i = 0; i < signatures.length; i++) {
var signature = signatures[i];
var symbol = signature.declaration && getSymbolOfNode(signature.declaration);
var parent = signature.declaration && signature.declaration.parent;
if (!lastSymbol || symbol === lastSymbol) {
if (lastParent && parent === lastParent) {
index++;
}
else {
lastParent = parent;
index = cutoffIndex;
}
}
else {
// current declaration belongs to a different symbol
// set cutoffIndex so re-orderings in the future won't change result set from 0 to cutoffIndex
index = cutoffIndex = result.length;
lastParent = parent;
}
lastSymbol = symbol;

// specialized signatures always need to be placed before non-specialized signatures regardless
// of the cutoff position; see GH#1133
if (signature.hasStringLiterals) {
specializedIndex++;
spliceIndex = specializedIndex;
// The cutoff index always needs to be greater than or equal to the specialized signature index
// in order to prevent non-specialized signatures from being added before a specialized
// signature.
cutoffIndex++;
}
else {
spliceIndex = index;
}

result.splice(spliceIndex, 0, signature);
}
}
}

function resolveCallExpression(node: CallExpression, candidatesOutArray: Signature[]): Signature {
Expand Down Expand Up @@ -6387,6 +6378,13 @@ module ts {
}

function resolveNewExpression(node: NewExpression, candidatesOutArray: Signature[]): Signature {
if (node.arguments && languageVersion < ScriptTarget.ES6) {
var spreadIndex = getSpreadArgumentIndex(node.arguments);
if (spreadIndex >= 0) {
error(node.arguments[spreadIndex], Diagnostics.Spread_operator_in_new_expressions_is_only_available_when_targeting_ECMAScript_6_and_higher);
}
}

var expressionType = checkExpression(node.expression);
// TS 1.0 spec: 4.11
// If ConstructExpr is of type Any, Args can be any argument
Expand Down Expand Up @@ -6532,9 +6530,14 @@ module ts {
}

function getTypeAtPosition(signature: Signature, pos: number): Type {
Copy link
Contributor

@JsonFreeman JsonFreeman Feb 6, 2015

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@ahejlsberg ahejlsberg Feb 6, 2015

Choose a reason for hiding this comment

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

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

if (pos >= 0) {
return signature.hasRestParameter ?
pos < signature.parameters.length - 1 ? getTypeOfSymbol(signature.parameters[pos]) : getRestTypeOfSignature(signature) :
pos < signature.parameters.length ? getTypeOfSymbol(signature.parameters[pos]) : anyType;
}
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]) :
Copy link
Contributor

@JsonFreeman JsonFreeman Feb 6, 2015

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@ahejlsberg ahejlsberg Feb 6, 2015

Choose a reason for hiding this comment

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

I don't understand what you mean.

Copy link
Contributor

@JsonFreeman JsonFreeman Feb 6, 2015

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@JsonFreeman JsonFreeman Feb 6, 2015

Choose a reason for hiding this comment

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

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.

anyArrayType;
}

function assignContextualParameterTypes(signature: Signature, context: Signature, mapper: TypeMapper) {
Expand Down
Loading