RFC: result_ffi_guarantees

[Lokathor]

Rendered

[clarfonthey]

Is there any particular downside to just making this guarantee for all enums similar to Option and Result in layout, like Poll for instance?

[Lokathor]

Yes that was mentioned in the Future Work portion. Basically it can probably become a semver hazard sort of thing if it's automatically done for all enums and people aren't opting in to the behavior. Keeping the RFC scoped small makes it more likely to be accepted and completed on the types people use most often.

[clarfonthey]

Ahh, I see; I completely missed that part. I guess that what I'm failing to understand is how you can actually run into that kind of hazard unless you use something like #[non_exhaustive], since any changes to the type would be breaking anyway.

[afetisov]

The examples need a bit more diversity. They are all of the form Result<T, ()>, while the proposal also covers Result<(), E> or Result<T, ZST>.

This needs some more discussion on which exact zero-sized types should have layout optimization guarantees, because it could be a semver hazard, particularly in FFI. A type struct Error {} could have new fields added in the future. Perhaps limit only to #[repr(C)] ZSTs? Or to structs of the form struct NoParams;, where any field addition is obviously a breaking change?

I have doubts whether the layout guarantees would be useful in practice, though. The primary motivation in the RFC isn't memory usage but FFI ergonomics. With Option<&T>, for example, a raw pointer is already treated as "either nothing or a valid *mut T" in C function APIs, so the guarantee is reasonable. But with fallible APIs, the conventions are all over the place. Some functions return negative integers on error, some - positive ones, some do both of those, some even return 0, or return garbage and set ERRNO. In principle, you could cover those cases, e.g. Result<(), NonZeroI32> covers the "0 for success, nonzero for error" part. But in practice, you would expect specific error codes and not just arbitrary integers, so you'd have to do some error classification on the Rust side anyway.

This means that a separate ABI-stable struct as the return type, with functions success(self) -> T and error(self) -> Error would likely be a better choice. Once the Try-trait gets stable, you could also use all the usual ? sugar with those types.

[nbdd0121]

The point about ZST is also brought up last week by @tmandry during lang team triage meeting (notes). We could use limit it to #[repr(transparent)] or #[repr(C)] ZSTs.

[ChrisDenton]

Tbh, I'm not particularly keen on having guarentees on repr(C) ZSTs. They are not a thing in ISO C and compiler extensions may (or may not) do different things with them which makes them potentially an FFI hazzard.

[afetisov]

@ChrisDenton Rust guarantees their specific behaviour, so this seems like a non-issue. On the FFI side none of those types would exist anyway, just like Result<_, _> doesn't. Though #[repr(C)] is likely indeed a poor choice for layout optimization guarantees, since one could just as easily add private fields as to a #[repr(Rust)] type. #[repr(transparent)] indeed looks like a better option.

[Lokathor]

repr(transparent) newtypes of () would be sufficient to allow for different errors, but a lot of people would probably ask why they have to put a "useless field" in their error type to make it work right.

[afetisov]

Why would you make it a newtype of ()? I'm thinking of the following definitions:

struct ZST;
#[repr(transparent)]
struct ZST();
#[repr(transparent)]
struct ZST {}
#[repr(transparent)]
struct ZST<T>(PhantomData<T>);

[Lokathor]

I had thought that a repr(transparent) needed to have a single non-zero sized field, but double checking using your example.. it seems that there can be one or zero non-zero sized fields.

So sure, repr(transparent) (or fieldless) seems a fine requirement on the ZST. One moment and I'll make an edit.

[joshtriplett]

@rfcbot merge

[rfcbot]

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @pnkfelix
• @scottmcm
• @tmandry

Concerns:

• needs-to-mention-non_exhaustive resolved by RFC: result_ffi_guarantees #3391 (comment)

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[scottmcm]

Overall seems reasonable.

@rfcbot concern needs-to-mention-non_exhaustive

It's implied by the definition of non_exhaustive that a non-exhaustive ZST can't be guaranteed here, but since we've had so many issues about it (rust-lang/rust#78586), I'd like to see it called out explicitly in here to make sure we get tests about it.

[scottmcm]

it seems that there can be one or zero non-zero sized fields.

Zero fields was FCPed in rust-lang/rust#77841

[CryZe]

There's been some ideas of potentially modifying the calling convention to support Result more cheaply. Does this RFC interfere with that?

[Lokathor]

There's been some ideas of potentially modifying the calling convention to support Result more cheaply. Does this RFC interfere with that?

This RFC requires that the combined type be treated as the primitive type, so I guess "yes, it could interfere with that". However, since the primitive types are generally single-register (except and wide pointers and ints bigger than a machine register), this doesn't seem like a strong problem. Other forms of Result (eg: Result<(),()>) could still (in the future) be optimized differently (eg: have the function "return the value" via status flag being set or not).

Zero fields was FCPed in rust-lang/rust#77841

It seems that the reference didn't get an update.

concern needs-to-mention-non_exhaustive

Updated.

[programmerjake]

one thing i'd specifically want to reserve space for is representing Result errors with unwinding (distinct from panicking) when the user opts into it, probably through an attribute on the error type or on the function. the idea is this is more efficient for functions that very rarely fail, because the generated LLVM IR don't have to construct Result values or match on them.

this may need a specific carve-out exception in the defined ABI for Result, so we should carve it out now before it becomes a breaking change.

[Lokathor]

Hm, could you say more about what you'd expect to be changed within the RFC to have that carved out? Some mentioning in Future Possibilities? I'm completely unfamiliar with how unwinding interacts with the foreign ABI modes.

[RalfJung]

1. I assume the nested list of requirements is a conjunction, but that should be made explicit.
2. I am surprised to see repr(transparent) here. In the past we stated that a repr(transparent) in the standard library does not mean this type will not change in the future -- basically repr(transparent) is a private attribute that the library may use, but not clients of the library. If this is intended to deal with issues like repr(transparent) should not consider non_exhaustive types as 1-ZSTs outside their crate rust#78586, I would have expected "has no private field" (which is then trivially true for fieldless types)

[Lokathor]

1. yeah it's a "must meet all of these", i can clarify the wording.

2. I'd be fine changing to "must have no fields" if necessary. Since it's in FCP though maybe a T-lang member can make a clear call here before I touch that part?

[afetisov]

The issue is that it's very easy to add a private field and accidentally break the guaranteed layout. Imho there should be some explicit way to opt into such optimizations for ZSTs which are guaranteed to remain ZST. I don't think #[repr(transparent)] is a good solution to the issue, but it's the closest approximation in current Rust, so it's at least something to consider.

Is the "private attribute" opinion documented somewhere? Afaik it isn't, and it's not uncommon to treat it as an API guarantee.

[Lokathor]

I asked Josh via Zulip and he said it would be fine to further restrict the rules here even while FCP is active.

I will make an update later today to simply require no fields at all (and no non_exhaustive). Then it will be the usual semver breaking change if a field is added. I believe that should satisfy things?

[afetisov]

No. The thing that really worries me is that, being a guaranteed optimization, breaking the layout won't produce any errors. The optimization will just silently stop applying. This will lead to UB on the FFI side, and I don't see any reliable way to check for such breakage.

An upstream crate makes a semver breaking change by adding some inner field (possibly even zero-sized one, but perhaps not in a guaranteed way). A downstream crate which used the changed type in extern fn() -> Result<&T, ZST> now silently gets UB, and there is no way for them to avoid it other than tracking all changes in all dependencies.

Currently the solution would involve explicitly ABI-stable types, and conversions between them and native Rust types. Such conversions would either work correctly, fail to compile or fail at runtime (assuming they are properly written and verify their preconditions).

Also note that this isn't an issue for current types with guaranteed layout optimizations, since their number is very small and they are all provided by the language. There is only a finite number of non-generic NonZeroX types, and they are all guaranteed by the language. If user-defined niches become a thing, I would be just as worried.

For this reason I would prefer to have an explicit way to specify that the layout optimization must apply, rather than deduce it from implicit properties of code, which may be changed by the programmer without even considering that someone could rely on them.

[Lokathor]

If the crate you're getting a type from puts some attribute on their type, can't they just remove that attribute (presumably as a semver break) in a future version? Then isn't it just as bad for the downstream crates relying on it?

Honestly I don't think people should do this in the first place with any type that's not their own type or (). However, I don't want to disallow user ZSTs entirely because it makes for much better internal error handling compared to having () as the error type.

[programmerjake]

the point of having an attribute is that explicitly having to remove it makes people consider it a semver break since it's obvious, whereas having it completely implicit makes it much more likely people won't realize they need a semvar break to change their library type -- most people aren't going to think about Result's ABI when they change their library type.

[nbdd0121]

An upstream crate makes a semver breaking change by adding some inner field (possibly even zero-sized one, but perhaps not in a guaranteed way). A downstream crate which used the changed type in extern fn() -> Result<&T, ZST> now silently gets UB, and there is no way for them to avoid it other than tracking all changes in all dependencies.

We do have improper_ctypes warning though.

[workingjubilee]

improper_ctypes is far too noisy to be useful, so we shouldn't rely on a fairly substandard lint.

[joshtriplett]

one thing i'd specifically want to reserve space for is representing Result errors with unwinding (distinct from panicking) when the user opts into it,

This RFC covers the FFI case, which wouldn't use that optimisation. And if it did then the future RFC proposing that would have a lot of specifying to do, and I think that specifying belongs there rather than here.

[programmerjake]

Hm, could you say more about what you'd expect to be changed within the RFC to have that carved out? Some mentioning in Future Possibilities? I'm completely unfamiliar with how unwinding interacts with the foreign ABI modes.

I'm basically expecting that the list of requirements to have Result have a defined ABI would have an entry kinda like this:

• Future RFCs may introduce additional requirements, e.g. some new 1-ZST error types may be marked such that they opt out of stable ABI for Result<T, MyErr> e.g. by representing the Err variant by unwinding instead of returning normally.

this way we tell users they can't just just check the current list of requirements for any arbitrary type and be guaranteed to always forevermore have stable ABI when those requirements are met, future RFCs may introduce exceptions for types using new features.

[nikomatsakis]

@rfcbot fcp reviewed

This makes sense to me.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[riking]

The primitive ZSTs include (), [T; 0], PhantomData<T>, and function pointer types

Slight correction: function item types. Function pointers have sizes.

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[dtolnay]

This RFC was discussed in this week's lang team triage meeting. We (I) weren't sure whether the compiler already treats types like Result<NonZeroI32, ()> with the same ABI as C int32_t, vs with the same ABI as C struct { int32_t; }. (The distinction between these 2 in extern "C", unlike in extern "Rust", is why RFC 1758 repr(transparent) was needed.)

lokathor: does compiler already do this?

nikomatsakis: Pretty sure it does.

cramertj: is this insta-stable?

nikomatsakis: I think so, or else we can fcp merge the PR to the reference.

If Result<NonZeroI32, ()> already works like int32_t on all platforms then implementing this RFC is a matter of exempting the appropriate types from improper_ctypes / improper_ctypes_definitions, and testing; if it works like struct { int32_t; } there would need to be codegen changes.

As far as I could tell, the former is the case, at least in my usual suspect ABI (32-bit x86).

// cargo asm --target i686-unknown-linux-gnu

use std::num::NonZeroI32;

#[repr(C)]
pub struct Wrapper {
    pub primitive: i32,
}

pub extern "C" fn primitive() -> i32 {
    1
}

// different code than `primitive`
pub extern "C" fn wrapper() -> Wrapper {
    Wrapper { primitive: 1 }
}

// same code as `primitive`
pub extern "C" fn result_nonzero() -> Result<NonZeroI32, ()> {
    Ok(unsafe { NonZeroI32::new_unchecked(1) })
}

[tmandry]

The @rust-lang/lang team has decided to accept this RFC.

To track further discussion, subscribe to the tracking issue here:
rust-lang/rust#110503