Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Result<(), error_enum> enum binding #2980

Open
jschwe opened this issue Nov 18, 2024 · 9 comments
Open

Add Result<(), error_enum> enum binding #2980

jschwe opened this issue Nov 18, 2024 · 9 comments
Assignees

Comments

@jschwe
Copy link
Contributor

jschwe commented Nov 18, 2024

For enums representing errors, often the Ok variant is the 0 value of the enum. It would be convenient if bindgen could replace this enum with a Result on the Rust side.

Example C-enum:

enum MY_C_ERRORCODE {
   MY_C_ERRORCODE_SUCCESS,
   MY_C_ERRORCODE_ERROR1,
   MY_C_ERRORCODE_ERROR2,
}

Expected Rust binding:

#[repr(transparent)]
pub struct MY_C_ERRORCODE_NZ(NonZero<c_int>);

impl MY_C_ERRORCODE {
    pub const ERROR1 = MY_C_ERRORCODE_NZ(NonZero::new(1).unwrap()));
    pub const ERROR2 = MY_C_ERRORCODE_NZ(NonZero::new(2).unwrap()))
}

// Perhaps this typedef could be omitted, and usages in the API could just use the result directly
type MY_C_ERRORCODE = Result<(), MY_C_ERRORCODE_NZ>;

This should work well at least when assuming that the generated non-zero enum representation is Newtype based.

@ojeda
Copy link
Contributor

ojeda commented Nov 18, 2024

When you say "usages in the API", do you mean using the Result in the generated function declarations by bindgen? If so, do your C APIs return the enum type (so that they can be mapped)? Could you show an example to clarify? Thanks!

@jschwe
Copy link
Contributor Author

jschwe commented Nov 18, 2024

Yes, exactly - the C API returns the enum type

extern "C" {

 // Example binding with current bindgen
 pub fn XXX_Get_Value(
        object: *mut MyObject,
        output_value: *mut f32,
    ) -> MY_C_ERRORCODE;

 // Example binding with proposed new result-enum binding
 pub fn XXX_Get_Value(
        object: *mut MyObject,
        output_value: *mut f32,
    ) -> Result<(), MY_C_ERRORCODE_NZ>;

}

A slightly higher level API could then perhaps wrap XXX_Get_Value and use map to put the output value (if the function has an output parameter) into the Result.

@ojeda
Copy link
Contributor

ojeda commented Nov 18, 2024

So your XXX_Get_Value returns enum MY_C_ERRORCODE rather than something like int, right? That should work then, since bindgen can map it (and if I understand correctly, with result_ffi_guarantees landed, it should be fine).

I guess the transformation would only make sense for certain enums, even if there are others with a 0, so perhaps a way to tag them would be useful. The interface for changing the behavior of enums may get modified by #2908 to allow for variations, so perhaps it is worth taking a look.

@pvdrz
Copy link
Contributor

pvdrz commented Nov 18, 2024

Hmm... I see this existing as a Builder method. However the CLI argument discussion would be more tricky as you want to do two selections:

  • First pick the enum that you want to transform into a Result
  • Second pick the variant of that enum that is the Ok variant. I assume that not all the time will be the one that's equal to zero as some folks don't even bother putting values on their enums.

So it would be something convoluted like --enum-as-result=MY_C_ERRORCODE=MY_C_ERRORCODE_SUCCESS

@jschwe
Copy link
Contributor Author

jschwe commented Nov 19, 2024

Personally I would be fine with only adding it as a Builder method (similar to the other enum variations).

Second pick the variant of that enum that is the Ok variant. I assume that not all the time will be the one that's equal to zero as some folks don't even bother putting values on their enums.

Are there really C error enums out there where 0 is not success? I would have assumed that to be pretty much universal.

@kulp
Copy link
Member

kulp commented Nov 19, 2024

Are there really C error enums out there where 0 is not success? I would have assumed that to be pretty much universal.

I have seen and written enums where the zero value is an invalid result, so that success must be explicitly chosen, as opposed to being arrived at by a happy accident of zero-initialization.

@jschwe
Copy link
Contributor Author

jschwe commented Nov 19, 2024

I have seen and written enums where the zero value is an invalid result, so that success must be explicitly chosen

That makes a lot of sense.

However, my proposal relies on Result<(), OtherTypeWithNiche> having the same layout as c_int (or whatever type the C-compiler chooses for the enum). As far as I know stable Rust doesn't support types with arbitrary niches yet, so on further thought I think we can't support the success type being anything other than 0 (and need to document this).

@ojeda
Copy link
Contributor

ojeda commented Nov 19, 2024

I assume that not all the time will be the one that's equal to zero as some folks don't even bother putting values on their enums.

enums start at 0 if there is no value, but of course, they may put the success one anywhere.

only adding it as a Builder method (similar to the other enum variations).

The other enum generation options can be set in the CLI (including the regex), no? Or do you mean other kinds of variations?

Are there really C error enums out there where 0 is not success? I would have assumed that to be pretty much universal.

Yeah, it is uncommon, although it is also true that there is a lot of C out there. I can imagine e.g. a hardware engineer adding a register with uncommon values, and then software mapping that as-is into an enum. Then there are things like Win32's HRESULT where 0 is success, but also all positive values, e.g. S_FALSE (those may not be actual enums, but I guess a similar API could have done something like that without 0 as success).

@bonzini
Copy link

bonzini commented Dec 9, 2024

Then there are things like Win32's HRESULT where 0 is success, but also all positive values, e.g. S_FALSE (those may not be actual enums, but I guess a similar API could have done something like that without 0 as success).

That would be something like Result<NonNegativeI32<T>, NegativeI32<T>>, neither of which exist :), so I guess it's easier to focus on just zero vs. everything else.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants