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

Empty tuple type now meaningful (at least, in unions) #13126

Closed
krryan opened this issue Dec 22, 2016 · 13 comments
Closed

Empty tuple type now meaningful (at least, in unions) #13126

krryan opened this issue Dec 22, 2016 · 13 comments
Assignees
Labels
Fixed A PR has been merged for this issue Suggestion An idea for TypeScript

Comments

@krryan
Copy link

krryan commented Dec 22, 2016

I have a situation where the only valid states for a property are [], [A], or [A, B, B], so I would like to define that property as [] | [A] | [A, B, B]. But this gives the error "A tuple element list cannot be empty," and indeed I see that there were a few prior issues relating to the compiler having (incorrectly) allowed it in the past. It seems to me that now, between overloading and unions, this type is useful. Also useful in a typeguard: isEmpty<A>(values: A[]): values is [] { return values.length === 0; }

I suppose the most difficult part of it would be actually tracking that a given array is empty, so that it could be matched against that definition.

Per the checklist, the grammar is simple enough, [] in type positions, shouldn't have any issues with respect to JavaScript. I don't foresee any semantic issues except the above bit about actually recognizing empty arrays as such. The emit isn't affected at all, and compatibility shouldn't be an issue. I'm not rightly sure about the concerns raised in the Other section.

@mhegazy mhegazy added Suggestion An idea for TypeScript In Discussion Not yet reached consensus labels Dec 22, 2016
@RyanCavanaugh RyanCavanaugh added Help Wanted You can do this and removed In Discussion Not yet reached consensus labels Jan 10, 2017
@RyanCavanaugh
Copy link
Member

Approved, accepting PRs. The type [] should have an index signature type of never, and needs to be assignable from the literal [] (in both strictNullChecks on and off)

@mhegazy mhegazy added this to the Community milestone Jan 10, 2017
@KiaraGrouwstra
Copy link
Contributor

From @Aleksey-Bykov at his #13239 on what distinguishes tuples (like this 0-ary one):

no push, shift, unshift, pop, slice, splice operations

[] [...] possesses some unique properties yet very similar to ReadOnlyArray (except for the index signature and length property)

the length property that is always 0

no index signature

To summarize compared to ReadOnlyArray:

  • same: no mutating methods
  • more granular: known length
  • proposed difference: no index signature

My instinct had been to have a Tuple sub-typing ReadOnlyArray to specify the known length. This implies retaining the index (yielding just a union of the tuple's constituent types), such as to keep e.g. [number] assignable to ReadOnlyArray<number>, and just having it return never for empty tuple as stated by Ryan. Would you mind commenting on your request to keep the index out, @Aleksey-Bykov?

Interestingly, specifying length may address #6229.

@KiaraGrouwstra
Copy link
Contributor

KiaraGrouwstra commented Aug 28, 2017

@gcnew at #17767 (comment):

I think it would have been best if tuples behaved like object with their indices as keys. All the normal object rules should be in effect. For example, the indexing type variable should be a keyof of the tuple, or the tuple should be intersected with an indexer to provide a default value.

// ['syntax', true] => { 0: 'syntax', 1: true }
type Tup = ['syntax', true]
type T1<I> = Tup[I] // error
type T2<I extends number> = Tup[I] // error
type T2<I extends keyof Tup> = Tup[I] // OK
type T2<I extends number> = (Tup & { [k: number]: undefined })[I] // OK

On a side note, I don't think tuples should be a subclass of Array. In the general sense, they are not arrays, just values packed together. That's further exacerbated by the fact that tuples are heterogeneous in the majority of cases.

Question based on that, also to @Aleksey-Bykov: would it not be unfortunate if dropping the index would mean tuples would even no longer match ArrayLike? It would be possible to refer to tuples in general if they had their own interface, but it would seem to get harder to refer to any array (be it homogeneous or tuple) this way.

@gcnew
Copy link
Contributor

gcnew commented Aug 28, 2017

@tycho01
If tuples have a length property, they will be assignable to ArrayLike.

type Tuple2<A, B> = { 0: A, 1: B, length: 2 }

declare function mkTuple2<A, B>(a: A, b: B): Tuple2<A, B>;
declare function useArrayLike<T>(x: ArrayLike<T>): void;

useArrayLike(mkTuple2(1, 'hello'));

On the other hand, an exact length type will make Tuple3 not assignable to Tuple2, which depending on the use-case might be a good or a bad thing.

To be honest, I haven't thought over all consequences and edge cases.

@KiaraGrouwstra
Copy link
Contributor

KiaraGrouwstra commented Aug 29, 2017

If tuples have a length property, they will be assignable to ArrayLike.

And here I thought that needed an index. Pretty cool!

On the other hand, an exact length type will make Tuple3 not assignable to Tuple2, which depending on the use-case might be a good or a bad thing.

Right. So the consequences of this one we know at least (#17765, see changed test files). Result: not so bad! Wait a second, it was that other PR (#17785) I tested over the whole project. Whoops.

@KiaraGrouwstra
Copy link
Contributor

KiaraGrouwstra commented Aug 29, 2017

To put discussions on a first-class Tuple interface (as a ReadonlyArray ripoff without number index) in further context, I've added an experimental branch for this here.
I tried for a bit, but am currently getting assignability errors like this one:

1430             addRange(result, <Symbol[]>checker.getSymbolsOfParameterPropertyDeclaration(<ParameterDeclaration>symbol.valueDeclaration, symbol.name));
src/services/findAllReferences.ts(1430,30): error TS2352: Type '[Symbol, Symbol]' cannot be converted to type 'Symbol[]'.
  Property 'push' is missing in type '[Symbol, Symbol]'.

Edit: note that this issue also occurs if you even just tie tuples to ReadonlyArray instead of Array.

@sandersn
Copy link
Member

sandersn commented Nov 2, 2017

Here's what I found from running the tests on a variant of #17765 that also makes tuples extend ReadonlyArray instead of Array. I have not run DefinitelyTyped yet, but may do so tomorrow.

Overview

There's not a lot of value from this change relative to the number of errors. It forces you to make your types a lot more precise, but doesn't find much more incorrect code than just adding a length property.

Good

  1. [number, number] ===> number[] is no longer assignable. Found in the test suite.
  2. Tuples are readonly now: tuple[0] = 'hi' is never legal. Not sure if this is good or bad.

Bad

  1. In a couple of places, Typescript itself assigns [Symbol, Symbol] to Symbol[]. Then it doesn't mutate them (mostly it does this as a result of assigning signatures). Changing this to ReadonlyArray<Symbol>, or to [Symbol, Symbol], would fix the error, but doesn't provide any other benefit.
  2. In a large internal Microsoft project that uses tuples heavily, there are 3 or so places where Node-like superclasses need to declare their children: ReadonlyArray<Node> instead of Node[], because many Node subclasses declare children: [DerivedNode] because they know they will have exactly one child. This causes a large number of errors, several per subclass, plus others wherever subclasses are used.

The code is correct here, the types are just not safe. In general, because tuples are limited, advanced usage will unsafely cast them to any[], and that code will have to be updated to ReadonlyArray<any> (or some safer type like Node[] in the previous example). This shows up in the test suite as well.

  1. firebase-js-sdk uses the loose definition of tuples to create two related types in one:
type SpecDoc = [string, Snapshot, Json<AnyJS>];
type DirtySpecDoc = [string, Snapshot, Json<AnyJS>, 'local'];

Then they use Array.splice(3) to convert DirtySpecDoc -> SpecDoc and Array.push('local') to convert SpecDoc -> DirtySpecDoc. The code is correct but the typing and encapsulation are both very smelly. To make it work with the ReadonlyArray change, firebase would have to add the DirtySpecDoc type and conversion functions to convert between the two (or just switch to objects with an optional property).
But this also doesn't fix any bugs; the code is correct as-is, just smelly.

Ugly

  1. More confusion between [T] and T[], this time in tslint and angular2.

@KiaraGrouwstra
Copy link
Contributor

KiaraGrouwstra commented Nov 3, 2017

@sandersn: thank you for summarizing; that's pretty insightful.
OOP devs may prefer tuple[0] = 'hi', yeah -- perhaps ReadonlyArray warrants a flag.
If ReadonlyArray<Symbol> is verbose for devs, using ArrayLike or aliasing (List?) may help.

Firebase example: concat / slice aid static typing, but error; type inference yields a unioned list over the expected tuple, and needs an explicit cast. Confusing, but tuple-proof inference needs #5453. Currently added value seems limited to disallowing mutating methods?

@sandersn
Copy link
Member

sandersn commented Nov 3, 2017

I'm running definitely typed right now to find out what OO devs prefer (since I'm horrible at guessing that :). The only mutation right now is in a test that asserts "tuples are mutable", which is not indicative of actual usage.

I don't think making array methods into tuple-type-altering methods is a great idea, even if it were possible, since I think it would only help a subset of cases.

Surprisingly, disallowing push et al really only exposed those [T] mistakes in tslint and angular2. firebase meant to push and splice tuples, even if it's a bad idea.

@KiaraGrouwstra
Copy link
Contributor

KiaraGrouwstra commented Nov 3, 2017

Great, pretty curious as to the results!

On tuple methods, my initial instinct is, when concat-ing to a tuple, the sensible default would be to retain a similar level of info. If the output type was expected to be a list, I'd expect the input type to be one as well.
In that sense, overall I'd expect more benefit than harm.
I may well be missing considerations there though -- I know erring in either direction can end up undesirable in some cases.

@sandersn
Copy link
Member

sandersn commented Nov 3, 2017

Definitely Typed catches a couple of good errors, and it exposes some limitations in type argument inference, return type inference and contextual typing, where the compiler gets a tuple type when it should be an array type, or vice versa. These bugs aren't the fault of the authors, but would have to be fixed before making this change.

Good

  • Leaflet (again): leaflet.Bounds should be ([Point, Pount] | [PointLiteral, PointLiteral]), but is currently defined as (Point[] | [PointLiteral, PointLiteral]).
  • memcached's tests want to assert that a tuple type is an array type, but this is no longer legal (although it would succeed at runtime).

Bad

  • 8 tests don't care that the return type is a tuple and just assign it to an equivalent array type.
  • 8 projects need to update their supertypes or fake tuple types with ReadonlyArray. ramda, chartist and d3-voronoi have fake tuple types (eg interface MyTuple extends Array<T | U> { 0: T, 1: U }).
  • mz: a completer callback must return [string[], string], but return-type inference for a function declaration only ever infers (string | string[])[].
  • diff-match-patch and wu tests use assertEqual, which can't infer a tuple type, which is now an error:
let pairs: [string, number][];
assertEqual([['a', 1], ['b', 1]], pairs)

This is a limitation of inference that is exposed by this change.

  • sparqljs tries to pass ['foo' as TaggedString] to a function that accepts TaggedString[] | ["*"]. Contextual typing is messed up here and assigns the argument the type [TaggedString], which is no longer assignable to TaggedString[].

This is a mistake in contextual typing exposed by this change.

Ugly

  • adone has a function arrify<T>(ts: T[]): T[]; arrify<T>(t: T): [T]. This is clever, but I don't expect that anybody relies on it.
  • adone defines a declare const exts: [".js", ".tjs", ".ajs"]. This is clever, but results in a tuple, not a string[]. It's only defined this way because we don't allow arrays of literals to initialise ambient constants.

@KiaraGrouwstra
Copy link
Contributor

I wonder if the fake tuples predated tuple syntax.

The assertEqual case looks familiar to me. When I tried a flag to allow unwidened types (#17785), <T>(a: T, b: T) functions were the main thing that broke as well.
It's probably just me, but perhaps the generics on such comparison functions aren't particularly productive, since they rely on the first type to be inferred as broad as the second.

If that's the total damage on DT for known length + ReadonlyArray though... not bad!

@ajafff
Copy link
Contributor

ajafff commented Aug 10, 2018

Is this resolved by the recent addition of empty tuple types?

@DanielRosenwasser DanielRosenwasser added Fixed A PR has been merged for this issue and removed Suggestion An idea for TypeScript Help Wanted You can do this labels Aug 10, 2018
@DanielRosenwasser DanielRosenwasser added the Suggestion An idea for TypeScript label Aug 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fixed A PR has been merged for this issue Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

9 participants