Clarify the relationship between forget() and ManuallyDrop.

[hniksic]

As discussed on reddit, this commit addresses two issues with the
documentation of mem::forget():

• The documentation of mem::forget() can confuse the reader because of the
  discrepancy between usage examples that show correct usage and the
  accompanying text which speaks of the possibility of double-free. The
  text that says "if the panic occurs before mem::forget was called"
  refers to a variant of the second example that was never shown, modified
  to use mem::forget instead of ManuallyDrop. Ideally the documentation
  should show both variants, so it's clear what it's talking about.

  Also, the double free could be fixed just by placing mem::forget(v)
  before the construction of s. Since the lifetimes of s and v
  wouldn't overlap, there would be no point where panic could cause a double
  free. This could be mentioned, and contrasted against the more robust fix
  of using ManuallyDrop.

• This sentence seems unjustified: "For some types, operations such as
  passing ownership (to a funcion like mem::forget) requires them to
  actually be fully owned right now [...]". Unlike C++, Rust has no move
  constructors, its moves are (possibly elided) bitwise copies. Even if you
  pass an invalid object to mem::forget, no harm should come to pass
  because mem::forget consumes the object and exists solely to prevent
  drop, so there no one left to observe the invalid state state.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @sfackler (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[Centril]

r? @RalfJung

[RalfJung]

This sentence seems unjustified: "For some types, operations such as
passing ownership (to a funcion like mem::forget) requires them to
actually be fully owned right now [...]".

I added that sentence because the following is UB:

fn main() { unsafe {
    let x = Box::new(4);
    let x_copy = std::ptr::read(&x);
    drop(x);
    std::mem::forget(x_copy);
} }

Click "Tools - Miri" on playground to see the UB.

In other words, passing invalid object to any function can cause UB (it is "producing an invalid value"). The fact that you seem convinced of the opposite despite the docs stating this makes it even more important to keep this remark, I think. ;) But the remark is probably not very clear, and could be improved. Any suggestions?

[hniksic]

@RalfJung Thanks for the clarification and the example. I can't suggest a better wording for that part because I don't yet understand the underlying problem.

Looking at the example, I cannot help but wonder if Miri is being too strict here. For example, changing let x = Box::new(4) to let x = 4 or even let x = Rc::new(4) removes the error from the Miri run. I'm curious in particular what is the difference between Box<i32> and Rc<i32> here - is some box-specific compiler magic going on?

[hniksic]

After thinking about this some more, I'm beginning to see your point: passing a dangling Box to any function, including mem::forget, is undefined behavior by definition. Even if it doesn't currently appear to produce a bad outcome, it could do so at any time, and the documentation should warn against it. The wording of the definition makes it apparent why let x = 4 is accepted by Miri, but the box is not - destroying 4 doesn't produce an invalid integer (no integer is invalid, although a bool or an enum can be), but deallocating the box does produce a very invalid box.

It is curious that the definition explicitly includes a dangling Box, but doesn't (explicitly) cover other types of smart pointers, such as Rc and Arc. Is that an oversight in the definition or the result of more subtle reasons why one is ok while the others are not?

[RalfJung]

I'm curious in particular what is the difference between Box and Rc here - is some box-specific compiler magic going on?

Box is in some sense a primitive type that is recognized and treated specially by the compiler. It knows that it is a valid pointer that can be dereferenced etc. Rc, at least currently, does not get such a treatment. But I wouldn't rely on that... that's why I phrased the warning more generally.

At least right now, you are correct that Box is the only type hat has non-trivial drop that gets that treatment. The other types the compiler tracks like that are the reference types. However, references wrapped in a newtype might eventually also get that treatment, and those could have drop glue as well.

passing a dangling Box to any function, including mem::forget, is undefined behavior by definition. Even if it doesn't currently appear to produce a bad outcome, it could do so at any time, and the documentation should warn against it.

Exactly.

Generally, ManuallyDrop is preferred over mem::forget. As usual, type-based approaches just work better. :)

[hniksic]

@RalfJung I've now pushed a change that restores the warning against producing invalid values, only reworded in a way that (hopefully) clarifies the issue.

[RalfJung]

Honestly I am wondering if it is time to soft-deprecate mem::forget in favor of ManuallyDrop -- given that we seem unable to come up with a good usecase for the former above the latter?

[hniksic]

mem::forget is still convenient for safe leaks, e.g. to prevent a File from getting closed in some situations. It could of course be emulated with ManuallyDrop, but mem::forget is much easier to understand and document in such cases.

I'll try to give one more shot at documenting the issues correctly (and fixing the grammar).

[RalfJung]

mem::forget is still convenient for safe leaks, e.g. to prevent a File from getting closed in some situations. It could of course be emulated with ManuallyDrop, but mem::forget is much easier to understand and document in such cases.

That is a good point.

[bors]

☔ The latest upstream changes (presumably #69804) made this pull request unmergeable. Please resolve the merge conflicts.

[hniksic]

I believe I've now fixed the issues raised in the review:

• the "example" of mem::forget usage is marked as erroneous, with the accompanying text explaining why;
• the merge conflict is fixed.

Additionally, the whole relationship with ManuallyDrop is moved to a separate section for clarity.

I believe this improves the current text, for reasons explained in the first bullet point of the PR's description.

The second bullet point of that description is largely invalidated, as the UB is shown to exist. Still, the PR hopefully now provides a more thorough explanation of the issue, especially for readers who (like me previously) think that the fact that Rust's moves are just bitwise copies makes it ok to invoke mem::forget with an invalid value.

[hniksic]

@RalfJung Does the S-waiting-on-author label mean that an action is required from my side? If so, what do I need to do?

[RalfJung]

No it's just not automatically updated. I have this in my queue and will get around to reviewing it eventually. :)

[RalfJung]

Thanks! :)
@bors r+

[bors]

📌 Commit 0335c037af4149b392d5782d3bc2e91e13b6d228 has been approved by RalfJung

[hniksic]

The len was also hard-coded in the ManuallyDrop example; I've now changed that one as well for consistency.

[RalfJung]

Good call.

Could you squash the commits a bit? 13 commits seems excessive for this change.^^

[RalfJung]

@bors r-

[hniksic]

Could you squash the commits a bit? 13 commits seems excessive for this change.^^

Sure! I kind of assumed you'd squash all of them at merge time, so I didn't bother tidying them up.

Is it ok to also rebase them on current master in order to avoid the bidirectional merge?

[RalfJung]

Sure, whatever makes squashing easier for you.

[hniksic]

I've now rebased the change and reduced the commit count to four, each of which makes logical sense. Feel free to let me know if you want me to cut it further.

[RalfJung]

Thanks a lot, that's perfect. :)

@bors r+

[bors]

📌 Commit 2bebe8d has been approved by RalfJung