Union initialization and Drop by RalfJung · Pull Request #2514 · rust-lang/rfcs (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation47 Commits12 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
hanna-kruppe, scottmcm, joshtriplett, burdges, eddyb, gnzlbg, comex, JonSeverinsson, SimonSapin, and sfleischman105 reacted with thumbs up emoji cramertj, eddyb, aturon, and haaami01 reacted with heart emoji
| // We can write into uninitialized inner fields: |
| u.f2.1 = S(42); |
| let _ = &u.f2.1; // This field is initialized now. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let _ is a bit of a footgun in terms of testing initialization._ pattern doesn't access the right side at all, so even something like this
successfully compiles.
You need let _y = x; or just x; to test for x being initialized.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops thanks!
EDIT: And fixed.
| let _ = &u.f2.0; |
|---|
| // Equivalently, we can assign the entire union: |
| u = U { f2: S(42) }; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't type-check; f2 has type (S, S).
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I also aligned the unions in the two examples.
Relevant to the language team, which will review and decide on the RFC.
label
| # Summary |
|---|
| [summary]: #summary |
| Unions do not allow fields of types that require drop glue, but they may still |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: "drop glue" is such a rustc-specific jargon, perhaps "trivial destructor drop", or "drop not running any code", or something like this could be better.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think "(doesn't) need(s) drop" or "noop drop" could also work as terms.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we have a term for "the code that runs when something is dropped"? I feel that is not such a strange rustc-specific concept, all Rust implementations and also e.g. C++ have it.
Personally, I find "noop drop" much less clear than "does not have drop glue".^^
| (which this RFC adapts from @petrochenkov's proposal) can sometimes be a little |
|---|
| surprising when looking at individual fields: Whether `u.f2 = ...;` drops |
| depends on whether `u.f1` has been previously initialized. @petrochenkov hence |
| proposes a lint to warn people that unions with drop-glue fields are not always |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lint was inherited from the original RFC rather than proposed by me :)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sorry. Fixed :)
| the rules that are currently only applied to unions that `impl Drop`. However, |
|---|
| that does not actually help with the pitfall described above. The more complex |
| rules allow more code that many will reasonably expect to work, and do not seem |
| to introduce any additional pitfalls. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Independent nested fields borrowed independently is probably the most natural expectation:
union U { a: (u8, u8), b: u16 }
let x = &u.a.0; let y = &u.a.1;
so it's borrow checker that needs to work in per-field fashion first of all.
Move checker just mirrors what borrow checker does (for consistency and also because they share common infrastructure in the compiler).
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I hadn't thought about borrowck here. So with the rules as stated, once a field is (partially) borrowed, its siblings all become completely blocked from borrowing?
Borrowing is unsafe, so this would not be necessary, but it still seems useful.
LGTM.
It would be nice to see some practical code ported from Drop fields to ManuallyDrop though to estimate impact on ergonomics and readability.
This RFC doesn't seem to prevent reintroducing Drop fields in uncertain future if the need arises, so we don't lose anything by adopting it.
The RFC summary should probably link to something that explains what a drop glue is.
(The Drop chapter of the Rust book doesn't contain the word "glue")
It would be nice to see some practical code ported from Drop fields to ManuallyDrop though to estimate impact on ergonomics and readability.
What would be a good example?
This RFC doesn't seem to prevent reintroducing Drop fields in uncertain future if the need arises, so we don't lose anything by adopting it.
Yes. (I avoided calling them "Drop fields" because a field can need drop glue even if it does not impl Drop -- e.g. if it is a struct and one of its fields has impl Drop.)
The RFC summary should probably link to something that explains what a drop glue is.
I added a brief explanation.
@rfcbot merge
Based on discussion in the lang team and with Ralf, I think this is ready to merge. Fields in unions that impl Drop have never been part of stable Rust, so this isn't a breaking change, and this fixes various concerns and painful corner cases that kept coming up.
| } |
|---|
| { |
| let u = U { f1: ManuallyDrop::new(Vec::new()) }; |
| foo(u.f2); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like moving out of a union that implements Drop-- how is this different from the let v = u.f1; case above?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, I will fix the example.
The only way to deinitialize a union with drop is to move away the entire thing, just like with structs.
| This requires `unsafe` because it desugars to `ManuallyDrop::deref_mut(&mut u.f).0`, |
|---|
| and while writing to a union field is safe, taking a reference is not. |
| For this reason, `DerefMut` auto-deref is not applied when working on a union or |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why ban just auto-deref rather than banning DerefMut entirely? Users could access the nested types using .as_ptr() or .as_mut_ptr() methods.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I felt that was too drastic, but sure.
Probably calling deref_mut manually is also still okay, just the sugar that lets you use * is not?
I also do not know what in technically feasible in this space.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm not sure what the right balance is. You want to allow the deref when the value has been initialized, but ban it as much as possible in cases where it creates an &mut T to a partially or wholly uninitialized T.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uh, I think I'd rather avoid such stateful lints. They are not complete enough IMHO (they cannot know which field you are now allowed to create a reference to).
@rfcbot concern pnkfelix-wants-a-chance-to-read-this-before-he-checks-his-box
I've been swamped, but this is an area I sort of care about and I want to read this
Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised.
label
🔔 This is now entering its final comment period, as per the review above. 🔔
| // Rejected |
| union Example4 { |
| // `T` might have drop glue, and then `Cell` would as well. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this mention RefCell<T>?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why that? I didn't intend to list every type with interior mutability.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specifically because the line below is f1: RefCell<T>. I assumed this comment was here to explain why, given that line, this struct is rejected.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I made a typo and didn't use the same type on both sides. Thanks for pointing that out! (GitHub's diff viw for comments embedded in the discussions is so bad...)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem - sorry I didn't point it out better initially! I'm still not entirely used to how github displays these things either.
@crlf0710 Thanks for pointing that out! I fixed the text. Do you agree it is correct now?
Would this auto trait be an accurate expression of the “has no drop glue” concept?
pub auto trait NoDropGlue {} impl !NoDropGlue for T where T: Drop {}
If it is and we make it a lang item (in core::marker), generic unions could be extended to allow fields that are not necessarily Copy:
union Foo<T: NoDropGlue> { f: T, }
I think it would begin to express that concept if it worked, but AFAIK those kind of negative bounds don't work...at least I recall that was the result of someone checking last time.
We'd however also need
impl NoDropGlue for ManuallyDrop
and then what about unions themselves? Do they get auto traits the same way everything else does, or are auto traits just never implemented for them? Either way we'd want NoDropGlue implemented for all unions
Good points. So leaving aside the auto-trait definition and assuming it can be implemented with ~compiler magic~ instead, would it be desirable for this trait to exist to enable such generic unions?
I would think so, yes. Copy has always been a bad approximation. IIRC either @eddyb or @nikomatsakis had other uses for such a trait as well.
However, introducing such a trait is firmly out of scope for this PR.
i think the “has no drop glue” approximately corresponds to std::mem::needs_drop which is implemented using a intrinsic. As the documentation says it's conservative, so... maybe it will be a matter of making it accurate and const fn... Maybe it's also a good idea to lift it to a trait(that's a common problem for all const fns, i think).
The final comment period, with a disposition to merge, as per the review above, is now complete.
