Finish stabilization of `result_ffi_guarantees` #130628

workingjubilee · 2024-09-20T16:31:05Z

The internal linting has been changed, so all that is left is making sure we stabilize what we want to stabilize.

rustbot · 2024-09-20T16:31:13Z

rustbot has assigned @BoxyUwU.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

workingjubilee · 2024-09-20T16:38:55Z

Also cc @rust-lang/lang for your information, as y'all already FCPed this in rust-lang/rfcs#3391 (comment)

compiler/rustc_feature/src/accepted.rs

Noratrieb · 2024-09-20T18:00:28Z

tests/ui/rfcs/rfc-3391-result-ffi-guarantees.rs

@@ -1,3 +1,4 @@
+//@ build-pass


can you add a test for your own custom result-like type here to ensure that it's not special cased to Result?
additionally, do we have tests that guarantee that, you know, this guarantee actually holds for various examples? would be good to have them :)

The RFC does not actually extend beyond Result, actually.

well then improper_ctypes is implemented incorrectly, because it allows all result-like enums. the comment above the feature declaration also says result-like

I'm happy to write more tests anyways, mind.

well, we'd need to make sure what we actually want to guarantee before writing tests or changing the lint^^

well then improper_ctypes is implemented incorrectly, because it allows all result-like enums. the comment above the feature declaration also says result-like

Hm.

i have been informed and see in the code that it also allows all option-like enums so... yolo?

The feature description should certainly at least be correct here.

The logic about "Option-like types" is ooooold, it exists at least Since Rust 1.5 where it got moved in this PR.

(There's also a "FIXME: This duplicates code in trans" there, which still exists, and which I have no idea if it is still true -- it is unclear to me where that duplicate code would be and what it would do.)

I think the original logic for recognizing Option-like types was added when the lint was originally implemented, in 25f9534. It was just always carried over since then.

So... I think for Result we don't want this, we want to specifically recognize that type. And probably for Option, too, but that's a different PR.

Noratrieb · 2024-09-20T18:22:30Z

https://doc.rust-lang.org/stable/std/primitive.fn.html#abi-compatibility, our "most canonical" place to put ABI docs, isn't updated yet, it only mentions Option<T>

workingjubilee · 2024-09-20T18:38:06Z

Oh, good catch!

ehuss · 2024-09-20T19:14:24Z

@ehuss has raised the concern that the documentation updates are unfinished.

To repeat my answer in the issue, I think if std's Result docs are correct¹, then that should be fine for now. The Reference docs are lagging and work is being done to catch up, but doesn't need to block anything here.

Though it seems like those docs were missing a caveat that the ABI was not guaranteed for FFI, which I would expect this PR to then remove. ↩

workingjubilee · 2024-09-20T20:02:38Z

current TODOs:

I want to write another test for sizedness checking of some types, at least
~~Correct feature description~~
Update std docs for fn(...T) -> R: https://doc.rust-lang.org/stable/std/primitive.fn.html#abi-compatibility
Fix comment
??? stabilization report???

workingjubilee · 2024-09-20T20:03:05Z

@rustbot author

tmandry · 2024-09-20T23:38:50Z

@rfcbot fcp merge

This completes stabilization of RFC 3391, which guarantees the layout of Option<T>, Result<T, ()>, and Result<(), T> where T is a nonzero type like Nonnull, function pointer, etc. to be the same as the underlying type.

These semantics were already documented prematurely without lang FCP in #124870. The effect of this RFC appears to be to stop linting on such types in extern "C" fn declarations as not being FFI safe.

This FCP serves as signoff that we are fine with fully stabilizing the RFC.

@rustbot label I-lang-nominated

rfcbot · 2024-09-20T23:38:52Z

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

No concerns currently listed.

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.

compiler/rustc_lint/src/types.rs

RalfJung · 2024-09-21T07:20:42Z

Update std docs for fn(...T) -> R: https://doc.rust-lang.org/stable/std/primitive.fn.html#abi-compatibility

Ah, I now have a separate PR for that before I saw you had this on your list: #130653.

Feel free to cherry-pick that commit into your PR an close mine.

workingjubilee · 2024-09-21T14:29:23Z

I'm happy-enough to just let that land.

Co-authored-by: Josh Stone <[email protected]>

chorman0773 · 2024-10-19T20:22:38Z

The RFC'd behaviour is PR'd to the reference in rust-lang/reference#1654

workingjubilee · 2024-10-19T21:09:59Z

Update to this, I

dropped a commit: https://github.com/rust-lang/rust/compare/9ad81f66e009f074d687f61fc31a2b4620a2445d..193352d5f8adeb34d34a421352aeff89524efe52
tried to modify the test then rebuild, but bootstrap problems, so was forced to rebase: https://github.com/rust-lang/rust/compare/193352d5f8adeb34d34a421352aeff89524efe52..1f8124255804ca35d4938ff7998277e7b5f8f019
expanded the existing test to cover a TwoVariantEnum type: 6111f84

I have a draft for the actual layout test. This can land as-is and that test can be a followup, or this PR can wait for that.

RalfJung

r=me with comment nit

RalfJung · 2024-10-20T21:04:45Z

tests/ui/rfcs/rfc-3391-result-ffi-guarantees.rs

+    fn result_transparent_struct_t(x: Result<TransparentStruct<num::NonZero<u8>>, ()>);
+    fn result_transparent_enum_t(x: Result<TransparentEnum<num::NonZero<u8>>, ()>);
+    fn result_phantom_t(x: Result<num::NonZero<u8>, std::marker::PhantomData<()>>);
+    fn result_1zst_exhaustive_no_variant_t(x: Result<num::NonZero<u8>, Z>);


Oh, we allow this even if the "unit" variant is an empty enum? I didn't expect that... but I guess an empty enum is a 1-ZST so makes sense. For repr(transparent) it may have been a mistake to treat uninhabited types as 1-ZST but this is not the right place or time to litigate that issue.

I think that we would probably want to accept this code whether or not the other variant is uninhabited, because we allow NonNull<T> where T: Sized in FFI in general. Right? But yes, a topic for discussion elsewhere.

Well that would require arguing that Result<T, !> is kinda like a transparent newtype around T. So far, we don't guarantee that.

But I agree the lint matches the FCP here. We'll have to extend the ABI compat tests (in tests/ui/abi/compatibility.rs and in Miri) to ensure these actually have the ABI we want, but I am fairly sure they do so that can be a follow-up PR. Miri anyway currently hard-codes our Option/Result type and needs to be adjusted for the "Option-like" definition.

oh lol I didn't even realize abi/compatibility.rs exists. nice, that's convenient

tests/ui/rfcs/rfc-3391-result-ffi-guarantees.rs

workingjubilee · 2024-10-21T07:47:58Z

Since I should rewrite the layout test to use the rustc_abi dump attribute anyways:

@bors r=RalfJung

bors · 2024-10-21T07:48:01Z

📌 Commit 7baf066 has been approved by RalfJung

It is now in the queue for this repository.

bors · 2024-10-21T08:38:48Z

⌛ Testing commit 7baf066 with merge 3e33bda...

bors · 2024-10-21T11:09:17Z

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing 3e33bda to master...

rust-timer · 2024-10-21T12:28:14Z

Finished benchmarking commit (3e33bda): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.5%	[-0.5%, -0.5%]	2
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (secondary 3.7%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.7%	[3.7%, 3.7%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

Results (secondary 8.1%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	8.1%	[8.1%, 8.1%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 780.893s -> 782.557s (0.21%)
Artifact size: 333.66 MiB -> 333.66 MiB (0.00%)

…cross update ABI compatibility docs for new option-like rules Documents the rules decided [here](rust-lang#130628 (comment)) for our ABI compatibility rules. Long-term this should be moved to the reference, but for now this is what we got. Cc `@rust-lang/lang` `@rust-lang/opsem`

Rollup merge of rust-lang#132003 - RalfJung:abi-compat-docs, r=traviscross update ABI compatibility docs for new option-like rules Documents the rules decided [here](rust-lang#130628 (comment)) for our ABI compatibility rules. Long-term this should be moved to the reference, but for now this is what we got. Cc `@rust-lang/lang` `@rust-lang/opsem`

rustbot assigned BoxyUwU Sep 20, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 20, 2024

workingjubilee mentioned this pull request Sep 20, 2024

Tracking Issue for result_ffi_guarantees (RFC 3391) #110503

Closed

7 tasks

cuviper reviewed Sep 20, 2024

View reviewed changes

compiler/rustc_feature/src/accepted.rs Outdated Show resolved Hide resolved

workingjubilee added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 20, 2024

This comment was marked as resolved.

Sign in to view

Noratrieb reviewed Sep 20, 2024

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Sep 20, 2024

tmandry added T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 20, 2024

rustbot added the I-lang-nominated Nominated for discussion during a lang team meeting. label Sep 20, 2024

rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Sep 20, 2024

RalfJung reviewed Sep 21, 2024

View reviewed changes

compiler/rustc_lint/src/types.rs Show resolved Hide resolved

RalfJung reviewed Sep 21, 2024

View reviewed changes

compiler/rustc_lint/src/types.rs Show resolved Hide resolved

traviscross mentioned this pull request Sep 21, 2024

ABI compatibility: mention Result guarantee #130653

Merged

workingjubilee and others added 2 commits October 19, 2024 13:01

result_ffi_guarantees stabilizes in $CURRENT_RUSTC_VERSION

62b9d4a

Co-authored-by: Josh Stone <[email protected]>

compiler: Remove outdated comment

1f81242

workingjubilee force-pushed the clean-up-result-ffi-guarantees branch from 193352d to 1f81242 Compare October 19, 2024 20:04

This comment has been minimized.

Sign in to view

workingjubilee force-pushed the clean-up-result-ffi-guarantees branch from e8fa36c to 6111f84 Compare October 19, 2024 21:00

RalfJung reviewed Oct 20, 2024

View reviewed changes

lang: Strengthen RFC 3391 guarantees to match T-lang consensus

7baf066

workingjubilee force-pushed the clean-up-result-ffi-guarantees branch 2 times, most recently from 830ff7a to 7baf066 Compare October 21, 2024 07:45

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 21, 2024

BoxyUwU assigned RalfJung and unassigned BoxyUwU Oct 21, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 21, 2024

bors merged commit 3e33bda into rust-lang:master Oct 21, 2024
7 checks passed

rustbot added this to the 1.84.0 milestone Oct 21, 2024

This was referenced Oct 21, 2024

abi/compatibility: also test Option-like types #132002

Open

ABI checks do not accept Option-"like" types rust-lang/miri#3983

Open

update ABI compatibility docs for new option-like rules #132003

Merged

chorman0773 mentioned this pull request Oct 21, 2024

Spec abi chapter rust-lang/reference#1545

Open

workingjubilee deleted the clean-up-result-ffi-guarantees branch October 21, 2024 21:16

Finish stabilization of result_ffi_guarantees #130628

Finish stabilization of result_ffi_guarantees #130628

Conversation

workingjubilee commented Sep 20, 2024 • edited Loading

rustbot commented Sep 20, 2024

workingjubilee commented Sep 20, 2024 • edited Loading

This comment was marked as resolved.

Noratrieb Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Noratrieb Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Noratrieb commented Sep 20, 2024

workingjubilee commented Sep 20, 2024

ehuss commented Sep 20, 2024

Footnotes

workingjubilee commented Sep 20, 2024 • edited Loading

workingjubilee commented Sep 20, 2024

tmandry commented Sep 20, 2024

rfcbot commented Sep 20, 2024 • edited Loading

RalfJung commented Sep 21, 2024

workingjubilee commented Sep 21, 2024

chorman0773 commented Oct 19, 2024

This comment has been minimized.

workingjubilee commented Oct 19, 2024 • edited Loading

RalfJung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

workingjubilee Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

RalfJung Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

workingjubilee commented Oct 21, 2024

bors commented Oct 21, 2024

bors commented Oct 21, 2024

bors commented Oct 21, 2024

rust-timer commented Oct 21, 2024

Overall result: ✅ improvements - no action needed

Finish stabilization of `result_ffi_guarantees` #130628

Finish stabilization of `result_ffi_guarantees` #130628

workingjubilee commented Sep 20, 2024 •

edited

Loading

workingjubilee commented Sep 20, 2024 •

edited

Loading

Noratrieb Sep 20, 2024 •

edited

Loading

Noratrieb Sep 20, 2024 •

edited

Loading

workingjubilee commented Sep 20, 2024 •

edited

Loading

rfcbot commented Sep 20, 2024 •

edited

Loading

workingjubilee commented Oct 19, 2024 •

edited

Loading

workingjubilee Oct 21, 2024 •

edited

Loading

RalfJung Oct 21, 2024 •

edited

Loading