Skip to content

RFC: result_ffi_guarantees #3391

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Apr 18, 2023
Merged

RFC: result_ffi_guarantees #3391

Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4b5a891
rfc text
Lokathor Feb 21, 2023
f647e37
restrict the ZST rules, provide further explanation.
Lokathor Feb 28, 2023
e729531
require that there NOT be the non_exhaustive attribute on the ZST
Lokathor Feb 28, 2023
2c258cc
clarify that the condition list is an "all" list.
Lokathor Mar 1, 2023
e8ecdc4
restrict the guarantee to when the ZST has no fields at all.
Lokathor Mar 2, 2023
40b65f3
missing comma
Lokathor Mar 23, 2023
7320c01
Add RFC number and tracking issue
tmandry Apr 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 94 additions & 0 deletions text/0000-result_ffi_guarantees.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
# RFC: result_ffi_guarantees

- Feature Name: `result_ffi_guarantees`
- Start Date: 2023-02-15
- RFC PR: [rust-lang/rfcs#0000](https://github.com/rust-lang/rfcs/pull/0000)
- Rust Issue: [rust-lang/rust#0000](https://github.com/rust-lang/rust/issues/0000)

# Summary
[summary]: #summary

This RFC gives specific layout and ABI guarantees when wrapping "non-zero" data types from `core` in `Option` or `Result`. This allows those data types to be used directly in FFI, in place of the primitive form of the data (eg: `Result<(), NonZeroI32>` instead of `i32`).

# Motivation
[motivation]: #motivation

Rust often needs to interact with foreign code. However, foreign function type signatures don't normally support any of Rust's rich type system. Particular function inputs and outputs will simply use 0 (or null) as a sentinel value and the programmer has to remember when that's happening.

Though it's common for "raw bindings" crates to also have "high level wrapper" crates that go with them (eg: `windows-sys`/`windows`, or `sdl2-sys`/`sdl2`, etc), someone still has to write those wrapper crates which use the foreign functions directly. Allowing Rust programmers to use more detailed types with foreign functions makes their work easier.

# Guide-level explanation
[guide-level-explanation]: #guide-level-explanation

I'm not sure how to write a "guide" portion of this that's any simpler than the "reference" portion, which is already quite short.

# Reference-level explanation
[reference-level-explanation]: #reference-level-explanation

When either of these two `core` types:

* `Option<T>`
* `Result<T, E>` where either `T` or `E`:
* Are a zero-sized type with alignment 1 (a "1-ZST").
* Either have no fields (eg: `()` or `struct Foo;`) or have `repr(transparent)` if there are fields.
* Do not have the `#[non_exhaustive]` attribute.
Copy link
Member

@RalfJung RalfJung Mar 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  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) accepts too many types as ZST (repr_transparent_non_zst_fields, formerly repr_transparent_external_private_fields) rust#78586, I would have expected "has no private field" (which is then trivially true for fieldless types)

Copy link
Contributor Author

@Lokathor Lokathor Mar 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  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?

Copy link

@afetisov afetisov Mar 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@Lokathor Lokathor Mar 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

@afetisov afetisov Mar 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@Lokathor Lokathor Mar 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@programmerjake programmerjake Mar 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@nbdd0121 nbdd0121 Mar 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@workingjubilee workingjubilee Mar 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


Is combined with a non-zero or non-null type (see the chart), the combination has the same layout (size and alignment) and the same ABI as the primitive form of the data.

| Example combined Type | Primitive Type |
|:-|:-|
| `Result<NonNull<T>, ()>` | `*mut T` |
| `Result<&T, ()>` | `&T` |
| `Result<&mut T, ()>` | `&mut T` |
| `Result<fn(), ()>` | `fn()` |
| `Result<NonZeroI8, ()>` | `i8` |
| `Result<NonZeroI16, ()>` | `i16` |
| `Result<NonZeroI32, ()>` | `i32` |
| `Result<NonZeroI64, ()>` | `i64` |
| `Result<NonZeroI128, ()>` | `i128` |
| `Result<NonZeroIsize, ()>` | `isize` |
| `Result<NonZeroU8, ()>` | `u8` |
| `Result<NonZeroU16, ()>` | `u16` |
| `Result<NonZeroU32, ()>` | `u32` |
| `Result<NonZeroU64, ()>` | `u64` |
| `Result<NonZeroU128, ()>` | `u128` |
| `Result<NonZeroUsize, ()>` | `usize` |

* While `fn()` is listed just once in the above table, this rule applies to all `fn` types (regardless of ABI, arguments, and return type).

For simplicity the table listing only uses `Result<_, ()>`, but swapping the `T` and `E` types, or using `Option<T>` is also valid.
What changes are the implied semantics:
* `Result<NonZeroI32, ()>` is "a non-zero success value"
* `Result<(), NonZeroI32>` is "a non-zero error value"
* `Option<NonZeroI32>` is "a non-zero value is present"
* they all pass over FFI as if they were an `i32`.

Which type you should use with a particular FFI function signature still depends on the function.
Rust can't solve that part for you.
However, once you've decided on the type you want to use, the compiler's normal type checks can guide you everywhere else in the code.

# Drawbacks
[drawbacks]: #drawbacks

* The compiler has less flexibility with respect to discriminant computation and pattern matching optimizations when a type is niche-optimized.

# Rationale and alternatives
[rationale-and-alternatives]: #rationale-and-alternatives

It's always possible to *not* strengthen the guarantees of the language.

# Prior art
[prior-art]: #prior-art

The compiler already suports `Option` being combined with specific non-zero types, this RFC mostly expands the list of guaranteed support.

# Unresolved questions
[unresolved-questions]: #unresolved-questions

None at this time.

# Future possibilities
[future-possibilities]: #future-possibilities

* This could be expanded to include [ControlFlow](https://doc.rust-lang.org/nightly/core/ops/enum.ControlFlow.html) and [Poll](https://doc.rust-lang.org/nightly/core/task/enum.Poll.html).
* This could be extended to *all* similar enums in the future. However, without a way to opt-in to the special layout and ABI guarantees (eg: a trait or attribute) it becomes yet another semver hazard for library authors. The RFC is deliberately limited in scope to avoid bikesheding.