ndk: Mark all enums as `non_exhaustive` and fix `repr` types #459

MarijnS95 · 2023-12-25T15:25:56Z

Depends on #458

We're currently in a painful situation, both when it comes to correctness of the wrapper functions that take or return enum values, as well as convenience and ability to upgrade. This is a summation of issues, and reasons why external crates like android-activity are copy-pasting and fixing up our implementation:

False mapping of unknown values to NDK _UNKNOWN constants. This makes the API lossy as the original unknown value does not have the KNOWN value that is assigned to the _UNKNOWN constant (typically 0);
Lacking #[non_exhaustive] in most places, not allowing us to add new and missing constants to enum;
#[repr(u32)] that follows the default u32 type used by bindgen when processing an "untyped" enum, rather than considering the type (i32, i8, etc) that is actually used by every function taking or returning this "integer constant";
Inconsistent use of either:
- Panicking on unknown values;
- Wrapping unknown values in a custom Err type (i.e. AudioError::UnknownValue or TryFromPrimitiveError);
- Assigning unknown values to an Unknown = ffi::XXX_UNKNOWN variant;
- Sometimes returning them in a typed Unknown(xxx) variant.

To solve this we have to make the following changes to every pub enum with custom discriminants in the NDK crate:

Drop all builtin type conversions and update repr(u32) to use the common type used by every function that takes or returns this enum value. Conversions always happen via .into() from IntoPrimitive (needed to automatically process __Unknown);
Annotate all those enums with #[non_exhaustive]. This requires callers to have a x => todo!("New constant {x:?}") or similar handling in a match block. It will neatly print the value for __Unknown added below;
Replace TryFromPrimitive with FromPrimitive;
Have a catch-all __Unknown variant, that is #[doc(hidden)]. This:
- Saves us from having a TryIntoPrimitiveError or custom Err type;
- Allows the caller to pass in custom/unknown values;
- Entices contributions/additions in the crate by having the type #[doc(hidden)], as a lot of enums are currently lagging behind their upstream representation;
Add IntoPrimitive to all enums that were using manual matches. FromPrimitive is not derived, and similarly an __Unknown variant is not added. This does not allow custom values to be passed, but it does again entice crate updates when there are new values.

TODO

For error types like AudioResult/AudioError, implement Error on them immediately like on MediaError. This is possible now that the custom UnknownValue variant is gone.
Apply the same changes to native_window, data_space, and hardware_buffer, for which I have purpose-built documented commits on a separate branch, but might/should as well squash in here. DataSpace is fairly recent and I don't remember why I didn't follow the above guidelines right from the get-go.

In a followup PR missing constants will be added to various enums.

MarijnS95 · 2024-01-18T14:15:46Z

ndk/src/data_space.rs

-#[repr(u32)]
-#[derive(Clone, Copy, Hash, PartialEq, Eq, TryFromPrimitive, IntoPrimitive)]
+#[repr(i32)]
+#[derive(Clone, Copy, Hash, PartialEq, Eq, FromPrimitive, IntoPrimitive)]
 #[doc(alias = "ADataSpace")]
 #[non_exhaustive]
 pub enum DataSpace {


Note that the native type here will change to i32:

https://android-review.googlesource.com/c/platform/frameworks/native/+/2902242

rib

Nice, this must have been quite a chore to go through and update - kudos!

I made a couple of minor comments, but overall it looks good to me!

ndk/src/audio.rs

ndk/src/bitmap.rs

ndk/src/configuration.rs

ndk/src/native_window.rs

MarijnS95 · 2024-01-18T20:19:22Z

Nice, this must have been quite a chore to go through and update - kudos!

Thanks! Besides reviewing the changes here, I hope you've also cross-checked that I didn't miss out on some enums?

Are you okay with the proposed idea of both having non_exhaustive and a hidden __Unknown?

I'll process the review ASAP and get this in to unblock many other changes, many thanks for the review!

rib · 2024-01-18T20:27:33Z

I hope you've also cross-checked that I didn't miss out on some enums?

I didn't, but can take a pass to cross check now...

rib · 2024-01-18T20:56:08Z

Going through cross checking all enums, it could be good go add non_exhaustive to InputEvent potentially.

At least in android-activity I defined InputEvent as non_exhaustive considering also exposing Text/IME events but maybe for the ndk API it's ok to just assume the types of InputQueue events won't get extended?

rib · 2024-01-18T21:03:39Z

Are you okay with the proposed idea of both having non_exhaustive and a hidden __Unknown?

Yeah I think it makes sense. This is at least what I settled on with android-activity after trying to ponder the trade offs. It's maybe not the perfect solution - ideally the compiler could give us a true repr(i32) enum but it seems like a pretty decent trade off for allowing extension of enums via Android updates.

rib

It probably makes sense to add an __Unknown variant to FamilyVariant I think

rib · 2024-01-18T21:18:37Z

I hope you've also cross-checked that I didn't miss out on some enums?

I didn't, but can take a pass to cross check now...

okey, I've taken a pass through all source for the ndk crate to try and find any enums missed and left a few comments above and haven't found anything else.

MarijnS95 · 2024-01-18T22:35:10Z

Going through cross checking all enums, it could be good go add non_exhaustive to InputEvent potentially.

At least in android-activity I defined InputEvent as non_exhaustive considering also exposing Text/IME events but maybe for the ndk API it's ok to just assume the types of InputQueue events won't get extended?

Good point. I ended up glossing over this but there are definitely more input types, though currently none has a type and specific functions. Let's mark this as non_exhaustive.

pub const AINPUT_EVENT_TYPE_KEY: _bindgen_ty_19 = 1;
pub const AINPUT_EVENT_TYPE_MOTION: _bindgen_ty_19 = 2;
pub const AINPUT_EVENT_TYPE_FOCUS: _bindgen_ty_19 = 3;
pub const AINPUT_EVENT_TYPE_CAPTURE: _bindgen_ty_19 = 4;
pub const AINPUT_EVENT_TYPE_DRAG: _bindgen_ty_19 = 5;
pub const AINPUT_EVENT_TYPE_TOUCH_MODE: _bindgen_ty_19 = 6;

MarijnS95 · 2024-01-18T22:39:21Z

It probably makes sense to add an __Unknown variant to FamilyVariant I think

Whoops, looks like I made an exception for an input-only enum here, to encourage NDK updates when needing to pass new values. Didn't do that anywhere else I think, so let's add it.

MarijnS95 · 2024-01-18T22:53:13Z

Thanks a lot @rib! I know it's a big ask to go through all enums, it's quite a bit to process. But glad to have it resolved in the end.

Who knows, once I import the new values, if we can just replace the enums in android-activity with these again.

ndk/src/bitmap.rs

rib

Cool, I think this is all looking good! Really good stuff!

We're currently in a painful situation, both when it comes to correctness of the wrapper functions that take or return enum values, as well as convenience and ability to upgrade. This is a summation of issues, and reasons why external crates like `android-activity` are copy-pasting and fixing up our implementation: - False mapping of unknown values to NDK `_UNKNOWN` constants. This makes the API lossy as the original unknown value does not have the **KNOWN** value that is assigned to the `_UNKNOWN` constant (typically `0`); - Lacking `#[non_exhaustive]` in most places, not allowing us to add new **and missing** constants to `enum`; - `#[repr(u32)]` that follows the default `u32` type used by `bindgen` when processing an "untyped" enum, rather than considering the type (`i32`, `i8`, etc) that is actually used by every function taking or returning this "integer constant"; - Inconsistent use of either: - Panicking on unknown values; - Wrapping unknown values in a custom `Err` type (i.e. `AudioError::UnknownValue` or `TryFromPrimitiveError`); - Assigning unknown values to an `Unknown = ffi::XXX_UNKNOWN` variant; - Sometimes returning them in a typed `Unknown(xxx)` variant. To solve this we have to make the following changes to every `pub enum` with custom discriminants in the NDK crate: - Drop all builtin type conversions and update `repr(u32)` to use the common type used by _every_ function that takes or returns this enum value. Conversions always happen via `.into()` from `IntoPrimitive` (needed to automatically process `__Unknown`); - Annotate all those `enum`s with `#[non_exhaustive]`. This requires callers to have a `x => todo!("New constant {x:?}")` or similar handling in a `match` block. It will neatly print the value for `__Unknown` added below; - Replace `TryFromPrimitive` with `FromPrimitive`; - Have a catch-all `__Unknown` variant, that is `#[doc(hidden)]`. This: - Saves us from having a `TryIntoPrimitiveError` or custom `Err` type; - Allows the caller to pass in custom/unknown values; - Entices contributions/additions in the crate by having the type `#[doc(hidden)]`, as a lot of enums are currently lagging behind their upstream representation; - Add `IntoPrimitive` to all enums that were using manual `match`es. `FromPrimitive` is not derived, and similarly an `__Unknown` variant is not added. This does not allow custom values to be passed, but it does again entice crate updates when there are new values. \# TODO - For error types like `AudioResult`/`AudioError`, implement `Error` on them immediately like on `MediaError`. This is possible now that the custom `UnknownValue` variant is gone. - Apply the same changes to native_window, data_space, and hardware_buffer, for which I have purpose-built documented commits on a separate branch, but might/should as well squash in here. `DataSpace` is fairly recent and I don't remember why I didn't follow the above guidelines right from the get-go.

…ch functions All functions that deal with this type take an `i32`; just the underlying `ndk-sys` brindings are misrepresented because `bindgen` does not know what integer type to use for an `enum`.

All functions that deal with these types take an `i8`; just the underlying `ndk-sys` brindings are misrepresented because `bindgen` does not know what integer type to use for an `enum`. Using an `i8` makes casting more natural, without unnecessary overflow checks. The compiler will make sure that the constants assigned to this enum never exceed `i8`, and the NDK would never overflow these constants in the first place as that breaks the existing bindings. Note that these types will be used with the same `i8` type in the `SurfaceControl`/`SurfaceTransaction` bindings.

MarijnS95 requested a review from rib December 25, 2023 15:25

MarijnS95 added the impact: breaking API/ABI-breaking change label Dec 25, 2023

MarijnS95 force-pushed the non-exhaustive-enums branch from fa9224f to 3f50eb9 Compare December 25, 2023 22:06

MarijnS95 commented Jan 18, 2024

View reviewed changes

MarijnS95 mentioned this pull request Jan 18, 2024

[BUG] surface_control.h and *font*.h contains C++ constructs android/ndk#1920

Closed

rib approved these changes Jan 18, 2024

View reviewed changes

ndk/src/audio.rs Show resolved Hide resolved

ndk/src/bitmap.rs Outdated Show resolved Hide resolved

ndk/src/configuration.rs Show resolved Hide resolved

ndk/src/native_window.rs Show resolved Hide resolved

ndk/src/native_window.rs Show resolved Hide resolved

rib requested changes Jan 18, 2024

View reviewed changes

MarijnS95 force-pushed the event-source-class branch from 485b34e to e5b8669 Compare January 18, 2024 22:03

MarijnS95 force-pushed the non-exhaustive-enums branch from 3f50eb9 to f178eec Compare January 18, 2024 22:47

MarijnS95 mentioned this pull request Jan 18, 2024

ndk/audio: Merge AudioResult variant enum into AudioError #467

Merged

MarijnS95 force-pushed the event-source-class branch 3 times, most recently from da5441f to 250f19b Compare January 19, 2024 18:20

rib reviewed Jan 26, 2024

View reviewed changes

ndk/src/bitmap.rs Outdated Show resolved Hide resolved

rib approved these changes Jan 26, 2024

View reviewed changes

MarijnS95 force-pushed the event-source-class branch 2 times, most recently from c724f6a to 71ee97d Compare January 27, 2024 15:37

Base automatically changed from event-source-class to master January 27, 2024 15:43

MarijnS95 force-pushed the non-exhaustive-enums branch from f178eec to aea39d3 Compare January 27, 2024 22:12

MarijnS95 and others added 3 commits January 29, 2024 09:28

native_window: Use i32 in NativeWindowTransform bitflags to mat…

32743e3

…ch functions All functions that deal with this type take an `i32`; just the underlying `ndk-sys` brindings are misrepresented because `bindgen` does not know what integer type to use for an `enum`.

MarijnS95 added 2 commits January 29, 2024 09:28

ndk/data_space: Catch-all and i32

84fc952

i32 for HardwareBufferFormat

8216763

MarijnS95 force-pushed the non-exhaustive-enums branch from aea39d3 to 8216763 Compare January 29, 2024 08:28

MarijnS95 merged commit 3a25c1a into master Jan 29, 2024
38 checks passed

MarijnS95 deleted the non-exhaustive-enums branch January 29, 2024 16:35

MarijnS95 mentioned this pull request Jul 4, 2024

input: Replace open-coded types with ndk::event definitions rust-mobile/android-activity#163

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ndk: Mark all enums as `non_exhaustive` and fix `repr` types #459

ndk: Mark all enums as `non_exhaustive` and fix `repr` types #459

MarijnS95 commented Dec 25, 2023 •

edited

Loading

MarijnS95 Jan 18, 2024

rib left a comment

MarijnS95 commented Jan 18, 2024

rib commented Jan 18, 2024 •

edited

Loading

rib commented Jan 18, 2024

rib commented Jan 18, 2024

rib left a comment

rib commented Jan 18, 2024

MarijnS95 commented Jan 18, 2024 •

edited

Loading

MarijnS95 commented Jan 18, 2024

MarijnS95 commented Jan 18, 2024

rib left a comment

ndk: Mark all enums as non_exhaustive and fix repr types #459

ndk: Mark all enums as non_exhaustive and fix repr types #459

Conversation

MarijnS95 commented Dec 25, 2023 • edited Loading

TODO

MarijnS95 Jan 18, 2024

Choose a reason for hiding this comment

rib left a comment

Choose a reason for hiding this comment

MarijnS95 commented Jan 18, 2024

rib commented Jan 18, 2024 • edited Loading

rib commented Jan 18, 2024

rib commented Jan 18, 2024

rib left a comment

Choose a reason for hiding this comment

rib commented Jan 18, 2024

MarijnS95 commented Jan 18, 2024 • edited Loading

MarijnS95 commented Jan 18, 2024

MarijnS95 commented Jan 18, 2024

rib left a comment

Choose a reason for hiding this comment

ndk: Mark all enums as `non_exhaustive` and fix `repr` types #459

ndk: Mark all enums as `non_exhaustive` and fix `repr` types #459

MarijnS95 commented Dec 25, 2023 •

edited

Loading

rib commented Jan 18, 2024 •

edited

Loading

MarijnS95 commented Jan 18, 2024 •

edited

Loading