Support: Add Expected<T>::moveInto() to avoid extra names

[dexonsmith]

Summary

Expected<T>::moveInto() takes as an out parameter any OtherT& that's
assignable from T&&. It moves any stored value before returning
takeError().

There are two existing patterns that moveInto() cleans up:

// If the variable is new:
Expected<std::unique_ptr<int>> ExpectedP = makePointer();
if (!ExpectedP)
  return ExpectedP.takeError();
std::unique_ptr<int> P = std::move(*ExpectedP);

// If the target variable already exists:
if (Expected<T> ExpectedP = makePointer())
  P = std::move(*ExpectedP);
else
  return ExpectedP.takeError();

moveInto() takes less typing and avoids needing to name (or leak into
the scope) an extra variable.

// If the variable is new:
std::unique_ptr<int> P;
if (Error E = makePointer().moveInto(P))
  return E;

// If the target variable already exists:
if (Error E = makePointer().moveInto(P))
  return E;

Another use case is writing unit tests, to log errors (but continue)
when there's an unexpected failure. E.g.:

// Crash on error, or undefined in non-asserts builds.
std::unique_ptr<MemoryBuffer> MB = cantFail(makeMemoryBuffer());

// Avoid crashing on error without moveInto() :(.
Expected<std::unique_ptr<MemoryBuffer>>
    ExpectedMB = makeMemoryBuffer();
ASSERT_THAT_ERROR(ExpectedMB.takeError(), Succeeded());
std::unique_ptr<MemoryBuffer> MB = std::move(ExpectedMB);

// Avoid crashing on error with moveInto() :).
std::unique_ptr<MemoryBuffer> MB;
ASSERT_THAT_ERROR(makeMemoryBuffer().moveInto(MB), Succeeded());

[dexonsmith]

I posted a handful of usage examples separately at https://reviews.llvm.org/D112280.

[dblaikie]

Sounds nice to have!

llvm/include/llvm/Support/Error.h

584

Should this be && qualified, so it can only be called on a temporary Expected, since it's going to consume it? (I mean takeError isn't rvalue ref qualified and get isn't - so it's not clear it has to be)

[dexonsmith]

llvm/include/llvm/Support/Error.h

584

Should this be && qualified, so it can only be called on a temporary Expected, since it's going to consume it?

Maybe? I can't think of why you'd want to call this on a non-temporary. I don't see any problems with doing it though... and I suppose it'd guide people toward the pattern of not naming it?

Happy to do it; if you feel strongly, let me know; otherwise I'll stew for a bit. Leaning toward your suggestion, and then && can be removed later if it turns out there's a good reason to allow it on lvalues.

(I mean takeError isn't rvalue ref qualified and get isn't - so it's not clear it has to be)

I don't feel like && is quite right for these APIs though. You need to name the value if you're going to both check the error and extract the value. Any std::move()s after that would feel like extra boilerplate.

[dblaikie]

llvm/include/llvm/Support/Error.h

584

Fair ponit re: the other operations clearly don't consume the /whole/ object, you have to use both operations. Whereas this new API does consume the whole object, there's nothing else you can do with it after using this operation, I think? (since the value and the error have been moved-from)

So, yeah, at that point I do lean in favor of &&. Not strongly, but yeah, probably enough to encourage it, give it a go, we can remove it later as you say.

llvm/docs/ProgrammersManual.rst

Error processFormattedFile(StringRef Path) {
     // On success, grab a reference to the file and continue.
     auto &File = *FileOrErr;
     ...
   }
 
 This second form is often more readable for functions that involve multiple
 ``Expected<T>`` values as it limits the indentation required.
 
+If an ``Expected<T>`` value will be moved into an existing variable then the
+``moveInto()`` method avoids the need to name an extra variable.  This is
+useful to enable ``operator->()`` the ``Expected<T>`` value has pointer-like
+semantics.  For example:
+
+.. code-block:: c++
+
+  Expected<std::unique_ptr<MemoryBuffer>> openBuffer(StringRef Path);
+  Error processBuffer(StringRef Buffer);
+
+  Error processBufferAtPath(StringRef Path) {
+    // Try to open a buffer.
+    std::unique_ptr<MemoryBuffer> MB;
+    if (auto Err = openBuffer(Path).moveInto(MB))
+      // On error, return the Error value.
+      return Err;
+    // On success, use MB.
+    return processContent(MB->getBuffer());
+  }
+
+This third form works with any type that can be assigned to from ``T&&``. This
+can be useful if the ``Expected<T>`` value needs to be stored an already-declared
+``Optional<T>``. For example:
+
+.. code-block:: c++
+
+  Expected<StringRef> extractClassName(StringRef Definition);
+  struct ClassData {
+    StringRef Definition;
+    Optional<StringRef> LazyName;
+    ...
+    Error initialize() {
+      if (auto Err = extractClassName(Path).moveInto(LazyName))
+        // On error, return the Error value.
+        return Err;
+      // On success, LazyName has been initialized.
+      ...
+    }
+  };
+
 All ``Error`` instances, whether success or failure, must be either checked or
 moved from (via ``std::move`` or a return) before they are destructed.
 Accidentally discarding an unchecked error will cause a program abort at the
 point where the unchecked value's destructor is run, making it easy to identify
 and fix violations of this rule.
 
 Success values are considered checked once they have been tested (by invoking
 the boolean conversion operator):

llvm/include/llvm/Support/Error.h

#endif
   }
 
   /// Returns a const reference to the stored T value.
   const_reference get() const {
     assertIsChecked();
     return const_cast<Expected<T> *>(this)->get();
   }
 
+  /// Returns \a takeError() after moving the held T (if any) into \p V.
+  template <class OtherT>
+  Error moveInto(OtherT &Value,
+                 std::enable_if_t<std::is_assignable<OtherT &, T &&>::value> * =
+                     nullptr) && {
+    if (*this)
+      Value = std::move(get());
+    return takeError();
+  }
+
   /// Check that this Expected<T> is an error of type ErrT.
   template <typename ErrT> bool errorIsA() const {
     return HasError && (*getErrorStorage())->template isA<ErrT>();
   }
 
   /// Take ownership of the stored error.
   /// After calling this the Expected<T> is in an indeterminate state that can
   /// only be safely destructed. No further calls (beside the destructor) should

llvm/unittests/Support/ErrorTest.cpp

TEST(Error, SubtypeStringErrorTest) {
 
   auto E4 = joinErrors(make_error<TestDebugError>(test_error_code::error_1,
                                                   "Detailed information"),
                        make_error<TestDebugError>(test_error_code::error_2));
   EXPECT_EQ(toString(std::move(E4)), "Error 1. Detailed information\n"
                                      "Error 2.");
 }
 
+static Error createAnyError() {
+  return errorCodeToError(test_error_code::unspecified);
+}
+
+struct MoveOnlyBox {
+  Optional<int> Box;
+
+  explicit MoveOnlyBox(int I) : Box(I) {}
+  MoveOnlyBox() = default;
+  MoveOnlyBox(MoveOnlyBox &&) = default;
+  MoveOnlyBox &operator=(MoveOnlyBox &&) = default;
+
+  MoveOnlyBox(const MoveOnlyBox &) = delete;
+  MoveOnlyBox &operator=(const MoveOnlyBox &) = delete;
+
+  bool operator==(const MoveOnlyBox &RHS) const {
+    if (bool(Box) != bool(RHS.Box))
+      return false;
+    return Box ? *Box == *RHS.Box : false;
+  }
+};
+
+TEST(Error, moveInto) {
+  // Use MoveOnlyBox as the T in Expected<T>.
+  auto make = [](int I) -> Expected<MoveOnlyBox> { return MoveOnlyBox(I); };
+  auto makeFailure = []() -> Expected<MoveOnlyBox> { return createAnyError(); };
+
+  {
+    MoveOnlyBox V;
+
+    // Failure with no prior value.
+    EXPECT_THAT_ERROR(makeFailure().moveInto(V), Failed());
+    EXPECT_EQ(None, V.Box);
+
+    // Success with no prior value.
+    EXPECT_THAT_ERROR(make(5).moveInto(V), Succeeded());
+    EXPECT_EQ(5, V.Box);
+
+    // Success with an existing value.
+    EXPECT_THAT_ERROR(make(7).moveInto(V), Succeeded());
+    EXPECT_EQ(7, V.Box);
+
+    // Failure with an existing value. Might be nice to assign a
+    // default-constructed value in this case, but for now it's being left
+    // alone.
+    EXPECT_THAT_ERROR(makeFailure().moveInto(V), Failed());
+    EXPECT_EQ(7, V.Box);
+  }
+
+  // Check that this works with optionals too.
+  {
+    // Same cases as above.
+    Optional<MoveOnlyBox> MaybeV;
+    EXPECT_THAT_ERROR(makeFailure().moveInto(MaybeV), Failed());
+    EXPECT_EQ(None, MaybeV);
+
+    EXPECT_THAT_ERROR(make(5).moveInto(MaybeV), Succeeded());
+    EXPECT_EQ(MoveOnlyBox(5), MaybeV);
+
+    EXPECT_THAT_ERROR(make(7).moveInto(MaybeV), Succeeded());
+    EXPECT_EQ(MoveOnlyBox(7), MaybeV);
+
+    EXPECT_THAT_ERROR(makeFailure().moveInto(MaybeV), Failed());
+    EXPECT_EQ(MoveOnlyBox(7), MaybeV);
+  }
+}
+
 } // namespace