Latest Fable fails to compile Fable.React

[kerams]

Description

.\src\Client\fable_modules\Fable.React.9.3.0\Fable.React.Props.fs(43,5): (43,12) error FABLE: Cannot test erased union cases

This is because the bundled FCS contains https://github.com/dotnet/fsharp/pull/16341, and Fable tries to create case tester functions for every case.

export function HTMLAttr__get_IsHref(this$, unitArg) {
    if (this$.tag === 0) {
        return true;
    }
    else {
        return false;
    }
}

export function HTMLAttr__get_IsCustom(this$, unitArg) {
    if (this$.tag === 1) {
        return true;
    }
    else {
        return false;
    }
}

export const x = HTMLAttr__get_IsCustom(new HTMLAttr(1, []));

for

type HTMLAttr =
    | Href of string
    | Custom

let x = Custom.IsCustom

Adding [<Fable.Core.Erase>] to Href or Custom introduces a compiler error.

I suggest Fable is adjusted to recognize the newly exposed case testers and to emit tag comparisons at call sites instead of creating properties. That way compilation will only fail when trying to use a case tester on an erased case.

export const x = (new HTMLAttr(1, [])).tag === 1;

Repro code

https://fable.io/repl/#?code=C4TwDgpgBAEgKgWQDIEFjAE5QLwCgoFQA+UAFAFRQDaAPAGICGARgDYQB0AwgPYYcCiGBgGcIAPgC6UcgEoYfAGZRuS4ZgCWAOwDm+QiU4BXNdwC2uXG2BQAHjihGTp9gElhj4GaA&html=Q&css=Q

Related information

• Fable version: 4.8.1

[MangelMaxime]

I agree that generating the check on site is better as this will reduce the bundle size and for this kind of checks I think this the good path to take.

[kerams]

Are people no longer using Fable.React or just not upgrading? I am a little surprised no one else has complained about this critical issue.

[ncave]

@kerams I'm not at all familiar with Fable.React bindings, so please excuse my ignorance, but I wonder if there is a better way to express arbitrary properties, instead of using an erased union case (which kind of defeats the purpose of using unions to model properties in the first place). I understand that's not helping with the existing version of Fable.React bindings, it's just that using erased union cases is not even documented in Fable docs, it looks like a very custom workaround, so I wonder if that's the best we can do.

e.g. perhaps something like this can work instead (i.e. lifting arbitrary props one level up):

open Fable.Core

type HTMLProp =
    | Href of string

type HTMLAttr = U2<HTMLProp, string * obj>

let test (attr: HTMLAttr) =
    match attr with
    | U2.Case1 (Href s) -> s
    | U2.Case2 (s, o) -> s

let x: HTMLAttr = U2.Case1 (Href "https://")
let y: HTMLAttr = U2.Case2 ("something", 10)

assert (test x + test y = "https://something")

I'm not saying this is a good solution or that it's ergonomically the same, it's just a question. (Perhaps the construction details can be hidden behind a create function, if that helps.)

[kerams]

While that would work, it's a massive breaking change since you'd suddenly need !^ everywhere.

[MangelMaxime]

I understand that's not helping with the existing version of Fable.React bindings, it's just that using erased union cases is not even documented in Fable docs, it looks like a very custom workaround, so I wonder if that's the best we can do.

If it is not documented this probably a mistake on my end, as I forgot to add it when doing the documentation rework.

While that would work, it's a massive breaking change since you'd suddenly need !^ everywhere.

We could probably workaround the !^ being needed everywhere by using static member op_ErasedCast on the type.

I am sorry that this bug has not been fixed yet, I always had it in the back of my head but never had the time or courage to dig into it. I didn't touch this portion of Fable yet. 😅

I will make sure to give it a try this week and as this is not only breaking Fable.React but also impacting the bundle size.

[kerams]

Thank you, Maxime.

[ncave]

@kerams

I am a little surprised no one else has complained about this critical issue.

Are you perhaps running .NET 9.0? Correct me if I'm wrong, but I don't think dotnet/fsharp #16341 is in .NET 8.0, since it was only merged on Dec 7, 2023, so that may be the reason why you don't see it (yet).

Anyway, IMO we have several ways we could fix this for the next Fable release:

• We could add an explicit attribute to mark those types of unions with arbitrary properties that we want to just declare bindings for, say something like Fable.Core.PojoUnionAttribute, and ignore (don't transpile) any compiler-generated members for unions with that attribute (as they're just bindings).
• Or, we could do the same without introducing a new explicit attribute (i.e. we can ignore all compiler-generated members for any union that has at least one erased union case).
• Or, we could elide only the union members that match specific pattern (i.e. have names like Is* where the * matches an union case name that has the Fable.Core.Erase attribute).

IMO they all have pros and cons, e.g. being more explicit about intent vs backwards compatibility with existing bindings. Here is approximately where a change can be implemented to skip some or all of the compiler-generated members that match one of the criteria above.

[kerams]

I am indeed on 9 (langversion and SDK-wise), but don't see why it matters since Fable uses its bundled FCS, not fsc in the SDK. The former contains changes from the PR you mentioned, so you can also replicate the issue in Fable repl.

[ncave]

@kerams That is correct, our current FCS snapshot is as of Dec 8, 2023, so it includes it. I just meant it's not technically part of .NET 8.0.

Anyway, @kerams @MangelMaxime please chime on the options above, and I can make that change. (unless you think a different fix is more appropriate. Technically we can even take https://github.com/dotnet/fsharp/pull/16341 out for now, as it's not part of .NET 8.0, but that's less ideal, as we'll have to revisit it again).

[MangelMaxime]

@ncave The problem is that in Fable 4.8.0 we updated the version of FCS.

But that version of FCS is having new F# features added to it which are for F# 7 (I believe). More specifically https://github.com/dotnet/fsharp/pull/16341 which exposed .Tag and .Is* method for unions. See the RFC for more details.

Because of that, Fable output changed between Fable 4.7 and Fable 4.8. I don't know why but I don't have the same generated code between the REPL and Fable running locally for this case...

Could it be that the new feature is only effective in Fable too if user is using .NET 9? @kerams Can you try forcing .NET 8 to your project using global.json? If this is works it could be a great workaround temporarily.

type HTMLAttr =
    | Href of string
    | Custom

Fable 4.8+ generated code (from the REPL)

import { Union } from "fable-library-js/Types.js";
import { union_type, string_type } from "fable-library-js/Reflection.js";

export class HTMLAttr extends Union {
    constructor(tag, fields) {
        super();
        this.tag = tag;
        this.fields = fields;
    }
    cases() {
        return ["Href", "Custom"];
    }
}

export function HTMLAttr_$reflection() {
    return union_type("Test.HTMLAttr", [], HTMLAttr, () => [[["Item", string_type]], []]);
}

export function HTMLAttr__get_IsHref(this$, unitArg) {
    if (this$.tag === 0) {
        return true;
    }
    else {
        return false;
    }
}

export function HTMLAttr__get_IsCustom(this$, unitArg) {
    if (this$.tag === 1) {
        return true;
    }
    else {
        return false;
    }
}

Fable 4.7 (and below) generated code:

import { Union } from "fable-library-js/Types.js";
import { union_type, string_type } from "fable-library-js/Reflection.js";

export class HTMLAttr extends Union {
    constructor(tag, fields) {
        super();
        this.tag = tag;
        this.fields = fields;
    }
    cases() {
        return ["Href", "Custom"];
    }
}

export function HTMLAttr_$reflection() {
    return union_type("Test.HTMLAttr", [], HTMLAttr, () => [[["Item", string_type]], []]);
}

See that ...IsHref and ...IsCustom was not generated. I believe what we want from now on is to support the new .Tag and .Is feature because it is bundled in Fable due to the FCS bump. We still need to figure out if this is active only if .NET 9 is used or not? Or why the REPL generate it while I don't generate it locally in my project.

But instead of generated them as standalone functions, I am in favour of generating them in place (inline). This will limit the impact on bundle size even if that should be removed by any decent bundler nowadays.

But more importantly it will keep supporting:

type HTMLAttr =
    | Href of string
    | [<Erase>] Custom. of string * string

which is the historical way for doing React bindings and a feature of Fable do generate POJO from DUs.

open Fable.Core
open Fable.Core.JsInterop

type HTMLAttr =
    | Href of string
    | [<Erase>] Custom of string * string

let o =
    keyValueList CaseRules.LowerFirst [
        Href "url"
        Custom ("data", "value")
    ]

generates:

import { declare, Union } from "fable-library/Types.js";
import { union_type, string_type } from "fable-library/Reflection.js";
export const HTMLAttr = declare(function Test_HTMLAttr(tag, name, ...fields) {
  this.tag = tag | 0;
  this.name = name;
  this.fields = fields;
}, Union);
export function HTMLAttr$reflection() {
  return union_type("Test.HTMLAttr", [], HTMLAttr, () => [["Href", [["Item", string_type]]], ["Custom", [["Item1", string_type], ["Item2", string_type]]]]);
}
export const o = {
  href: "url",
  data: "value"
};

REPL Fable 2 demo

[MangelMaxime]

But instead of generated them as standalone functions, I am in favour of generating them in place (inline)

I am not sure if this is a good idea of not to do it this way as we divide a bit from F# default code.

[ncave]

@MangelMaxime

But instead of generated them as standalone functions, I am in favour of generating them in place (inline).

We'll have to suppress these member declarations anyway, in order to inline them, so that sounds like an additional step to fix the call sites. That may not be necessary if the bundler will not include them anyway, unless used.

Anyway, sounds like this use case is very specific for JS/TS bindings, so whatever we decide to do needs to be well documented.

[kerams]

Getting the same outcome regardless of global.json, TFM and langversion.

[MangelMaxime]

Getting the same outcome regardless of global.json, TFM and langversion.

Ok, I am not sure why I don't have them personally.

---

The more I dig, and the more I think the problem is perhaps due to the error message instead.

Fable 2 output:

type HTMLAttr =
    | [<Fable.Core.EraseAttribute>] Href of string
    | Custom of string

let a = Href "href" = Custom "custom"

import { declare, Union } from "fable-library/Types.js";
import { union_type, string_type } from "fable-library/Reflection.js";
import { equals } from "fable-library/Util.js";
export const HTMLAttr = declare(function Test_HTMLAttr(tag, name, ...fields) {
  this.tag = tag | 0;
  this.name = name;
  this.fields = fields;
}, Union);
export function HTMLAttr$reflection() {
  return union_type("Test.HTMLAttr", [], HTMLAttr, () => [["Href", [["Item", string_type]]], ["Custom", [["Item", string_type]]]]);
}
export const a = equals(["href"], new HTMLAttr(1, "Custom", "custom"));

It is true that this code equals(["href"], new HTMLAttr(1, "Custom", "custom")); looks strange because the Href is not built using new HTMLAttr( but that's to be expected because it is decorated with Erase.

However, doing a comparaison on Href "href" = Href "href" generates equals(["href"], ["href"]); which works indeed. It is true that by cheat with the code the user could make the equality pass with something else that a HTMLAttr because we don't have a constructor.

Href "href" = unbox ([| "href" |])
// generates
equals(["href"], ["href"]);

but is it a real problem? Yes, think this has always been like that. I mean this is what Fable 2 generates and I don't anyone complained about it.

---

Something even more strange is that I cannot reproduce the Cannot test erased union cases error on my machine. And even more, I would say this is not happening either on the CI because the code that is causing this issue is part of the test suite:

https://github.com/fable-compiler/Fable/blob/e2285d6e6f914f3f622bcb3165a23eca0153b856/tests/Js/Main/JsInteropTests.fs#L137-L139

The code linked above is enough to trigger the error in the REPL but Fable CI / Tests is not complaining about it. Which is another hint in the direction of having an issue with the error message instead of the generated code instead.

Any idea, why @kerams machine and the REPL fails while the CI and my machine succeed? And I suppose @ncave machine also works because they send PR and would have seen the test failing locally I think.

[ncave]

@MangelMaxime

The more I dig, and the more I think the problem is perhaps due to the error message instead.

I don't think so. All I can think of, without digging further, is that there are probably some additional checks for .NET version in FCS that prevent the new properties from being generated.

For me, Fable.React package compiles just fine to JS on .NET 8.0 with Fable 4.19, when added to a small test project. I don't see any of the new properties too. But I don't have .NET 9.0 installed, I only have .NET 6.0 and .NET 8.0.

[MangelMaxime]

But I don't have .NET 9.0 installed, I only have .NET 6.0 and .NET 8.0.

I tried with .NET 9 installed and can't reproduce either with a minimal reproduction.

@kerams Could you please confirm that my reproduction project fails for you too?

https://github.com/MangelMaxime/fable-react-reproduction-3658

I didn't use Fable.React in it because I can reproduce in the REPL without it. But if the reproduction fails, please feel free to send PR with Fable.React or tells me to include it.

Not being able to reproduce the bug is not help us here 😅

Edit: I forced a specific version .NET 9 to try minimise the source of difference.

[ncave]

@MangelMaxime I can now reproduce on .NET 8.0 with Fable 4.19, if I add <LangVersion>preview</LangVersion> to the project.

[kerams]

Yes, works without langversion, since case testers are not exposed without preview. I didn't clear the cache, so that was probably it.

Add <LangVersion>preview</LangVersion> to your repro fsproj and you'll get the error.

[MangelMaxime]

I can now reproduce on .NET 8.0 with Fable 4.19, if I add <LangVersion>preview</LangVersion> to the project.

Great news 🎉 Never been so happy to see some red ahah.

But now, we are back to understand what code generation is causing this error.

[MangelMaxime]

Coming back to @ncave proposition

• We could add an explicit attribute to mark those types of unions with arbitrary properties that we want to just declare bindings for, say something like Fable.Core.PojoUnionAttribute, and ignore (don't transpile) any compiler-generated members for unions with that attribute (as they're just bindings).

If possible, I would avoid adding a new attribute. This will needs all binding that use this syntax to update. Also, keyValueList is not anymore the best way to generate POJO IHMO now that we have ParamObject which feels much more F# native.

Or, we could do the same without introducing a new explicit attribute (i.e. we can ignore all compiler-generated members for any union that has at least one erased union case).

I believe this proposition is a good compromise between restoring support for DUs used for interop to create POJO with an Erased case and supporting the new addition of F# feature.

If we do that, we should probably add a warning / error is user is trying to call any of the compiler generated members. This is because the IDE will not report this issue which is Fable specific.

• Or, we could elide only the union members that match specific pattern (i.e. have names like Is* where the * matches an union case name that has the Fable.Core.Erase attribute).

I think going for the option before, is best for consistency? What I mean by that is that like that none of the compiled generated member are available at all. So user don't need to question himself why some are available and some not.

[kerams]

You can use this to decide whether to generate a union member or not - IsCompilerGenerated would also imply stuff like Equals. As a start, I would be more than happy if I could compile my project without being able to use any case testers. We could look into what it would take to generate case tests inline at a later time.

[ncave]

@kerams For some reason IsUnionCaseTester is throwing, cause the tester is neither IsProperty nor IsMethod, not even IsCompilerGenerated. But it is IsPropertyGetterMethod.

Not sure if that's expected, perhaps it has been fixed after we took the FCS snapshot, but it sounds like a bug. Perhaps it's just missing the | V v -> ... case:

    member _.IsUnionCaseTester =
        checkIsResolved()
        match d with
        | P p -> p.IsUnionCaseTester
        | M m -> m.IsUnionCaseTester
        | V v -> v.IsPropertyGetterMethod && v.LogicalName.StartsWith("get_Is") // <-- fix
        | E _ | C _ -> invalidOp "the value or member is not a property"

So for now I'll use memb.IsPropertyGetterMethod && memb.LogicalName.StartsWith("get_Is").

[ncave]

@kerams Opened an issue in F# repo, see https://github.com/dotnet/fsharp/issues/17301

[kerams]

Works great, many thanks, sirs, and sorry for being impatient.

[MangelMaxime]

No worries, thanks for confirming that all works now.

You can enjoy features/fixes from more than 10 releases since this bug 🎉