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

Tuple types accepting incorrect data #19463

Closed
deregtd opened this issue Oct 25, 2017 · 16 comments
Closed

Tuple types accepting incorrect data #19463

deregtd opened this issue Oct 25, 2017 · 16 comments
Assignees
Labels
Fixed A PR has been merged for this issue Suggestion An idea for TypeScript

Comments

@deregtd
Copy link

deregtd commented Oct 25, 2017

TypeScript Version: 2.6.0-rc

Code

let a: [string] = ['a', 'b'];

Expected behavior:
Error: [string, string] is not compatible with [string].

Actual behavior:
Compiles A-ok

@kitsonk
Copy link
Contributor

kitsonk commented Oct 25, 2017

The type of string[] and [string, string] is assignable to [string]. Tuple types are not checked for excess elements when fresh, unlike objects, so the right hand side is assignable to left hand side.

It is arguable though when the array is fresh, it should follow the same rules as #3823 for objects.

@DanielRosenwasser
Copy link
Member

Think of it like this

interface Tuple1<T> extends Array<T> {
    0: T;
}

interface Tuple2<T, U> extends Array<T | U> {
    0: T;
    1: U;
}

declare let a: Tuple1<string>;
declare let b: Tuple2<string, string>;

a = b; // works, 'b' is also viewed as a `string[]` with a `0: string`

@DanielRosenwasser
Copy link
Member

See also: #6229

@DanielRosenwasser DanielRosenwasser added the Duplicate An existing issue was already created label Oct 25, 2017
@deregtd
Copy link
Author

deregtd commented Oct 25, 2017

Did you make some specific decision to untype arrays then, compared to normal types? I don't see why this is any different from:

let a: { x: number } = { x: 4, y: 5 };

... which compile errors as you'd expect. If this is intended typescript behavior permanently, then we will have to excise all usage of tuples from our codebase and go to slower object-based types everywhere. This implementation is almost completely un-typesafe. :(

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Oct 25, 2017

let a: { x: number } = { x: 4, y: 5 };

TypeScript's original behavior allowed this. The motivating example for disallowing it was options-bag-style APIs where people were misspelling optional properties. Are you frequently running into excess tuple errors? (edit: not being cheeky, I'm legitimately looking for motivating use cases)

@deregtd
Copy link
Author

deregtd commented Oct 25, 2017

Yes, we just made a correction to our codebase to change over a hundred usages of tuples that were being used incorrectly.

@deregtd
Copy link
Author

deregtd commented Oct 25, 2017

The question in my book is what the pros and cons of each way are. What are the pros of allowing you to do what feels like, to me, "invalid" assignments to tuples? I.e. why SHOULD it be allowed to do:

let a: [string] = [ 'hi', 12345 ];

That feels like it's an engineer bug in 100% of circumstances, and if we have the ability to fix it with TSC, then we should.

@kitsonk
Copy link
Contributor

kitsonk commented Oct 26, 2017

Actually that example is disallowed. Excess assignment slots only work when the excess values are assignable to one of the member of the tuple. 🤷‍♂️

let a: [ string, number ] = [ 'hi', 12345, 'foo' ]; // ok
let b: [string] = ['hi', 12345]; // bad
let c: [string | number] = ['hi', 12345]; // ok

Personal opinion too, when the array is fresh, it should be strictly checked against the left hand, because as @deregtd indicates, how is that ever a desired situation? Especially considering how the following works:

let a: [string, number] = ['hi', 12345];
let b: [string] = a; // not assignable
let c: [string|number] = a; // ok

That, again, while maybe sound from a type system perspective, I can't see why the assignment to b is bad while c is good from an identifying constructs that are likely to be errors. Either both are desirable, or neither.

@mhegazy mhegazy added Suggestion An idea for TypeScript In Discussion Not yet reached consensus and removed Duplicate An existing issue was already created labels Oct 26, 2017
@mhegazy
Copy link
Contributor

mhegazy commented Oct 28, 2017

From #19536, we would like to consider one of two alternatives here:

  1. Treat all tuples as exact types, where the length property is manifest in the type, and two tuples are only assignable if they have the exact same length. Tuples will need to be ReadonlyArray for this to work. We will have to include this under a strict flag.
  2. Apply freshness rules on tuple literals, and flag excess members just like we do with objects.

@deregtd
Copy link
Author

deregtd commented Oct 30, 2017

#1 sounds perfect to me. The way we often use tuples, it's basically just a quick and strong dynamic type that doesn't require wrapping in an interface.

@RyanCavanaugh RyanCavanaugh added Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature and removed In Discussion Not yet reached consensus labels Oct 30, 2017
@RyanCavanaugh
Copy link
Member

See notes in #19585

@Igorbek
Copy link
Contributor

Igorbek commented Oct 31, 2017

@mhegazy (1) seems more preferable and is what #6229 and #17765 propose.

@sandersn sandersn added this to Not started in Rolling Work Tracking Nov 1, 2017
@sandersn sandersn moved this from Not started to Feature in Rolling Work Tracking Nov 1, 2017
@sandersn
Copy link
Member

sandersn commented Nov 1, 2017

I'll implement (2) just to see how well it works, but I played with (1) and it works great. Note that #17765 implements (1) but that doesn't include the open-length tuples from #6629 (or their improved spread checking).

@Igorbek
Copy link
Contributor

Igorbek commented Nov 1, 2017

@sandersn I guess you meant 6629 #6229.

@gcnew
Copy link
Contributor

gcnew commented Nov 1, 2017

My vote goes for freshness, unless the full Strict/Open length tuples proposal gets implemented. Working with non-aliasable tuples is just painful. It will lead to many overloads, which is hardly ideal.

E.g. this is a Haskell library trying to provide uniform usage patterns for tuples of disparate arities: https://github.com/augustss/tuple. The implementation code consists of ugly, automatically generated type class instances (e.g. Select). I don't think we'd like to go in that direction.

@sandersn sandersn moved this from Feature to In Review in Rolling Work Tracking Nov 8, 2017
@mhegazy
Copy link
Contributor

mhegazy commented Nov 8, 2017

Should be fixed by #17765

@mhegazy mhegazy closed this as completed Nov 8, 2017
@mhegazy mhegazy added Fixed A PR has been merged for this issue and removed Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature labels Nov 8, 2017
@sandersn sandersn removed this from In Review in Rolling Work Tracking Nov 10, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Fixed A PR has been merged for this issue Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

8 participants