Skip to content

Resolve layout of single variant enums #159

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.

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 6 commits into from
Aug 27, 2019

Conversation

gnzlbg
Copy link
Contributor

@gnzlbg gnzlbg commented Jul 8, 2019

Addresses part of #79. Addresses part of #37 .

I don't see any reasons not to guarantee this, and AFAICT nobody has mentioned any either.


EDIT: Note that this doesn't say anything about the layout of enums without variants, or where all variants are uninhabited. That should better be tackled in a different PR.

@gnzlbg gnzlbg force-pushed the resolve_enum_layout branch from 82e66dc to 0ecb9d2 Compare July 8, 2019 13:20
@danielhenrymantilla
Copy link
Contributor

What about #[non_exhaustive] enums?

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Jul 8, 2019

AFAICT #[non_exhaustive] does not interact with layout.

@hanna-kruppe
Copy link

hanna-kruppe commented Jul 8, 2019

To elaborate: users of non-exhaustive enums can't rely on the number of variants not changing under their feet, so for their purposes this part of the UCG would not guarantee anything about the layout of the non-exhaustive enum, since it is not guaranteed to meet the precondition (only one variant). Similarly, most guarantees about the layout of structs automatically wouldn't apply to non-exhaustive structs since fields might be added to them.

(Within the same crate/module that defines the non-exhaustive enum, it would technically be allowed, though I guess it's a bit fragile to rely on it when you intend to expend the set of variants later.)

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Jul 8, 2019

@rkruppe

I don't know what value would we add by not allowing such code to rely on the #[non_exhaustive] enums layout; AFAICT there aren't any extra optimizations that this would allow.

So I think I disagree (you kind of hint things both ways) ? I think that the layout guarantees specified here apply to both #[non_exhaustive] and "exhaustive" enums because #[non_exhaustive] does not affect layout and unsafe code is allowed to rely on layout guarantees.

Whether that unsafe code is safe or not would be a different issue - e.g. if the layout changes and the unsafe code is not updated, then it would probably be incorrect, but this is true for all unsafe code. Still, unsafe code written in the same module as a #[non_exhaustive] enum can make use of layout guarantees and be safe.

@hanna-kruppe
Copy link

I didn't mean to imply layout computation or what reliance on layout is considered UB by the Rust language should be affected by non_exhaustive. I was just speaking of contracts between libraries: an external library that assumes for soundness that a non_exhaustive struct or enum has a certain layout (based on information that is strictly ) is just as wrong as if it relies on private fields or other information not part of the API. As we both noted, code within the same API boundary as the non-exhaustive type is not subject to the same restrictions.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Jul 8, 2019

@rkruppe

I was just speaking of contracts between libraries: an external library that assumes for soundness that a non_exhaustive struct or enum has a certain layout (based on information that is strictly ) is just as wrong as if it relies on private fields or other information not part of the API.

Such a library is ugly, brittle, non-portable (to different library versions / toolchain), etc. but it is only wrong if its assumptions are incorrect.

If a Rust library exposes a #[non_exhaustive] #[repr(C)] enum, and one wants to interface with that library from C, then one can make it work, e.g., by just fixing the exact version of the dependency and toolchain and manually inspecting the variants of the enum.

Ideally, nobody would ever need to do this, and such APIs are obviously bad, but doing this types of things is the bread and butter of, for example, the libc crate.

I don't think we can do much about this either. Making a type public leaks its layout, and while features like #[non_exhaustive] protect users from accidental misuse, they can't prevent a motivated user from exploiting layout for profit, nor should that be their goal. The only way I can think of in which we could help, would be to make the rules clearer, so that those who need to exploit API details know where they stand, and maybe give them better tools, so that they don't have to rely on undefined behavior for their hacks.

@RalfJung
Copy link
Member

The file says at the beginning that we try to mark what is RFC'ed and what not. Should these new sections get such comments?

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Jul 13, 2019

Should these new sections get such comments?

They probably should, I'll add that these sections have not gone through the RFC process, although the other sections more explicitly call what has gone through the process.

Before turning the reference or a subset of it into an RFC, we are going to have to do multiple rounds of making everything consistent, and that will include deciding to call out either what has been RFC'ed already, what hasn't, or both.

As layout has shown, some of the fundamental definitions evolve over time as we work through newer topics like validity. Each time they change, we kind of have to go through everything.

@RalfJung RalfJung added the A-layout Topic: Related to data structure layout (`#[repr]`) label Aug 14, 2019
@gnzlbg gnzlbg force-pushed the resolve_enum_layout branch from cf9647e to 11b6c4d Compare August 15, 2019 05:21
@gnzlbg
Copy link
Contributor Author

gnzlbg commented Aug 15, 2019

I think all issues have been resolved. Please take a look.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Aug 15, 2019

I've recorded the proposal for multi-variant enums with one inhabited variant in the tracking issue: #79 (comment)

Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

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

r=me with the nit resolved.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Aug 16, 2019

@RalfJung nit resolved

@RalfJung
Copy link
Member

@rkruppe was the merge here fine with you? (Your last comment in this PR was quite a while ago.)

@hanna-kruppe
Copy link

Yes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-layout Topic: Related to data structure layout (`#[repr]`)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants