[WIP] Erase unions and records

[ncave]

• Erase unions and records to named tuples, behind a compiler switch.
• This is experimental and WIP.

[alfonsogarciacaro]

Seems some test failing because this is overruling StringEnum or Erase attribute. Is that intended? I guess Erased and StringEnum unions should behave the same as without the optimization as they're used for JS interop.

[alfonsogarciacaro]

Right now we have three (potential) optimization flags, so it may be a good idea to tidy things up before the release of Fable 3 stablish. We have two options:

1. Do nothing (about the optimization flags), announce nothing. Defer things to Fable 3.1
2. Merge the standing PRs behind compiler flags and write a blog post announcing them as experimental so users can start playing with them in their apps or the repl and report the results.

If we choose 2, we probably should do the following:

• Make sure that [WIP] Erase unions and records #2279 and [WIP] Array-based List type #2154 pass the tests (compiled with fable-compiler-js too), also fix test with --optimize flag enabled
• For [WIP] Array-based List type #2154, rename the "optimized" List.fs to something like ListOpt.fs and make sure it has the same API as current List.fs. When Fable copies fable-library to .fable dir, if --optimizeList flag is enabled it will rename ListOpt.js to List.js.
• Merge the PRs, release, announce the flags as experimental, profit (maybe)

@ncave What do you think?

[ncave]

@alfonsogarciacaro Kind of depends on the timeline for v3.0 release.
IMO, given that we're already in RC territory, and the stated goal of stability and backwards compatibility of v3.0, we should probably refrain from adding more features at this point, so I would say option 1. There is nothing in those two PRs that cannot wait until v3.1, as they're both WIP and experimental.

Performance issues on Firefox can probably be dealt with by post-build down-compiling to ES5, if well documented.
If somehow we quickly finish all the tasks in option 2, then v3.1 can be a fast follower.

[ncave]

Seems some test failing because this is overruling StringEnum or Erase attribute. Is that intended? I guess Erased and StringEnum unions should behave the same as without the optimization as they're used for JS interop.

@alfonsogarciacaro Yes, this is still WIP and there is some fine-tuning to do, like Reflection and JS interop.
I added this PR just so you're aware of it.

[alfonsogarciacaro]

You're right. I need to refrain myself from adding features at the last moment, even if there are behind a compiler flag 😅

Actually, it's been a few days without serious issues being reported so as soon as I'm done with a couple of blog posts we can a publish a "real" RC, and if there are no critical bugs reported we can republish again as stablish 👍

[Zaid-Ajaj]

as soon as I'm done with a couple of blog posts

I would love to either write a standalone blog post on latest changes with respect and Fable 3 and a bunch libraries that are using the latest goodies:

• Feliz and friends
• SimpleJson
• Remoting
• Snowflaqe

I am thinking of pushing out Feliz v2.0 that uses many of the new stuff like units of measure support with witnesses, and deprecating the old React.functionComponent but the problem is that it makes Feliz 2.0 incompatible with Fable 2.x :/ is there maybe a way to distinguish Fable 2 from Fable 3 in compile time? Right now I can only do hacky things in runtime to make a library compatible with both compiler versions (SimpleJson/Remoting)

[alfonsogarciacaro]

That'd be great @Zaid-Ajaj! There's currently no way to get the compiler version from code right now, but we should add one. I was thinking in these two ways, we can implement one or both:

• Define constant, so besides FABLE_COMPILER we have also FABLE_COMPILER_3. Only for major versions (from Fable 3 on) and works at compile time with #if.
• Add a helper to Fable.Core to get the version like these ones. Gives you the specific version, you get it at runtime but the helpers compile to literals in generated code so minifiers should be able to remove unused if branches for production (like JS devs do with NODE_ENV == "development").

[Zaid-Ajaj]

The compiler constant is just great! Really need that for a couple of stuff. As for the runtime value of the compiler, having maybe two properties would be even better:

majorVersion : int 
version : string

[alfonsogarciacaro]

@ncave I just had an idea, what if convert the Record/Union/ValueType constructor into a singleton. The "erased" objects would contain a reference to their "type" so we can get the best of the two worlds: lightweight runtime types (basically arrays) but still we can identify the exact type and kind of an object at runtime. So something like this:

type Foo = Foo of string | Bar of int * int

let b = Bar(3,4)
let name = Fable.Core.Reflection.getCaseName b

Would compile to something like this:

// The Union object contains default equality, etc, can be overridden.
const Foo = new Union("Foo", "Bar")

// Reflection info would be emitted as before

const b = [Foo, 1, 3, 4]

// This is a helper that returns the case name by accessing the Foo object
const name = getCaseName(b)

What do you think?

[ncave]

@alfonsogarciacaro Sure, perhaps that solves some corner cases, but would you elaborate on what would be the benefits vs. just a string for the case name in the array? Also, the case tag has to be first, so the comparison (order of cases) can work properly.

[alfonsogarciacaro]

I would have to run the tests with this PR as I don't remember exactly which tests are broken, but off the top of my head:

• We can recognize if a runtime array represents a union (this is important for serializers)
• Probably using an integer instead of a string helps to have smaller bundles and faster case tests
• We can tell apart two "unions" with the same shape but different constructors
• We can override default members like Equals or ToString (an alternative would be to use classes as of now only for unions doing this)

[alfonsogarciacaro]

@ncave I'm thinking now the erasure should happen in the Fable2Babel step because the Fable transforms and the plugins should work on union expressions in the AST instead of checking all the possible shapes an erased union can take.

[ncave]

@alfonsogarciacaro Doing it before the Fable AST phase makes this PR a lot smaller. But perhaps what you suggest makes sense, if we assume plugins want to know if something was a union or a record or a value type. I'm not entirely clear how the plugin flow works, what is the output of a plugin, is it Fable AST? Are they executed in a sequence? If so, then potentially they can have the same issue (one plugin destroying information by transforming the Fable AST before the other plugin sees it).

[alfonsogarciacaro]

Sorry @ncave, I should have given you more context. I thought of this when writing a plugin for Svelte integration. For example, this pattern match won't work if the union is erased, because we would lose the union type with the case names and fields. Likewise, if we want to match a new union expression, we would have to check for the erased form too which can quickly become very complicated.

what is the output of a plugin, is it Fable AST?

Yes. Conceptually they are part of the Fable2Fable transforms. Right now there's only a plugin to transform member declarations and call expressions. For a technical reason, the call transformation is in the FSharp2Fable step but probably that's not a good idea and we have to change it at some point.

Are they executed in a sequence?

Yes, they will. And as you say, the day users apply several plugins to the same declaration/expression we're going to have a lot of fun. My hope is that most plugins won't be overlapping (they are only activate when the member is decorated with a specific attribute).

[alfonsogarciacaro]

In conclusion, my current thinking is unions (besides lists and options) should be represented as NewUnion in the Fable AST, and make the erasure in the Fable2Bable step. But I'd have to give it a try, because maybe we're depending on the current behavior somewhere. There's also the "explicitly" erased unions, used to represent Typescript unions. So maybe we need to make a distinction between unions erased explicitly (hopefully they land in F# at some point) or for optimization.

[alfonsogarciacaro]

Sorry @ncave, I didn't check the current status of the PR but I had another "idea"💡 If we want to make the distinction into erased unions that simulate Typescript unions and erased unions for performance, we could have a compiler switch to erase structs (for performance) as this is what F# developers use when they want to make a union more performant. Although we have to make clear the semantics of a struct union with the compiler switch would still be different in Fable and .NET.

About records, we should check what's the best way to erase them. I guess arrays would make smaller bundle sizes but JS objects would work better with hidden classes.

[ncave]

@alfonsogarciacaro IMO using a compiler switch (like it is right now in the PR) is cleaner, as you can change the generated code output without making changes to the source code.

The PR already makes exceptions for custom equality and a few other cases. These exceptions can perhaps be replaced later with static method invocations for specific types.

Perhaps we can rename the flag from --eraseUnions to eraseTypes or eraseTypeDefs, or another one that you like.

The current state of the PR is that it works, it's operational. Just a few test errors are left, 17 out of 1904, mostly having to do with JsInterop and toString/reflection. My definition of "operational" is that it's running the FCS-Fable benchmark, which has a bit of everything.

The preliminary results are encouraging, even though not breathtaking. There is at least 5% performance improvement, and 10% bundle reduction (measured after treeshaking/minimization). That obviously depends on the code base usage of F# types.

What is left to do is to move the transformation downstream to the Fable-to-JS phase, as you suggested, and fix the remaining tests, but it already works.

About your other suggestion to replace the type name in the array with a constructor, it's worth trying but would it not impact the bundle size, as now all classes will be retained? Perhaps, I haven't tried.