reference icon indicating copy to clipboard operation
reference copied to clipboard

Specify guarantees for repr(rust) structs

Open Darksonn opened this issue 2 years ago • 10 comments

Closes: #1151

Darksonn avatar Jan 31 '22 13:01 Darksonn

I'm not sure who is to review this PR, but as the author of #1151, I wanted to make explicit that IMO, merging this PR would completely resolve this issue.

My thanks to the author of the PR.

dureuill avatar Feb 07 '22 16:02 dureuill

Nominating for t-lang. These seem like reasonable guarantees to add to me, and want to get your input if you agree or have comments.

The only open question I had was would it help to define the layout of struct Foo() to be the same as ().

ehuss avatar Feb 15 '22 02:02 ehuss

The only open question I had was would it help to define the layout of struct Foo() to be the same as ().

This would be very helpful. E.g., in zerocopy, we want to implement FromBytes for ZSTs like Foo(), but currently aren't convinced that it's sound. If structs like Foo() had the same layout as (), we'd be sure that it is sound.

joshlf avatar Feb 16 '22 18:02 joshlf

@rfcbot merge

joshtriplett avatar Mar 22 '22 17:03 joshtriplett

@Darksonn have you seen this write-up here?

https://rust-lang.github.io/unsafe-code-guidelines/layout/structs-and-tuples.html

I didn't do a detailed comparison, but I would like to ensure they are compatible as that text had a fair bit of thought put into it :)

nikomatsakis avatar Mar 22 '22 17:03 nikomatsakis

No, I haven't looked at that one in detail either.

Darksonn avatar Mar 22 '22 17:03 Darksonn

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

  • [x] @cramertj
  • [x] @joshtriplett
  • [ ] @nikomatsakis
  • [ ] @pnkfelix
  • [x] @scottmcm

Concerns:

  • ~~tuple-layout~~ resolved by https://github.com/rust-lang/reference/pull/1152#issuecomment-1233226776

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!

See this document for info about what commands tagged team members can give me.

rfcbot avatar Mar 22 '22 18:03 rfcbot

@cramertj It looks like your concern didn't get registered; can you try again in a top-level comment?

joshtriplett avatar May 03 '22 17:05 joshtriplett

@rfcbot concern tuple-layout

See https://github.com/rust-lang/reference/pull/1152#discussion_r832497012

cramertj avatar May 17 '22 17:05 cramertj

What do I need to do to get this PR finished? I believe that the tuple-layout concern is resolved per the conversation above.

Darksonn avatar Jul 25 '22 12:07 Darksonn

@rfcbot resolve tuple-layout

I think the intention is to clarify here that tuple layout and struct layout are not guaranteed to be the same. With that in mind, the rest of this looks great to me! Sorry for forgetting this thread for ages :(

cramertj avatar Aug 31 '22 17:08 cramertj

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

psst @joshtriplett, I wasn't able to add the final-comment-period label, please do so.

rfcbot avatar Aug 31 '22 18:08 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.

psst @joshtriplett, I wasn't able to add the finished-final-comment-period label, please do so.

rfcbot avatar Sep 10 '22 18:09 rfcbot