#[deriving(PartialEq, Show)] generates poor implementations for large enums

[sfackler]

I have an enum with ~250 variants, all of which are nullary except one which contains a String: http://sfackler.github.io/doc/postgres/enum.SqlState.html. The optimized eq and ne implementations generated by deriving are pretty awful: https://gist.github.com/sfackler/d14a653dfc8affd11e99. It's compiling the huge match generated by deriving into an enormous if, elseif chain.

The ideal implementation seems like it would be

impl PartialEq for SqlState {
    fn eq(&self, other: &SqlState) -> bool {
        match (*self, *other) {
            (SqlState::Unknown(ref a), SqlState::Unknown(ref b)) => a == b,
            _ => magic_tag_extraction_intrinsic(self) == magic_tag_extraction_intrinsic(other)
        }
    }
}

Would it make sense to add that intrinisic and teach deriving to use it for all nullary variants? Can we instead teach trans/LLVM to be smarter about analyzing enormous match statements?

cc @Aatch

[sfackler]

cc @Manishearth

[Aatch]

So a discriminant_value intrinsic wouldn't be too hard to implement.

There are likely always going to be cases a bit like this, so I think that such an intrinsic would be valuable to have. Since it's just discriminant retrieval, it shouldn't expose any implementation details beyond the chosen value of the discriminant.

[sfackler]

I think we could even make it a safe function with the proper warnings about the return value not being stable across compilations.

[sfackler]

A possible alternative would be a fn discriminants_match<T>(a: &T, b: &T) which would avoid exposing the tag values entirely.

[Diggsey]

That would mean that comparing discriminants is at best linear - by exposing the discriminant it can be used in less/greater comparisons, for jump tables or for fast lookup in a hash table.

[sfackler]

#[deriving(Show)] also appears to have some problems. A derived impl looks like this: https://gist.github.com/sfackler/bf4df1d0cc0b0c1a1b6b, while the following manual impl ends up like this: https://gist.github.com/sfackler/33ae5600f822cb3593ab

        impl fmt::Show for SqlState {
            fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
                let s = match *self {
                    $(SqlState::$error => stringify!($error),)+
                    SqlState::Unknown(ref s) => return write!(fmt, "Unknown({:?})", s),
                };
                fmt.write_str(s)
            }

Interestingly, changing the fmt.write_str(s) at the bottom to write!(fmt, "{}", s) massively blows up the compiled version to around what deriving generates: https://gist.github.com/sfackler/2a95a39f31bb4c00c48f

[huonw]

Historical discussion: #15503, #15375.

[mahkoh]

#15375 is horrible and still generates a 1.6M rlib with #[derive(PartialEq)] compared to 128K without it. Here is an easy fix that gets it down to 132K:

impl PartialEq for Key {
    fn eq(&self, other: &Key) -> bool {
        discriminant(self) == discriminant(other)
    }
}

pub fn discriminant(k: &Key) -> usize {
    match *k {
        Key::Unknown                 => 0,
        Key::Backspace               => 1,
        Key::Tab                     => 2,
        Key::Return                  => 3,
        Key::Escape                  => 4,
        // and so on
    }
}

This can naturally be extended to enums with fields:

impl PartialEq for Key {
    fn eq(&self, other: &Key) -> bool {
        if discriminant(self) != discriminant(other) {
            return false;
        }
         match (*self, *other) {
                (Key::Sleep(n), Key::Sleep(m)) => n == m,
                _ => true,
         }
    }
}

So #[derive(Discriminant)] would be enough.

[mahkoh]

For some reason this still generates a table instead of directly using the value stored in memory which explains why it's 132K instead of 128K.

Using the smallest possible type for the discriminant can reduce this size further.

[pnkfelix]

Now that we've put out the alpha, it seems like a good time to try to actually implement the compiler intrinsic to extract the discriminant (#15620). I'd personally rather try to jump to that, rather than add a derive(Descriminant), unless there is some motivation for a Discriminant trait...

[pnkfelix]

(such a compiler intrinsic to extract the discriminant might also provide a good foundation for a solution to #15523 )

[Aatch]

I'm happy to implement the discriminant instrinsic. There doesn't seem to be any opposition to it and I'm sure people will come up with some crazy uses for it.

[sfackler]

👍

[sfackler]

This appears to be resolved.