PhantomData: fix documentation wrt interaction with dropck #103413
Conversation
|
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
|
r? @scottmcm (rust-highfive has picked a reviewer for you, use r? to override) |
|
Looks like this was brought up more than 2 years ago already, in #70841. @pnkfelix pointed at the problematic docs paragraph that this PR is changing, and also raised the concern that
|
|
It seems that the use std::marker::PhantomData;
struct Foo<T>(Box<i32>, PhantomData<T>);
struct Bar<T>(T);
impl<T> Drop for Bar<T> {
fn drop(&mut self) {}
}
fn main() {
let _foo;
let s = String::from("Hello, world!");
_foo = helper(&*s);
}
fn helper<'a>(_: &'a str) -> Foo<Bar<&'a str>> {
Foo(Box::new(1), PhantomData)
}Without |
|
our proposed MCP for dropck bounds might be useful to explain the PhantomData behaviour. tho with our MCP's flexibility, we'd argue this should be changed in the future. so, don't rely on it. (but hey, it is only an experiment so far) |
|
@mu001999 Interesting example... I first thought this was not dropck, but this compiles, indicating that it is indeed dropck. Looks like we need a research project to even figure out our current dropck rules... |
|
I outlined the current drop check rules for myself:
NB: all For the first situation , |
|
From all we've seen, I've been able to gather the following understanding of the dropck behavior. Given a To illustrate, consider Code examplestruct Box<T> {
ptr: *const T,
// … (e.g., should we `PhantomData<T>` or not)
}
impl<T> Drop for Box<T> { /* or:
unsafe impl<#[may_dangle] T> Drop for Box<T> { */
fn drop(&mut self) {
unsafe { ptr::drop_in_place::<T>(self.ptr); } // no-op if `T` has no drop glue (so `T` could dangle).
unsafe { alloc::dealloc(self.ptr, …); } // does not use values of type `T`.
}
}
/* e.g.
impl<'dangling_in_drop> for Box<&'dangling_in_drop str> {
// or
unsafe impl<#[may_dangle] 'dangling_in_drop> for Box<&'dangling_in_drop str> {
// as well as:
impl<'dangling_in_drop> for Box<DisplayOnDrop<&'dangling_in_drop str>> {
// or
unsafe impl<#[may_dangle] 'dangling_in_drop> for Box<DisplayOnDrop<&'dangling_in_drop str>> {
*/
// where
struct DisplayOnDrop<T : Display>(T);
impl<T : Display> Drop for DisplayOnDrop<T> {
fn drop(&mut self) {
println!("{}", self.0);
}
}
fn main() {
let (b1, b2);
{
let dropped_before_the_boxes = String::from("…");
let thus_dangling_in_drop = &*dropped_before_the_boxes;
b1 = Box::new(thus_dangling_in_drop);
b2 = Box::new(DisplayOnDrop(thus_dangling_in_drop));
} // `drop(dropped_before_the_boxes)`
/* `thus_dangling_in_drop` is dangling here! */
} // `drop((b1, b2))`
The practical TL,DR with drop and
But there are other edge cases which may require a more detailed exposition of the rules at play, here:
Footnotes
|
|
nope, ManuallyDrop doesn't have drop glue, while PhantomData does. see also #103318 (comment) |
|
Ah, interesting, let me update the post. It does mean that " |
|
we think it's best to describe these things in terms of bounds. it makes it really intuitive that way. here's hoping we get our MCP through and then we have bounds-based dropck. and then we should no longer need "PhantomData is relevant for Drop" at all |
These rules are outlined very clearly! With these rules, it can be said that In turn, Thus, |
|
This is the “needs drop” what I said above. |
|
I’m still curious about this question, #70841 (comment) If the following struct MyBox<T>(NonNull<T>);
impl<T> Drop for MyBox<T> { … }
IMO, generic params in
I think there’s a subtle difference between 1 and 3.(maybe 3 is weaker than 1, I’m not sure). We usually mark |
|
@TOETOE55T From my understanding, |
|
I just glanced at NLL's RFC, layer-3-accommodating-dropck section https://rust-lang.github.io/rfcs/2094-nll.html#layer-3-accommodating-dropck
But the status quo still do the dropck on variables are dropped: https://play.rust-lang.org/?version=stable&mode=release&edition=2021&gist=936fe411cc2d2eb720223c0fdc74f8f2 use std::marker::PhantomData;
struct Foo<T>(Box<i32>, PhantomData<T>);
struct Bar<T>(T);
impl<T> Drop for Bar<T> {
fn drop(&mut self) {}
}
fn main() {
let _foo;
let s = String::from("Hello, world!");
_foo = helper(&*s);
std::mem::drop(_foo); // or `drop(_foo.0)
}
fn helper<'a>(_: &'a str) -> Foo<Bar<&'a str>> {
Foo(Box::new(1), PhantomData)
} |
|
I think the correct rules are:
Another way to put it:
But I've not run any experiments, so maybe I'm missing something. Have to pull back in the examples we tried last time. Per these rules:
If a struct actually implements Drop, then it is always assumed to drop all the things (modulo the unstable feature). So if there is a |
|
we should probably have an RFC for what to do here. |
|
We don't need an RFC to document the existing analysis. We just need an RFC to change it.
|
|
nod we should probably have an RFC to remove the existing spooky action at a distance instead of documenting it. |
|
@rfcbot concern resolve how we handle @rfcbot concern want to take another look at rust-lang/rfcs#3390 whether it changes anything about my reasoning here |
|
@rfcbot resolved want to take another look at rust-lang/rfcs#3390 whether it changes anything about my reasoning here rust-lang/rfcs#3390 (comment) still summarizes my feelings about this PR even after feeling like I now have a good understanding of dropck: "Spooky-dropck-at-a-distance" does not feel spooky anymore. It just feels unfortunate. Even if accepted, to my knowledge the RFC is not able to resolve the "having outlives constraints" is necessary for the sound usage of We could change I guess a potential alternative is to make @SoniEx2, would you be available for a sync meeting about this topic in general? @rustbot concern potential way to remove "Spooky-dropck-at-a-distance" @rustbot resolve resolve how we handle [T; 0] wrt to dropck opened #110288 |
this is effectively what rust-lang/rfcs#3390 proposes - tho it tries to do so in a much more formalized way, basically defining some syntax and semantics and a whole system around it, as well as trying to explain how the current and desired behaviours fit into the proposed model (in other words: in a rather roundabout way). but for stable code, yeah, the observable result would be the same. maybe we should've leaned harder on spooky-dropck-at-a-distance as motivation, given that it (and being opposed to this very pull request) is what led us to write the RFC in the first place (indeed, the zulip thread is literally called "deprecating spooky-dropck-at-a-distance")... but anyway! we can try a sync meeting? uh we've never really done of those tho? so we're not sure what to expect or how to prepare for it? >.< |
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
wouldn't have needed to prepare anything for it 😀 mostly just wanted to have a quicker conversation because having conversations via github is a horrible experience in my opinion. going to instead use the existing zulip thread. https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/deprecating.20spooky-dropck-at-a-distance |
|
There's an argument in rust-lang/rfcs#3417 that we shouldn't stably document this behavior, i.e. we shouldn't land this PR, because we want to change this behavior anyway. I guess alternatively we could add a preface saying that this is not guaranteed? That makes one wonder about the point of the docs though. Either way I guess I won't spend time on this PR until that discussion is resolved. Seems like a waste of time if we're not going to document this anyway. |
There was a problem hiding this comment.
we finally got to this issue in t-types triage: https://rust-lang.zulipchat.com/#narrow/stream/326132-t-types.2Fmeetings/topic/2023-05-08.20triage.20meeting/near/356728590
we intend to have a deep dive about the dropck rules and how to change them (rust-lang/types-team#92), but are open (some of us were in favor) of merging this documentation until then with the expectation that this does not block us from changing the subtle details of dropck later.
lgtm on the current documentation, but please add a note that this is not a stability guarantee. after that r=me
library/core/src/ops/drop.rs
Outdated
| /// The Nomicon discusses the need for [drop check in more detail][drop check]. | ||
| /// | ||
| /// To reject such code, the "drop check" analysis determines which types and lifetimes need to | ||
| /// still be live when `T` gets dropped: |
There was a problem hiding this comment.
please state somewhere that these exact rules may change in the future.
Co-authored-by: lcnr <rust@lcnr.de>
|
I made it clear that these rules are not stably guaranteed. Looks like we can finally land this. Thanks a lot to everyone involved! |
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (9850584): comparison URL. Overall result: ❌ regressions - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis 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.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 659.199s -> 659.629s (0.07%) |
As far as I could find out, the
PhantomData-dropck interaction only affects code usingmay_dangle. The documentation in the standard library has not been updated for 8 years and thus stems from a time when Rust still used "parametric dropck", before RFC 1238. Back then what the docs said was correct, but withmay_dangledropck it stopped being entirely accurate and these days, with NLL, it is actively misleading.Fixes #102810
Fixes #70841
Cc @nikomatsakis I hope what I am saying here is right.^^