Make ErrorList class default constructible and add simple push_back() method

[sgraenitz]

Summary

ErrorList appears to have some unused potential. It could be a bit easier to use I think. This change proposes the following idiomatic usage. (My actual use-case is still in progress.) Thoughts?

Error idiomaticUse() {
  ErrorList Errs;
  Errs.push_back(...);
  return make_error<ErrorList>(std::move(Errs));
}

[lhames]

This seems like a nice change for ErrorList.

What are you using it for though? I actually keep thinking I would like to remove ErrorList, as it has some awkward properties: it doesn't interact sensibly with isA tests (isA should really be changed to a pair of utilities: containsA, and containsOnly), and it means that Expected values have a weird asymmetry in the success and failure case: They contain exactly one success value, or (potentially) many failure values. This makes it tricky, if not impossible, to safely write a utility function that maps failures to recovery values:

int attemptWithRecovery(Expected<int> Val) {
  if (Val)
    return *Val;
  int Fallback = 0;
  handleAllErrors(Val,
               [&](StringError &SE) { Fallback = 1; },      // What should Fallback's value be if Val contains
               [&](ErrorInfoBase &EIB) { Fallback = 0; });  // both a StringError and another Error?
  return Fallback;
}

In this case the answer is "the fallback value will be based on the last error in the list", but that means you've ignored the recovery mappings for all the other errors, which probably isn't what you wanted.

In my experience, whenever I have used ErrorList it was really because I wanted to be able to report multiple errors, never because I wanted to be able to recover from them. However I think this kind of reporting is better handled by a logging idiom (like ExecutionSession::reportError).

[lhames]

Actually, since I'm pontificating about Error on this thread anyway, the other big change that I would like to see (which is also impacted by ErrorList) would be to ditch handleErrors entirely. If we had the "one Error/Expected == one failure" invariant, we could switch to an "err_cast" idiom:

Instead of:

Error foo();

handleErrors(foo(),
             [](ErrType1 &E1) {
               ...
             },
             [](ErrType2 &E2) {
               ...
             },
             ...);

We could have:

auto Err = foo();
if (auto E1 = err_cast<ErrType1>(Err)) {
  ..
} else if (auto E2 = err_cast<ErrType2>(Err)) {
  ...
}

There are pros and cons to each scheme:
handleErrors -- Pros: Failure value names (E1, E2) scoped to handler only. Cons: Not intuitive -- you need to read the docs to figure out how to use it. Can't be used in C API.
err_cast -- Pros: Intuitive, and C API implementation would be a straightforward mapping. Cons: Failure value names leak into else-if scopes.

On balance I think err_cast is a better answer. But we need to remove (or at least demote) ErrorList before we could switch.

[sgraenitz]

In D72176#1834446, @lhames wrote:

This seems like a nice change for ErrorList.

What are you using it for though?
[...]
In my experience, whenever I have used ErrorList it was really because I wanted to be able to report multiple errors, never because I wanted to be able to recover from them. However I think this kind of reporting is better handled by a logging idiom (like ExecutionSession::reportError).

Yes, basically I'd use it for reporting errors and indeed it's better done like logging. At first, I like how ErrorList packed more errors in the Payload to make a list, that seemed very elegant to me. But you are right, if it adds unnecessary complexity to the primary usage, it probably not the right way to go.

I would like to see (which is also impacted by ErrorList) would be to ditch handleErrors entirely. If we had the "one Error/Expected == one failure" invariant, we could switch to an "err_cast" idiom

That seems like a good idea. Not sure if I ever saw a handleErrors with more than one handler in production code. I agree that if we have to drop ErrorList for it, we should do so and not make it more convenient to use.

llvm/include/llvm/Support/Error.h

class ErrorList final : public ErrorInfo<ErrorList> {
   // ErrorList.
   template <typename... HandlerTs>
   friend Error handleErrors(Error E, HandlerTs &&... Handlers);
 
   // joinErrors is implemented in terms of join.
   friend Error joinErrors(Error, Error);
 
 public:
+  ErrorList() = default;
+
   void log(raw_ostream &OS) const override {
     OS << "Multiple errors:\n";
     for (auto &ErrPayload : Payloads) {
       ErrPayload->log(OS);
       OS << "\n";
     }
   }
 
   std::error_code convertToErrorCode() const override;
 
+  void push_back(Error Other) {
+    if (Other.isA<ErrorList>()) {
+      auto OtherPayload = Other.takePayload();
+      auto &OtherErrorList = static_cast<ErrorList &>(*OtherPayload);
+      for (std::unique_ptr<ErrorInfoBase> &P : OtherErrorList.Payloads)
+        Payloads.push_back(std::move(P));
+    } else {
+      Payloads.push_back(Other.takePayload());
+    }
+  }
+
   // Used by ErrorInfo::classID.
   static char ID;
 
 private:
   ErrorList(std::unique_ptr<ErrorInfoBase> Payload1,
             std::unique_ptr<ErrorInfoBase> Payload2) {
     assert(!Payload1->isA<ErrorList>() && !Payload2->isA<ErrorList>() &&
            "ErrorList constructor payloads should be singleton errors");

llvm/unittests/Support/ErrorTest.cpp

 #include "llvm/ADT/Twine.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gtest/gtest-spi.h"
 #include "gtest/gtest.h"
 #include <memory>
+#include <sstream>
 
 using namespace llvm;
 
 namespace {
 
 // Custom error class with a default base class and some random 'info' attached.
 class CustomError : public ErrorInfo<CustomError> {
 public:
handleAllErrors(
                 make_error<CustomError>(7))),
         [&](const CustomError &CE) {
           Sum += CE.getInfo();
         });
     EXPECT_EQ(Sum, 28) << "Failed to correctly concatenate error lists.";
   }
 }
 
+// Test direct usage of ErrorList.
+static Error parseInts(std::vector<std::string> Is, void (*H)(int)) {
+  ErrorList Errs;
+  for (const std::string &I : Is) {
+    int Result;
+    std::istringstream SStream(I);
+    SStream >> Result;
+
+    if (SStream.fail()) {
+      Errs.push_back(make_error<StringError>("Failed to parse int: " + I,
+                                             inconvertibleErrorCode()));
+    } else if (!SStream.eof()) {
+      Errs.push_back(
+          make_error<StringError>("Failed to parse full string as int: " + I,
+                                  inconvertibleErrorCode()));
+    } else {
+      H(Result);
+    }
+  }
+  return make_error<ErrorList>(std::move(Errs));
+}
+
+TEST(Error, CheckErrorListIdiomatic) {
+  unsigned Count = 0;
+  Error Err = parseInts({"_", "42", "-0", "1a", "+4", "b5"}, [](int N) {
+    EXPECT_TRUE(N == 42 || N == 0 || N == 4);
+  });
+  handleAllErrors(std::move(Err), [&Count](const StringError &E) { Count++; });
+  EXPECT_EQ(Count, 3u);
+}
+
 // Test that we can consume success values.
 TEST(Error, ConsumeSuccess) {
   Error E = Error::success();
   consumeError(std::move(E));
 }
 
 TEST(Error, ConsumeError) {
   Error E = make_error<CustomError>(7);