Skip to content

Adding support for tuple types (e.g. [number, string]) #428

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 10 commits into from
Sep 15, 2014
Merged

Adding support for tuple types (e.g. [number, string]) #428

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
5b25524
Adding support for tuple types (e.g. [number, string])
ahejlsberg Aug 11, 2014
3b1dbad
Addressing CR feedback, adding baselines.
ahejlsberg Aug 11, 2014
ef52312
Addressing CR feedback.
ahejlsberg Aug 13, 2014
92b3677
Adding error message for empty tuple types.
ahejlsberg Aug 13, 2014
f0b33b3
Accepting new baselines.
ahejlsberg Aug 13, 2014
0cf503f
Merge branch 'master' into tupleTypes
ahejlsberg Aug 16, 2014
c0e802d
Accepting new baselines after merge.
ahejlsberg Aug 16, 2014
63b83e7
Contextual typing of array literals is now based on the presence or a…
ahejlsberg Aug 16, 2014
be7e0a7
Merge branch 'master' into tupleTypes
ahejlsberg Sep 5, 2014
b4cddc3
Merge branch 'master' into tupleTypes
ahejlsberg Sep 15, 2014
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
132 changes: 104 additions & 28 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ module ts {
var globalBooleanType: ObjectType;
var globalRegExpType: ObjectType;

var tupleTypes: Map<TupleType> = {};
var stringLiteralTypes: Map<StringLiteralType> = {};

var fullTypeCheck = false;
Expand Down Expand Up @@ -619,15 +620,14 @@ module ts {
}

function isOptionalProperty(propertySymbol: Symbol): boolean {
if (propertySymbol.flags & SymbolFlags.Prototype) {
return false;
}
// class C {
// constructor(public x?) { }
// }
//
// x is an optional parameter, but it is a required property.
return (propertySymbol.valueDeclaration.flags & NodeFlags.QuestionMark) && propertySymbol.valueDeclaration.kind !== SyntaxKind.Parameter;
return propertySymbol.valueDeclaration &&
propertySymbol.valueDeclaration.flags & NodeFlags.QuestionMark &&
propertySymbol.valueDeclaration.kind !== SyntaxKind.Parameter;
}

function forEachSymbolTableInScope<T>(enclosingDeclaration: Node, callback: (symbolTable: SymbolTable) => T): T {
Expand Down Expand Up @@ -843,6 +843,9 @@ module ts {
else if (type.flags & (TypeFlags.Class | TypeFlags.Interface | TypeFlags.Enum | TypeFlags.TypeParameter)) {
writer.writeSymbol(type.symbol, enclosingDeclaration, SymbolFlags.Type);
}
else if (type.flags & TypeFlags.Tuple) {
writeTupleType(<TupleType>type);
}
else if (type.flags & TypeFlags.Anonymous) {
writeAnonymousType(<ObjectType>type, allowFunctionOrConstructorTypeLiteral);
}
Expand All @@ -855,6 +858,15 @@ module ts {
}
}

function writeTypeList(types: Type[]) {
for (var i = 0; i < types.length; i++) {
if (i > 0) {
writer.write(", ");
}
writeType(types[i], /*allowFunctionOrConstructorTypeLiteral*/ true);
}
}

function writeTypeReference(type: TypeReference) {
if (type.target === globalArrayType && !(flags & TypeFormatFlags.WriteArrayAsGenericType)) {
// If we are writing array element type the arrow style signatures are not allowed as
Expand All @@ -865,16 +877,17 @@ module ts {
else {
writer.writeSymbol(type.target.symbol, enclosingDeclaration, SymbolFlags.Type);
writer.write("<");
for (var i = 0; i < type.typeArguments.length; i++) {
if (i > 0) {
writer.write(", ");
}
writeType(type.typeArguments[i], /*allowFunctionOrConstructorTypeLiteral*/ true);
}
writeTypeList(type.typeArguments);
writer.write(">");
}
}

function writeTupleType(type: TupleType) {
writer.write("[");
writeTypeList(type.elementTypes);
writer.write("]");
}

function writeAnonymousType(type: ObjectType, allowFunctionOrConstructorTypeLiteral: boolean) {
// Always use 'typeof T' for type of class, enum, and module objects
if (type.symbol && type.symbol.flags & (SymbolFlags.Class | SymbolFlags.Enum | SymbolFlags.ValueModule)) {
Expand Down Expand Up @@ -1649,6 +1662,23 @@ module ts {
return [createSignature(undefined, classType.typeParameters, emptyArray, classType, 0, false, false)];
}

function createTupleTypeMemberSymbols(memberTypes: Type[]): SymbolTable {
var members: SymbolTable = {};
for (var i = 0; i < memberTypes.length; i++) {
var symbol = <TransientSymbol>createSymbol(SymbolFlags.Property | SymbolFlags.Transient, "" + i);
Copy link
Contributor

Choose a reason for hiding this comment

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

consider adding a createTransientSymbol helper so you can avoid the 'or' and the redundant cast.

Copy link
Member Author

Choose a reason for hiding this comment

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

Only used in a few places, don't think it is worth it to create a helper.

Copy link
Contributor

Choose a reason for hiding this comment

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

Currently we have three places where we do this (and this will bring us to four). Once we're repeating ourselves that many times, it seems desirable to start using a helper :)

createSymbol itself is only used 9 times (so only 6 times without making something transient). It seems like nearly half the times we create a symbol, it's a transient symbol. This seems worthwhile to me :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. As well as a comment about when to create a transient symbol vs a nontransient symbol.

symbol.type = memberTypes[i];
members[i] = symbol;
}
return members;
}

function resolveTupleTypeMembers(type: TupleType) {
var arrayType = resolveObjectTypeMembers(createArrayType(getBestCommonType(type.elementTypes)));
var members = createTupleTypeMemberSymbols(type.elementTypes);
addInheritedMembers(members, arrayType.properties);
setObjectTypeMembers(type, members, arrayType.callSignatures, arrayType.constructSignatures, arrayType.stringIndexType, arrayType.numberIndexType);
}

function resolveAnonymousTypeMembers(type: ObjectType) {
var symbol = type.symbol;
var members = emptySymbols;
Expand Down Expand Up @@ -1682,6 +1712,9 @@ module ts {
else if (type.flags & TypeFlags.Anonymous) {
resolveAnonymousTypeMembers(<ObjectType>type);
}
else if (type.flags & TypeFlags.Tuple) {
resolveTupleTypeMembers(<TupleType>type);
}
else {
resolveTypeReferenceMembers(<TypeReference>type);
}
Expand Down Expand Up @@ -2037,7 +2070,7 @@ module ts {
if (type.flags & (TypeFlags.Class | TypeFlags.Interface) && type.flags & TypeFlags.Reference) {
var typeParameters = (<InterfaceType>type).typeParameters;
if (node.typeArguments && node.typeArguments.length === typeParameters.length) {
type = createTypeReference(<GenericType>type, map(node.typeArguments, t => getTypeFromTypeNode(t)));
type = createTypeReference(<GenericType>type, map(node.typeArguments, getTypeFromTypeNode));
Copy link
Member

Choose a reason for hiding this comment

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

Good ol' η-reduction.

Copy link
Contributor

Choose a reason for hiding this comment

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

someone ate my tea

Copy link
Contributor

Choose a reason for hiding this comment

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

Someone eta my tea

Copy link
Member

Choose a reason for hiding this comment

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

👍

}
else {
error(node, Diagnostics.Generic_type_0_requires_1_type_argument_s, typeToString(type, /*enclosingDeclaration*/ undefined, TypeFormatFlags.WriteArrayAsGenericType), typeParameters.length);
Expand Down Expand Up @@ -2123,6 +2156,24 @@ module ts {
return links.resolvedType;
}

function createTupleType(elementTypes: Type[]) {
var id = getTypeListId(elementTypes);
var type = tupleTypes[id];
if (!type) {
type = tupleTypes[id] = <TupleType>createObjectType(TypeFlags.Tuple);
type.elementTypes = elementTypes;
}
return type;
}

function getTypeFromTupleTypeNode(node: TupleTypeNode): Type {
var links = getNodeLinks(node);
if (!links.resolvedType) {
links.resolvedType = createTupleType(map(node.elementTypes, getTypeFromTypeNode));
}
return links.resolvedType;
}

function getTypeFromTypeLiteralNode(node: TypeLiteralNode): Type {
var links = getNodeLinks(node);
if (!links.resolvedType) {
Expand Down Expand Up @@ -2172,6 +2223,8 @@ module ts {
return getTypeFromTypeQueryNode(<TypeQueryNode>node);
case SyntaxKind.ArrayType:
return getTypeFromArrayTypeNode(<ArrayTypeNode>node);
case SyntaxKind.TupleType:
return getTypeFromTupleTypeNode(<TupleTypeNode>node);
case SyntaxKind.TypeLiteral:
return getTypeFromTypeLiteralNode(<TypeLiteralNode>node);
default:
Expand Down Expand Up @@ -2327,6 +2380,9 @@ module ts {
if (type.flags & TypeFlags.Reference) {
return createTypeReference((<TypeReference>type).target, instantiateList((<TypeReference>type).typeArguments, mapper, instantiateType));
}
if (type.flags & TypeFlags.Tuple) {
return createTupleType(instantiateList((<TupleType>type).elementTypes, mapper, instantiateType));
}
}
return type;
}
Expand Down Expand Up @@ -3015,20 +3071,16 @@ module ts {
while (isArrayType(type)) {
type = (<GenericType>type).typeArguments[0];
}

return type;
}

function getWidenedTypeOfArrayLiteral(type: Type): Type {
var elementType = (<TypeReference>type).typeArguments[0];
var widenedType = getWidenedType(elementType);

type = elementType !== widenedType ? createArrayType(widenedType) : type;

return type;
}

/* If we are widening on a literal, then we may need to the 'node' parameter for reporting purposes */
function getWidenedType(type: Type): Type {
if (type.flags & (TypeFlags.Undefined | TypeFlags.Null)) {
return anyType;
Expand Down Expand Up @@ -3125,9 +3177,9 @@ module ts {
inferFromTypes(sourceTypes[i], targetTypes[i]);
}
}
else if (source.flags & TypeFlags.ObjectType && (target.flags & TypeFlags.Reference || (target.flags & TypeFlags.Anonymous) &&
target.symbol && target.symbol.flags & (SymbolFlags.Method | SymbolFlags.TypeLiteral))) {
// If source is an object type, and target is a type reference, the type of a method, or a type literal, infer from members
else if (source.flags & TypeFlags.ObjectType && (target.flags & (TypeFlags.Reference | TypeFlags.Tuple) ||
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 getting more confusing. can you extract out a method that breaks out the checks, to make it more understandable what it is doing.

Copy link
Member Author

Choose a reason for hiding this comment

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

There's a comment on the next line that explains what the test is doing.

Copy link
Contributor

Choose a reason for hiding this comment

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

The comment is really just an English transliteration of the Boolean check. it doesn't really convey what's actually happening. i.e. a comment for "source.flags & TypeFlags.ObjectType && (target.flags & TypeFlags.Reference" saying "if the source is an object and the target is a reference" doesn't really explain things.

What I meant was more have English prose explaining why these are the right set of checks. For example, why is is right that 'inferFromTypes' cares about tuples. And why is right that inferFromTypes does not care about the other sorts of types not checked here.

In other words, say someone has to come along and try to determine if this code is correct or not. Having the English prose explaining why it is precisely supposed to be this way is enormously helpful.

(target.flags & TypeFlags.Anonymous) && target.symbol && target.symbol.flags & (SymbolFlags.Method | SymbolFlags.TypeLiteral))) {
// If source is an object type, and target is a type reference, a tuple type, the type of a method, or a type literal, infer from members
if (!isInProcess(source, target) && isWithinDepthLimit(source, sourceStack) && isWithinDepthLimit(target, targetStack)) {
if (depth === 0) {
sourceStack = [];
Expand Down Expand Up @@ -3574,7 +3626,19 @@ module ts {
function getContextualTypeForElementExpression(node: Expression): Type {
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure what this is doing in the context of tuples. Can you clarify why this is necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

If the contextual type is a tuple type we want to return the tuple element type at the same index as the expression node.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I meant "clarify in the source code." These comments in the code review are not something people see when working with the code.

var arrayLiteral = <ArrayLiteral>node.parent;
var type = getContextualType(arrayLiteral);
return type ? getIndexTypeOfType(type, IndexKind.Number) : undefined;
if (type) {
if (type.flags & TypeFlags.Tuple) {
var index = indexOf(arrayLiteral.elements, node);
Copy link
Contributor

Choose a reason for hiding this comment

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

seems odd that you have to recompute the index of the expresssion. Is this not known by the caller of this function?

Copy link
Member Author

Choose a reason for hiding this comment

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

The caller of this function is always getContextualType(node) and it's job is to return the contextual type for the node regardless of context. So, if we need to know the index, we need to compute it.

if (index >= 0) {
var prop = getPropertyOfType(type, "" + index);
if (prop) {
return getTypeOfSymbol(prop);
}
}
}
return getIndexTypeOfType(type, IndexKind.Number);
}
return undefined;
}

function getContextualTypeForConditionalOperand(node: Expression): Type {
Expand Down Expand Up @@ -3633,17 +3697,23 @@ module ts {
}

function checkArrayLiteral(node: ArrayLiteral, contextualMapper?: TypeMapper): Type {
var contextualType = getContextualType(node);
var isTupleLiteral = contextualType && (contextualType.flags & TypeFlags.Tuple) !== 0;
var elementTypes: Type[] = [];
forEach(node.elements, element => {
if (element.kind !== SyntaxKind.OmittedExpression) {
var type = checkExpression(element, contextualMapper);
if (!contains(elementTypes, type)) elementTypes.push(type);
var type = element.kind !== SyntaxKind.OmittedExpression ? checkExpression(element, contextualMapper) : undefinedType;
if (isTupleLiteral || !contains(elementTypes, type)) {
elementTypes.push(type);
}
});
var contextualType = isInferentialContext(contextualMapper) ? undefined : getContextualType(node);
var contextualElementType = contextualType && getIndexTypeOfType(contextualType, IndexKind.Number);
if (isTupleLiteral) {
return createTupleType(elementTypes);
}
var contextualElementType = contextualType && !isInferentialContext(contextualMapper) ? getIndexTypeOfType(contextualType, IndexKind.Number) : undefined;
var elementType = getBestCommonType(elementTypes, contextualElementType, true);
if (!elementType) elementType = elementTypes.length ? emptyObjectType : undefinedType;
if (!elementType) {
elementType = elementTypes.length ? emptyObjectType : undefinedType;
}
return createArrayType(elementType);
}

Expand Down Expand Up @@ -3711,11 +3781,11 @@ module ts {
}

function getDeclarationKindFromSymbol(s: Symbol) {
return s.flags & SymbolFlags.Prototype ? SyntaxKind.Property : s.valueDeclaration.kind;
return s.valueDeclaration ? s.valueDeclaration.kind : SyntaxKind.Property;
Copy link
Contributor

Choose a reason for hiding this comment

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

the former way seemed better. It was explicit that only the special .Prototype member was considered to be a property, and anything else went to the decl. Now, the new code is assuming anything without a valueDecl is a property. It's not clear why that invariant would be true.

Copy link
Member Author

Choose a reason for hiding this comment

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

Some symbols have no declarations associated with them and we need a default policy for those. This establishes that policy. Also, note that the function is simply a helper for checkPropertyAccess and not referenced anywhere else.

Copy link
Contributor

Choose a reason for hiding this comment

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

"This establishes that policy"

When establishing policy we should be documenting it (at least with some sort of comment). For example, in the comment for 'valueDeclaration' on Symbol we should be documenting: "if a symbol does not have a valueDeclaration, it is assumed to be a property. Example of this are the synthesized '.prototype' property as well as synthesized tuple index properties."

Copy link
Contributor

Choose a reason for hiding this comment

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

Definitely agree with Cyrus 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.

Ok, will add a comment.

}

function getDeclarationFlagsFromSymbol(s: Symbol) {
return s.flags & SymbolFlags.Prototype ? NodeFlags.Public | NodeFlags.Static : s.valueDeclaration.flags;
return s.valueDeclaration ? s.valueDeclaration.flags : s.flags & SymbolFlags.Prototype ? NodeFlags.Public | NodeFlags.Static : 0;
}

function checkPropertyAccess(node: PropertyAccess) {
Expand Down Expand Up @@ -4991,7 +5061,11 @@ module ts {
}

function checkArrayType(node: ArrayTypeNode) {
getTypeFromArrayTypeNode(node);
checkSourceElement(node.elementType);
}

function checkTupleType(node: TupleTypeNode) {
forEach(node.elementTypes, checkSourceElement);
}

function isPrivateWithinAmbient(node: Node): boolean {
Expand Down Expand Up @@ -6197,6 +6271,8 @@ module ts {
return checkTypeLiteral(<TypeLiteralNode>node);
case SyntaxKind.ArrayType:
return checkArrayType(<ArrayTypeNode>node);
case SyntaxKind.TupleType:
return checkTupleType(<TupleTypeNode>node);
case SyntaxKind.FunctionDeclaration:
return checkFunctionDeclaration(<FunctionDeclaration>node);
case SyntaxKind.Block:
Expand Down
20 changes: 20 additions & 0 deletions src/compiler/parser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,8 @@ module ts {
return children((<TypeLiteralNode>node).members);
case SyntaxKind.ArrayType:
return child((<ArrayTypeNode>node).elementType);
case SyntaxKind.TupleType:
return children((<TupleTypeNode>node).elementTypes);
case SyntaxKind.ArrayLiteral:
return children((<ArrayLiteral>node).elements);
case SyntaxKind.ObjectLiteral:
Expand Down Expand Up @@ -352,6 +354,7 @@ module ts {
Parameters, // Parameters in parameter list
TypeParameters, // Type parameters in type parameter list
TypeArguments, // Type arguments in type argument list
TupleElementTypes, // Element types in tuple element type list
Count // Number of parsing contexts
}

Expand Down Expand Up @@ -379,6 +382,7 @@ module ts {
case ParsingContext.Parameters: return Diagnostics.Parameter_declaration_expected;
case ParsingContext.TypeParameters: return Diagnostics.Type_parameter_declaration_expected;
case ParsingContext.TypeArguments: return Diagnostics.Type_argument_expected;
case ParsingContext.TupleElementTypes: return Diagnostics.Type_expected;
}
};

Expand Down Expand Up @@ -837,6 +841,7 @@ module ts {
case ParsingContext.Parameters:
return isParameter();
case ParsingContext.TypeArguments:
case ParsingContext.TupleElementTypes:
return isType();
}

Expand Down Expand Up @@ -872,6 +877,7 @@ module ts {
// Tokens other than ')' are here for better error recovery
return token === SyntaxKind.CloseParenToken || token === SyntaxKind.SemicolonToken;
case ParsingContext.ArrayLiteralMembers:
case ParsingContext.TupleElementTypes:
return token === SyntaxKind.CloseBracketToken;
case ParsingContext.Parameters:
// Tokens other than ')' and ']' (the latter for index signatures) are here for better error recovery
Expand Down Expand Up @@ -1390,6 +1396,17 @@ module ts {
return finishNode(node);
}

function parseTupleType(): TupleTypeNode {
var node = <TupleTypeNode>createNode(SyntaxKind.TupleType);
var startTokenPos = scanner.getTokenPos();
var startErrorCount = file.syntacticErrors.length;
node.elementTypes = parseBracketedList(ParsingContext.TupleElementTypes, parseType, SyntaxKind.OpenBracketToken, SyntaxKind.CloseBracketToken);
if (!node.elementTypes.length && file.syntacticErrors.length === startErrorCount) {
grammarErrorAtPos(startTokenPos, scanner.getStartPos() - startTokenPos, Diagnostics.Type_argument_list_cannot_be_empty);
}
return finishNode(node);
}

function parseFunctionType(signatureKind: SyntaxKind): TypeLiteralNode {
var node = <TypeLiteralNode>createNode(SyntaxKind.TypeLiteral);
var member = <SignatureDeclaration>createNode(signatureKind);
Expand Down Expand Up @@ -1420,6 +1437,8 @@ module ts {
return parseTypeQuery();
case SyntaxKind.OpenBraceToken:
return parseTypeLiteral();
case SyntaxKind.OpenBracketToken:
return parseTupleType();
case SyntaxKind.OpenParenToken:
case SyntaxKind.LessThanToken:
return parseFunctionType(SyntaxKind.CallSignature);
Expand All @@ -1443,6 +1462,7 @@ module ts {
case SyntaxKind.VoidKeyword:
case SyntaxKind.TypeOfKeyword:
case SyntaxKind.OpenBraceToken:
case SyntaxKind.OpenBracketToken:
case SyntaxKind.LessThanToken:
case SyntaxKind.NewKeyword:
return true;
Expand Down
Loading