Skip to content

Proposal to provide cleaner representation for poly variants #4295

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

Closed
Risto-Stevcev opened this issue Apr 10, 2020 · 14 comments
Closed

Proposal to provide cleaner representation for poly variants #4295

Risto-Stevcev opened this issue Apr 10, 2020 · 14 comments

Comments

@Risto-Stevcev
Copy link

Risto-Stevcev commented Apr 10, 2020

One of the pain points of using bucklescript over typescript are the type representations when compiled to js. Having the unboxed option type turns out to be extremely useful both for targeting js (ie: writing libs in bucklescript that can be used in js projects) and ingesting js libs.

The proposal here is provide a cleaner js representation for poly variants or both poly and regular variants. For example:

let a = `foo

becomes

const a = "foo"

and

let b = `bar 123

becomes

const b = { bar: 123 }

This would make it much easier to interact with js libs, making something like [@bs.unwrap] unnecessary. It would also make it easier to write libraries that can be used as both a bucklescript library and a plain js library, because the representations would compile to clean js code without the need for a compatibility module for js with jsConverters that could potentially lead to a really large bundle size.

@Risto-Stevcev Risto-Stevcev changed the title Proposal to "unbox" poly variants Proposal to provide cleaner representation for poly variants Apr 10, 2020
@TheSpyder
Copy link
Contributor

This is a cool idea, but perhaps as an optional extension? I really like how poly variants are represented in JS at the moment, at a certain size binary tree lookup will be more efficient than a switch statement (although it would be nice if the numbers used were smaller to reduce bundle size).

What about tuples though?

let b = `bar(123, 456)

var b = { bar: [123, 456] } is still a bit weird.

@TheSpyder TheSpyder mentioned this issue Apr 16, 2020
4 tasks
@sgrove
Copy link

sgrove commented Apr 16, 2020

There's also a question of whether this could be implemented in a way that would allow modeling of discriminated/tagged unions like we've seen from @ryyppy https://dev.to/ryyppy/tagged-unions-and-reasonml-variants-4428 (and @zth has mentioned this idea as well)

It might be out of scope for this idea, but it also might be a nice thing to consider to see if it's capable of being succinctly represented.

@yawaramin
Copy link
Contributor

So in #3801 Bob is proposing to encode ordinary variants like Error "foo" ==> { tag: 1, _1: "foo" } (note numeric tag), I think it then makes sense to encode polyvariants like `Error "foo" ==> { tag: "Error", _1: "foo" } (note string tag).

@sgrove
Copy link

sgrove commented Apr 17, 2020

In that case, you'd presumably want to do the equivalent of [@bs.discriminator_as "kind/tag/type"] ?

@TheSpyder
Copy link
Contributor

Polymorphic variants also have a numeric tag today, it's just a long generated number. That might make it quite difficult to use strings for the tag field.

@bobzhang
Copy link
Member

I am thinking to make it like this {hash: long_number, value: .. , name : "Error"}

@sgrove
Copy link

sgrove commented Apr 24, 2020

@bobzhang Is that also for flat polymorphic variants?

To model a lot of the enums that underlying JavaScript uses strings for, it'd be great to be able to compile out to strings (especially if it also works with [@bs.as]):

module Column = {
    [@bs.deriving abstract]
    type props = {
      [@bs.optional]
      align: [ | `left | `right | [@bs.as "Center"] `center],
    };
  }
}

Have left, right, and center compile out to "left", "right", and "Center" would really lighten the burden of good, safe interop.

@zth
Copy link
Collaborator

zth commented Apr 25, 2020

For interop I think what would be really nice is:

  • Like @sgrove says, raw strings corresponding to the variant name for poly and regular variants that don't have a payload. Again, just like @sgrove says, this would make bindings much cleaner.
  • For variants with a payload, compiling them to objects with a (configurable?) discriminator key.

Let me expand a bit on that last point. As can be seen in the article from @ryyppy and from looking at how most ASTs (and unions overall) are represented in TS/JS, there's a whole host tools that use a structure like this for unions in TypeScript/Flow:

interface VariableNode {
  kind: "VariableNode"; // String literal
  value: string; // ...any props specific to this particular node is just put right on the node itself
  name: string;
}

interface ArgumentNode {
  kind: "ArgumentNode"; // String literal
  argumentName: string;
}

type ast = VariableNode | ArgumentNode;

If we could get BuckleScript to somehow natively understand that type of structure without needing conversion, that would be very powerful. Some pseudo code:

type ast [@bs.discriminator "kind"] = VariableNode({ value: string, name: string }) | ArgumentNode({ argumentName: string });

This would mean that we could bind to a whole host of ASTs in JS with zero runtime costs for the developer (Babel, TypeScript, GraphQL are just a few I've seen that uses that structure, but I think using kind as a discriminator is a convention that most use), which in turn would unlock some really interesting use cases since Reason is very well fit for working with ASTs and unions.

Again, this is just what I think would be great for interop, no other cases taken into consideration here ;)

@yawaramin
Copy link
Contributor

@zth yeah, similar to #3801 (comment) . Fwiw I think for polymorphic variants (with payloads) a tag field with the actual variant tag as a string makes a lot of sense. It makes exporting to JS/TS/Flow simpler.

For polyvariants a [@bs.discriminator] etc. attribute makes less sense because polyvariant types are often not defined beforehand and their values are often not annotated anyway. That's part of their lightweight nature. Maybe a compiler flag would make sense (not sure).

@Risto-Stevcev
Copy link
Author

@yawaramin I can see a use case for both forms:

type t = [ `foo | `bar of int ]
let a = `foo
let b = `bar 123
  1. Compiled to simple strings:
var a = "foo"
var b = { type: "bar", value: 123 }
  1. Compiled to uniform syntax so that you can 'pattern match' on it with a switch statement (with hopefully the value staying in a single key value for ergonomics):
var a = { type: "foo" }
var b = { type: "bar", value: 123 }

I don't think a compiler flag would fix all the issues, because you might want to mix and match styles. And I can also see there being a big split on this issue since there's a desire for both styles.

The thing is though that, unlike #1, the #2 form can be achieved with some reworking of the code:

type t = [ `foo of unit | `bar of 123 ]
let a = `foo ()
let b = `bar 123

compiling to:

var a = { type: "foo" }
var b = { type: "bar", value: 123 }

Which would make the #1 version preferable in my opinion, since #2 is still achievable and not unpleasant to work with.
And the same could be applied to ordinary variants
cc @bobzhang @chenglou

@yawaramin
Copy link
Contributor

@Risto-Stevcev oh I didn't mean a compile flag for the simple string vs uniform shape outputs, I meant for the name of the type prop. Some people call it tag, others kind, etc. For polyvariants without a payload I kind of assumed a simple string is the best option.

@bobzhang
Copy link
Member

As an intermediate step, in next version , for non-nullable poly-variant
``bar 123is going to be compiled into{ hash : HASH_OF_BAR, value : 123, name : "bar}`
I agree the data-representation could be made even better, that's something we plan to do in the future

@TheSpyder
Copy link
Contributor

Ah so you're keeping the hash for all, nice. At least my concern won't be a problem, even if it's still less than ideal. I can guess how hard it would be to not have the hash available at runtime.

@bobzhang
Copy link
Member

bobzhang commented Jun 1, 2020

so for non-debug mode it would be

{ HASH: xx, VAL : xx}

for debug mode, it is

{ HASH: xx,  VAL : xx, [Symbol.for("name")] : "bar"}

Note that the current encoding is better but still not ideal, we will stop here and revisit it when we upgrade the upstream compiler

@bobzhang bobzhang closed this as completed Jun 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants