Remove `ptr::Unique` #71530

SimonSapin · 2020-04-24T20:08:33Z

#71507 (comment) discusses whether Unique not actually being unique is UB.

Unique has very limited use over NonNull, so it doesn’t seem worth spending a lot of effort defining an abstraction and coming up with rules about what uses of it are or aren’t sound.

rust-highfive · 2020-04-24T20:08:37Z

r? @hanna-kruppe

(rust_highfive has picked a reviewer for you, use r? to override)

SimonSapin · 2020-04-24T20:08:46Z

@rust-lang/libs What do you think?

rust-lang#71507 (comment) discusses whether `Unique` not actually being unique is UB. `Unique` has very limited use over `NonNull`, so it doesn’t seem worth spending a lot of effort defining an abstraction and coming up with rules about what uses of it are or aren’t sound.

Amanieu · 2020-04-24T22:34:27Z

I've found that Unique is actually very hard to use correctly in practice and a lot of unsafe code actually violates its aliasing requirements (which we weren't even enforcing anyways). So I'm OK with with removing it.

SimonSapin · 2020-04-25T08:18:13Z

src/liballoc/boxed.rs

        // Box is kind-of a library type, but recognized as a "unique pointer" by
        // Stacked Borrows.  This function here corresponds to "reborrowing to
        // a raw pointer", but there is no actual reborrow here -- so
        // without some care, the pointer we are returning here still carries
        // the tag of `b`, with `Unique` permission.
        // We round-trip through a mutable reference to avoid that.


@RalfJung Could you say some more about how and why Box is special in Stacked Borrows? Could it be "just" another library type? Are Vec or Rc for example also special?

The how: Box is basically like &mut (except that unlike &mut we never add "protectors" for Box, but that's a detail).

The why: As far as I know, rustc emits noalias annotations for Box. This demonstrates that it is not "just another library type". Those annotations are incorrect unless our aliasing model (i.e., Stacked Borrows) actually treats Box special. Additionally, back when I started with Stacked Borrows, we also added dereferencable annotations. Right now we do not do that any more, but the plan is to start doing that again once LLVM weakened dereferencable semantics to mean "dereferencable on entry to the function" (as opposed to "dereferencable throughout the entire execution of the function", which is what it means now). That would be another way in which Box is not just a library type.

Long-term, the plan is to not make Box special but to make Unique special instead. At least, @Gankra told me that that was the plan. ;) But that is blocked on figuring out better how exactly we want it to be special.

Are Vec or Rc for example also special?

No, right now only Box is special.

Eventually, when Unique is special, that would be inherited by Vec. Vec is, in fact, the main motivation for making Unique special. See e.g. #71354.

RalfJung · 2020-04-25T10:34:44Z

Cc @rust-lang/lang -- not sure what the lang team's long-term plans around Unique are, if any. IIRC the plan was to actually make it mean something eventually. Whether or not keeping Unique around until then is useful, I do not know.

RalfJung · 2020-04-25T10:36:14Z

src/liballoc/boxed.rs

-    pub fn into_unique(b: Box<T>) -> Unique<T> {
-        let b = mem::ManuallyDrop::new(b);
-        let mut unique = b.0;
+        let mut b = mem::ManuallyDrop::new(b);


Looks like this PR conflicts badly with #71168.

I’ll deal with it after one of them lands.

bors · 2020-04-25T21:10:27Z

☔ The latest upstream changes (presumably #71556) made this pull request unmergeable. Please resolve the merge conflicts.

Gankra · 2020-04-25T21:39:22Z

i agree the motivation for Unique is weak, but i also agree with Ralf that removing accompishes ~nothing as long as Box has these special semantics that Unique is supposed to just be a generalization of.

RalfJung · 2020-04-26T12:43:44Z

Instead of removing it, what about removing the From instances from &T and NonNull<T> (which are clearly dangerous, given the aliasing implications), and then seeing where that fails and considering if Unique is appropriate there?

@Amanieu

a lot of unsafe code actually violates its aliasing requirements (which we weren't even enforcing anyways)

Could you give an example or two?

Amanieu · 2020-04-26T12:53:33Z

Could you give an example or two?

Hmm never mind, I seem to be misremembering.

SimonSapin · 2020-04-26T13:01:34Z

What are the aliasing rules for Unique? For example Box<T>::deref returns a &T reference with the same address as the Unique<T> inside Box. Isn’t that aliasing?

Amanieu · 2020-04-26T13:06:22Z

The intention is that it matches the semantics of C restrict pointers. ~~In layman's terms it roughly means that every time you derive a new pointer from the Unique it invalidates all previous pointers that were derived from it.~~

EDIT: Actually disregard that, I need to review the rules for restrict in C.

Amanieu · 2020-04-26T13:10:58Z

Actually I'll just point to the docs for C's restrict: https://en.cppreference.com/w/c/language/restrict

hanna-kruppe · 2020-04-26T13:43:46Z

I don't think the details of C's restrict are likely to be appropriate for Rust as-is. Nowadays we have Stacked Borrows as a concrete proposal for Rust's aliasing rules, so I'd tentatively say Unique should integrate into that model in a way similar to other "unique" pointers like Box<T> and &mut T. As a rule of thumb (with a lot of hand-waving and some omissions), you can derive borrows from such pointers and freely use them until the original pointer is used again, at which point the borrows are invalidated.

But in any case, I expect that whenever we do settle on some aliasing rules for Unique, we'll have to audit all the code using it, manually and with a dynamic checker (which has proven very useful with Stacked Borrows). Most likely there will be a bunch of violations at first, and that will be our chance to see where the rules are broken and how that could be fixed (e.g. by tweaking the rules, tweaking the code, or removing Unique entirely from some code).

That kind of experiment would actually be easier if we left Unique around in the code, so if we do plan to make Unique mean something in the future I'm inclined to leave it be for now.

RalfJung · 2020-04-26T13:46:22Z

What are the aliasing rules for Unique? For example Box::deref returns a &T reference with the same address as the Unique inside Box. Isn’t that aliasing?

In terms of Stacked Borrows, roughly speaking I'd say: like &mut T, but without the "dereferencable" part. Where &mut T is unique exactly for the memory it covers (as determined by the size of T), Unique<T> is unique for the memory that it and its derived pointers are actually used for. But the way Stacked Borrows works, we don't have a good way to say that a pointer is unique without know for which memory it is unique, which is why currently Unique has no special meaning in Stacked Borrows.

SimonSapin · 2020-04-26T14:49:01Z

Alright, thanks for the feedback everyone.

Given no enthusiasm for removing and some against, I’ll close this.

rust-highfive assigned hanna-kruppe Apr 24, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 24, 2020

SimonSapin mentioned this pull request Apr 24, 2020

Document unsafety in core::ptr #71507

Merged

SimonSapin force-pushed the unique branch from b86fd38 to fa98ff5 Compare April 24, 2020 21:09

SimonSapin commented Apr 25, 2020

View reviewed changes

RalfJung reviewed Apr 25, 2020

View reviewed changes

SimonSapin closed this Apr 26, 2020

SimonSapin deleted the unique branch April 26, 2020 14:49

Uh oh!

Remove ptr::Unique #71530

Remove ptr::Unique #71530

Uh oh!

Conversation

SimonSapin commented Apr 24, 2020

Uh oh!

rust-highfive commented Apr 24, 2020

Uh oh!

SimonSapin commented Apr 24, 2020

Uh oh!

Amanieu commented Apr 24, 2020

Uh oh!

SimonSapin Apr 25, 2020

Choose a reason for hiding this comment

Uh oh!

RalfJung Apr 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung Apr 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung commented Apr 25, 2020

Uh oh!

RalfJung Apr 25, 2020

Choose a reason for hiding this comment

Uh oh!

SimonSapin Apr 25, 2020

Choose a reason for hiding this comment

Uh oh!

bors commented Apr 25, 2020

Uh oh!

Gankra commented Apr 25, 2020

Uh oh!

RalfJung commented Apr 26, 2020

Uh oh!

Amanieu commented Apr 26, 2020

Uh oh!

SimonSapin commented Apr 26, 2020

Uh oh!

Amanieu commented Apr 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Amanieu commented Apr 26, 2020

Uh oh!

hanna-kruppe commented Apr 26, 2020

Uh oh!

RalfJung commented Apr 26, 2020

Uh oh!

SimonSapin commented Apr 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Remove `ptr::Unique` #71530

Remove `ptr::Unique` #71530

RalfJung Apr 25, 2020 •

edited

Loading

RalfJung Apr 25, 2020 •

edited

Loading

Amanieu commented Apr 26, 2020 •

edited

Loading