User-defined placement-box

[pnkfelix]

Add support for user-defined placement-box expressions (i.e. box (<place>) <value> for types other than Box<T>).

This is a prototype design that @nikomatsakis and @pnkfelix have been working on. It works by desugaring an instance of placement-box into a protocol where we first create the intermediate storage with a pseudo-out-pointer, then initialize the storage, and then convert the out-pointer into the desired box type.

The code was originally written to handle both box (<place>) <value> and box <value> via the same uniform desugaring, and it is easy to revise support for the latter; but as is, this PR only puts box (<place>) <value> into the desugaring. The reason for this is that the error messages get a lot nastier to handle from the desugared form, and pnkfelix did not want to hold this work up further trying to resolve that.

The final design for this will require an RFC; this code is entirely experimental for now.

Proto RFC: http://discuss.rust-lang.org/t/pre-rfc-placement-box-with-placer-trait/729/5

[rust-highfive]

Warning

• These commits modify unsafe code. Please review it carefully!

[pnkfelix]

r? @nikomatsakis

[pnkfelix]

agh failed make tidy ... will fix pronto.

(okay, fixed now; earlier review request still stands. :)

[pnkfelix]

(on reflection, I'm not sure why I thought printing from drop would be any better than failing from drop. I will revise those to just call fail as well.)

[thestinger]

The term exchange heap really shouldn't be used. It's not how dynamic allocation works in Rust. It's wrong to refer to this as HEAP at all. The Box<T> type represents a dynamic allocation from the same allocator as other types like Vec<T> and Rc<T>. Rust doesn't draw any distinction between memory on a "heap" and other memory mappings.

[thestinger]

The box placement protocol should be designed with the same flexibility as placement new / perfect forwarding in C++. It should be possible to push onto a vector by building the element in-place rather than constructing it and copying it inside. I don't think these concerns are being addressed by the design effort put into box, so I'm against progressing with it further.

[thestinger]

There needs to be a clear plan on how this is going to work for use cases like vector or hash table inserts before I'm comfortable with it, because I don't want to see Rust end up with an inadequate language feature that needs to be duplicated.

[nikomatsakis]

@thestinger Good point, that is an important use case. I don't see any trouble expressing something like emplace_back. (In fact, @pnkfelix just pushed a commit showing how it can be done: b293984). What do you think? In any case, as @pnkfelix said, we plan to write an RFC, so it'd be great to get a good list of any other requirements you have in mind so that we can write up how the design handles them.

[thestinger]

If it can support emplace_back then I think it's flexible enough to cover most use cases of perfect forwarding / placement new in C++. I can't think of another major use case for it at the moment.

[nikomatsakis]

Since this is public, we'll be committing to the continued existence of this type, at least. I'm ok with this but cc @aturon. We'll probably want to bikeshed the name :)

[pnkfelix]

(Of course there's no way to avoid having this be public, at least not without private trait methods at the very least...)

[nikomatsakis]

In particular I expect we want to reuse the "agent" term

[nikomatsakis]

On Thu, Nov 06, 2014 at 07:48:41AM -0800, Felix S Klock II wrote:

(Of course there's no way to avoid having this be public, at least not without private trait methods at the very least...)

Yes. And I think that's ok.

[nikomatsakis]

heap::allocate() can return null on failure, we should check for this and panic

[nikomatsakis]

why the cast here? seems unnecessary.

[pnkfelix]

(agreed, that was probably an artifact of an earlier version of the IntermediateBox structure.)

[nikomatsakis]

does it not work to just right box (HEAP) Default::default()?

[pnkfelix]

I think the type-inference applied to the desugaring might reject that, but I'll double check.

[nikomatsakis]

Is there a way for us to mark these macro-artifacts as unstable? cc @aturon

[nikomatsakis]

This may be a moot point now that UFCS support has landed.

[nikomatsakis]

We have UFCS now

[nikomatsakis]

So I imagine we can remove make_place

[nikomatsakis]

Also pointer

[nikomatsakis]

And finalize

[nikomatsakis]

...with UFCS, I suspect we don't need this.

[pnkfelix]

you're right; I had thought we would need e.g. impl Trait to assert that the input placer expression had a suitable type, but we can make the assertion via UFCS. The only question is whether the resulting error messages will be usable. We will see about that.

[nikomatsakis]

Only question is whether b.pointer() should be evaluated before V or not. Also, we should add a FIXME regarding the matter of temporary lifetimes.

[pnkfelix]

I switched b.pointer() to evaluate before V.

Can you refresh my memory as to which matter of temporary lifetimes you were referring to here?

[pnkfelix]

(oh, maybe that was about the "vector should be tied up" note below... which may indeed go away with the revisions to take the placer by value in Placer::make_place(self);)

[nikomatsakis]

The expr place is not consumed, actually, but rather &mut borrowed, no? Probably this doesn't matter because anything with a place that is not None winds up being macro-expanded away, in which case maybe we can just add a call to span_bug

[pnkfelix]

i was just blindly doing a mechanical transformation here. (But now I think we've further revised the protocol so that make_place takes self by-value ... and of course yes, in principle we should never get here except with place = None, so I will just put the asserting span_bug in.)

[nikomatsakis]

I think just using Placer::place(&mut <place_expr>) and so on will have a similar effect

[nikomatsakis]

So sad. What are the other cases that do similar things? I guess it is backwards compat to fix this, so it's just fine.

[pnkfelix]

You can see the other cases listed with a grep like this:

% ack 'mod std {' lib*/lib.rs
liballoc/lib.rs
129:mod std {

libcollections/lib.rs
114:mod std {

libcore/lib.rs
139:mod std {

librand/lib.rs
488:mod std {

librustrt/lib.rs
171:mod std {

libstd/lib.rs
261:mod std {

libsync/lib.rs
78:mod std {

libunicode/lib.rs
80:mod std {

In libcollections it is even nice enough to point out the motivating cases:

#[cfg(not(test))]
mod std {
    pub use core::fmt;      // necessary for panic!()
    pub use core::option;   // necessary for panic!()
    pub use core::clone;    // deriving(Clone)
    pub use core::cmp;      // deriving(Eq, Ord, etc.)
    pub use hash;           // deriving(Hash)
}

[nikomatsakis]

odd but hopefully irrelevant

[nikomatsakis]

I feel like this agent should carry a lifetime that ensures that the vector is "tied up" somehow

[nikomatsakis]

probably it just wants to carry an &mut Vec<T>

[gereeter]

With the current design, make_place could be called repeatedly, and so that would result in aliasing &muts. However, there are other problems with calling make_place repeatedly. Since reserve_additional reserves space relative to the length of the vector, multiple make_places will only reserve space for a single extra element. The asserts in .pointer prevent unsafety, but it still is an issue. Arguably worse, the implication of the description of .pointer implies that different PlacementAgents should return different pointers, but this is violated by having multiple EmplaceBackAgents on the same Vec.

Personally, I'm in favor of just making make_place take the Placer by value. This shouldn't hurt ergonomics, as HEAP and RC would be Copy.

[nikomatsakis]

can you move this emplace_back implementation into the auxiliary directory, and transform this test to test that it executes correctly, and add a compile-fail test that tries to do something bogus like:

let mut v: Vec<int> = vec![];
box(v.emplace_back()) { v.push(22); 23 }

[nikomatsakis]

It probably makes sense to leave these for follow-up PRs, but it'd be nice to have placer implementations for:

1. Rc
2. Arc
3. &TypedArena (as discussed)
4. emplace_back, perhaps something with hashtable insert, etc

We should perhaps make a tracker issue (after this lands) for those sorts of things. Not a huge deal as they can be added as we like.

[pnkfelix]

(added link to Proto RFC on discuss to the description; you can see it here: http://discuss.rust-lang.org/t/pre-rfc-placement-box-with-placer-trait/729/5 )

[steveklabnik]

This needs a rebase.

[pnkfelix]

(closing; I no longer think this is on the 1.0 timeframe)

[pnkfelix]

see #22086