soundness bugs when used with throwing move constructors

[sarah-ek]

i wrote a small example where it's possible to cause undefined behavior while switching the state of Result<A, B>
the idea is that to assign Err<B> into the Result, we first destroy A then move-construct B into the union. if the move constructor throws we end up in a state with neither of the two objects, which violates the invariants of Result, and in this example. causes us to destroy A one time too many.

#include "stx/option.h"
#include <iostream>

using stx::Err;
using stx::Ok;
using stx::Result;

struct A {
  // count the number of instances alive
  inline static int count_A = 0;
  A() { ++count_A; }
  A(A&&) { ++count_A; }
  A& operator=(A) { return *this; }
  ~A() { --count_A; }
};

struct B {
  B() {}
  B(B&&) {
    static int doom_counter = 3;
    --doom_counter;
    std::cout << doom_counter << " moves until doom" << '\n';
    if (doom_counter == 0) { throw 1; }
  }
  B& operator=(B) { return *this; }
};

auto main() -> int {
  {
    Result<A, B> res(Ok(A{}));
    try {
      res = Err(B{});
    } catch (int) {
    }
  }
  std::cout << A::count_A << '\n';
}

on my machine, this prints

2 moves until doom
1 moves until doom
0 moves until doom
A count: -1 <---- this shouldn't ever happen

(there are similar issues with Option<T>, but i tried to avoid them as best as i could in my earlier PR)

there are ways to avoid this issue. the simplest one is to forbid types that can throw in move construction/assignment. they're a bit rare and can have surprising behavior (though they do exist in real-world scenarios, like std::list on msvc

there are solutions that don't involve restricting the types we can work on, but they make the implementation a bit more complex

the issue only arises while changing the active member. without loss of generality, we can assume T is the active member, and we're trying to assign E to the Result. we can first check if either of the types is nothrow_move_constructible

if E is nothrow_move_constructible, we just do the normal thing. destroy T, move construct E.
if T is nothrow_move_constructible, we can move T out of the Result and into a temporary object, destroy T, move E into the result. if this throws, then move the temporary (which used to be T) back into the Result

if neither is nothrow_move_constructible. we'll have to use separate storage for each of them, kinda like using a tuple<Option<T>, Option<E>>, with the added invariant that at exactly one of the two is alive at any given time. this costs some extra storage, but it's better than UB.

the solution the standard library's variant chose is to allow the variant to be empty, which is called a `valueless_by_exception' state. i'm personally not a big fan of this since that adds one more case to needlessly keep track of

[lamarrr]

Hi, thanks for the note. I suppose that's related with exception guarantees.
The library is meant to replace exceptions and intended for people who don't use exceptions.
I have no intention of supporting throwing types so I suppose I'd have to explicitly state that in the documentation.