type test is always true for Erase'd union case with obj value

[joprice]

Description

When using an erased union, if one of the cases is obj, the type test function always returns true.

This makes sense from the ast leaf node's perspective, since it is trivially true that everything is an object, but when taking into account of how a union type is meant to be used, it seems that they should either be tagged or mutually exclusive types. For instance, a similar issue also happens when two cases have the same type, where they will both receive a typecheck typeof this$ === "number". Ideally, an error would be thrown for both the case where the types are not comparable (no valid typecheck can be generated), or the two overlap. TypeScriptTaggedUnion could be suggested for the case where the erasure is only meant to slim down the code, and using non-overlapping types and type-testable types can be suggested for cases where it is used in a binding.

If it is for some reason useful to have erased obj or overlapping cases like this, perhaps the definition of the type could be allowed but the call-site that attempts to generate a match or other syntax that resolves to the comparison operator fails.

Repro code

[<Erase>]
type X =
      | A of o:obj
      | B of t:string

let isA (s: X) = 
  match s with 
    | A _ -> true 
    | _ -> false

produces

import { some } from "fable-library-js/Option.js";

export function X__get_IsA(this$, unitArg) {
    return true;
}

export function X__get_IsB(this$, unitArg) {
    if (typeof this$ === "string") {
        return true;
    }
    else {
        return false;
    }
}

export function isA(s) {
    return true;
}

Expected and actual results

The Erase attribute should fail for cases that generate incorrect type tests.

Related information

• Fable version: 4.19.2
• Operating system: OSX

[MangelMaxime]

This seems to be triggered because the DUs is decorated with Erase.

If we remove Erase then the check looks correct to me:

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

export class X extends Union {
    constructor(tag, fields) {
        super();
        this.tag = tag;
        this.fields = fields;
    }
    cases() {
        return ["A", "B"];
    }
}

export function X_$reflection() {
    return union_type("Test.X", [], X, () => [[["o", obj_type]], [["t", string_type]]]);
}

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

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

export function isA(s) {
    if (s.tag === 0) {
        return true;
    }
    else {
        return false;
    }
}

I don't think there is something to fix here because if we create an instance of a DUs decorated with Erase then we can't build a "real" case and only work with the type on the right.

open Fable.Core

[<Erase>]
type X =
    | A of o:obj
    | B of t:string

let x = A ""

type Y =
    | A of o:obj
    | B of t:string

let y = A ""

// [...]

export const x = "";

export class Y extends Union {
    constructor(tag, fields) {
        super();
        this.tag = tag;
        this.fields = fields;
    }
    cases() {
        return ["A", "B"];
    }
}

// [...]
export const y = new Y(0, [""]);

Based on that the match is testing correctly that the provided value as the same type as the right side of the case.

[joprice]

Yea, not using Erase makes sense if you can, but if you do use it, I would think it should fail in cases where it will never generate valid code. It still seems to me to be the case when you have multiple cases with the same type, or an object type. I can try writing some unit tests I think should pass.

This was originally motivated by code I found in a project I was browsing, and was surprised that the author used Erase instead of TypeScriptTaggedUnion. There are probably other users using it without realizing the wrong case will be hit, resulting in subtle bugs.

In some cases, you might be pushed into using it instead of the class-based default where a library expects a pojo-like object. In my case, I hit it when using Solid's SSR, which relies on https://github.com/lxsmnsyc/seroval to serialize objects. TypeScriptTaggedUnion ended up being the correct solution for me, but I spent a lot of time trying to make erase work, writing unit tests and inspecting the generated code, until I realized it was generating effectively unreachable branches.

[MangelMaxime]

We could perhaps generate a warning or error if trying to use a comparaison on a DUs with 2 similars cases and/or obj.

If this is indeed confirmed to be the correct implementation and we can detect it.