Skip to content

Conversation

kerams
Copy link
Contributor

@kerams kerams commented Apr 24, 2022

Implements #2710 (comment).

Given

[<Fable.Core.NoReflectionAttribute>]
type U =
    | A
    | B of int

type R = {
    W: U
}

I get

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

//export function U$reflection() {
//    return union_type("QuickTest.U", [], U, () => [[], [["Item", int32_type]]]);
//}

export class R extends Record {
    constructor(W) {
        super();
        this.W = W;
    }
}

export function R$reflection() {
    //return record_type("QuickTest.R", [], R, () => [["W", U$reflection()]]);
    return record_type("QuickTest.R", [], R, () => [["W", null]]);
}

with comments denoting changes with the attribute. There's also a warning warning FABLE: QuickTest.U is annotated with NoReflectionAttribute, but is being used in types which support reflection. This may lead to runtime errors. because R uses U, but does not itself have the attribute.

@alfonsogarciacaro, if you think this is a good addition, I can add tests and try to figure out #2710 (comment).

One thing that worries me is that Union.name() refers to the case names, and it's used both in toString and toJSON. Perhaps NoReflection could also imply that default stringification/serialization is for all intents and purposes not supported? For these cases we could have a different base union class in TS that would just use the tag instead of case name. No specialized classes for unions with no reflection would then be needed to be emitted at all.

@kerams
Copy link
Contributor Author

kerams commented Apr 24, 2022

Ok, unions are now "erased" in terms of the type system but not to the degree achieved with EraseAttribute.

Given

[<Fable.Core.NoReflectionAttribute>]
type U =
    | A
    | B of int

type R = {
    W: U
}

let a = { W = B 1 }

match a.W with
| A -> ()
| B 1 -> ()

I get

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

//export function U$reflection() {
//    return union_type("QuickTest.U", [], U, () => [[], [["Item", int32_type]]]);
//}

export class R extends Record {
    constructor(W) {
        super();
        this.W = W;
    }
}

export function R$reflection() {
    //return record_type("QuickTest.R", [], R, () => [["W", U$reflection()]]);
    return record_type("QuickTest.R", [], R, () => [["W", null]]);
}

//export const a = new R(new U(1, 1));
export const a = new R(new CommonUnion(1, 1));

(function () {
    const matchValue = a.W;
    if (matchValue.tag === 1) {
        if (matchValue.fields[0] === 1) {
        }
        else {
            throw (new Error("Match failure: QuickTest.U"));
        }
    }
})();

public Equals(other: CommonUnion) {
if (this === other) {
return true;
} else if (!sameConstructor(this, other)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not exactly sure what the idea behind this check is, but it probably does not make sense in the context of this shared union class.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was to prevent a case where you compare two unions with the same shape but different type (after casting them to obj). As you said, it doesn't make sense if you're not generating different classes for each union.

Copy link
Contributor Author

@kerams kerams Apr 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Yeah, calling equals on 2 different union types may return true here. It's one of the trade-offs for size savings. If you're not comparing obj, the type system should prevent that though.

@kerams kerams closed this Apr 26, 2022
@kerams kerams deleted the ref branch April 26, 2022 09:54
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.

2 participants