tonic: Unhide docs for Ascii and Binary

[adamchalmers]

MetadataValue::from<i16> and other numeric types aren't shown in Rustdoc, because they're implemented for Metadata<Ascii>, and Ascii is hidden.

Motivation

The docs for MetadataValue don't show any of its From<> impls for numerics like i16/i32/usize/etc.

Solution

Unhide the docs. I understand this makes the docs a bit noisier, so if there's a better solution I'd be open to that instead.

[LucioFranco]

I wonder if we want to add #[non_exhaustive] to the enums?

[adamchalmers]

I wonder if we want to add #[non_exhaustive] to the enums?

"The non_exhaustive attribute indicates that a type or variant may have more fields or variants added in the future" (doc), which I guess is true.

I think the main advantage of adding #[non_exhaustive] is to prevent people from matching on the enum, so that if we add more variants in the future we don't cause backwards-compatibility problems. But it seems really unlikely that anyone is matching on it right now, because neither Binary nor Ascii have any variants, so they're impossible to instantiate. Nobody can even have a value to match on!

So I don't think it would make any practical difference. It's simple enough to add it. But I think I rule slightly in favor of not adding this annotation until it actually serves a purpose... if you decide to ever add a variant to one of these, you could always add #[non_exhaustive] then, and you'd know it wasn't a breaking change because nobody is using these types directly. I think it just makes the code a little noisier without solving a real problem?

Instead, I'm adding a comment to explain that this type can't be instantiated, and is only used as a type parameter in MetadataValue. Let me know what you think, I'm happy to add #non_exhaustive if you think it makes sense.

[LucioFranco]

Yeah, that sounds fair to me. I wonder if we want to convert them to be

struct Ascii {
   _p:  ()
}

So they actually can never be constructed. I don't actually know why they are enums in the first place lol.

Anyways, I appreciate the thorough responses :)

[adamchalmers]

I think the current solution (enum with no variants) is impossible to construct, but your solution is actually constructable (see playground)? My guess is that's why they were built as no-variant enum, I think that's the standard way to make a type that can never be constructed.

[davidpdrsn]

You can construct

struct Ascii {
   _p:  ()
}

because the field is public within your module (the root module). It wont be constructable in other modules or crates.

Generally speaking I've moved to using #[non_exhaustive] for all my crates. It works the same as a private field but is more modern I'd say.

Re breaking changes: Technically one could write code like this

// defined in tonic
pub enum Ascii {}

// defined in the user's crate
trait IntoInfallible {
    fn into_infallible(self) -> Infallible;
}

impl IntoInfallible for Ascii {
    fn into_infallible(self) -> Infallible {
        match self {}
    }
}

which would break if Ascii gained private fields.

I think we should keep as they are now (but make the docs public) and in the next major convert them to

#[non_exhaustive]
pub struct Ascii;

#[non_exhaustive]
pub struct Binary;

[LucioFranco]

Well we can make the breaking change now since they were doc hidden and thus there are not technically apart of our public API, right?

[davidpdrsn]

I guess kind of gray area but yeah I'd be fine with that.

[adamchalmers]

OK, done. I kept them as enums but added non_exhaustive.

[LucioFranco]

Thanks! I appreciate you following up on this!

[adamchalmers]

My pleasure, I'm glad I can contribute, in my own small way, to a project I use pretty heavily.