Skip to content
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

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

Merged
merged 10 commits into from Sep 15, 2014

Conversation

Projects
None yet
8 participants
@ahejlsberg
Copy link
Member

ahejlsberg commented Aug 11, 2014

This commit adds support for typle types. A tuple type is written as a comma separated sequence of types enclosed in square brackets:

[T0, T1, ... , Tn]

A tuple type corresponds to an object type that extends Array<T>, where T is the best common type of the tuple element types, with a sequence of numerically named members:

{
    0: T0;
    1: T1;
    ...
    n: Tn;
}

When an array literal is contextually typed by a tuple type, each of the array literal expressions are contextually typed by the corresponding tuple element type, and the result is a tuple type:

var t: [number, string] = [1, "hello"];
t = [];                 // Error
t = [1];                // Error
t = [2, "test"];        // Ok
t = ["test", 2];        // Error
t = [2, "test", true];  // Ok

When a value of a tuple type is indexed with a numeric constant, the resulting type is that of the corresponding type element. For example:

var x: [number, string] = [1, "hello"];
var x0 = x[0];  // Type number
var x1 = x[1];  // Type string
var x2 = x[2];  // Type {}

A tuple type is assignable to a compatible array type. For example:

var a1: number[];
var a2: {}[];
var t1: [number, string];
var t2: [number, number];
a1 = t1;  // Error
a1 = t2;  // Ok
a2 = t1;  // Ok
a2 = t2;  // Ok

Type inference works as expected for tuple types:

function tuple2<T0, T1>(item0: T0, item1: T1): [T0, T1] {
    return [item0, item1];
}
var t = tuple2("test", true);
var t0 = t[0];  // string
var t1 = t[1];  // boolean

Next step is to support EcmaScript 6 style destructuring of objects, arrays, and tuples.

// 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 &&

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Aug 11, 2014

Contributor

add newline after the &&

This comment has been minimized.

@ahejlsberg

ahejlsberg Aug 11, 2014

Author Member

Yup.

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

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Aug 11, 2014

Contributor

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

This comment has been minimized.

@ahejlsberg

ahejlsberg Aug 11, 2014

Author Member

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

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Aug 12, 2014

Contributor

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

This comment has been minimized.

@JsonFreeman

JsonFreeman Aug 12, 2014

Contributor

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

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

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Aug 11, 2014

Contributor

can you just write map(node.elementTypes, getTypefromTypeNode) instead? That saves the allocation of an unnecessary function exprssion.

This comment has been minimized.

@ahejlsberg

ahejlsberg Aug 11, 2014

Author Member

Yup, good catch.

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

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Aug 11, 2014

Contributor

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.

This comment has been minimized.

@ahejlsberg

ahejlsberg Aug 11, 2014

Author Member

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

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Aug 12, 2014

Contributor

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.

return type ? getIndexTypeOfType(type, IndexKind.Number) : undefined;
if (type) {
if (type.flags & TypeFlags.Tuple) {
var index = indexOf(arrayLiteral.elements, node);

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Aug 11, 2014

Contributor

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

This comment has been minimized.

@ahejlsberg

ahejlsberg Aug 11, 2014

Author Member

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.

@@ -3574,7 +3625,19 @@ module ts {
function getContextualTypeForElementExpression(node: Expression): Type {

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Aug 11, 2014

Contributor

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

This comment has been minimized.

@ahejlsberg

ahejlsberg Aug 11, 2014

Author Member

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

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Aug 12, 2014

Contributor

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

@@ -3711,11 +3780,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;

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Aug 11, 2014

Contributor

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.

This comment has been minimized.

@ahejlsberg

ahejlsberg Aug 11, 2014

Author Member

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.

This comment has been minimized.

@CyrusNajmabadi

CyrusNajmabadi Aug 12, 2014

Contributor

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

This comment has been minimized.

@JsonFreeman

JsonFreeman Aug 12, 2014

Contributor

Definitely agree with Cyrus here

This comment has been minimized.

@ahejlsberg

ahejlsberg Aug 13, 2014

Author Member

Ok, will add a comment.

@CyrusNajmabadi

This comment has been minimized.

Copy link
Contributor

CyrusNajmabadi commented Aug 11, 2014

This looks good. but i'm not sure now is the right time to be working on new features when we haven't even reached parity yet with the existing compiler. We need to get things like the TypeWriter and what not back online before we make changes that might introduce regressions that we don't have the infrastructure in place to catch.

a1 = a2; // Error
a1 = a3; // Error
a3 = a1;
a3 = a2;

This comment has been minimized.

@danquirk

danquirk Aug 11, 2014

Member

Would be worth breaking this up into a few different files by concept (ex tupleTypeInference, etc).

We also need a lot more coverage here. We should have tests for things like

var x = 0;
var y = 3;
var tt = tuple2(1, "a");
var r1 = tt[x];
var r2 = tt[y];

Tuples crossing module boundaries, usages in class hierarchies with array types, assignability with nested tuples and nested arrays, etc.

@basarat

This comment has been minimized.

Copy link
Contributor

basarat commented Aug 12, 2014

♨️ 😎

@JsonFreeman

This comment has been minimized.

Copy link
Contributor

JsonFreeman commented on src/compiler/checker.ts in 5b25524 Aug 12, 2014

I kind of like the original way better. It meant we knew the one precise case that .valueDeclaration would be undefined.

@JsonFreeman

This comment has been minimized.

Copy link
Contributor

JsonFreeman commented on src/compiler/checker.ts in 5b25524 Aug 12, 2014

Will we handle the Iterable case too?

This comment has been minimized.

Copy link
Member Author

ahejlsberg replied Aug 12, 2014

Not sure I understand what you mean. We will support destructuring of the form [x, y, z] = getSomeIterable() in the same way as an array, i.e. by inferring the element type of the iterable for x, y, and z. Is that what you're asking?

This comment has been minimized.

Copy link
Contributor

JsonFreeman replied Aug 12, 2014

Yes, exactly. I guess that is more about destructuring, and is separate from tuple types.

@JsonFreeman

This comment has been minimized.

Copy link
Contributor

JsonFreeman commented on src/compiler/checker.ts in 5b25524 Aug 12, 2014

getOrCreateTupleType

This comment has been minimized.

Copy link
Member Author

ahejlsberg replied Aug 13, 2014

I definitely don't like 'getOrCreateXXX'. Since we already use 'getXXX' everywhere for lazily created objects this should just be 'getTupleType'. And I suppose we should use 'createXXX' only when the function always creates a new object.

This comment has been minimized.

Copy link
Member Author

ahejlsberg replied Aug 13, 2014

Actually, on second thought I prefer sticking with 'createTupleType'. It is similar to 'createArrayType' and 'createTypeReference'. Common to all of them is that the returned object is immutable any may have come from a cache. I think it is perfectly fine to use 'createXXX' for such methods.

@JsonFreeman

This comment has been minimized.

Copy link
Contributor

JsonFreeman commented on src/compiler/checker.ts in 5b25524 Aug 12, 2014

This needs to be broken up more. The comment just says the same thing as the code, but it's difficult to read

This comment has been minimized.

Copy link
Contributor

JsonFreeman replied Aug 12, 2014

Specifically, I would extract it into a variable whose name describes the conditions you are checking for. Then put a comment describing the conditions you are excluding.

This comment has been minimized.

Copy link
Member Author

ahejlsberg replied Aug 12, 2014

I honestly don't see the problem here. The boolean expression isn't that complicated and there is a comment below that says what it does. Why would assigning the expression to a variable make it any easier to understand? Should there be variables for the preceding tests? If not, when is an expression simple enough to not warrant a variable?

This comment has been minimized.

Copy link
Contributor

JsonFreeman replied Aug 12, 2014

3 reasons why I think it is difficult to read:

  1. I had to keep looking back and forth within the first line to correctly match parentheses with their partners. It took me several tries to correctly parse the precedence of the logical operators.
  2. It does not say which flags we deliberately excluded.
  3. It does not say the principle behind why we chose the flags we did.

The first one can be solved with a helper function or variable. The second and third can be solved with comments.

@JsonFreeman

This comment has been minimized.

Copy link
Contributor

JsonFreeman commented on src/compiler/checker.ts in 5b25524 Aug 12, 2014

This should probably be an assert, not a check

This comment has been minimized.

Copy link
Member Author

ahejlsberg replied Aug 13, 2014

No need for a test or even an assert. I will just get rid of it.

@JsonFreeman

This comment has been minimized.

Copy link
Contributor

JsonFreeman commented on src/compiler/parser.ts in 5b25524 Aug 12, 2014

This says "Type argument"!

This comment has been minimized.

Copy link
Member Author

ahejlsberg replied Aug 13, 2014

Didn't think it justified a new error message.

This comment has been minimized.

Copy link
Member

danquirk replied Aug 13, 2014

The cost to add a new error message is very low and we should prioritize giving the best user experience by using the most specific error message we can in a given situation even if that means creating many unique errors

This comment has been minimized.

Copy link
Contributor

JsonFreeman replied Aug 13, 2014

And it is certainly not a type argument.

This comment has been minimized.

Copy link
Member Author

ahejlsberg replied Aug 13, 2014

Just added a new error message.

This comment has been minimized.

Copy link
Contributor

JsonFreeman replied Aug 13, 2014

Thanks

@JsonFreeman

This comment has been minimized.

Copy link
Contributor

JsonFreeman commented Aug 12, 2014

Looks good, although please validate this with typeWriter (not online yet) before merging into master

@@ -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));

This comment has been minimized.

@DanielRosenwasser

DanielRosenwasser Aug 13, 2014

Member

Good ol' η-reduction.

This comment has been minimized.

@basarat

basarat Aug 13, 2014

Contributor

someone ate my tea

This comment has been minimized.

@JsonFreeman

JsonFreeman Aug 13, 2014

Contributor

Someone eta my tea

This comment has been minimized.

@DanielRosenwasser

ahejlsberg added some commits Aug 13, 2014

@RyanCavanaugh RyanCavanaugh referenced this pull request Aug 15, 2014

Closed

Destructuring (ES6) #240

ahejlsberg added some commits Aug 16, 2014

Merge branch 'master' into tupleTypes
Conflicts:
	tests/baselines/reference/typeName1.errors.txt
Contextual typing of array literals is now based on the presence or a…
…bsence

of numerically named properties and doesn't directly test for tuple types.
@ahejlsberg

This comment has been minimized.

Copy link
Member Author

ahejlsberg commented Aug 17, 2014

I have revised the contextual typing logic for array literals based on the discussions we had at the design meeting. Specifically:

In an array literal contextually typed by a type T, the contextual type of an element expression at index N is the type of the property with the numeric name N in T, if one exists. Otherwise, it is the type of the numeric index signature in T, if one exists. When at least one element expression is contextually typed by a numerically named property, the resulting type of the array literal expression is a tuple type. Otherwise the resulting type is an array type.

With these rules there's nothing special about tuple types when it comes to contextual typing, and manually defined tuple-like type works just as well. For example:

interface Tuple2<T0, T1> {
    0: T0;
    1: T1;
}
var t: Tuple2<number, string> = [1, "hello"];
var x = t[0];  // number
var y = t[1];  // string
@JsonFreeman

This comment has been minimized.

Copy link
Contributor

JsonFreeman commented on src/compiler/checker.ts in 63b83e7 Aug 17, 2014

No need for type annotation

@JsonFreeman

This comment has been minimized.

Copy link
Contributor

JsonFreeman commented on src/compiler/core.ts in 63b83e7 Aug 17, 2014

Why would array be undefined here?

@JsonFreeman

This comment has been minimized.

Copy link
Contributor

JsonFreeman commented on src/compiler/core.ts in 63b83e7 Aug 17, 2014

Why would array be undefined here?

@JsonFreeman

This comment has been minimized.

Copy link
Contributor

JsonFreeman commented on src/compiler/core.ts in 63b83e7 Aug 17, 2014

Why would array be undefined here?

@JsonFreeman

This comment has been minimized.

Copy link
Contributor

JsonFreeman commented on src/compiler/core.ts in 63b83e7 Aug 17, 2014

Why would array be undefined here?

This comment has been minimized.

Copy link
Member Author

ahejlsberg replied Aug 17, 2014

All of these helpers allow the array argument to be undefined such that they can be used on optional properties without a guard. We have a number of those, e.g. the typeArguments in TypeReference and CallExpression and declarations in Symbol. We basically say that undefined is treated as an empty array.

This comment has been minimized.

Copy link
Contributor

JsonFreeman replied Aug 18, 2014

Got it. We should put a general comment above the first of these functions, which says they are safe to call on undefined.
In general, I think I would prefer to have the caller check for undefined first, but it's a personal preference.

This comment has been minimized.

Copy link
Member

DanielRosenwasser replied Aug 18, 2014

If the argument is efficiency, we might benefit more from omitting the check when it's not necessary, and when it is, simply using !!tr.typeArguments && contains(tRef.typeArguments, value).

My feeling is that the more we wander into catch-all territory, the more we build up a big ball of nully uncertainty that gets harder to reason about - we'll be accumulating technical debt.

This comment has been minimized.

Copy link
Contributor

JsonFreeman replied Aug 18, 2014

I agree with that. Someone could be modifying code that calls contains without checking the array first. I could imagine thinking,
"Oh, contains does not check the argument. It must be that it will never be undefined. Therefore I don't need to check it either".
Then I dot into it and it turns out to be undefined.

This comment has been minimized.

Copy link
Member

DanielRosenwasser replied Aug 18, 2014

@JsonFreeman indexOf and filter are better examples, but I definitely see exactly the point you're making. I expect filter(something).length to just work.

@JsonFreeman

This comment has been minimized.

Copy link
Contributor

JsonFreeman commented on src/compiler/core.ts in 63b83e7 Aug 17, 2014

Why would array be undefined here?

@JsonFreeman

This comment has been minimized.

Copy link
Contributor

JsonFreeman commented on src/compiler/checker.ts in 63b83e7 Aug 17, 2014

Interesting, so as long as there was one element contextually typed by a tuple type element, we will create properties for all the other elements as well. So in the example:

var tup: [number, string] = [0, "", 0, ""];

We will actually create a type with properties 0-3, not just 0 and 1. Correct?

This comment has been minimized.

Copy link
Member Author

ahejlsberg replied Aug 17, 2014

Correct. An array type never reflects the types of the individual elements, so the minute we see an indicated interest in those types we need to generate a tuple type.

@JsonFreeman

This comment has been minimized.

Copy link
Contributor

JsonFreeman commented on src/compiler/checker.ts in 63b83e7 Aug 17, 2014

It seems like we did all the work to get the contextual type for the array, and possibly even for the element, but we hold back on pushing it down into checkExpression. Then if a contextual type is needed within the element, it will have to redo all the work to get the contextual array type, and the contextual element type. I guess the pull model prohibits passing a contextual type even in this case?

This comment has been minimized.

Copy link
Member Author

ahejlsberg replied Aug 17, 2014

In this particular case it would actually be enough to just check whether a property with the name "" + i exists in the contextual type. We have no use for the specific type.

You're correct that if a contextual type is needed within the element expression, the work to obtain the contextual type will be redone. But in general that is actually the strength of the pull model: Only do the work if you need to. And in the vast majority of cases the element expression doesn't need the contextual type, and we actually come out ahead.

This comment has been minimized.

Copy link
Contributor

JsonFreeman replied Aug 18, 2014

I see, that makes sense. Also, the pure pull model (as opposed to both pull and push) makes it so that every contextual typing rule only has to be implemented in one place.

@@ -149,6 +149,7 @@ module ts {
TypeQuery,
TypeLiteral,
ArrayType,
TupleType,

This comment has been minimized.

@JsonFreeman

JsonFreeman Aug 19, 2014

Contributor

After you merge the changes from typeWriter (now in master), be sure to modify SyntaxKind.LastTypeNode accordingly.

ahejlsberg added a commit that referenced this pull request Sep 15, 2014

Merge pull request #428 from Microsoft/tupleTypes
Adding support for tuple types (e.g. [number, string])

@ahejlsberg ahejlsberg merged commit 11b9118 into master Sep 15, 2014

1 check passed

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

@ahejlsberg ahejlsberg deleted the tupleTypes branch Sep 15, 2014

@yuit yuit referenced this pull request Oct 4, 2014

Merged

Tuple conformance #822

@SlurpTheo

This comment has been minimized.

Copy link

SlurpTheo commented Nov 30, 2016

Was reading and wondered why ~Array#shift() is available on a Tuple Type?

const a2: [number, string] = [1, "2"];
const a3: string | number = a2.shift();	// ["2"] left in array
const a4: string = a2[0];		// ERROR: Type 'number' is not assignable to type 'string'.

Just got this in the (2.0) playground.

@aluanhaddad

This comment has been minimized.

Copy link
Contributor

aluanhaddad commented Dec 1, 2016

@SlurpTheo because Tuple is a subtype of Array.
The error is because unshift doesn't retain the tuple type.
Technically it is because unshift is available to a value of the type [number, string] by way of
[number, string] being a subtype of (number | string)[].

@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.
You can’t perform that action at this time.