Closed rib closed 11 months ago
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.
One specific limitation of this approach is that it's not compatible with
#[non_exhaustive]
which is unfortunate. I guess that the combination ofrepr(u32)
and the code generated bynum_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
.
Maybe it had to do with ordering of the attributes?
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
).
By the way having a non-unit enum variant appropriately disallows
MyEnum::Value as u32
, forcing users to use the generatedInto<u32> for MyEnum
bynum_enum
which will appropriately unpack the__Unknown(u32)
, whichever discriminant it ends up receiving (typicallyprev+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.
One specific limitation of this approach is that it's not compatible with
#[non_exhaustive]
which is unfortunate. I guess that the combination ofrepr(u32)
and the code generated bynum_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.
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.
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 onUnknown(_)
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 onIntoPrimitive
to convert theunknown
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 theClass
enum though this enum has been removed in favour of adding methods likeis_button_class()
andis_pointer_class()
to theSource
type, since the class flags aren't guaranteed to be mutually exclusive and since they are an attribute of theSource
.This removes some reliance
try_into().unwrap()
that was put in place anticipating that we would supportinto()
vianum_enum
, once we could update our rust-version.