Placement protocol should not consume Placer

[hanna-kruppe]

This came up in the discussion of #1228, but it's mostly independent from the syntax change proposed there. Currently creating a Place for "placement box" will consume the value that produces the place (the one that's written out in the placement expression, i.e. PLACE in box (PLACE) EXPR). This hasn't been an issue so far because the only implementation (in std and, as far as I can tell, on GitHub overall) in ExchangeHeapSingleton which is Copy. That type (and its public name, boxed::HEAP) is hanging by a thread though, box thing is widely preferred to in (HEAP) thing or in HEAP { thing } or HEAP <- thing or any variant of that.

Some hypothetical types that would implement Placer in the future (such as &Arena and the results of map.entry(k), vec.back()) would likely be Copy or inherently "one-off", but other possible uses of placement box would be ruled out by taking self. Examples raised in #1228 are vec <- thing; (instead of vec.push(thing)), set <- thing; and (disclaimer: my example and pet interest) allocators that can't be behind a & reference.

@pnkfelix said:

I am personally becoming more convinced that it should be &mut self. I spent a while last night looking through my notes to see why I had picked self but did not find a motivating example.

[pnkfelix]

For reference purposes, this issue is proposing that we amend RFC 809, which is what documented the placement protocol. See also potentially relevant discussion at its associated PR #809

[pnkfelix]

The issue description isn't clear about whether it is asking for the argument to be &self or &mut self.

Obviously some use cases would not be satisfied by requiring &mut self, but my gut feeling is that the options here are either leaving it as self or changing it to &mut self -- by default, I want to be able to write a Placer implementation and not have to worry about it being aliased within make_place.

(This should mesh in with the Allocator design, where I am currently planning to use &mut self, and so it should mesh with this.)

[nagisa]

Being one who proposed it initially, I’m personally very strongly in favour (of &mut self).

[hanna-kruppe]

The issue description isn't clear about whether it is asking for the argument to be &self or &mut self.

That's because I haven't made up my mind on that yet. &mut self is the obvious alternative, and I expect that it will turn out to be the best option, but anything that allows the use cases mentioned above can be discussed here. (But for the record, I agree that &self seems undesirable for most use cases because aliasing.)

One such alternative would be leaving the signature to be self but make it easy and neat to have an impl for &mut A even when PLACE usually won't be a &mut reference. In other words, if we can write vec <- thing; and have it desugar to something involving Placer::make_place(&mut vec) while also allowing Placer::make_place(one_off_placer), that's also fine by me. I simply don't know yet if this feasible and a good trade off (implementation complexity etc.).

[arielb1]

@rkruppe

Doesn't malloc use &self (or self with a zero-sized type)?

[hanna-kruppe]

I don't really know what you mean. If this is about "allocators that can't be behind a & reference" in the issue text, I'm not talking about some global allocator like C's malloc, I mean weirder, more specialized ones like Scope Stacks.

[bluss]

You need to be very careful about evaluation order and panic safety, as well as forcing the placer to be used:

vector.back() <- x()

How do we handle panics in x() here? back() will already have ran and created more space in the vector; but it cannot update the length of the vector until after x() has completed without panic.

for just

vector.back(); panic!()

This cannot be allowed, would it leave an unitinialized slot?

[hanna-kruppe]

That's what InPlace::finalize(self) is for, isn't it? Regardless of whether the Placer is Vec or some one-off VecBack type returned by vec.back(), the placement protocol will first retrieve an InPlace from it, get a raw pointer from it, then evaluate x() and ptr::write the result. At the last and final step, InPlace::finalize is called. Only this last step would update the length and it would run after the evaluation of x(). In the case of panics, at worst the vector has re-allocated pointlessly.

All of this is completely independent of whether the first step consumes the Placer or not. Or am I missing something?

[nagisa]

@bluss I see what you mean, but I don’t think it is a problem, because the protocol explicitly addresses preconditions for panic safety:

If evaluating EXPR fails, then the destructor for the implementation of Place to clean up any intermediate state (e.g. deallocate box storage, pop a stack, etc).

-- Last paragraph of std::ops::Place documentation.

---

EDIT: actually it might be worse than I initially thought, because destructors are safe to not run now (mem::leak is safe) and we cannot rely on them to ensure safety, can we?

e.g.

let something = Something::new();
let place = something.make_place();
// place relies on destructors running for panic safety (e.g. decreasing length in vector case)
mem::forget(place);
panic!();
// destructor for Something reads uninitialized data?

[bluss]

@nagisa Thanks, looks like I didn't realize this was well enough covered, it's hopefully a matter of adjusting the mockup implementations and it should be possible to solve with the protocol.

Not sure the protocol is at risk because of mem::forget, it should be baked into sugar anyway. Worst case is that more methods need to be marked unsafe.

[glaebhoerl]

Just a note that the way to make mem::forget(place) impossible, other than by making it impossible for safe code to even get its hands on one, would be to make it a linear type (just like &out, unsurprisingly).

[shepmaster]

because the only implementation

FWIW, I created one for TypedArena and had no issues with consuming the Placer.

[hanna-kruppe]

Yes, because the Placer is a & reference, which is also Copy. But now I wonder whether GitHub search will find your PR.

[shepmaster]

I can't fully articulate this, but &mut self seems like it will be annoying. At least in the cases that I have been using (arenas), the arena has internal mutability, so suddenly needing to thread mutability throughout seems likely to cause pain.