Tracking Issue for result_ffi_guarantees (RFC 3391)

[tmandry]

This is a tracking issue for the RFC "result_ffi_guarantees" (rust-lang/rfcs#3391).

About tracking issues

Tracking issues are used to record the overall progress of implementation.
They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions.
A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature.
Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

Steps

• Implement (test) the RFC
  • Support Result<T, E> across FFI when niche optimization can be used #122253
  • Support Result<T, E> across FFI when niche optimization can be used (v2) #124747
  • Update Result docs to the new guarantees #124870
  • Add more ABI test cases to miri (RFC 3391) #125672
• Adjust documentation (see instructions on rustc-dev-guide)
• Stabilization PR (see instructions on rustc-dev-guide) (not necessary, reference FCP should be all that's needed)

Unresolved Questions

None

Implementation history

[RalfJung]

I did a bunch of work on ABI tests for repr(transparent) that could also be useful for testing this RFC:

• Miri now has this test: https://github.com/rust-lang/miri/blob/master/tests/pass/function_calls/abi_compat.rs
• and rustct will hopefully soon have this: add rustc_abi(assert_eq) to test some guaranteed or at least highly expected ABI compatibility guarantees #115372

[MasterAwesome]

Is anyone still working on this? If not, I'd love to try my shot at it.

[Lokathor]

I have not heard word of anyone working on this in a very long time. It's probably a safe bet to begin a PR.

[RalfJung]

Besides adjusting the improper-ctypes lint (which is happening in #122253), some other things that should be done:

• For Option we have this documentation. I assume we want the same for Result? Is there some way to not repeat the table?
• Miri has a test here that currently only checks Option, not Result.

[MasterAwesome]

Should the documentation change be a separate PR?

[RalfJung]

Yes, that probably makes review easier.

[Lokathor]

The lint update PR has merged.

[RalfJung]

I hope docs are next, then. :) We shouldn't go too long with such a gap between what we document and what we implement.

The Result type itself should get suitable documentation. And probably there's somewhere in the reference as well that needs updating?

[Lokathor]

philosophically speaking I'm not sure how much the reference needs to say in addition to what the standard library type docs say, but yeah docs on Option and Result are the next step. I will likely have time for a docs PR very soon if no one else does it first

[Lokathor]

that's just what @tmandry wrote in the start of the thread next to the checked box.

[RalfJung]

Where is the reference PR they are mentioning there?

[Lokathor]

I don't know, actually. I just saw a checked box and assumed the box wouldn't be checked if it wasn't completed yet.

[tmandry]

I must have been under the impression that this was only a documentation change. It probably needs a lang team signoff since it's more than that.

[workingjubilee]

Hm? Lang FCPed it? rust-lang/rfcs#3391 (comment)

[RalfJung]

Indeed the change already landed and hit stable with Rust 1.80.

The only thing that's missing, AFAIK, is updating the Reference? Or maybe that has been done already but we forgot to close this issue?
Cc @ehuss

[cuviper]

The feature gate is still unstable:

rust/compiler/rustc_feature/src/unstable.rs

Lines 575 to 577 in 1a5a224

	/// Allows enums like Result<T, E> to be used across FFI, if T's niche value can
	/// be used to describe E or vise-versa.
	(unstable, result_ffi_guarantees, "1.80.0", Some(110503)),

That needs to move to accepted in the neighboring module, and might need updates in places that use it too, like:

rust/compiler/rustc_lint/src/types.rs

Lines 1147 to 1149 in 1a5a224

	if !tcx.features().result_ffi_guarantees {
	return None;
	}

[workingjubilee]

I have opened #130628 on the assumption that all necessary documentation changes have landed already. Please let me know if that is incorrect.

[ehuss]

I do not believe this has been documented. I think that will be needed before stabilization should continue.

Also, it would be helpful to update the top comment with links to the implementation, since it's not really clear to me what was actually changed.

[workingjubilee]

I believe it was documented on the Result type. Should we add it to the Reference?

[workingjubilee]

Indeed the Result type was documented accordingly: https://doc.rust-lang.org/std/result/index.html#representation

Unfortunately some very peculiar events happened with CI around the time, so the history is somewhat harder to actually track.

[workingjubilee]

I have lifted all PRs relevant to the implementation history to the top.

[workingjubilee]

@ehuss It seems none of the ABI guarantees of Result or Option have been documented in the Reference that I can see, and I'm not sure where the other ABI guarantees that we offer are in the Reference either. So I think this is a larger concern than the matter of this feature and a separate issue should be opened for it. Type layout doesn't seem even close to exhaustive.

[workingjubilee]

@ehuss I can post a bare-minimum PR to the reference regarding this specific guarantee, but it will mostly serve to make it more obvious that the work is direly unfinished w/r/t documenting ABI in the Reference.

[ehuss]

Thanks @workingjubilee!

I am a bit confused, since I don't fully understand the connection between result_ffi_guarantees versus the already documented behavior in https://doc.rust-lang.org/std/result/index.html#representation. Is that current documentation making a stronger statement than what is actually true today? Or is there something missing?

I am also quite confused about what is being stabilized, or about the process being used. From the timeline I see:

1. RFC: result_ffi_guarantees rfcs#3391 was merged to guarantee certain layouts for Result and Option used in FFI.
2. Support Result<T, E> across FFI when niche optimization can be used (v2) #124747 implemented this for Result for FFI under the result_ffi_guarantees feature flag.
3. Update Result docs to the new guarantees #124870 added documentation for Result, even though it is still behind a feature flag?

Some questions:

• Does result_ffi_guarantees actually change the layout, or is it just related to squelching the improper-ctypes lint?
• Why was the layout guarantee documented before it was stabilized? Is there some difference between repr-Rust (guaranteed on stable) and repr-C (not niche-optimized on stable)?
• Where is the stabilization report, and the t-lang approval of that?
• The RFC mentions Option, but the two PRs above seem to only be for Result. What happened with Option?

Sorry for all the dumb questions, but I am very confused as to what is being proposed.

As for layout guarantees, since the reference currently doesn't cover that, then I think it is fine to skip it for now. We are inching our way forward with rust-lang/reference#1545, and I think this might be included there (cc @chorman0773).

[chorman0773]

As for layout guarantees, since the reference currently doesn't cover that, then I think it is fine to skip it for now. We are inching our way forward with rust-lang/reference#1545, and I think this might be included there (cc @chorman0773).

Will probably also want to go into the type-layout chapter as well. It can be added to abi as well once 1545 is merged.

[workingjubilee]

I am a bit confused, since I don't fully understand the connection between result_ffi_guarantees versus the already documented behavior in https://doc.rust-lang.org/std/result/index.html#representation. Is that current documentation making a stronger statement than what is actually true today? Or is there something missing?

That IS the behavior with this feature.

The RFC mentions Option, but the two PRs above seem to only be for Result. What happened with Option?

Option already had such an ABI guarantee documented: https://doc.rust-lang.org/std/option/index.html#representation

Does result_ffi_guarantees actually change the layout, or is it just related to squelching the improper-ctypes lint?

My understanding is that it is not a functional behavioral change. It is a promise that the behavior will not change, and that if it varies from the documented behavior, it will be treated as a bug. To many developers, this is much more important than "how the compiler actually behaves".

Is there some difference between repr-Rust (guaranteed on stable) and repr-C (not niche-optimized on stable)?

I don't understand the question. Result is not a repr(C) type so this is irrelevant?

[workingjubilee]

the RFC is not about:

• Option, which already is in the desired state.
• Other enums, which have no such guarantee.

the RFC is about:

• Result<T, ()> and Result<(), T> where the T formed allows a niche optimization to track the Ok(()) or Err(()) tag, and other ZST types in lieu of (). In practice this means T is actually &T, &mut T, and the other NonWhatever types.

[chorman0773]

FTR, I already have plans to redo the type-layout chapter, particularily the enum section. Option and Result will get their own few clauses in there.

[workingjubilee]

Where is the stabilization report, and the t-lang approval of that?

gonna level with you here, this RFC was basically a stabilization report for a behavior we already implemented. 🙃

Why was the layout guarantee documented before it was stabilized?

I don't really know what to tell you here? This entire RFC arose because T-lang has such a crushingly low bandwidth that people have to use extremely high-volume signals (like RFCs) to get their attention for long. Then they have to make the RFC extremely conservative to avoid it being nitpicked to death. Then T-lang doesn't have sufficient bandwidth to properly shepherd something through its process so that missteps don't happen. So missteps happen.

I expect that can only realistically be solved by T-lang deciding to prioritize onboarding sufficient members in some capacity as to at least have a successorship. Perhaps they are actually doing that? But such would also consume their bandwidth, so either way, something like this was inevitable in some case.

I have no magical power to wave a wand and make them do that if they are not.

[chorman0773]

Doing a stabilization report is probably a good idea nonetheless.

[workingjubilee]

I suppose I will draft one, then! No promises it won't be very silly though.

[ehuss]

I don't understand the question. Result is not a repr(C) type so this is irrelevant?

Oh, sorry, yea, that didn't make sense. My thinking was, do we have a stable guarantee such that let x: Result<NonZeroI32, ()> = unsafe { std::mem::transmute(12) }; is well-defined (all "Rust"-land), but extern "C" { fn f() -> Result<NonZeroI32, ()>; } is not (in "C" ABI land).

gonna level with you here, this RFC was basically a stabilization report for a behavior we already implemented. 🙃

That's what I'm trying to drill down to; is this just a documentation change, or is it a behavior change? If it was a documentation change, then I would not have expected #122253 to exist, unless the only goal there was to squelch improper-ctypes. The description of the PR doesn't really clarify what it does, and the RFC never mentions that lint.

For documentation-only changes, we have generally not required a stabilization report or separate FCP for an accepted RFC. Documentation-only RFCs normally get their doc changes merged almost right away, and there is no tracking issue. This seems slightly more than that, though.

I have no magical power to wave a wand and make them do that if they are not.

100% agree with you¹, and I don't want to put up roadblocks where it's not appropriate. However, I do want to make sure that there is awareness and understanding of what is happening.

Footnotes

1. You might be interested in https://github.com/rust-lang/lang-team/pull/290, which is a proposal to help scale some decision making. ↩

[Lokathor]

@ehuss I don't believe we have any guarantee that something is not defined.

In fact the entire point of the RFC is to make your example cose snippit be defined, as far as i can read it

[workingjubilee]

@ehuss Of course! tbh I'm mostly just playing my occasional role of "clown that pratfalls into thing everyone is confused about, prompting everyone to laugh, cheer, boo, throw tomatoes, or buy popcorn". :^)

[workingjubilee]

For documentation-only changes, we have generally not required a stabilization report or separate FCP for an accepted RFC. Documentation-only RFCs normally get their doc changes merged almost right away, and there is no tracking issue. This seems slightly more than that, though.

...speaking of documentation, is this written anywhere? I mean oral tradition is well and dandy but this actually seems worth formalizing because of the whole "T-lang has a tiny amount of bandwidth" 🫠 so it's good to note down when something only hard-requires one Formal Team Decision instead of two Formal Team Decisions.

I do think a tracking issue sometimes is warranted simply because "wait how many places do we have to update the documentation exactly...?" sometimes uhh needs a Formal Issue.

And there technically were related compiler changes in this re: the lint. I won't start an argument about whether lints are metaphysically also documentation, tho'. :^)

[workingjubilee]

And yeah, with this feature, these are now both correct:

let x: Result<NonZeroI32, ()> = unsafe { std::mem::transmute(12) };
extern "C" {
    fn f() -> Result<NonZeroI32, ()>;
}

The latter even should, IIRC, be correct under the "integer normalization" CFI flag (which reduces entropy but enables certain ABI-compatible equivalencies). This is because the equivalence is sorta defined in terms of transmute:

If the declared signature and the signature of the function pointer are ABI-compatible, then the function call behaves as if every argument was transmuted from the type in the function pointer to the type at the function declaration, and the return value is transmuted from the type in the declaration to the type in the pointer. All the usual caveats and concerns around transmutation apply; for instance, if the function expects a NonZero<i32> and the function pointer uses the ABI-compatible type Option<NonZero<i32>>, and the value used for the argument is None, then this call is Undefined Behavior since transmuting None::<NonZero<i32>> to NonZero<i32> violates the non-zero requirement.

[workingjubilee]

I have opened rust-lang/reference#1623 to track "documenting a complete explanation of our ABI rules, also what even is an ABI, huh? what?"

[RalfJung]

The entire RFC was basically "just" a docs change. I didn't know there is a feature gate, all it can be doing is affecting the lint as feature gates affecting layout would be unsound so I hope we don't do it. ;) So if this is anyway not in the reference, no update is needed there. Which means I think all that is left is stabilizing the feature - after we confirm what exactly it does.^^