Skip to content

Conversation

KiaraGrouwstra
Copy link
Contributor

@KiaraGrouwstra KiaraGrouwstra commented Aug 13, 2017

Add fixed length for tuples following @Aleksey-Bykov's a suggestion in #13239.
Fixes #6229, so [number, number] would not match [number] (length 2 wouldn't match 1).
I put this behind a strictTuples flag so people can also still use the old behavior.

@aluanhaddad
Copy link
Contributor

Shouldn't the length property of the new interface be readonly?

@zpdDG4gta8XKpMCd
Copy link

practically it doesn't have to be, consider:

interface X { length: 0; }
const x: X = { length: 0 };
x.length = Math.random(); // <-- type error
x.length = 1; // <-- type error
x.length = 0; // <-- allowed, but why?

@KiaraGrouwstra
Copy link
Contributor Author

It hadn't occurred to me; Array was missing readonly from length too. Adding it there makes it not compile anymore, but on Tuple it's fine, and seems consistent with most other interfaces with length-like properties. I'll go along then.

@dead-claudia
Copy link

I'd probably add readonly length: TLength just to be a little more semantically descriptive of the type, even if it doesn't really affect anything in practice.

Note that for dumb reasons, readonly is assignable to read-write types, but that's already been reported by someone else.

@KiaraGrouwstra
Copy link
Contributor Author

KiaraGrouwstra commented Aug 15, 2017

@isiahmeadows: Yeah, changed it.

I feel a bit on the fence about #13347; I hope assignability from this Tuple -> ReadonlyArray -> arrays, if viable, could facilitate granular inference through #17785.

KiaraGrouwstra added a commit to KiaraGrouwstra/TypeScript that referenced this pull request Aug 16, 2017
tuple-array assignability blocked by microsoft#17765
@KiaraGrouwstra KiaraGrouwstra force-pushed the 6229-known-length-tuples branch from fa530ff to 1f77317 Compare August 19, 2017 18:56
@KiaraGrouwstra KiaraGrouwstra changed the title prepare Tuple interface for tuples add strictTuples flag giving tuples known length Aug 19, 2017
@KiaraGrouwstra
Copy link
Contributor Author

Update: should be good now, ready for feedback.

@KiaraGrouwstra
Copy link
Contributor Author

Travis randomly complaining:

npm ERR! code EPEERINVALID npm ERR! peerinvalid The package typescript@2.6.0-dev.20170819 does not satisfy its siblings' peerDependencies requirements! npm ERR! peerinvalid Peer tslint@5.6.0 wants typescript@>=2.1.0 || >=2.1.0-dev || >=2.2.0-dev || >=2.3.0-dev || >=2.4.0-dev || >=2.5.0-dev || >=2.6.0-dev npm ERR! peerinvalid Peer gulp-typescript@3.2.1 wants typescript@~2.0.3 || >=2.0.0-dev || >=2.1.0-dev || >=2.2.0-dev || >=2.3.0-dev || >=2.4.0-dev || >=2.5.0-dev 
@ikatyang
Copy link
Contributor

Tests should be fixed by ivogabe/gulp-typescript#536.

@KiaraGrouwstra
Copy link
Contributor Author

Merged for retry, Travis seems good again. Thanks @ikatyang.

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

I like this change, but I'm going to try implementing tuple-freshness first to see if it works well too.

if (strictTuples) {
const lengthSymbol = createSymbol(SymbolFlags.Property, "length" as __String);
lengthSymbol.type = getLiteralType(arity);
lengthSymbol.checkFlags = CheckFlags.Readonly;
Copy link
Member

Choose a reason for hiding this comment

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

I don't see how adding Readonly helps if the type is literally 2, because only 2 can be assigned to it. If the intent is future-proofing in case Readonly will someday propagate through assignments, I think that it's as likely to harm as it is to help.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, fair enough. I had no special intentions in adding it. Feel free to adjust as you see fit! :)

@sandersn
Copy link
Member

sandersn commented Nov 7, 2017

  • Only one additional error in firebase, but it's the same usage that was already flagged by the strict-length, discussed above.
  • No changes in Definitely Typed.
  • No changes in internal MS projects.
  • A few tests changes to assert that push and pop do not work on tuples.

Given that the total impact of this change is 3 good breaks (firebase, leaflet and highcharts), plus incorrect array syntax (9), I think that it is small enough to ship without a flag.

@sandersn
Copy link
Member

sandersn commented Nov 8, 2017

We had one more meeting, and decided to ship this PR without a flag and without TupleBase. Shipping without a flag allows everybody to get the better checking with no work, and more easily unlocks future features based on fixed-length tuples. TupleBase is a nice idea in theory, but it's too cute in relying on the specifics of never assignability, and we suspect it will get us in trouble later.

@sandersn sandersn merged commit 90f87ef into microsoft:master Nov 8, 2017
@sandersn
Copy link
Member

sandersn commented Nov 8, 2017

Thanks @tycho01!

@mhegazy
Copy link
Contributor

mhegazy commented Nov 8, 2017

@sandersn please add a note about this change in the breaking change section for TS 2.7.

We need also some guidance for the firebase use case

@mightyiam
Copy link

Hooray!

@Igorbek
Copy link
Contributor

Igorbek commented Nov 9, 2017

@tycho01 @sandersn you are the best! Thank you! @sandersn now this should unblock #5453, right? :)

@mhegazy mhegazy changed the title add strictTuples flag giving tuples known length Make tuples have known length Nov 9, 2017
@KiaraGrouwstra
Copy link
Contributor Author

Thanks for the merge! At this rate I may get the motivation to finish those last few PRs too. :)

gcanti added a commit to gcanti/io-ts that referenced this pull request Nov 10, 2017
gcanti added a commit to gcanti/io-ts that referenced this pull request Nov 10, 2017
gcanti added a commit to gcanti/io-ts that referenced this pull request Nov 27, 2017
gcanti added a commit to gcanti/io-ts that referenced this pull request Nov 27, 2017
gcanti added a commit to gcanti/io-ts that referenced this pull request Nov 27, 2017
@michaeljota
Copy link

This should be added to the January release notes roadmap.

@sandersn
Copy link
Member

sandersn commented Jan 2, 2018

@michaeljota I added it to the roadmap.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet