Prevent at compile time converting from Error::success() to Expected<T>

This would trigger an assertion at runtime otherwise.

Differential Revision: https://reviews.llvm.org/D26482

llvm-svn: 286562

3 files changed, 30 insertions, 17 deletions

diff --git a/llvm/include/llvm/Support/Error.h b/llvm/include/llvm/Support/Error.h
index e4f352ab95e..a09a212c12f 100644
--- a/llvm/include/llvm/Support/Error.h
+++ b/llvm/include/llvm/Support/Error.h
@@ -27,6 +27,7 @@ namespace llvm {
 
 class Error;
 class ErrorList;
+class ErrorSuccess;
 
 /// Base class for error info classes. Do not extend this directly: Extend
 /// the ErrorInfo template subclass instead.
@@ -157,9 +158,8 @@ protected:
   }
 
 public:
-  /// Create a success value. This is equivalent to calling the default
-  /// constructor, but should be preferred for readability where possible.
-  static Error success() { return Error(); }
+  /// Create a success value.
+  static ErrorSuccess success();
 
   // Errors are not copy-constructable.
   Error(const Error &Other) = delete;
@@ -279,6 +279,13 @@ private:
   ErrorInfoBase *Payload;
 };
 
+/// Subclass of Error for the sole purpose of identifying the success path in
+/// the type system. This allows to catch invalid conversion to Expected<T> at
+/// compile time.
+class ErrorSuccess : public Error {};
+
+inline ErrorSuccess Error::success() { return ErrorSuccess(); }
+
 /// Make a Error instance representing failure using the given error info
 /// type.
 template <typename ErrT, typename... ArgTs> Error make_error(ArgTs &&... Args) {
@@ -645,6 +652,11 @@ public:
     new (getErrorStorage()) error_type(Err.takePayload());
   }
 
+  /// Forbid to convert from Error::success() implicitly, this avoids having
+  /// Expected<T> foo() { return Error::success(); } which compiles otherwise
+  /// but triggers the assertion above.
+  Expected(ErrorSuccess) = delete;
+
   /// Create an Expected<T> success value from the given OtherT value, which
   /// must be convertible to T.
   template <typename OtherT>
diff --git a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
index d2c2ec774c1..270a1c49997 100644
--- a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
+++ b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
@@ -4330,7 +4330,7 @@ Expected<std::string> BitcodeReader::parseTriple() {
     case BitstreamEntry::Error:
       return error("Malformed block");
     case BitstreamEntry::EndBlock:
-      return Error::success();
+      return "";
 
     case BitstreamEntry::SubBlock:
       if (Entry.ID == bitc::MODULE_BLOCK_ID)
@@ -4363,14 +4363,14 @@ Expected<std::string> BitcodeReader::parseIdentificationBlock() {
     // we need to make sure we aren't at the end of the stream before calling
     // advance, otherwise we'll get an error.
     if (Stream.AtEndOfStream())
-      return Error::success();
+      return "";
 
     BitstreamEntry Entry = Stream.advance();
     switch (Entry.Kind) {
     case BitstreamEntry::Error:
       return error("Malformed block");
     case BitstreamEntry::EndBlock:
-      return Error::success();
+      return "";
 
     case BitstreamEntry::SubBlock:
       if (Entry.ID == bitc::IDENTIFICATION_BLOCK_ID) {
@@ -4421,7 +4421,7 @@ Expected<bool> BitcodeReader::hasObjCCategory() {
     case BitstreamEntry::Error:
       return error("Malformed block");
     case BitstreamEntry::EndBlock:
-      return Error::success();
+      return false;
 
     case BitstreamEntry::SubBlock:
       if (Entry.ID == bitc::MODULE_BLOCK_ID)
diff --git a/llvm/unittests/Support/ErrorTest.cpp b/llvm/unittests/Support/ErrorTest.cpp
index 7468a857b89..1fe901745ca 100644
--- a/llvm/unittests/Support/ErrorTest.cpp
+++ b/llvm/unittests/Support/ErrorTest.cpp
@@ -97,14 +97,15 @@ static void handleCustomErrorUPVoid(std::unique_ptr<CustomError> CE) {}
 // Test that success values implicitly convert to false, and don't cause crashes
 // once they've been implicitly converted.
 TEST(Error, CheckedSuccess) {
-  Error E;
+  Error E = Error::success();
   EXPECT_FALSE(E) << "Unexpected error while testing Error 'Success'";
 }
 
 // Test that unchecked succes values cause an abort.
 #ifndef NDEBUG
 TEST(Error, UncheckedSuccess) {
-  EXPECT_DEATH({ Error E; }, "Program aborted due to an unhandled Error:")
+  EXPECT_DEATH({ Error E = Error::success(); },
+               "Program aborted due to an unhandled Error:")
       << "Unchecked Error Succes value did not cause abort()";
 }
 #endif
@@ -121,7 +122,7 @@ void errAsOutParamHelper(Error &Err) {
 
 // Test that ErrorAsOutParameter sets the checked flag on construction.
 TEST(Error, ErrorAsOutParameterChecked) {
-  Error E;
+  Error E = Error::success();
   errAsOutParamHelper(E);
   (void)!!E;
 }
@@ -129,7 +130,7 @@ TEST(Error, ErrorAsOutParameterChecked) {
 // Test that ErrorAsOutParameter clears the checked flag on destruction.
 #ifndef NDEBUG
 TEST(Error, ErrorAsOutParameterUnchecked) {
-  EXPECT_DEATH({ Error E; errAsOutParamHelper(E); },
+  EXPECT_DEATH({ Error E = Error::success(); errAsOutParamHelper(E); },
                "Program aborted due to an unhandled Error:")
       << "ErrorAsOutParameter did not clear the checked flag on destruction.";
 }
@@ -197,31 +198,31 @@ TEST(Error, HandlerTypeDeduction) {
 
   handleAllErrors(
       make_error<CustomError>(42),
-      [](const CustomError &CE) mutable { return Error::success(); });
+      [](const CustomError &CE) mutable  -> Error { return Error::success(); });
 
   handleAllErrors(make_error<CustomError>(42),
                   [](const CustomError &CE) mutable {});
 
   handleAllErrors(make_error<CustomError>(42),
-                  [](CustomError &CE) { return Error::success(); });
+                  [](CustomError &CE) -> Error { return Error::success(); });
 
   handleAllErrors(make_error<CustomError>(42), [](CustomError &CE) {});
 
   handleAllErrors(make_error<CustomError>(42),
-                  [](CustomError &CE) mutable { return Error::success(); });
+                  [](CustomError &CE) mutable -> Error { return Error::success(); });
 
   handleAllErrors(make_error<CustomError>(42), [](CustomError &CE) mutable {});
 
   handleAllErrors(
       make_error<CustomError>(42),
-      [](std::unique_ptr<CustomError> CE) { return Error::success(); });
+      [](std::unique_ptr<CustomError> CE) -> Error { return Error::success(); });
 
   handleAllErrors(make_error<CustomError>(42),
                   [](std::unique_ptr<CustomError> CE) {});
 
   handleAllErrors(
       make_error<CustomError>(42),
-      [](std::unique_ptr<CustomError> CE) mutable { return Error::success(); });
+      [](std::unique_ptr<CustomError> CE) mutable -> Error { return Error::success(); });
 
   handleAllErrors(make_error<CustomError>(42),
                   [](std::unique_ptr<CustomError> CE) mutable {});
@@ -365,7 +366,7 @@ TEST(Error, CheckJoinErrors) {
 
 // Test that we can consume success values.
 TEST(Error, ConsumeSuccess) {
-  Error E;
+  Error E = Error::success();
   consumeError(std::move(E));
 }