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

[Kent Gibson <warthog618@xxxxxxxxx>]

On Thu, Nov 24, 2022 at 03:46:27PM +0100, Miguel Ojeda wrote:
> On Thu, Nov 24, 2022 at 2:32 PM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> >
> > I don't see this as a problem for generics.  Whether the enum is signed
> > or unsigned doesn't need to affect the Error variant, much less the whole
> > Error type.  The Error doesn't need to respresent the type of the source
> > of the error, it needs to represent the type required to convey
> > information to the user.
> > Just accepting that the InvalidEnumValue variant expects i32, and casting
> > from u32 if necessary, seems appropriate to me.  Unless there are some
> > extreme values you are concerned about - but then you always switch it
> > up to  i64 ;-).
> 
> Yeah, I am not sure what a generic `Error` buys us here.
> 
> If one really wants to preserve whether it is signed or not, that is
> only two possibilities, not an open set of them. Moreover, I imagine
> one wants to constraint them for users, rather than let users provide
> the type `E`.
> 
> Thus one could have a sum type with 2 variants like
> `InvalidEnumValue(..., Either<i32, u32>)` or something more explicit.
> 
> But that is assuming there is a need to preserve it. What is the
> variant meant to be used for by users? e.g. if it is just for
> reporting, it probably doesn't matter. Actually, does the user even
> need the number? Could `InvalidArguments` be enough?
> 

AIUI, it is just for reporting.  The value itself is helpful to
understand the root cause of the problem.  Not critical, but nice to
have.

> Looking at it a bit more, I am confused why the error is possible to
> begin with. There is `Value::new()` which appears to be public and can
> return `InvalidEnumValue`, but why should it be used by users? They
> should create the `enum` directly, no? And for the other `new()`s that
> also return `InvalidEnumValue`s, I see they are `pub(crate)`. That is
> what I would expect, but still, why is the error a possibility to
> begin with?
> 

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.

Cheers,
Kent.

> For instance, somewhere else the library does `Direction::new(...)`
> with a value from the C side. The values from the C side must be
> correct, i.e. it is a bug otherwise, right? Thus one can trust them,
> or assert them, or if one wants to avoid panics for something that is
> a bug, one could return a `InternalLibraryError` with no extra
> information, because there is really not much users can do with the
> error details apart from knowing there is a bug in either the Rust or
> the C side.
> 
> I have taken a quick look at the C++ side for that same case, and from
> a quick look, C++ appears to throw if the mappings are wrong, so it
> sounds to me like you can similarly assert the validity in Rust and
> remove the `InvalidEnumValue` variant altogether.
> 
> Cheers,
> Miguel