refactor NonZero, Shared, and Unique APIs

[Gankro]

Major difference is that I removed Deref impls, as apparently LLVM has
trouble maintaining metadata with a &ptr -> &ptr API. This was cited
as a blocker for ever stabilizing this API. It wasn't that ergonomic
anyway.

• Added get to NonZero to replace Deref impl
• Added ptr getter to Shared/Unique to replace Deref impl
• Added Unique's get and get_mut conveniences to Shared
• Deprecated as_mut_ptr on Shared in favour of ptr

Note that Shared used to primarily expose only *const but there isn't
a good justification for that, so I made it *mut.

[rust-highfive]

r? @alexcrichton

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

[Gankro]

CC @eddyb who brought up the Deref issue.

[Gankro]

NOTE: I removed these assumes because they should be implied from NonZero, and the point of these changes is to make that metadata propagate more readily. Should probably try to validate that somehow? Not sure the best way forward.

[alexcrichton]

I think you can test this out by taking a look at the IR for:

fn foo(r: &Rc<i32>) {
    drop(r.clone());
}

Ideally that's a noop function.

[eddyb]

You may need to copy it to NonZero::get (if you can), at least until we have it in the compiler.

[Gankro]

I'm a bit concerned that copying an assume to NonZero::get will be disastrous to compile times (as this will show up in all accesses to every collection). Have those bloat problems with assume been resolved?

[eddyb]

Oh that's a good question (sorry I missed this). cc @dotdash @nagisa @arielb1

[Gankro]

• TODO: update the nomicon to reflect these changes. Waiting for feedback first.

[Gankro]

Oh I also made Unique Copy/Clone, because it's unclear what value move semantics has here.

[SimonSapin]

Oh I also made Unique Copy/Clone, because it's unclear what value move semantics has here.

This is kind of a weak argument, but copying or cloning seems contrary to the name "unique" or the docs "indicates that the possessor of this wrapper owns the referent".

And is this useful? Something that contains Unique<_> owns the allocation, so it’s responsible for deallocating, so it needs to implements Drop, so it can’t implement Copy. As to cloning such a thing, that is done by allocating new memory, cloning the contents, and using Unique::new with the new pointer.

[Gankro]

Primary motivation for Copyable Unique was to have a by-value getter and maybe tax the optimizer a tiny bit less. Also to make it more of a drop-in-replacement for *mut; possibly if you're just trying to use it as an optimizer hint?

I'm not particularly passionate about it either way. Maybe @nikomatsakis can speak to aliasing model implications?

[Ericson2314]

I appreciate not stressing the optimizer, but doing that does make for an annoying footgun.

[Gankro]

Every Unique lives in a struct that owns it, so I don't know what the footgun could possibly be; especially since using the Unique requires you to immediately turn it into a primitive pointer that can be copied.

[Ericson2314]

It's been a while, but I vaguely recall using "raw" Uniques not inside some other struct as a last-ditch attempt to prevent unwanted aliasing.

Even if the wrapped struct case, there is still code working with the wrapper.

[alexcrichton]

I think we may call this get_ref nowadays?

[Gankro]

How do you feel about ref and ref_mut?

[durka]

ref is a keyword

[alexcrichton]

These are so low-level I'm ok with adding Clone and Copy. It's not safe to use these types with or without those trait bounds, and with them it's just a little easier to work with Unique and Shared

[bors]

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

[eddyb]

Should this be on Shared at all? I'd expect interior mutability to be required - or I suppose we could just have something along the lines "you shouldn't ever use this unless there is no other reference around" in the documentation (e.g. Rc with a refcount of 1).

[eddyb]

Note that Shared used to primarily expose only *const but there isn't a good justification for that, so I made it *mut.

That's a breaking change AFAIK, I think it's for variance.

[Gankro]

@eddyb it still contains a *const; it's just that there's no API that goes -> *const anymore, because that's just making developers jump through hoops.

Shared has, basically, no semantics other than "it's non-null" and "it doesn't have *mut's useless variance". In other words, it's the unsafe pointer that everyone wishes existed. I don't know where you'd expect to put any kind of interior mutability annotation; certainly Arc and Rc haven't bothered to do any such thing.

It's possible there's interesting aliasing rules to document here, but until someone figures out what Rust's aliasing rules are, that's simply impossible.

[eddyb]

@Gankro Ahh, okay, keeping *const T in the struct and returning *mut T is fine.

The refcounts are Cells in Rc and AtomicUsizes in Arc, that's what I was referring to, and that all user data has to be wrapped in interior mutability to get a &mut, with the notable exception of make_mut, which is presumably the only user of Shared::get_mut in Rc/Arc.
If it's for symmetry, I'm fine with it, although the usecase is so rare I'd almost have it be done with .ptr().

[nikomatsakis]

Maybe @nikomatsakis can speak to aliasing model implications?

I can't speak to it with certainty, except to say that I think there is pretty broad consensus that errors ought to be mostly about which pointers get dereferenced and when, not which get copied from place to place.

[carols10cents]

Ping to keep this on your radar @Gankro!

[Gankro]

I'm not sure this is really blocked on me. The only real requested change is from @alexcrichton, but really the blocker is on libs/lang team people agreeing on the design.

[Gankro]

Another API question that I kinda spaced out on: the current design does (&self) -> &T so the lifetime is bounded. This works well when the ptr is in self, but can behave poorly otherwise (see LinkedList's code). The alternative is to do (self) -> &T and return an unbounded lifetime. Maybe this is a case where we want both? ref() and ref_unbound()?

[carols10cents]

@Gankro Oh I thought it should be in your court because there are merge conflicts and test failures. Are you waiting on approval from libs/lang on the design before fixing those?

[Gankro]

Yeah

[alexcrichton]

I'll cc @rust-lang/libs to see if anyone else would like to leave a comment, but typically if we're not stabilizing something we don't discuss PRs during triage (as it's just too much to process).

I would be comfortable r+'ing w/ my minor comment and a rebase.

[bors]

⌛️ Testing commit e8234e0 with merge 42a4f37...

[twmb]

I believe the documentation needs updated to no longer refer to ptr and ptr_mut, and instead to refer to as_ptr and as_mut_ptr.