Improve forwards compatibility of input API

[rib]

This adds a #[doc(hidden)] __Unknown(u32) variant to the various enums to keep them extensible without requiring API breaks.

We need to consider that most enums that are based on Android SDK enums may be extended across different versions of Android (i.e. effectively at runtime) or extended in new versions of android-activity when we pull in the latest NDK/SDK constants.

In particular in the case that there is some unknown variant we at least want to be able to preserve the integer value to allow the values to be either passed back into the SDK (it doesn't always matter whether we know the semantics of a variant at compile time) or passed on to something downstream that could be independently updated to know the semantics.

We don't want it to be an API break to extend these enums in future releases of android-activity.

It's not enough to rely on #[non-exhaustive] because that only really helps when adding new variants in sync with android-activity releases.

On the other hand we also can't rely on a catch-all Unknown(u32) that only really helps with unknown variants seen at runtime. (If code were to have an exhaustive match that would include matching on Unknown(_) values then they wouldn't be compatible with new versions of android-activity that would promote unknown values to known ones).

What we aim for instead is to have a hidden catch-all variant that is considered (practically) unmatchable so code is forced to have a unknown => {} catch-all pattern match that will cover unknown variants either in the form of Rust variants added in future versions or in the form of an __Unknown(u32) integer that represents an unknown variant seen at runtime.

Any unknown => {} pattern match can rely on IntoPrimitive to convert the unknown variant to the integer that comes from the Android SDK in case that values needs to be passed on, even without knowing it's semantic meaning at compile time.

Instead of adding an __Unknown(u32) variant to the Class enum though this enum has been removed in favour of adding methods like is_button_class() and is_pointer_class() to the Source type, since the class flags aren't guaranteed to be mutually exclusive and since they are an attribute of the Source.

This removes some reliance try_into().unwrap() that was put in place anticipating that we would support into() via num_enum, once we could update our rust-version.

[MarijnS95]

Hmm, on the ndk I had to assign a bogus discriminant here.

[MarijnS95]

I like this idea and I should have done it this way in the ndk. Because for now we have both:

• non_exhaustive to allow extending enums (mostly formats and error codes) without breaking changes;
• Unknown variant that isn't properly hidden with public i32 value, so that missing values are at least properly Debug-printed, easing the process of adding unknown members in the future.

[MarijnS95]

Yikes, none of these enums have really been marked appropriately.

[MarijnS95]

I guess this works (but haven't really reviewed in too great detail - got a tight schedule and am about to leave), unfortunate that the ndk::event types are so rigid.

[rib]

Thanks for the quick review.

Yeah I'm not 100% sure whether we might come up with a better strategy for allowing for this extensibility but for now this feels like a reasonably viable approach.

One specific limitation of this approach is that it's not compatible with #[non_exhaustive] which is unfortunate. I guess that the combination of repr(u32) and the code generated by num_enum convinces the compiler that the full range of variants have been covered if you match against __Unknown(u32) and so you aren't strictly forced to have a catch-all in your pattern matching.

I.e. Any code that ends up exhaustively matching __Unknown(u32) could break without using #[non_exhaustive] forcing them to add a _ => {} catch all when pattern matching.

If we were to implement the From<u32> / Into<u32> manually then we could probably get slightly better ergonomics that would be compatible with #[non_exhaustive] by avoiding the repr(u32) constraint.

Still though, the intent is hopefully quite obvious with the #[doc(hidden)] and the __ prefix, so it won't be considered an API break to add new variants to these enums in the future. Maybe in the future there will be a way to also use #[non_exhaustive] to be even more explicit here.

[MarijnS95]

One specific limitation of this approach is that it's not compatible with #[non_exhaustive] which is unfortunate. I guess that the combination of repr(u32) and the code generated by num_enum convinces the compiler that the full range of variants have been covered if you match against __Unknown(u32) and so you aren't strictly forced to have a catch-all in your pattern matching.

Not exactly sure what you are referring to, since we have exactly such a setup in the ndk.

https://github.com/rust-mobile/ndk/blob/f414cabc12211ebcba4edb3da90950967d4a1933/ndk/src/media_error.rs#L16-L20

Maybe it had to do with ordering of the attributes?

[MarijnS95]

By the way having a non-unit enum variant appropriately disallows MyEnum::Value as u32, forcing users to use the generated Into<u32> for MyEnum by num_enum which will appropriately unpack the __Unknown(u32), whichever discriminant it ends up receiving (typically prev+1).

[rib]

By the way having a non-unit enum variant appropriately disallows MyEnum::Value as u32, forcing users to use the generated Into<u32> for MyEnum by num_enum which will appropriately unpack the __Unknown(u32), whichever discriminant it ends up receiving (typically prev+1).

Yep I think it's reasonable that you have to use .into() instead of as. It's probably not the most common thing to need to be converting to the primitive type.

It's sort of unfortunate that there's no magic compiler trick for being able to avoid having the separate discriminant / value for this case so we could really have a true repr(u32) enum here but it also doesn't seem like a big deal that we lose out on that.

[rib]

One specific limitation of this approach is that it's not compatible with #[non_exhaustive] which is unfortunate. I guess that the combination of repr(u32) and the code generated by num_enum convinces the compiler that the full range of variants have been covered if you match against __Unknown(u32) and so you aren't strictly forced to have a catch-all in your pattern matching.

Not exactly sure what you are referring to, since we have exactly such a setup in the ndk.

Yeah I didn't see that it cause any error to use #[non_exhaustive] it's just that it didn't seem to have any effect (no matter the order) - but now it's just occurred to me my testing was probably flawed because I forget that it has different behaviour for code in the same crate! hmmm 🤦

So actually we probably can add #[non_exhaustive] too so the compiler will force apps to add a catch-all pattern.

[MarijnS95]

It's sort of unfortunate that there's no magic compiler trick for being able to avoid having the separate discriminant / value for this case so we could really have a true repr(u32) enum here but it also doesn't seem like a big deal that we lose out on that.

That was my main concern with this system/implementation; while "opaque to the user" it just feels ugly to now have to come up with a bogus discriminant (that could possibly be mapped in the future) just to have __Unknown, as well as spend extra bytes on that tuple-value while it "is" a discriminant.

That said, if there was builtin support for such a #[catch_all] __Unknown(u32), there would also be boilerplate - and/or it would suddenly be valid - to cast any u32 to a #[repr(u32)] enum (all without any help from num_enum).

In the end I might even like the associated-enum-constants representation from bindgen the most.