May I use `ManuallyDrop` as `Option`'s unsafe counterpart?

The following code uses "Option" type for "partial destruction". Similar code was mentioned in TRPL, so I hope this is not an anti-pattern.

struct S {
  // just using String for example, the actual code uses a MutexGuard object
  x: String,
  y: Option<String>
}

impl S {
  fn remove_y(&mut self) -> String {
    self.y.take().unwrap()
  }
  
  fn add_y(&mut self, new_y: String) {
      let prev: Option<String> = self.y.replace(new_y);
      assert!(prev.is_none());
  }
}

Here comes the problem. In my situation, the liveliness of field y has already been determined by some other restrictions, thus I don't want Option's "checked" behavior. After searching through the documentation, I guess ManuallyDrop could satisfy my need:

use std::mem::ManuallyDrop;

struct S {
  x: String,
  y: ManuallyDrop<String>
}

impl S {
  unsafe fn remove_y(&mut self) -> String {
    ManuallyDrop::take(&mut self.y)
  }
  
  unsafe fn add_y(&mut self, new_y: String) {
    self.y = ManuallyDrop::new(new_y);
  }
}

But I'm not very sure about its correctness. I want to know is it correct or incorrect, and whether there're better solutions.

Making correct unsafe fn is similar with making correct C/++ function. It is correct if it's possible to call it correctly, usually with enough context specific knowledges and absence of mistake, and document it correctly. User may misuse the function but it's not my fault as I documented it. It is still correct even if virtually nobody can use it correctly. It's just a correct but bad function. What really matter is how to use those unsafe fns.

Also keep in mind that it's more recommended to keep those needless redundant checks unless you measure and figure out it really matter. Memory errors are the most fatal and hardest to debug error. If it actually happen, it may take months if not years to debug and fix it. It's guaranteed that bug in safe Rust code itself can't involve any memory error.

1 Like

ManuallyDrop contents must not be invalid. You probably want MaybeUninit.

3 Likes

Technically speaking, bit pattern of once-valid-but-dropped string doesn't violates the validity invariant so it's safe to keep it in the ManuallyDrop.

https://www.ralfj.de/blog/2018/08/22/two-kinds-of-invariants.html

6 Likes

Sure. But I'm only using String for example so... perhaps I still need MaybeUninit.

I second the suggestion to start with Option. Compilers are complicated and if you have not done actual measurements you are very likely to accidentally pessimize your code by second-guessing the compiler. Note that Option<String> is the same size as String, so you don't pay a memory penalty in this case.

Actually there is also a third option: use mem::take to replace the current String with an empty one. This means that you don't have to check it when merely accessing it because even an empty String contains a valid pointer (unlike an Option<String>), and you also don't have to manually drop it to avoid memory leaks (but doing so is still correct, just unnecessary).

As to your original code, there's no way to incur UB with add_y, so it should not be marked unsafe.

9 Likes

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.