[Support] Add a checked flag to Expected<T>, require checks before access or

destruction.

This makes the Expected<T> class behave like Error, even when in success mode.
Expected<T> values must be checked to see whether they contain an error prior
to being dereferenced, assigned to, or destructed.

llvm-svn: 265446

2 files changed, 136 insertions, 27 deletions

diff --git a/llvm/include/llvm/Support/Error.h b/llvm/include/llvm/Support/Error.h
index fb17a0dbe8a..04d6132bb7f 100644
--- a/llvm/include/llvm/Support/Error.h
+++ b/llvm/include/llvm/Support/Error.h
@@ -131,6 +131,10 @@ class Error {
   template <typename... HandlerTs>
   friend Error handleErrors(Error E, HandlerTs &&... Handlers);
 
+  // Expected<T> needs to be able to steal the payload when constructed from an
+  // error.
+  template <typename T> class Expected;
+
 public:
   /// Create a success value. Prefer using 'Error::success()' for readability
   /// where possible.
@@ -587,6 +591,8 @@ template <class T> class Expected {
   static const bool isRef = std::is_reference<T>::value;
   typedef ReferenceStorage<typename std::remove_reference<T>::type> wrap;
 
+  typedef std::unique_ptr<ErrorInfoBase> error_type;
+
 public:
   typedef typename std::conditional<isRef, wrap, T>::type storage_type;
 
@@ -598,8 +604,14 @@ private:
 
 public:
   /// Create an Expected<T> error value from the given Error.
-  Expected(Error Err) : HasError(true) {
-    assert(Err && "Cannot create Expected from Error success value.");
+  Expected(Error Err)
+      : HasError(true)
+#ifndef NDEBUG
+        ,
+        Checked(false)
+#endif
+  {
+    assert(Err && "Cannot create Expected<T> from Error success value.");
     new (getErrorStorage()) Error(std::move(Err));
   }
 
@@ -609,7 +621,12 @@ public:
   Expected(OtherT &&Val,
            typename std::enable_if<std::is_convertible<OtherT, T>::value>::type
                * = nullptr)
-      : HasError(false) {
+      : HasError(false)
+#ifndef NDEBUG
+        ,
+        Checked(false)
+#endif
+  {
     new (getStorage()) storage_type(std::move(Val));
   }
 
@@ -643,20 +660,32 @@ public:
 
   /// Destroy an Expected<T>.
   ~Expected() {
+    assertIsChecked();
     if (!HasError)
       getStorage()->~storage_type();
     else
-      getErrorStorage()->~Error();
+      getErrorStorage()->~error_type();
   }
 
   /// \brief Return false if there is an error.
-  explicit operator bool() const { return !HasError; }
+  explicit operator bool() {
+#ifndef NDEBUG
+    Checked = !HasError;
+#endif
+    return !HasError;
+  }
 
   /// \brief Returns a reference to the stored T value.
-  reference get() { return *getStorage(); }
+  reference get() {
+    assertIsChecked();
+    return *getStorage();
+  }
 
   /// \brief Returns a const reference to the stored T value.
-  const_reference get() const { return const_cast<Expected<T> *>(this)->get(); }
+  const_reference get() const {
+    assertIsChecked();
+    return const_cast<Expected<T> *>(this)->get();
+  }
 
   /// \brief Check that this Expected<T> is an error of type ErrT.
   template <typename ErrT> bool errorIsA() const {
@@ -668,20 +697,35 @@ public:
   /// only be safely destructed. No further calls (beside the destructor) should
   /// be made on the Expected<T> vaule.
   Error takeError() {
-    return HasError ? std::move(*getErrorStorage()) : Error();
+#ifndef NDEBUG
+    Checked = true;
+#endif
+    return HasError ? Error(std::move(*getErrorStorage())) : Error::success();
   }
 
   /// \brief Returns a pointer to the stored T value.
-  pointer operator->() { return toPointer(getStorage()); }
+  pointer operator->() {
+    assertIsChecked();
+    return toPointer(getStorage());
+  }
 
   /// \brief Returns a const pointer to the stored T value.
-  const_pointer operator->() const { return toPointer(getStorage()); }
+  const_pointer operator->() const {
+    assertIsChecked();
+    return toPointer(getStorage());
+  }
 
   /// \brief Returns a reference to the stored T value.
-  reference operator*() { return *getStorage(); }
+  reference operator*() {
+    assertIsChecked();
+    return *getStorage();
+  }
 
   /// \brief Returns a const reference to the stored T value.
-  const_reference operator*() const { return *getStorage(); }
+  const_reference operator*() const {
+    assertIsChecked();
+    return *getStorage();
+  }
 
 private:
   template <class T1>
@@ -695,18 +739,22 @@ private:
   }
 
   template <class OtherT> void moveConstruct(Expected<OtherT> &&Other) {
-    if (!Other.HasError) {
-      // Get the other value.
-      HasError = false;
+    HasError = Other.HasError;
+
+#ifndef NDEBUG
+    Checked = false;
+    Other.Checked = true;
+#endif
+
+    if (!HasError)
       new (getStorage()) storage_type(std::move(*Other.getStorage()));
-    } else {
-      // Get other's error.
-      HasError = true;
-      new (getErrorStorage()) Error(Other.takeError());
-    }
+    else
+      new (getErrorStorage()) error_type(std::move(*Other.getErrorStorage()));
   }
 
   template <class OtherT> void moveAssign(Expected<OtherT> &&Other) {
+    assertIsChecked();
+
     if (compareThisIfSameType(*this, Other))
       return;
 
@@ -732,16 +780,35 @@ private:
     return reinterpret_cast<const storage_type *>(TStorage.buffer);
   }
 
-  Error *getErrorStorage() {
+  error_type *getErrorStorage() {
     assert(HasError && "Cannot get error when a value exists!");
-    return reinterpret_cast<Error *>(ErrorStorage.buffer);
+    return reinterpret_cast<error_type *>(ErrorStorage.buffer);
+  }
+
+  void assertIsChecked() {
+#ifndef NDEBUG
+    if (!Checked) {
+      dbgs() << "Expected<T> must be checked before access or destruction.\n";
+      if (HasError) {
+        dbgs() << "Unchecked Expected<T> contained error:\n";
+        (*getErrorStorage())->log(dbgs());
+      } else
+        dbgs() << "Expected<T> value was in success state. (Note: Expected<T> "
+                  "values in success mode must still be checked prior to being "
+                  "destroyed).\n";
+      abort();
+    }
+#endif
   }
 
   union {
     AlignedCharArrayUnion<storage_type> TStorage;
-    AlignedCharArrayUnion<Error> ErrorStorage;
+    AlignedCharArrayUnion<error_type> ErrorStorage;
   };
   bool HasError : 1;
+#ifndef NDEBUG
+  bool Checked : 1;
+#endif
 };
 
 /// This class wraps a std::error_code in a Error.
diff --git a/llvm/unittests/Support/ErrorTest.cpp b/llvm/unittests/Support/ErrorTest.cpp
index 7d714777c65..6a1400aa540 100644
--- a/llvm/unittests/Support/ErrorTest.cpp
+++ b/llvm/unittests/Support/ErrorTest.cpp
@@ -401,13 +401,47 @@ TEST(Error, ExitOnError) {
       << "exitOnError returned an unexpected error result";
 }
 
-// Test Expected<T> in success mode.
-TEST(Error, ExpectedInSuccessMode) {
+// Test Checked Expected<T> in success mode.
+TEST(Error, CheckedExpectedInSuccessMode) {
   Expected<int> A = 7;
   EXPECT_TRUE(!!A) << "Expected with non-error value doesn't convert to 'true'";
+  // Access is safe in second test, since we checked the error in the first.
   EXPECT_EQ(*A, 7) << "Incorrect Expected non-error value";
 }
 
+// Test Unchecked Expected<T> in success mode.
+// We expect this to blow up the same way Error would.
+// Test runs in debug mode only.
+#ifndef NDEBUG
+TEST(Error, UncheckedExpectedInSuccessModeDestruction) {
+  EXPECT_DEATH({ Expected<int> A = 7; },
+               "Expected<T> must be checked before access or destruction.")
+    << "Unchecekd Expected<T> success value did not cause an abort().";
+}
+#endif
+
+// Test Unchecked Expected<T> in success mode.
+// We expect this to blow up the same way Error would.
+// Test runs in debug mode only.
+#ifndef NDEBUG
+TEST(Error, UncheckedExpectedInSuccessModeAccess) {
+  EXPECT_DEATH({ Expected<int> A = 7; *A; },
+               "Expected<T> must be checked before access or destruction.")
+    << "Unchecekd Expected<T> success value did not cause an abort().";
+}
+#endif
+
+// Test Unchecked Expected<T> in success mode.
+// We expect this to blow up the same way Error would.
+// Test runs in debug mode only.
+#ifndef NDEBUG
+TEST(Error, UncheckedExpectedInSuccessModeAssignment) {
+  EXPECT_DEATH({ Expected<int> A = 7; A = 7; },
+               "Expected<T> must be checked before access or destruction.")
+    << "Unchecekd Expected<T> success value did not cause an abort().";
+}
+#endif
+
 // Test Expected<T> in failure mode.
 TEST(Error, ExpectedInFailureMode) {
   Expected<int> A = make_error<CustomError>(42);
@@ -423,7 +457,7 @@ TEST(Error, ExpectedInFailureMode) {
 #ifndef NDEBUG
 TEST(Error, AccessExpectedInFailureMode) {
   Expected<int> A = make_error<CustomError>(42);
-  EXPECT_DEATH(*A, "!HasError && \"Cannot get value when an error exists!\"")
+  EXPECT_DEATH(*A, "Expected<T> must be checked before access or destruction.")
       << "Incorrect Expected error value";
   consumeError(A.takeError());
 }
@@ -435,7 +469,7 @@ TEST(Error, AccessExpectedInFailureMode) {
 #ifndef NDEBUG
 TEST(Error, UnhandledExpectedInFailureMode) {
   EXPECT_DEATH({ Expected<int> A = make_error<CustomError>(42); },
-               "Program aborted due to an unhandled Error:")
+               "Expected<T> must be checked before access or destruction.")
       << "Unchecked Expected<T> failure value did not cause an abort()";
 }
 #endif
@@ -446,10 +480,18 @@ TEST(Error, ExpectedCovariance) {
   class D : public B {};
 
   Expected<B *> A1(Expected<D *>(nullptr));
+  // Check A1 by converting to bool before assigning to it.
+  (void)!!A1;
   A1 = Expected<D *>(nullptr);
+  // Check A1 again before destruction.
+  (void)!!A1;
 
   Expected<std::unique_ptr<B>> A2(Expected<std::unique_ptr<D>>(nullptr));
+  // Check A2 by converting to bool before assigning to it.
+  (void)!!A2;
   A2 = Expected<std::unique_ptr<D>>(nullptr);
+  // Check A2 again before destruction.
+  (void)!!A2;
 }
 
 TEST(Error, ErrorCodeConversions) {