Implement sound map value aliasing

[jonhoo]

This implementation gets rid of the unsound ShallowCopy trait (#74 &
rust-lang/unsafe-code-guidelines#35 (comment)),
and replaces it with a wrapper type around aliased values.

The core mechanism is that the wrapper type holds a MaybeUninit<T>,
and aliases it by doing a ptr::read of the whole MaybeUninit<T> to
alias. It then takes care to only give out &Ts, which is allowed to
alias. To drop, the implementation sets a thread-local that the wrapper
uses to determine if it should truly drop the inner T (i.e., to
indicate that this is the last copy, and the T is no longer aliased).

The removal of ShallowCopy makes some of the API nicer, and obviates
the need for evmap-derive. Unfortunately the introduction of the
wrapper also complicates certain bounds which now need to know about the
wrapping. Overall though, an ergonomic win.

The implementation passes all tests, and I think it is sound (if you
think it's not, please let me know!). The one bit that I'm not sure
about is the thread-local if a user nests evmaps (since they'll share
that thread local).

Note that this does not take care of #78.

Fixes #74.
Fixes #55 since ShallowCopy is no longer neeeded.

---

This change is 

[codecov]

Codecov Report

Merging #83 (1de0cef) into master (9f3d73d) will decrease coverage by 1.22%.
The diff coverage is 54.59%.

Impacted Files	Coverage Δ
evmap/tests/quick.rs	93.65% <ø> (ø)
left-right/src/lib.rs	45.45% <ø> (ø)
evmap/src/read/read_ref.rs	60.46% <37.50%> (-4.54%)	⬇️
evmap/src/write.rs	56.80% <43.93%> (-7.93%)	⬇️
left-right/src/aliasing.rs	44.44% <44.44%> (ø)
evmap/src/aliasing.rs	50.00% <50.00%> (ø)
evmap/src/inner.rs	50.00% <50.00%> (ø)
evmap/src/values.rs	85.71% <75.00%> (-1.71%)	⬇️
evmap/src/lib.rs	55.55% <100.00%> (+21.55%)	⬆️
evmap/src/read.rs	82.00% <100.00%> (ø)
... and 3 more

[RustyYato]

Just a suggestion to resolve nested evmaps

[jonhoo]

I'd like to draw attention to a couple of key unsafeties that I'd like a second opinion on before I merge this (though arguably this is definitely better than what we had, which we know is unsound...):

---

First, aliasing a MaybeUninit<T> by copying it with ptr::read. I think this is fine since MaybeUninit allows pretty much any content.

https://github.com/jonhoo/rust-evmap/blob/4e83da000cfee94810eb790f25c1dc159def1b12/evmap/src/aliasing.rs#L33-L38

---

Next, extracting a &T from Aliased<T>'s MaybeUninit<T>. I believe this is sound assuming the T hasn't been dropped because the MaybeUninit<T> was created from a valid T, and we are allowed to alias &T even if T itself cannot be aliased (e.g., Box).

https://github.com/jonhoo/rust-evmap/blob/4e83da000cfee94810eb790f25c1dc159def1b12/evmap/src/aliasing.rs#L108-L113

---

Then, to complete the circle, the code that causes Aliased<T> to drop the inner T. As long as the caller guarantees that this Aliased<T> is the only copy of T, it should be safe to drop T when this Aliased<T> is dropped even for types that disallow aliasing (like Box). The reasoning is that by the time we actually have a Box, there are no aliases of it any more (by the safety assumption of drop_copies).

https://github.com/jonhoo/rust-evmap/blob/4e83da000cfee94810eb790f25c1dc159def1b12/evmap/src/aliasing.rs#L76-L79

The actual drop code I think is uncontroversial:

https://github.com/jonhoo/rust-evmap/blob/4e83da000cfee94810eb790f25c1dc159def1b12/evmap/src/aliasing.rs#L94-L100

---

unsafe impl of Send and Sync. I think these are stricter than what the compiler would auto-infer, and I believe they must be this strict since we give out &T across thread boundaries if Aliased<T> is shared or sent.

https://github.com/jonhoo/rust-evmap/blob/4e83da000cfee94810eb790f25c1dc159def1b12/evmap/src/aliasing.rs#L56-L62

[code-elf]

As with ShallowCopy before, I think it would be important to move Aliased into left-right (or its own crate) because it's going to be a common concern among libraries that use it.

I don't believe left-right could abstract any more of Aliased''s usage than it could with ShallowCopy`, but it's still important for it to be available.

[jonhoo]

Yes, I think I agree that Aliased could be lifted into left-right — there's nothing evmap-specific about it. There is also nothing left-right specific about it really, but it's unlikely to be used much anywhere else, and likely to be used in many implementations. Unfortunately, by moving it into left-right, we're now compounding the thread-local problem. Now any data structure that uses left-right that is embedded into any other data structure that uses left-right share the same thread-local...

[code-elf]

So while I still think the explicit drop option has merit, it's definitely not the only one.

First off, there may be something I'm missing here, and please correct me if I'm wrong - but considering Aliased is repr(transparent), is there a reason we can't in absorb_second just cast the inner map to the type with non-Aliased values instead, and just drop them? If that works, we could just remove the drop behaviour from Aliased entirely.

If that's not an option, I think it would the flag would need to be map-local rather than thread-local. We could have some kind of AliasedFactory that has an internal drop_for_real flag and exposes functions to create Aliased instances that refer to it and to toggle the flag. This could be a separate struct or built into WriteHandle directly. Or instead of a reference, we could make the factories have an internal ID and expand DROP_FOR_REAL into some kind of HashSet or something. None of this really feels ideal, but thread_local just feels like asking for trouble down the line.

[jonhoo]

@MayaWolf Yes, unfortunately that is unsound. That's in part why the current implementation on master that uses ManuallyDrop is unsound. See also the discussion in rust-lang/unsafe-code-guidelines#35.

Map-local would be nice, but sadly that's also hard unless we're willing for every value to hold a pointer back to a bool shared with the map that it uses to check whether things should be dropped. That's potentially a lot of memory overhead. And ignoring that, this might also be tricky to do safely, since we'd be trying to get at a value that is inside the map which we hold out a &mut self to in order to absorb. The bool itself may have to be on the heap somewhere, outside the map but created by the map, and then reclaimed when the map is eventually dropped.

I have an idea that might let us do this without overhead and without the thread-local though. It hinges on this being safe: rust-lang/unsafe-code-guidelines#35 (comment). Commit coming soon.

[jonhoo]

Unfortunately it doesn't seem like such a transmute is safe either 😢
rust-lang/unsafe-code-guidelines#35 (comment)

[code-elf]

I'll be honest, even after having read through this multiple times I'm confused as to what the actual issue of casting between ManuallyDrop<T> (or Aliased<T>) and T would be. Being repr(transparent) it should be irrelevant (and therefore correct) from a memory point of view, and I don't see how "associated type shenanigans" could cause any issue either.

I think an important thing to consider is we do control the SomeType being discussed in this thread to an extent - after all, left-right is intended as a concurrency primitive, not a plug-and-play solution, so if there really is a way to break this (which, I'll be honest, I'm still not seeing it), then I think the best way to go may be a big warning to library authors of "don't use types that do X as your Absorb".

That's obviously not a perfect solution at all, but I don't think it makes sense to sacrifice efficiency or versatility in trying to work around something that honestly seems like it's "theoretically questionable, but practically fine." Unless I'm just underestimating how big of a problem this really is, in which case the AliasedFactory thing is probably the best thing I can come up with.

[jonhoo]

Yeah, it's fairly involved. The best relevant summary I know of is from the nomicon's page on repr(Rust):

[..] the layout of data is not specified by default. Given the two following struct definitions:

struct A {
    a: i32,
    b: u64,
}

struct B {
    a: i32,
    b: u64,
}

Rust does guarantee that two instances of A have their data laid out in exactly the same way. However Rust does not currently guarantee that an instance of A has the same field ordering or padding as an instance of B.

That's what bites us here. Even though Aliased<T> is #[repr(transparent)], that does not mean that the compiler guarantees that it lays out, say HashMap<K, T> and HashMap<K, Aliased<T>> the same. From what I can piece together from rust-lang/unsafe-code-guidelines#35, part of the issue is that the outer type can use the different types to cause its layout to change. For example, consider this wonky type:

trait Wonky { type Weird; }
struct HashMap<K, V: Wonky>(K, V::Weird, V);

impl<T> Wonky for Aliased<T> { type Weird = u32; }
impl<T> Wonky for T { type Weird = u16; }

That is obviously very contrived, but it does demonstrate a case where the layout of the wrapper changes if you cast V between types, even though the types you case between are repr(transparent). That's why I was trying to get at (in rust-lang/unsafe-code-guidelines#35 (comment)) a way where we can guarantee that the outer type cannot distinguish between the cast-between types (since they only differ by a private type that will never implement any traits).

While it's tempting to say "We know that HashMap doesn't do this, and never will", that's not quite sufficient either. First, have you checked? Second, are you willing to commit to continue checking forever? And third, the compiler does not give this guarantee even if there aren't any wonky associated types involved (see the A and B example earlier), so we'd have to rely on the compiler never changing layout between different same-layout generic parameters. That seems too risky to me, and is why I wanted to push in the other discussion for a way for us to indeed get that guarantee. But so far no dice.

The factor idea I am also super hesitant about unfortunately — storing an extra (identical) pointer for every value is some pretty huge overhead, and not one I'm very keen to swallow. I would rather say "don't embed one evmap in another", though of course someone is still going to do it unwittingly one day...

[jonhoo]

You can see my current tinkering down that path ignoring the possible unsoundness over on sound-aliasing...drop-by-generic-type (the commit message has more details).

[code-elf]

For comparison, here is my attempt at making dropping Aliased's values explicit. I frankly don't know how this affects performance, that would need testing - but in terms of complexity it's entirely manageable.

I had to make drop() take &self rather than &mut self because hashbag's retain function does not pass you a &mut T - if you want to go this path that may be worth fixing in hashbag, but for now, since we're already in an unsafe context anyway and this was mostly an experiment, I figured I'd just cast the pointer.

[code-elf]

It seems I can't leave comments on the commit you posted (or maybe I'm just not pressing the right buttons) - but what's the advantage to passing around DropBehavior types rather than just having a second, otherwise identical type to Aliased (i.e. DropAliased<T>) that is then used in the cast in absorb_second? Does the extra type parameter make a difference from a stability/safety point of view?

also I keep switching up the words absorb and alias in my head and typing the wrong one and I'm going to miss one at some point and you're going to be confused so sorry in advance

[jonhoo]

@MayaWolf It's funny, because that's kind of what evmap used to do. "Back in the day" evmap would explicitly forget values in apply_first, which resembles explicitly dropping values in apply_second: 2f4edc6#diff-8e31462ea4b11fdadc15fd98e895c1902595d9d34947c58eecca1125a5e6224dL525. I think "opt-in" dropping is better than "opt-out" dropping like evmap did back in the day though!

My main concern with the explicit drop approach is that is requires somewhat careful manual and error-prone work to ensure that everything is dropped correctly, and it's not always the case that the implementations even have the requisite methods to do so safely. For example, I'm not even sure it's okay to drop through a &self even if we're storing the T in a MaybeUninit. And the reason HashBag::retain gives you &T is because HashMap's retain does the same. It used to be that I basically re-implemented retain just to get it to work correctly. I'm also not sure if it's sound to leave behind values where you have dropped the inner T. For example, in this segment: https://github.com/MayaWolf/rust-evmap/commit/268d320058e07ee33cd9a0aa0d99e8f9c64dad4f#diff-8d277c2c886b7c5feed13712789e0be48fcdea4d0f293cf41958df2e93cc1de1R551-R553 you are leaving behind the Ts that have been dropped. It's true that the structure is dropped immediately after, but I don't know if it's actually sound. I think it is, but I also thought the ManuallyDrop transmute was sound :p

I want to stress that I'm not trying to say that explicit drop isn't the way to go. It's probably still preferable to the thread-local magic. I'm more saying that I want to try to push a bit more on my current strategy to see if I can get it "for free".

It seems I can't leave comments on the commit you posted (or maybe I'm just not pressing the right buttons) - but what's the advantage to passing around DropBehavior types rather than just having a second, otherwise identical type to Aliased (i.e. DropAliased<T>) that is then used in the cast in absorb_second? Does the extra type parameter make a difference from a stability/safety point of view?

Ah, yes, it does. Specifically, there has to be no way for any code outside of the current crate to distinguish between the types we are casting between in any way. If we made it be DropAliased, then that type would need to be public since it appears in some public APIs, which in turn would mean that some outside code could distinguish between Aliased and DropAliased since it can name both.

also I keep switching up the words absorb and alias in my head and typing the wrong one and I'm going to miss one at some point and you're going to be confused so sorry in advance

You are not the only one. I keep messing up "absorb" and the old "apply" myself :p

[code-elf]

it appears in some public APIs

Does it? It might be public from the point of view of left-right (assuming Aliased is moved into it), but that's just a concurrency primitive after all. But from the point of view of evmap, the only usage of DropAliased would be a cast within absorb_second - it would never need to be exposed, and at that point, the only way for any outside code to gain access to it would be to explicitly use it from left_right and then do an unsafe cast to it. Am I missing anything?

[jonhoo]

If it's exposed by left-right, then anyone can just pull in left-right as a dependency and implement it that way. In my design above, NoDrop and DoDrop would have to be completely private types, and so would have to live in evmap (though I think Aliased could still live in left-right). So DropAliased would also need to live only in evmap and be private.

Unfortunately, even if you did that, transmuting from Wrapper<Aliased<T>> to Wrapper<DropAliased<T>> would not be sound because Aliased<T> is public, so a sneaky implementor could still implement a trait for Aliased<T> differently from all other types (which would then include DropAliased<T> even though it is not public. We could make Aliased<T> not be public, but then it can't live in left-right. Even if we moved it to evmap, I'm not sure if we could make it private, since it still peeks out in some places like https://github.com/jonhoo/rust-evmap/blob/b6c7a31175b60bf5068d02dde68ad46c61fb7013/evmap/src/read.rs#L211 and https://github.com/jonhoo/rust-evmap/blob/b6c7a31175b60bf5068d02dde68ad46c61fb7013/evmap/src/values.rs#L113-L118
That first one in particular is the one that's causing me headaches with my approach, since it's not clear how to express that bound without naming NoDrop, which has to be private for the reasons above.

[code-elf]

Oh, I get it now! So basically, the problem we're trying to work around is that someone might do an impl<T> X for Aliased<T> which would cause the two types to subtly diverge, so both types that we cast between need to be private.

I'll check out your branch tomorrow and see whether I can come up with a solution to the problem you mentioned!

[jonhoo]

Exactly!

In theory I think the problem is minor, I just haven't been able to wrap my head around how to fix it. We could fix the particular issue with contains_value by simply not supporting the niceties that Borrow gives there, but I don't know how to fix the more general case. There are some places where NoDrop just ends up being visible, such as as the default type for the last generic parameter on Values, and I don't know how to get rid of it from there. Maybe we can pull some shenanigans with impl Trait?

[jonhoo]

I think what we're going to have to do is define a #[repr(transparent)] newtype around Values that holds a Values<..., NoDrop>, and make sure that all read methods only ever expose that newtype wrapper (and hence never have to publicly expose NoDrop). Then we'd have to modify anything that today returns a Values to return the newtype instead. This may get annoying in places where we return &Values; we'd have to transmute to &PubValues, which I think should be safe.

EDIT: I'll add that all the methods that are currently on Values should probably be placed on the newtype wrapper instead, since internal operations rarely use them anyway. And Values should probably be the name for the newtype, and we should come up with some better name for the internal one. Or maybe we can re-use ValuesInner for this purpose, I'm not sure?

Now, that doesn't take care of the Aliased<T, U>: Borrow bound, but it should get us a little closer for what remains.

[code-elf]

Yeah, I've had another look through it and thought about it, and I agree that this probably needs a second wrapper around Values.I can't really come up with another way that wouldn't end up exposing DropBehavior.

[jonhoo]

Ah, so, it's not actually a problem to expose the DropBehavior trait. That can be public. It's only the specific types NoDrop and DoDrop that have to be private.

[jonhoo]

I'm getting real close. NoDrop is still pub at the moment, but it's now only because of of these trait bounds:
https://github.com/jonhoo/rust-evmap/blob/457c109417e8f2e972421131ae1a62ecd9f3d37b/evmap/src/read.rs#L211
https://github.com/jonhoo/rust-evmap/blob/457c109417e8f2e972421131ae1a62ecd9f3d37b/evmap/src/read/read_ref.rs#L154
https://github.com/jonhoo/rust-evmap/blob/457c109417e8f2e972421131ae1a62ecd9f3d37b/evmap/src/values.rs#L112

Unfortunately, I still have no idea how to go about hiding it there except by making it unable to go through Borrow...

[jonhoo]

Posted on users.r-l.o: https://users.rust-lang.org/t/private-type-in-bound-of-public-method/52375

[jonhoo]

Huh, well, that was as simple as having NoDrop be a public type but in a private module! Basically the same pattern as sealed traits — neat. So now I think this PR is basically ready to land :o

[jonhoo]

I've released left-right 0.9.1 with the aliasing module, and evmap 11.0.0-alpha.3 that uses the new (hopefully now sound) aliasing technique 🎉

@MayaWolf If you run into issues with it for your implementations, please let me know!