Implement more ergonomic rustls_result?

[cpu]

In error.rs we define a repr(u32) enum, rustls_result, and use it throughout the API as an ABI-stable, FFI-friendly way of representing what a Rustacean would probably write as Result<(), NonZero<u32>>. This is mechanically safe, but also makes the Rust code awkward, in particular precluding use of ?. It also reduces type-safety by using the same type for both success and error (rustls_result::OK & rustls_result::* vs Ok(()) & Err(_) variants).

It was pointed out to me a while ago by @complexspaces that Result is documented to have the same stable ABI/repr as Option<T>, subject to some caveats. The docs say:

In some cases, Result<T, E> will gain the same size, alignment, and ABI guarantees as Option has. One of either the T or E type must be a type that qualifies for the Option representation guarantees, and the other type must meet all of the following conditions:

• Is a zero-sized type with alignment 1 (a “1-ZST”).
• Has no fields.
• Does not have the #[non_exhaustive] attribute.

For example, NonZeroI32 qualifies for the Option representation guarantees, and () is a zero-sized type with alignment 1, no fields, and it isn’t non_exhaustive. This means that both Result<NonZeroI32, ()> and Result<(), NonZeroI32> have the same size, alignment, and ABI guarantees as Option. The only difference is the implied semantics:

• Option is “a non-zero i32 might be present”
• Result<NonZeroI32, ()> is “a non-zero i32 success result, if any”
• Result<(), NonZeroI32> is “a non-zero i32 error result, if any”

I think changing our API to use Result<(), NonZeroU32> would be an ergonomics/safety improvement for the Rust code assuming there aren't any additional "gotchas" lurking here. My thinking is we would rename rustls_result to rustls_error and remove the rustls_result::Ok value.

From an API compatibility standpoint my main concern is how invasive this will be due to rustls_result::Ok being defined as 7000 presently. AIUI a caller that got an Ok(()) from an extern "C" fn rustls_foo() -> Result<(), NonZeroU32> would now have 0 in hand, as opposed to 7000. This is mainly a concern if someone wasn't using the .h constants (e.g. if(rr == 7000) vs if(rr == RUSTLS_RESULT_OK)) - it seems to me it's fair to say those people (if they exist) are already begging for bugs?

We have a sem-ver incompatible 0.15.0 release coming up Soon ™️ so maybe we should do this?

[cpu]

@jsha I would be curious to hear your thoughts on this as the originator of rustls_result 🙇

[cpu]

I think the key change that made this viable was RFC 3391: rust-lang/rfcs#3391

It's not clear to me yet if this is limited to specific Rust versions+

[cpu]

I did a bit of experimentation and the results (so far) are not super promising. I'm not sure yet if the root cause is my own misunderstanding or a limitation of cbindgen.

Based on the discussion above I'd expect this function to be FFI-safe (and indeed it produces no "not FFI-safe" warning, as expected):

pub const MAY_FAIL_ARG_ZERO_ERR: u32 = 99;

#[no_mangle]
extern "C" fn may_fail(arg: u32) -> Result<(), NonZeroU32> {
    match arg == 0 {
        true => Err(NonZeroU32::new(MAY_FAIL_ARG_ZERO_ERR).unwrap()),
        false => Ok(()),
    }
}

However, when run through cbindgen (0.27) the return type is translated into an opaque struct like:

#define MAY_FAIL_ARG_ZERO_ERR 99

typedef struct Result_u32 Result_u32;
struct Result_u32 may_fail(uint32_t arg);

Changing may_fail to return Option<NonZeroU32> gives the result I'd expect, but has the wrong "implied semantics" for the Rust-side.

#define MAY_FAIL_ARG_ZERO_ERR 99

uint32_t may_fail(uint32_t arg);

Edit: opened mozilla/cbindgen#1035 with cbindgen to see if they have thoughts.

[jsha]

@jsha I would be curious to hear your thoughts on this as the originator of rustls_result 🙇

This does have some significant advantages! My original thoughts in defining RUSTLS_RESULT_OK == 7000 was that some C APIs return 0 for success; others return non-zero for success; and in my unscientific observations of bug histories, it seems like confusion between the two is a moderately common source of bugs. The fact that it enables shorthand like if(some_function()) or if(!some_function()) is both a benefit (shorter code!) and a detriment (less explicit code!). By defining success to be a nonzero value, we make explicit comparison necessary, hopefully making call sites clearer.

However, I don't feel strongly enough about the decision to say we shouldn't change it if we can reap some ergonomic benefits.

typedef struct Result_u32 Result_u32;

My guess here is that (a) cbindgen may not have code to take advantage of the representation guarantees you mention, and (b) it would probably require some opt-in since not all Option/Result returns necessarily want to export the details of their representation.

[cpu]

Thanks for your input!

My original thoughts in defining RUSTLS_RESULT_OK == 7000 was that some C APIs return 0 for success; others return non-zero for success; and in my unscientific observations of bug histories, it seems like confusion between the two is a moderately common source of bugs.

Agreed - this is a nice property. Maybe for a crate like this it makes more sense to prioritize the C API user experience (especially given the danger that misuse presents in that context) as opposed to the Rust developer experience.

My guess here is that (a) cbindgen may not have code to take advantage of the representation guarantees you mention

That's my intuition too. The guarantee for the Result layout is pretty new.

I think considering: how much time I have to invest in this particular task, the state of cbindgen development velocity (seems fairly sleepy) and the downside of a zero OK result for C code, my conclusion is that we should table this for now.

It might be worth revisiting in the future but I'd consider cbindgen having support (which may require implementing it ourselves) a blocker.