Support for tuples #71

jchavarri · 2018-10-21T14:08:18Z

Fixes #68.

This was way easier than I expected, so I guess there must be many things missing 😄 I'll add some inline comments below.

Side note: I find the codebase to be really beautiful ✨ and I'm quite enjoying its discovery. What a delight to see how building on top of recursive types can lead to such a modular architecture!!

jchavarri · 2018-10-21T14:08:45Z

examples/reason-react-example/src/basics/Tuples.re

@@ -0,0 +1,48 @@
+open Belt;


Should I add Tuples.re also to the TypeScript example folder?

jchavarri · 2018-10-21T14:10:33Z

src/Converter.re

@@ -222,6 +226,8 @@ let rec converterIsIdentity = (~toJS, converter) =>
    }

  | RecordC(_) => false
+  | TupleC(innerTypesC) =>
+    innerTypesC |> List.for_all(converterIsIdentity(~toJS))


Mostly the same as the ObjectC case above, but didn't use the OptionC switch because I didn't think it was necessary (doesn't that case get process naturally when the recursion continues?)

Option has no special role here, while it does for objects. So this is perfect.

jchavarri · 2018-10-21T14:14:10Z

src/Dependencies.re

+  | Ttuple(listExp) =>
+    let innerTypesTranslation =
+      listExp |> translateTypeExprs_(~language, ~typeVarsGen, ~typeEnv);
+    let innerTypes = innerTypesTranslation |> List.map(({typ, _}) => typ);


Very inspired on the Tconstr(path, typeParams, _) case below.

jchavarri · 2018-10-21T14:17:36Z

src/EmitTyp.re

+    "["
+    ++ (
+      innerTypes
+      |> List.map(typ => typ |> renderTyp(~language, ~indent, ~inFunType))


Should tuples be emitted like immutable arrays?

It’s a good question. They’re immutable on the Reason side so I’d say yes.
Nit: typ => typ |> can be omitted: curry.

Also, TS has to is peculiar about naming types. Worth adding the examples. It might dislike the “[...]” for non-basic types.
Immutable arrays would get you out of that notation anyway.

cristianoc

Looks awesome.
A couple of small comments inline.

cristianoc · 2018-10-21T16:06:50Z

src/EmitTyp.re

+    "["
+    ++ (
+      innerTypes
+      |> List.map(typ => typ |> renderTyp(~language, ~indent, ~inFunType))


It’s a good question. They’re immutable on the Reason side so I’d say yes.
Nit: typ => typ |> can be omitted: curry.

Also, TS has to is peculiar about naming types. Worth adding the examples. It might dislike the “[...]” for non-basic types.
Immutable arrays would get you out of that notation anyway.

cristianoc · 2018-10-21T16:07:15Z

examples/reason-react-example/src/basics/Tuples.re

@@ -0,0 +1,48 @@
+open Belt;


cristianoc · 2018-10-21T16:08:08Z

src/Converter.re

@@ -222,6 +226,8 @@ let rec converterIsIdentity = (~toJS, converter) =>
    }

  | RecordC(_) => false
+  | TupleC(innerTypesC) =>
+    innerTypesC |> List.for_all(converterIsIdentity(~toJS))


Option has no special role here, while it does for objects. So this is perfect.

cristianoc · 2018-10-21T16:24:06Z

@jchavarri another question, more like an afterthought is: how much can we re-use that exists already?
E.g. can we just re-use everything done for the array type?
I guess not as the types of tuples can be heterogeneous. Can we then re-use something -- e.g. the copying of elements (i.e. ArrayC) and is it worth it, and would it type check anyway?

jchavarri · 2018-10-21T19:35:26Z

Thanks for the review! I have added the TS tests, and removed the unneeded lambda with typ param.

Should tuples be emitted like immutable arrays?

They’re immutable on the Reason side so I’d say yes.

I have done a bit of research, and immutable tuples seem to have very bad support at the moment in both Flow and TypeScript.

In Flow, there is no way to mark a tuple or its inner types as immutable. $ReadOnly can only be used in objects, and adding + decorators only works for object fields. See related: facebook/flow#4509.

In TypeScript, wrapping the tuple with Readonly seems to be valid syntactically, but it has very little effect in the final result:

let tuple: Readonly<[string, number]> = ['text', 3];

tuple[0] = 'new text'; // TS says: "All good!"
tuple.shift(); // Can remove elements from the tuple, even if it's read-only
let a: string = tuple[0]; // Type checks, but it'll be a number at runtime

Playground.

I don't really understand the purpose of Readonly in this case. 😕

Considering this, I wonder if it's more appropriate to just leave tuples as they are, just to make more explicit they are mutable, to reflect better these limitations.

can we just re-use everything done for the array type?

I thought about this. I'm not sure there are many things to reuse.

At the type definition level, TS syntax for tuples ([one, two]) is different than arrays (string[]), where the brackets go at the end. Flow syntax for arrays and tuples with brackets is the same, so in this case it could be reused. But considering how the Flow emitters for arrays are currently using the $ReadOnlyArray<> syntax, then this case would not allow for reusability either.

At the converter levels, arrays and tuples are different, with the first using map and the second being able to expand the array inline.

So in practice there seem to be different constraints for both cases, but I could explore trying to unify them more if you think it's worth it.

TS has to is peculiar about naming types. Worth adding the examples.

I added the Tuple.re tests to the TypeScript example, and I'm actually getting some errors, but I think they are not related to this addition.

The type couple gets converted to an abstract class and this causes a few issues:

tslint warns that a class must be named with pascal case
but more importantly, the functions that return or accept a value of type couple fail to type check because "Property 'opaque' is missing in type '{ name: any; age: any; }[]".

Do you have any pointers? I think in this case couple shouldn't be abstract / opaque 🤔

Thanks again!

cristianoc · 2018-10-21T20:07:33Z

Thanks for digging, this is great info! OK so it looks like immutability for tuples falls into the high investment low return category, and can be dropped.

couple sounds like the newly introduced tuple types are considered opaque. There's some pattern matching case analysis to decide what types are opaque, and this is probably a case of tuples missing from there.

cristianoc · 2018-10-21T20:10:37Z

Wrong guess. You've handled the code for checking what's opaque.

cristianoc · 2018-10-21T20:14:11Z

OK it was not far off. It's this line:

| Ident(_) => !(typ == booleanT || typ == numberT || typ == stringT)

meaning that any other compound of a tuple than those would be considered opaque.

So pre-existing.
Would you comment-out the example that triggers this. We can merge this, and I'll look into that separately.

jchavarri · 2018-10-21T20:54:21Z

@cristianoc Done! 🙂

cristianoc · 2018-10-21T21:12:50Z

So good. Thank you so much for this.

cristianoc · 2018-10-21T21:23:57Z

The issue is strongly correlated with Type Expansion mentioned in #70. So it makes sense to handle all of that together.
(Some type expansion is required to figure out that the inlined version of the type is not opaque).

Support for tuples

046e9be

jchavarri commented Oct 21, 2018

View reviewed changes

cristianoc approved these changes Oct 21, 2018

View reviewed changes

Add tuples tests to TypeScript example

533d428

Remove failing code from TS example

2a59183

cristianoc merged commit 6b694a8 into rescript-association:master Oct 21, 2018

cristianoc mentioned this pull request Oct 21, 2018

API design and language cleanup. #70

Closed

jchavarri deleted the tuples branch October 21, 2018 21:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for tuples #71

Support for tuples #71

jchavarri commented Oct 21, 2018 •

edited

Loading

jchavarri Oct 21, 2018 •

edited

Loading

cristianoc Oct 21, 2018

jchavarri Oct 21, 2018

cristianoc Oct 21, 2018

jchavarri Oct 21, 2018

jchavarri Oct 21, 2018

cristianoc Oct 21, 2018

cristianoc left a comment

cristianoc Oct 21, 2018

cristianoc Oct 21, 2018

cristianoc Oct 21, 2018

cristianoc commented Oct 21, 2018

jchavarri commented Oct 21, 2018

cristianoc commented Oct 21, 2018

cristianoc commented Oct 21, 2018

cristianoc commented Oct 21, 2018

jchavarri commented Oct 21, 2018

cristianoc commented Oct 21, 2018

cristianoc commented Oct 21, 2018

Support for tuples #71

Support for tuples #71

Conversation

jchavarri commented Oct 21, 2018 • edited Loading

jchavarri Oct 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cristianoc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cristianoc commented Oct 21, 2018

jchavarri commented Oct 21, 2018

cristianoc commented Oct 21, 2018

cristianoc commented Oct 21, 2018

cristianoc commented Oct 21, 2018

jchavarri commented Oct 21, 2018

cristianoc commented Oct 21, 2018

cristianoc commented Oct 21, 2018

jchavarri commented Oct 21, 2018 •

edited

Loading

jchavarri Oct 21, 2018 •

edited

Loading