Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
MaybeDangling #3336
MaybeDangling #3336
Changes from 11 commits
f0b49f0
d28f052
baf3d9c
c147c8a
982b51d
3c7735a
f1064b0
53d8d84
481f2ca
f5c12b3
4d23931
8f5f5bf
2077313
572f23b
c5a4988
64bf786
d6bfbe7
be7581c
ee08c28
d04f509
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: what about transitive references? e.g. in
MaybeDangling<Box<Vec<T>>>
, is the inner vec also allowed to break aliasing guarantees?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only sensible interpretation is that it also applies to transitive references. If the box itself isn't necessarily a valid pointer then how could we even talk about the stuff it points to?
An alternative explanation of this behavior is that
MaybeDangling<T>
"truncates" the validity invariant ofT
to only the portion that does not involve the state of memory - only the bytes withinT
itself are relevant. So clearly memory pointing to more memory is also irrelevant.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree that that's the only way this feature is useful, but I'm not familiar enough with e.g. miri to know if that's easily implementable. Probably.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Miri (and Stacked Borrows more generally) already does not care about the memory contents of a reference beyond the fact that it exists. Under the Stacked Borrows model,
MaybeDangling
only impacts theBox
inMaybeDangling<Box<Vec<T>>>
, but theVec
doesn't have any validity requirements enforced on it from passing around theBox
anyway.It's also completely impossible for the
Box
to not have a validity requirement to point to something real and also have a requirement on what theBox
points to. If the pointed to thing mayn't exist, it can't be required to be well formed.So
MaybeDangling
does remove all indirected-memory-based validity requirements of the type it wraps, including any transitive requirements that may exist. None do under Stacked Borrows, but could theoretically exist.MaybeDangling
as currently specified by the RFC does not change safety invariants, as it implements safeDeref
conversion likeManuallyDrop
does. I argue that this is likely undesirable, andMaybeDangling
should instead make itunsafe
to reactivate the wrapped type and its invariants.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth explicitly mentioning this interaction in the RFC so it's clear that nested allocations in self-ref types will still work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The RFC already says this:
That was meant to clarify exactly this point. Happy for suggestions for how to make this more clear!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, hm, that makes sense. And there are examples of self-ref stuff as well. Perhaps we should mention nested pointers in the examples too? I think that's why I missed it.
While RFCs aren't documentation, in general I think good unsafe abstraction docs:
i think that's been one of the biggest problems with the Pin docs (to be fixed by rust-lang/rust#88500, but that's stale now) because it's incredibly unclear where the actual compiler safety magic lies, if any.
Not a big deal though, we can iterate on this whilst writing docs, and I'm happy to be involved then. Thanks for the clarifications!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on my current understanding of ref-to-never, this can change to "
MaybeDangling<&!>
has an always-violated safety invariant"There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
&!
has an impossible validity invariant according to the Reference, Miri, and MiniRust. So I don't think the text should change.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is it possible to have both (1)
MaybeDangling<P>
andP
are safely convertible and (2)P
holds more guarantees thanMaybeDangling<P>
? If they are convertible, one could safely pass sayMaybeDangling<&'a mut T>
to afn(&'a mut T, ...)
which itself is compiled with noalias assumption, and result in undefined behaviors. IfP -> MaybeDangling<P>
is safe andMaybeDangling<P> -> P
is unsafe, then what's the difference betweenMaybeDangling
andMaybeUninit
?I also disagree with the disagreement by @RalfJung 's #3336 (comment):
No. It's intrinsically unsafe to have
fn(SomeType<'some_life>)
where'some_life
may ends in the middle of the function invocation depending on its logic. In that case (example 2 of this RFC), the validity is controlled by the runtime logic and is impossible to check at compile time (eg. cannot prevent you from "accidentally" swappingclosure();
andcontrol.signal_done();
), thus guarantees should be made by programmer,unsafe
is required, with or withoutMaybeDangling
. Then I don't really see the advantage of introducing anotherMaybeDangling
thanMaybeUninit
.The dereferenceability of
ManuallyDrop
in example 1, I think, is a separate issue. It's a choice between UB on state observed and UB on state exists.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are validity invariants and safety invariants. The safety invariant of
P
andMaybeDangling<P>
are identical, but the validity invariants are not.MaybeUninit
is a terrible solution for example 2 since one has to calldrop
manually. That's fragile and error-prone. I think it is important that we have a way to not lose automatic drop but still pass around "opaque arbitrary data" without making extra claims about its memory validity.Once we have implicit
drop
, there's no reason to makeinto_inner
unsafe, sincedrop
will already implicitly convert to the inner type without unsafe code.