Re: libgpiod: rust bindings and bindgen issue with C enums

[Kent Gibson <warthog618@xxxxxxxxx>]

On Thu, Nov 24, 2022 at 05:18:20PM +0100, Miguel Ojeda wrote:
> On Thu, Nov 24, 2022 at 3:58 PM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> >
> > The possibility for error can arise from running against a later
> > libgpiod that has additional values for the enum that these bindings are
> > obviously unaware of. e.g. the hte event clock recently added.  If you
> > had bindings built prior to that addition there is no Rust variant
> > in the event clock enum for that to map to.
> 
> If using older bindings against newer libraries is intended to be
> supported (and not just the other way around), then I would expect at
> least some of the enums to be stable (i.e. exhaustive), no? Say,
> `Direction` or `EdgeKind`. Are new variants expected in those (before
> a major 2.0)?
> 

Not sure on the policy for Rust bindings, just going by what has
happened in the past for C++ and Python, which are linked dynamically,
and assuming the same for Rust.
Hmmm, the examples in the current Rust bindings are statically linked
to libgpiod, which isn't what I expected, so there you go.
Not sure if that will always be the case.

Anyway, for statically linked Rust bindings, the source of the error can
be a newer kernel (e.g. an extra value in the enum gpio_v2_line_attr_id),
or a bug in libgpiod itself.

The enums drawn from flags are effectively stable even if new flags get
added to the kernel - libgpiod will just ignore them.  Though I'm not
sure there is any advantage in treating them differently, particularly
if we do want to support dynamic linking.

> And for those that aren't designed to be stable, `InvalidEnumValue` is
> a bit misleading. It is just that it is unknown to the bindings (which
> is more useful for users: "I should update my bindings"), or
> "unexposed" (a term some bindings use when the value is still usable
> even if opaque).
> 

The naming is probably a carry over from the C/C++ where the invalid
values can originate from the user, but a valid point wrt Rust.

What is "invalid" depends on perspective.  It is invalid from the user's
PoV, even if it may be valid in an absolute sense.
Is `UnknownEnumValue` a little clearer?  Though that implies you don't
know the actual value passed - and you do.
`UnexpectedEnumValue`?

> Now, the C++ side throws even for the clock mapping (and the exception
> says "thrown when the core C library returns an invalid value"), which
> to me it sounds like the C++ bindings are not intended to be used if
> there is a mismatch. Thus Rust could panic too.
> 

C++ is a slightly different case, as the invalid values can originate from
the user as well.

Disagree on the equivalence between C++ exceptions and Rust panics.
In C++, exceptions are used for general error handling whereas Rust panics,
similar to Go panics, are used for unrecoverable errors.
In Rust, general errors are handled with Results.

I don't see these errors as unrecoverable.  In practice, any lines you
request can only take values that you understand, so you would only see
such errors when reading info about lines you didn't request, and then
the app should just note "well that's odd" and move on.

> On the other hand, if the bindings are actually intended to be used
> even when encountering unknown values, then I would say C++ shouldn't
> throw, Rust shouldn't panic, and the enums should be marked as stable
> or not as needed. Then, for the unstable ones, depending on whether an
> unknown value can still be useful, it could be made an `Unknown`
> variant on the particular `enum` (e.g. `EventClock::Unknown`) instead
> of an error (the "unexposed" idea above).
> 

That approach certainly makes sense if the accessors don't already return
Results - but they do as there are other sources of error.
Given that they do return Results, the decision is whether the invalid
value should be handled in the error path or the normal path.
The error path makes more sense to me as this case is quite unlikely
and forcing the user to always deal with potential Unknown variants in
the normal path is a bit unfriendly.  Much nicer to bunch it in with
the "well that didn't work as expected" path.

So, in short, I agree that the naming could be improved, but I'm otherwise
ok with the approach the bindings are taking here.

Cheers,
Kent.