Make minor improvements to `ErrorOr` based on usage (#5857)

When using this with filesystem errors, a few issues came up that I'm
fixing here. They're small enough and near enough in code that it didn't
seem worth splitting part.

- It's nice to forward declare custom error types and an API using them
and then define both later. That doesn't work with `requires` but works
fine with `static_assert`, so go back to that pattern here. A test is
added that checks this pattern compiles.

- The `operator*` didn't support moving out of `ErrorOr`, which is
especially important when writing code that is happy with just
`CARBON_CHECK`-failing on any errors. For example, we have a lot of
filesystem code in tests that is made *much* more concise by just using
`*` on a function return and letting the built-in checking ensure no
errors were present. But when the value is move-only, this requires
special overloading. Add that and add a test with a move-only value.

- There wasn't an idiomatic way to do something like `operator*` for
`ErrorOr<Success, ...>`. This PR factors out the checking for `ok()`
into a `Check()` method that can be used to make code more readable that
is intentionally just verifying no error. Also makes the result of
`operator*` `[[nodiscard]]` to improve error messages and help void
accidental bugs.

- The `IsError` and `IsSuccess` test helpers required printable values
which isn't always realistic. Teach the printing logic to be conditional
on some indication of a printable value and gracefully fall back to a
generic string otherwise for testing output.

- The use of the `listener` in `IsError` and `IsSuccess` assumed a
non-null stream. Instead, streaming should go directly to the `listener`
as it is configured to only actually do the output when a stream is
installed. When a stream isn't installed, the previous code would crash
if the `MatchAndExplain` method ended up called without an 'interesting'
stream attached to the listener.

- When doing a `CARBON_CHECK` that there isn't an error, print the error
out as the check failure message. Without this, all the nice error
message work doesn't end up helping the debugging of test code that hits
these errors, etc.

common/error.h

@@ -91,13 +91,17 @@ class [[nodiscard]] ErrorBase : public Printable<ErrorT> {
 //
 // This is nodiscard to enforce error handling prior to destruction.
 template <typename T, typename ErrorT = Error>
-  requires(!std::is_reference_v<ErrorT> &&
-           (std::same_as<ErrorT, Error> ||
-            std::derived_from<ErrorT, ErrorBase<ErrorT>>))
 class [[nodiscard]] ErrorOr {
  public:
   using ValueT = std::remove_reference_t<T>;
 
+  // Check that the custom error type is structured the way we expect. These
+  // need to be `static_assert`s to enable forward declared error types to be
+  // used with `ErrorOr` in function signatures.
+  static_assert(!std::is_reference_v<ErrorT>);
+  static_assert(std::same_as<ErrorT, Error> ||
+                std::derived_from<ErrorT, ErrorBase<ErrorT>>);
+
   // Constructs with an error; the error must not be Error::Success().
   // Implicit for easy construction on returns.
   // NOLINTNEXTLINE(google-explicit-constructor)
@@ -154,20 +158,31 @@ class [[nodiscard]] ErrorOr {
     return std::get<ErrorT>(std::move(val_));
   }
 
+  // Checks that `ok()` is true.
+  // REQUIRES: `ok()` is true.
+  auto Check() const -> void { CARBON_CHECK(ok(), "{0}", error()); }
+
   // Returns the contained value.
   // REQUIRES: `ok()` is true.
-  auto operator*() -> ValueT& {
-    CARBON_CHECK(ok());
+  [[nodiscard]] auto operator*() & -> ValueT& {
+    Check();
     return std::get<StoredT>(val_);
   }
 
   // Returns the contained value.
   // REQUIRES: `ok()` is true.
-  auto operator*() const -> const ValueT& {
-    CARBON_CHECK(ok());
+  [[nodiscard]] auto operator*() const& -> const ValueT& {
+    Check();
     return std::get<StoredT>(val_);
   }
 
+  // Returns the contained value.
+  // REQUIRES: `ok()` is true.
+  [[nodiscard]] auto operator*() && -> ValueT&& {
+    Check();
+    return std::get<StoredT>(std::move(val_));
+  }
+
   // Returns the contained value.
   // REQUIRES: `ok()` is true.
   auto operator->() -> ValueT* { return &**this; }

common/error_test.cpp

@@ -7,6 +7,7 @@
 #include <gtest/gtest.h>
 
 #include <concepts>
+#include <memory>
 
 #include "common/error_test_helpers.h"
 #include "common/raw_string_ostream.h"
@@ -16,6 +17,7 @@ namespace {
 
 using ::Carbon::Testing::IsError;
 using ::Carbon::Testing::IsSuccess;
+using ::testing::_;
 using ::testing::Eq;
 
 TEST(ErrorTest, Error) {
@@ -36,6 +38,11 @@ TEST(ErrorTest, ErrorBuilderOperatorImplicitCast) {
   EXPECT_THAT(result, IsError("msg"));
 }
 
+// Make sure a custom error type can be forward declared and used with `ErrorOr`
+// until the `ErrorOr` is required to be complete itself.
+class CustomError;
+auto TestFunction() -> ErrorOr<int, CustomError>;
+
 class CustomError : public ErrorBase<CustomError> {
  public:
   auto Print(llvm::raw_ostream& os) const -> void {
@@ -43,6 +50,14 @@ class CustomError : public ErrorBase<CustomError> {
   }
 };
 
+auto TestFunction() -> ErrorOr<int, CustomError> { return CustomError(); }
+
+TEST(ErrorTest, UseErrorOrWithCustomError) {
+  // Uses `TestFunction` to ensure it compiles correctly with forward
+  // declarations above.
+  EXPECT_THAT(TestFunction(), IsError("Custom test error!"));
+}
+
 template <typename ErrorT>
 class ErrorOrTest : public ::testing::Test {
  public:
@@ -117,6 +132,32 @@ TYPED_TEST(ErrorOrTest, IndirectErrorOrSuccess) {
   EXPECT_TRUE(IndirectErrorOrSuccessTest<TypeParam>().ok());
 }
 
+TYPED_TEST(ErrorOrTest, MoveValue) {
+  using TestErrorOr = ErrorOr<std::unique_ptr<int>, TypeParam>;
+
+  auto make_value = []() -> TestErrorOr { return std::make_unique<int>(42); };
+
+  std::unique_ptr<int> p = *make_value();
+  EXPECT_THAT(*p, Eq(42));
+
+  auto result = make_value();
+  std::unique_ptr<int> p2 = *std::move(result);
+  EXPECT_THAT(*p2, Eq(42));
+}
+
+TYPED_TEST(ErrorOrTest, UnprintableValue) {
+  struct X {
+    int i;
+  };
+  using TestErrorOr = ErrorOr<X, TypeParam>;
+
+  TestErrorOr value(X{.i = 42});
+  EXPECT_THAT(value, IsSuccess(_));
+
+  TestErrorOr error = this->MakeError();
+  EXPECT_THAT(error, IsError(this->ErrorStr()));
+}
+
 TYPED_TEST(ErrorOrTest, ReturnIfErrorNoError) {
   using TestErrorOr = ErrorOr<Success, TypeParam>;
   auto result = []() -> TestErrorOr {

common/error_test_helpers.h

@@ -25,7 +25,7 @@ class IsError {
   auto MatchAndExplain(const ErrorOr<T, ErrorT>& result,
                        ::testing::MatchResultListener* listener) const -> bool {
     if (result.ok()) {
-      *listener->stream() << "is a success";
+      *listener << "is a success";
       return false;
     } else {
       RawStringOstream os;
@@ -65,7 +65,7 @@ class IsSuccessMatcher {
     if (result.ok()) {
       return ::testing::Matcher<T>(matcher_).MatchAndExplain(*result, listener);
     } else {
-      *listener->stream() << "is an error with `" << result.error() << "`";
+      *listener << "is an error with `" << result.error() << "`";
       return false;
     }
   }
@@ -99,7 +99,15 @@ template <typename T, typename ErrorT>
 auto operator<<(std::ostream& out, const ErrorOr<T, ErrorT>& error_or)
     -> std::ostream& {
   if (error_or.ok()) {
-    out << llvm::formatv("ErrorOr{{.value = `{0}`}}", *error_or);
+    // Try and print the value, but only if we can find a viable `<<` overload
+    // for the value type. This should ensure that the `formatv` below can
+    // compile cleanly, and avoid erroring when using matchers on `ErrorOr` with
+    // unprintable value types.
+    if constexpr (requires(const T& value) { out << value; }) {
+      out << llvm::formatv("ErrorOr{{.value = `{0}`}}", *error_or);
+    } else {
+      out << "ErrorOr{{.value = `<unknown>`}}";
+    }
   } else {
     out << llvm::formatv("ErrorOr{{.error = \"{0}\"}}", error_or.error());
   }