Alternative variants no clear behavior.

[GilShoshan94]

Hi,

While trying to fix #84, I realized that the there is no clear behavior define on how to handle to discriminants attribution to each variants in the case of alternative variants.

If I have an enum, we all agree that unless explicitly set, the discriminants start from 0 and are auto incremented by 1 for each variants. When we meet a variant with a specified discriminant, it gets the said specified discriminant, and the following variants get incremented by one from there.

But what happens if alternative variants are specified ?

Does the following variants have to 1) be explicitly set or 2) if we want to increment from the biggest value or 3) if we want to increment from the last discriminant value but skipping automatically on the taken values.

For example:

#[derive(Copy, Clone, Debug, PartialEq, Eq, TryFromPrimitive)]
#[repr(u8)]
pub enum EncodingFormat {
    Postcard = 2,
    Json,
    MessagePack,
    #[num_enum(alternatives = [7,8,10])]
    Pickle,
    BSON,
    CBOR,
    CSV,
    Bencode,
}

Json = 3, MessagePack = 4, Pickle is 5 | 7 | 8 | 10, and now for BSON ?
If we pick the 3) way, it would be BSOM = 6, CBOR=9, CSV=11, and Bencode=12.

(Unless an explicit discriminant is set for one of those variants of course. And we need to error on collision also.)

I am in favor of the 3) way.

I am trying to implement it.

[illicitonion]

In Rust in general, this enum:

#[repr(u8)]
pub enum EncodingFormat {
    Postcard = 2,
    Json,
    MessagePack,
    Pickle,
    BSON,
    CBOR,
    CSV,
    Bencode,
}

is equivalent to this one:

#[repr(u8)]
pub enum EncodingFormat {
    Postcard = 2,
    Json = 3,
    MessagePack = 4,
    Pickle = 5,
    BSON = 6,
    CBOR = 7,
    CSV = 8,
    Bencode = 9,
}

Implicit and explicit discriminants work the same. Our attribute doesn't change this.

So I think the only real question is whether we treat the attribute as an override (i.e. EncodingFormat::try_from(7).unwrap() == EncodingFormat::Pickle) or a compile-time error.

I'd suggest we should probably treat it as a compile-time error.

[GilShoshan94]

For now, the way I implemented it is like follow:

Variants are treated in order top to bottom.
If a variant has a discriminant explicitly set, it get that value.
If it has alternatives values (by the attribute #[num_enum(alternatives = (...)]), those values are reserved for this variant.

The following variants that have no there discriminant explicitly set get one automatically, BUT this auto discriminant is equal to the next ascendent available value (not reserved by any previous variant).
It then does not fail (except in the case where we get out-of-range, such as the next logical value is 256 for a #[repr(u8)], but for this rustc will complains already)

If instead a following variant has an explicitly set discriminant, it get that discriminant, and collisons are checked, and id found, it is reported.

---

But I totally understand your approach of let's mimic the default behavior first and let's be more specific.

So here is what I suggest:

Variants are treated in order top to bottom.
If a variant has a discriminant explicitly set, it get that value.
If it has alternatives values (by the attribute #[num_enum(alternatives = (...)]), those values are marked for this variant.

The following variants that have no there discriminant explicitly set get one automatically, BUT this auto discriminant is equal to the next ascendent value (we don't care if the value is already marked for a previous variant).

We then check for collision and report them, If the users really want to use the alternative values for a previous variant, they will need to explicitly set the discriminant to avoid collision.

Example:

The following will error and report a collision on CBOR because it its discriminant is 7 but collides with an alternative value of Pickle.

#[derive(Copy, Clone, Debug, PartialEq, Eq, TryFromPrimitive)]
#[repr(u8)]
pub enum EncodingFormat {
    Postcard = 2,
    Json,
    MessagePack,
    #[num_enum(alternatives = [7,8,10])]
    Pickle,
    BSON,
    CBOR,
    CSV,
    Bencode,
}

The way to fix this would be:

#[derive(Copy, Clone, Debug, PartialEq, Eq, TryFromPrimitive)]
#[repr(u8)]
pub enum EncodingFormat {
    Postcard = 2,
    Json,
    MessagePack,
    #[num_enum(alternatives = [7,8,10])]
    Pickle,
    BSON,
    CBOR=9,
    CSV=11,
    Bencode,
}

---

What do you say ? Does it makes sense ?

[illicitonion]

Sounds great, thanks! (Also sounds like something we should have better test coverage for than we currently do, both in the happy case and in the conflict case)!

[GilShoshan94]

OK great :) !
I am updating the code with the BTreeSet and changing the behavior to this for the alternative variants.
For the test, I can try to add one, but it will be in a seperate PR.

[GilShoshan94]

Ok it's done (without the test).

[GilShoshan94]

PR #89 resolves this. Closing the issue.