Skip to content

Add NoReflectionAttribute #2861

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

Closed
wants to merge 3 commits into from
Closed

Add NoReflectionAttribute #2861

wants to merge 3 commits into from

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