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

Make tuples have known length #17765

Merged
merged 17 commits into from
Nov 8, 2017

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: strict and open-length tuple types