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

Support tagged union types from TypeScript #2618

Merged
merged 2 commits into from
Nov 24, 2021

Conversation

cannorin
Copy link
Contributor

@cannorin cannorin commented Nov 17, 2021

Summary

This PR adds support for the "tagged" union types from TypeScript.

interface Base { kind: string }

interface Foo extends Base { kind: 'foo', foo: string }

interface Bar extends Base { kind: 'bar', bar: string }

interface Baz extends Base { kind: 'baz', baz: string }

type DU = Foo | Bar | Baz

function f(x: DU) {
  switch (x.kind) {
    case 'foo':
      console.log('foo: ' + x.foo); break;
    case 'bar':
      console.log('bar: ' + x.bar); break;
    case 'baz':
      console.log('baz: ' + x.baz); break;
  }
}
type Base =
  abstract kind: string
  
type Foo =
  inherit Base
  abstract foo: string

type Bar =
  inherit Base
  abstract bar: string

type Baz =
  inherit Base
  abstract baz: string

[<TypeScriptTaggedUnion("kind")>]
type DU = Foo of Foo | Bar of Bar | Baz of Baz

let f = function
  | Foo x -> printfn "foo: %s" x.foo
  | Bar x -> printfn "bar: %s" x.bar
  | Baz x -> printfn "baz: %s" x.baz

Details:

  • It works much like erased unions, but it checks the value of the certain property ("tag") to discriminate its cases.
    • This pattern is commonly found in TypeScript (see).
  • Each case must have one single field of any type.
    • It is expected to be an interface type or an anonymous record type.
  • It uses the name of the DU case as the value of the "tag" property.
    • TypeScriptTaggedUnion respects CaseRule if given.
    • The cases also accept the CompiledName attribute.
    • The behavior here is exactly the same as that of string enums.
  • If the value of the "tag" property is not a string, the new CompiledValue attribute is used instead of CompiledName.
    • It accepts int, float, bool, and enum types of 32bit integer values.
type Kind = Foo = 0 | Bar = 1 | Baz = 2

type Base =
    abstract kind: Kind

type Foo =
    inherit Base
    abstract foo: string

type Bar =
    inherit Base
    abstract bar: int

type Baz =
    inherit Base
    abstract baz: bool

[<TypeScriptTaggedUnion("kind")>]
type EnumTaggedDU =
    | [<CompiledValue(Kind.Foo)>] Foo of Foo
    | [<CompiledValue(Kind.Bar)>] Bar of Bar
    | [<CompiledValue(Kind.Baz)>] Baz of Baz

TODOs:

  • Ask for more comments
  • Move quick tests to the appropriate file
  • Update docs?

@ncave
Copy link
Collaborator

ncave commented Nov 17, 2021

@cannorin Perhaps Case can be renamed to Tag?

@cannorin
Copy link
Contributor Author

@ncave I think that's not a very good naming compared to Case, because it would look like it is used to specify the tag itself, while it is actually used for specifying the value of the tag property. Something like TagValue may be better but a bit wordy I think.

@inosik
Copy link
Contributor

inosik commented Nov 18, 2021

For reference, this would enable #2543.

@ncave
Copy link
Collaborator

ncave commented Nov 18, 2021

@cannorin

it is actually used for specifying the value of the tag property

It is used to specify the name of the tag property, I get your point about it being ambiguous.
So perhaps using something like CaseTag will be unambiguous, even if it is longer than just Tag.
Anyway, it's just a suggestion.

@cannorin
Copy link
Contributor Author

For the record, I used Case here because it is analogous to the switch statement which would be used in TypeScript to match tagged unions.

[<TaggedUnion("kind")>]
type TypeScriptDU =
    | [<Case("foo")>] Foo of {| kind: string; foo: string |}
    | [<Case("bar")>] Bar of {| kind: string; bar: string |}
    | [<Case("baz")>] Baz of {| kind: string; baz: string |}
function f(x: DU) {
  switch (x.kind) {
    case 'foo':
      console.log('foo: ' + x.foo); break;
    case 'bar':
      console.log('bar: ' + x.bar); break;
    case 'baz':
      console.log('baz: ' + x.baz); break;
  }
}

@alfonsogarciacaro
Copy link
Member

Hi @cannorin, thanks a lot for this! TBH I've been always a bit skeptical about trying to assimilate TS tagged unions with F# unions, moreover now that we're trying to make Fable more "language agnostic". But if it brings benefits to consume TS tagged unions in a more F# idiomatic way, we could do it if we can keep changes minimal and consistent with other union transformations (Erase, StringEnum).

I'll try to reply to your comments below:

  • Compiler transformations

To keep the syntax clean I would use the union case field names to build the object instead of an anonymous record, and also omit the tagged field. I think at one point it was possible to use [<Pojo>] on unions to compile them as objects and it was working fine. For the case/tag name I would add an optional CaseRules argument (camel case by default) to TaggedUnion to infer the value from the F# case as with StringEnum. And for the special cases, I would use CompiledName also for consistency with StringEnum.

[<TaggedUnion("kind")>]
type TypeScriptDU =
    | Foo of foo: string                                // { kind: "foo", foo }
    | Bar of bar: string                                // { kind: "bar", bar }
    | FooBar of foo: string * bar: string               // { kind: "fooBar", foo, bar }
    | [<CompiledName("__baz__")>] Baz of baz: string    // { kind: "__baz__", baz }
  • Attribute name

Given that we're trying to extend Fable to other languages and this is a feature very specific to Typescript (not sure if it's applicable to other languages), maybe we could be entirely explicit about it with TypescriptTaggedUnion or TsTaggedUnion.

  • Default case

Yes, as you noticed, the F# compiler already creates a "default" branch for the decision tree corresponding to one of the cases as it assumes the union is closed. I think the first case becomes the default one, but not sure if it's always like this.

In any case, I think trying to match these open unions with F# unions is already a big stretch and forces too much the semantics to become useful. For these cases, I'd recommend to use interfaces and write/generate a helper for matching:

type OpenUnion =
    abstract kind: string

type OpenUnionFoo =
    inherit OpenUnion
    abstract foo: string

type OpenUnionBar =
    inherit OpenUnion
    abstract bar: int

[<AutoOpen>]
module OpenUnionExt =
    type OpenUnion with
        member this.Match() =
            match this.kind with
            | "foo" -> Choice1Of3(this :?> OpenUnionFoo)
            | "bar" -> Choice2Of3(this :?> OpenUnionBar)
            | _ -> Choice3Of3 this

let test (x: OpenUnion) =
    match x.Match() with
    | Choice1Of3 x -> x.foo
    | Choice2Of3 x -> string x.bar
    | Choice3Of3 _ -> "default"
  • Class union

If you use an Erased union Fable will already use instanceof in pattern matching for classes:

type Foo() =
    member _.Foo = "foo"

type Bar() =
    member _.Bar = "bar"

[<Erase>]
type T = Foo of Foo | Bar of Bar

let test = function
    | Foo x -> x.Foo
    | Bar x -> x.Bar

@cannorin
Copy link
Contributor Author

cannorin commented Nov 22, 2021

@alfonsogarciacaro good point!

  • Compiler transformations
    • Actually, it is more convenient to allow any types (not just anonymous records, but also classes and interfaces) than only union case field names.
      • Tagged unions in TypeScript usually looks like type DU = InterfaceA | InterfaceB | InterfaceC, so it's logical to allow an interface type as a single union case field.
      • Each interface in union cases can have many fields, so writing them down as union case fields can be tedious.
      • Also, I want to avoid re-implementing the TS interfaces as union case fields when users already have the corresponding F# interfaces for them (which is almost always the case).
      • The current implementation accepts single unnamed union case field of any type which already achieves the above. I don't think it's good to remove it, but I can add support for multiple named union case fields in addition to that.
    • Using union names as the case values and supporting CaseRules and CompiledName seems better than the current approach. I'll replace the current one with that.
  • Attribute name
    • Yes, I don't think it applies to other languages as well. I think I would go with TypeScriptTaggedUnion because users would have to write it only once for a type so we don't have to care about the visual noise too much.
  • Default case
    • I totally agree to that. I think I won't be doing this because it introduces too much complexity.
  • Class union
    • I wasn't aware of that. It may be documented in somewhere since I couldn't find it in the docs?
      • The current docs state only about typeof and isArray.

@alfonsogarciacaro
Copy link
Member

Actually, it is more convenient to allow any types (not just anonymous records, but also classes and interfaces) than only union case field names. [..] Also, I want to avoid re-implementing the TS interfaces as union case fields when users already have the corresponding F# interfaces for them (which is almost always the case).

Ah, you're right. That makes sense! 👍

I wasn't aware of that. It may be documented in somewhere since I couldn't find it in the docs?

The docs say patter matching with erased unions will be compiled as type testing, although it's true it doesn't show an specific example with classes. In any case, I prefer not to "promote" too much erased unions in the docs because I noticed some users think they are more performant than "standard" unions and try to use them all the time which causes issues because type testing has many pitfalls in Fable.

@cannorin
Copy link
Contributor Author

cannorin commented Nov 24, 2021

@alfonsogarciacaro I just realized that the value of tag field can also be a number, an enum case, or a boolean, where CompiledName cannot be used. So I added CompiledValue attribute which is CompiledName but for those types.

type Kind = Foo = 0 | Bar = 1 | Baz = 2

type Base =
    abstract kind: Kind

type Foo =
    inherit Base
    abstract foo: string

type Bar =
    inherit Base
    abstract bar: int

type Baz =
    inherit Base
    abstract baz: bool

[<RequireQualifiedAccess; TypeScriptTaggedUnion("kind")>]
type EnumTagged =
    | [<CompiledValue(Kind.Foo)>] Foo of Foo
    | [<CompiledValue(Kind.Bar)>] Bar of Bar
    | [<CompiledValue(Kind.Baz)>] Baz of Baz
  • I'm not very sure whether we should allow string values in CompiledValue so that it can be used in place of CompiledName.
  • Sadly, string enum types don't work here because attribute constructors don't accept 0-ary union cases as constant expressions.

@cannorin cannorin marked this pull request as ready for review November 24, 2021 03:44
@cannorin cannorin changed the title [WIP] Support tagged union types from TypeScript Support tagged union types from TypeScript Nov 24, 2021
@alfonsogarciacaro
Copy link
Member

Awesome work @cannorin, thanks a lot!

@alfonsogarciacaro alfonsogarciacaro merged commit 4ef51d9 into fable-compiler:main Nov 24, 2021
@cannorin
Copy link
Contributor Author

@alfonsogarciacaro I haven't moved the quick tests to the appropriate test file yet 😅 Can you revert the merge so that I can do that?

@cannorin
Copy link
Contributor Author

@alfonsogarciacaro I saw your commit moving the tests, thank you!

@cannorin cannorin deleted the custom-union-types branch November 24, 2021 07:57
@alfonsogarciacaro
Copy link
Member

Testing this, looks great! A couple of remarks:

  • We should probably erase the declaration of the type decorated with TypeScriptTaggedUnion as it's not actually used (if I'm not mistaken).
  • It'd be great if we could check the tag when creating a TypeScriptTaggedUnion case, at least when it's a constant (so you get an error if you try to do Foo !!{| kind = "bar |}). For non-constants we could just emit a warning saying the tag cannot be checked.
  • It'd be even more awesome to get a PR to the docs about the feature :)

@cannorin
Copy link
Contributor Author

cannorin commented Nov 25, 2021

@alfonsogarciacaro Thanks!

  • Yes, TS tagged union is essentially an erased union with custom testing, so we should erase the declaration.
  • I'm still learning the internals of Fable compiler so it will take some time to implement that. Feel free to overtake it!
  • I'll work on it soon🙂

@cannorin
Copy link
Contributor Author

Created a follow-up PR: #2623

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants