Remove & -> &mut transmutation from libarena

[renato-zannon]

Update

I've reimplemented this using Cell and RefCell, as suggested by @alexcrichton. By taking care with the duration of the borrows, I was able to maintain the recursive allocation feature (now covered by a test) without the use of Unsafe, and without breaking the non-aliasing &mut invariant.

Original

Changes both Arena and TypedArena to contain an inner struct wrapped in a Unsafe, and change field access to go through those instead of transmuting &self to &mut self.

Part of #13933

[huonw]

This still feels unsafe... especially with the closure like this, since one could write

let some_arena = ...;

some_arena.alloc(|| {
    some_arena.alloc(|| {
        // uh oh: two &mut's to some_arena.inner, both being mutated.
    });

    foo()
})

[huonw]

(I know this is how it was before... it would be good to at least have a FIXME here, with a filed bug (other than #13933).)

[renato-zannon]

Makes sense. Would it be OK to use a RefCell instead of Unsafe (to maintain the invariant at least in runtime)?

[renato-zannon]

(let me know if I should open another issue instead of discussing here)

If ArenaInner.alloc took the object directly, instead of the closure, then the multiple simultaneous borrows could be avoided - at the expense of doing a couple more moves/copies, which I'm not sure it's acceptable.

Also, there's a comment in the file mentioning that "alloc() can be invoked recursively", but I'm not sure I understand the use case for this (and couldn't find any example of this usage in the codebase).

[huonw]

Ah, good catch with the comment, I believe that's addressing whether the T is fully created or not (i.e. whether to run the destructor), while the problem here is an implementation detail of &muts aliasing.

RefCell would be a way to fix this, but it does change semantics somewhat: you can no longer allocate inside an arena recursively. I'm not really sure what the appropriate path is here.

Taking T by value would also work, again, not really sure about it for the reasons you mention.

[renato-zannon]

My understanding is that, when a given function is inlined in the caller, the parameters of the call don't need to be copied anymore, as there's no new stack context. Is that correct?

If that's the case, could we just mark alloc_copy and alloc_noncopy with #[inline(always)] to mitigate/remove the copy overhead, and thus be able to pass the object by value without a performance regression?

[alexcrichton]

I agree with huon that RefCell and/or Cell should be used here where appropriate instead of Unsafe. It looks like there's no need for an active borrow to be in scope when op is invoked, so you can preserve recursive allocation.

[renato-zannon]

RefCell and/or Cell should be used here where appropriate

Okay! I will try to change it later today/tomorrow. Should I just rebase and then push --force?

you can preserve recursive allocation

I don't think the current tests would catch if this wasn't preserved. Can I include a few to enforce this in this same PR?

[huonw]

(The RefCell/Cell idea was all @Riccieri's. :) )

[renato-zannon]

I've just realized I'm not sure if github notifies mentioned people on edits or new pushes. If it does, I apologize 😳 So, here it goes: ping @alexcrichton

Anyway, please let me know if on my next pull requests I should be pinging people after updates, or if this is annoying :)

[alexcrichton]

Ah yes, please do ping us! That's exactly what I like to see :)