Fix fallout from #57667

[ishitatsuyuki]

PR #57667 fixed a memory leak in P::filter_map (where P is the special pointer type used in the compiler's AST).

but @eddyb pointed out a bug in it: #57667 (comment)

[eddyb]

cc @RalfJung this is a clever trick I didn't consider!

[RalfJung]

Yeah I think this should work. I don't like the raw ptr cast though, it's so easy to get those wrong -- but I also don't know another way here.

[eddyb]

r=me if @RalfJung can confirm it's a correct approach

[ishitatsuyuki]

Do you suggestions on alternatives to raw pointer cast? If not, I guess the code should be merged as-is.

[ollie27]

This might be a shot in the dark but would something like the following work (and deal with the FIXME):

pub fn filter_map<F>(self, f: F) -> Option<P<T>> where
    F: FnOnce(T) -> Option<T>,
{
    unsafe {
        // Transmute to `Box<MaybeUninit<T>>` so if `f` `panic`s or returns
        // `None` the `Box` will be freed but not the inner T.
        let mut maybe_box: Box<MaybeUninit<T>> = mem::transmute(self.ptr);
        let x = f(ptr::read(maybe_box.as_ptr()))?;
        maybe_box.set(x);
        Some(P { ptr: mem::transmute(maybe_box) })
    }
}

[ishitatsuyuki]

I think @ollie27's suggestion is pretty good, any opinions/alternatives regarding the use of transmute?

[RalfJung]

It seems to me that using ManuallyDrop instead of MaybeUninit is sufficient?

[ishitatsuyuki]

@RalfJung the snippet above uses MaybeUninit::set.

[ollie27]

I guess this would work:

pub fn filter_map<F>(self, f: F) -> Option<P<T>> where
    F: FnOnce(T) -> Option<T>,
{
    unsafe {
        // Transmute to `Box<ManuallyDrop<T>>` so if `f` `panic`s or returns
        // `None` the `Box` will be freed but not the inner T.
        let mut manaully_drop_box: Box<ManuallyDrop<T>> = mem::transmute(self.ptr);
        let x = f(ManuallyDrop::take(&mut manaully_drop_box))?;
        *manaully_drop_box = ManuallyDrop::new(x);
        Some(P { ptr: mem::transmute(manaully_drop_box) })
    }
}

[RalfJung]

Yeah, something like that.

Either way though I think it'd be good to avoid "open" transmute and use helper functions instead. Turning Box<MaybeUninit<T>> into Box<T> should probably even become a stable (unsafe) operation some day.

[Dylan-DPC-zz]

ping from triage @ishitatsuyuki @eddyb what's the update on this?

[petrochenkov]

r? @RalfJung

[RalfJung]

@ishitatsuyuki could you update this to one of the proposed panic-safe schemes? If not I can r+ this, because it is an improvement, but the other version is even better.

[ishitatsuyuki]

Actually I invented one that is drastically simpler, based on ManuallyDrop.

The pains are with the Box<[T]> methods. I know that the best way would be adding some methods to Box itself, but I'm not inclined to do that since that's not future-proof (a pre-RFC has been on internals, for example).

[RalfJung]

Not sure if I like changing P itself -- for once, didn't you forget to add a impl Drop?

But it seems like some local private conversion functions between Box<T> and Box<ManuallyDrop<T>> would help. Long-term we probably want something like this in libstd. (We also want that for MaybeUninit, this should probably be co-designed.)

[ishitatsuyuki]

I spent some effort and wrote things again. Hopefully this is what you wanted.

[RalfJung]

I meant local helper functions in the P module... this also works but now you have to create a tracking issue and add it here.

[RalfJung]

Can you use into_inner instead of take?

[ishitatsuyuki]

I don't think so, that would reallocate (or at least consume) the Box which defeats the point of P.

[RalfJung]

It would move out of the box and then later move back into it, why would it reallocate? This code does not reallocate either.

[SimonSapin]

Oh wow. Box turns out to be even more magical than I realized. In this playground example shows that not only we can move a value out of Box without a DerefMove trait, but the compiler tracks such moves like for local variables and plain struct fields. Attempting to use the box before its contents are reassigned (if *x = … is removed) results in a compile time error[E0382]: use of moved value: `x` . But the box is still there and can be used again after its contents are reassigned.

And this is panic-safe too. In the assembly for https://play.rust-lang.org/?version=nightly&mode=release&edition=2018&gist=d4907435865f18159268fc36a8546016 we see that foo calls box_free but not drop_t.

So I think @RalfJung’s suggestion is the way to go.

[SimonSapin]

Bonus possibly-unexpected detail:

rust/src/liballoc/boxed.rs

Lines 290 to 295 in f22dca0

	#[stable(feature = "rust1", since = "1.0.0")]
	unsafe impl<#[may_dangle] T: ?Sized> Drop for Box<T> {
	fn drop(&mut self) {
	// FIXME: Do nothing, drop is currently performed by compiler.
	}
	}

[eddyb]

Wow, I was aware of Box special-casing, but I do not recall being able to reinitialize the Box and reuse it!

@nikomatsakis @pnkfelix Do you happen to know when that might've happened?

[nikomatsakis]

I believe this probably happened as part of the NLL transition. We decided to stop hobbling the treatment of Box during that discussion.

[ishitatsuyuki]

I will do the tracking issue thing tomorrow.

[RalfJung]

@rust-lang/libs any advise on how to proceed here -- should we add two new unstable methods with a tracking issue, or instead add two local private helper functions in syntax::ptr?

[SimonSapin]

Per #58021 (comment) it looks like the one use case for those methods can be solved with plain Box<T> without ManuallyDrop. Unless there’s another use case, let’s not add public APIs.

[RalfJung]

I hadn't even thought so far, but you are right... this proper move tracking for boxes entirely sidesteps the need for ManuallyDrop. We can even write these methods in safe code.

[ishitatsuyuki]

(Short on time this week, will update this weekend)

[bors]

⌛ Testing commit a03e20d with merge e68bf8a...

[bors]

☀️ Test successful - checks-travis, status-appveyor
Approved by: RalfJung
Pushing e68bf8a to master...

[pnkfelix]

Discussed in this week's T-compiler meeting. Approved for beta-backport.

(In parallel, @pnkfelix wants to make a regression test. But than need not hold up the backport.)

[pnkfelix]

(In parallel, @pnkfelix wants to make a regression test. But than need not hold up the backport.)

I was careless and misread the patch, thinking that this was some fn map and fn filter_map in libstd; but that wasn't true at all, its in libsyntax. I'm not going to worry about a regression test.