[Error] Add non-consuming toString

[nikic]

There are some places that want to convert an Error to string, but still retain the original Error object, for example to emit a non-fatal warning.

This currently isn't possible, because the entire Error infra is move-based. And what people end up doing in this case is to move the Error... twice.

This patch introduces a toStringWithoutConsuming() function to accommodate this use case. This also requires some infrastructure that allows visiting Errors without consuming them.

[llvmbot]

@llvm/pr-subscribers-debuginfo

@llvm/pr-subscribers-llvm-support

Author: Nikita Popov (nikic)

Changes

There are some places that want to convert an Error to string, but still retain the original Error object, for example to emit a non-fatal warning.

This currently isn't possible, because the entire Error infra is move-based. And what people end up doing in this case is to move the Error... twice.

This patch introduces a toStringWithoutConsuming() function to accommodate this use case. This also requires some infrastructure that allows visiting Errors without consuming them.

---

Full diff: https://github.com/llvm/llvm-project/pull/95375.diff

5 Files Affected:

• (modified) llvm/include/llvm/Support/Error.h (+28)
• (modified) llvm/lib/Support/Error.cpp (+8)
• (modified) llvm/tools/dsymutil/DwarfLinkerForBinary.cpp (+2-2)
• (modified) llvm/tools/llvm-pdbutil/DumpOutputStyle.cpp (+2-2)
• (modified) llvm/unittests/Support/ErrorTest.cpp (+9-1)

diff --git a/llvm/include/llvm/Support/Error.h b/llvm/include/llvm/Support/Error.h
index 1fa0d8cb709cc..5120f6ab57c03 100644
--- a/llvm/include/llvm/Support/Error.h
+++ b/llvm/include/llvm/Support/Error.h
@@ -166,6 +166,9 @@ class [[nodiscard]] Error {
   // handleErrors needs to be able to set the Checked flag.
   template <typename... HandlerTs>
   friend Error handleErrors(Error E, HandlerTs &&... Handlers);
+  // visitErrors needs direct access to the payload.
+  template <typename HandlerT>
+  friend void visitErrors(const Error &E, HandlerT H);
 
   // Expected<T> needs to be able to steal the payload when constructed from an
   // error.
@@ -369,6 +372,10 @@ class ErrorList final : public ErrorInfo<ErrorList> {
   // ErrorList.
   template <typename... HandlerTs>
   friend Error handleErrors(Error E, HandlerTs &&... Handlers);
+  // visitErrors needs to be able to iterate the payload list of an
+  // ErrorList.
+  template <typename HandlerT>
+  friend void visitErrors(const Error &E, HandlerT H);
 
   // joinErrors is implemented in terms of join.
   friend Error joinErrors(Error, Error);
@@ -977,6 +984,23 @@ inline void handleAllErrors(Error E) {
   cantFail(std::move(E));
 }
 
+/// Visit all the ErrorInfo(s) contained in E by passing them to the respective
+/// handler, without consuming the error.
+template <typename HandlerT> void visitErrors(const Error &E, HandlerT H) {
+  const ErrorInfoBase *Payload = E.getPtr();
+  if (!Payload)
+    return;
+
+  if (Payload->isA<ErrorList>()) {
+    const ErrorList &List = static_cast<const ErrorList &>(*Payload);
+    for (const auto &P : List.Payloads)
+      H(*P);
+    return;
+  }
+
+  return H(*Payload);
+}
+
 /// Handle any errors (if present) in an Expected<T>, then try a recovery path.
 ///
 /// If the incoming value is a success value it is returned unmodified. If it
@@ -1031,6 +1055,10 @@ void logAllUnhandledErrors(Error E, raw_ostream &OS, Twine ErrorBanner = {});
 /// is used to separate error messages.
 std::string toString(Error E);
 
+/// Like toString(), but does not consume the error. This can be used to print
+/// a warning while retaining the original error object.
+std::string toStringWithoutConsuming(const Error &E);
+
 /// Consume a Error without doing anything. This method should be used
 /// only where an error can be considered a reasonable and expected return
 /// value.
diff --git a/llvm/lib/Support/Error.cpp b/llvm/lib/Support/Error.cpp
index 34ec31e3b833f..93481ca916d2a 100644
--- a/llvm/lib/Support/Error.cpp
+++ b/llvm/lib/Support/Error.cpp
@@ -82,6 +82,14 @@ std::string toString(Error E) {
   return join(Errors.begin(), Errors.end(), "\n");
 }
 
+std::string toStringWithoutConsuming(const Error &E) {
+  SmallVector<std::string, 2> Errors;
+  visitErrors(E, [&Errors](const ErrorInfoBase &EI) {
+    Errors.push_back(EI.message());
+  });
+  return join(Errors.begin(), Errors.end(), "\n");
+}
+
 std::error_code ErrorList::convertToErrorCode() const {
   return std::error_code(static_cast<int>(ErrorErrorCode::MultipleErrors),
                          getErrorErrorCat());
diff --git a/llvm/tools/dsymutil/DwarfLinkerForBinary.cpp b/llvm/tools/dsymutil/DwarfLinkerForBinary.cpp
index 83473704398df..f6a35708dc076 100644
--- a/llvm/tools/dsymutil/DwarfLinkerForBinary.cpp
+++ b/llvm/tools/dsymutil/DwarfLinkerForBinary.cpp
@@ -147,7 +147,7 @@ DwarfLinkerForBinary::loadObject(const DebugMapObject &Obj,
   if (!ObjectEntry) {
     auto Err = ObjectEntry.takeError();
     reportWarning(Twine(Obj.getObjectFilename()) + ": " +
-                      toString(std::move(Err)),
+                      toStringWithoutConsuming(Err),
                   Obj.getObjectFilename());
     return errorToErrorCode(std::move(Err));
   }
@@ -156,7 +156,7 @@ DwarfLinkerForBinary::loadObject(const DebugMapObject &Obj,
   if (!Object) {
     auto Err = Object.takeError();
     reportWarning(Twine(Obj.getObjectFilename()) + ": " +
-                      toString(std::move(Err)),
+                      toStringWithoutConsuming(Err),
                   Obj.getObjectFilename());
     return errorToErrorCode(std::move(Err));
   }
diff --git a/llvm/tools/llvm-pdbutil/DumpOutputStyle.cpp b/llvm/tools/llvm-pdbutil/DumpOutputStyle.cpp
index 1cecbfc463fec..b2362ecb75703 100644
--- a/llvm/tools/llvm-pdbutil/DumpOutputStyle.cpp
+++ b/llvm/tools/llvm-pdbutil/DumpOutputStyle.cpp
@@ -1494,13 +1494,13 @@ Error DumpOutputStyle::dumpModuleSymsForPdb() {
           if (auto EC = Visitor.visitSymbolStreamFiltered(ModS.getSymbolArray(),
                                                           Filter)) {
             P.formatLine("Error while processing symbol records.  {0}",
-                         toString(std::move(EC)));
+                         toStringWithoutConsuming(EC));
             return EC;
           }
         } else if (auto EC = Visitor.visitSymbolStream(ModS.getSymbolArray(),
                                                        SS.Offset)) {
           P.formatLine("Error while processing symbol records.  {0}",
-                       toString(std::move(EC)));
+                       toStringWithoutConsuming(EC));
           return EC;
         }
         return Error::success();
diff --git a/llvm/unittests/Support/ErrorTest.cpp b/llvm/unittests/Support/ErrorTest.cpp
index 5d866a67c0eaa..aac9f4afdac01 100644
--- a/llvm/unittests/Support/ErrorTest.cpp
+++ b/llvm/unittests/Support/ErrorTest.cpp
@@ -738,17 +738,25 @@ TEST(Error, ErrorCodeConversions) {
 
 // Test that error messages work.
 TEST(Error, ErrorMessage) {
-  EXPECT_EQ(toString(Error::success()), "");
+  Error E0 = Error::success();
+  EXPECT_EQ(toStringWithoutConsuming(E0), "");
+  EXPECT_EQ(toString(std::move(E0)), "");
 
   Error E1 = make_error<CustomError>(0);
+  EXPECT_EQ(toStringWithoutConsuming(E1), "CustomError {0}");
   EXPECT_EQ(toString(std::move(E1)), "CustomError {0}");
 
   Error E2 = make_error<CustomError>(0);
+  visitErrors(E2, [](const ErrorInfoBase &EI) {
+    EXPECT_EQ(EI.message(), "CustomError {0}");
+  });
   handleAllErrors(std::move(E2), [](const CustomError &CE) {
     EXPECT_EQ(CE.message(), "CustomError {0}");
   });
 
   Error E3 = joinErrors(make_error<CustomError>(0), make_error<CustomError>(1));
+  EXPECT_EQ(toStringWithoutConsuming(E3), "CustomError {0}\n"
+                                          "CustomError {1}");
   EXPECT_EQ(toString(std::move(E3)), "CustomError {0}\n"
                                      "CustomError {1}");
 }

[pogo59]

LGTM, one test suggestion.

[pogo59]

I'd want to keep this, just to have one example that doesn't go through toStringWithoutConsuming or visitErrors.