diff --git a/common/error.h b/common/error.h index d08475d50b25..03bee83d437f 100644 --- a/common/error.h +++ b/common/error.h @@ -91,13 +91,17 @@ class [[nodiscard]] ErrorBase : public Printable { // // This is nodiscard to enforce error handling prior to destruction. template - requires(!std::is_reference_v && - (std::same_as || - std::derived_from>)) class [[nodiscard]] ErrorOr { public: using ValueT = std::remove_reference_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); + static_assert(std::same_as || + std::derived_from>); + // 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(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(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(val_); } + // Returns the contained value. + // REQUIRES: `ok()` is true. + [[nodiscard]] auto operator*() && -> ValueT&& { + Check(); + return std::get(std::move(val_)); + } + // Returns the contained value. // REQUIRES: `ok()` is true. auto operator->() -> ValueT* { return &**this; } diff --git a/common/error_test.cpp b/common/error_test.cpp index a271b9a0fb73..15880c471061 100644 --- a/common/error_test.cpp +++ b/common/error_test.cpp @@ -7,6 +7,7 @@ #include #include +#include #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; + class CustomError : public ErrorBase { public: auto Print(llvm::raw_ostream& os) const -> void { @@ -43,6 +50,14 @@ class CustomError : public ErrorBase { } }; +auto TestFunction() -> ErrorOr { return CustomError(); } + +TEST(ErrorTest, UseErrorOrWithCustomError) { + // Uses `TestFunction` to ensure it compiles correctly with forward + // declarations above. + EXPECT_THAT(TestFunction(), IsError("Custom test error!")); +} + template class ErrorOrTest : public ::testing::Test { public: @@ -117,6 +132,32 @@ TYPED_TEST(ErrorOrTest, IndirectErrorOrSuccess) { EXPECT_TRUE(IndirectErrorOrSuccessTest().ok()); } +TYPED_TEST(ErrorOrTest, MoveValue) { + using TestErrorOr = ErrorOr, TypeParam>; + + auto make_value = []() -> TestErrorOr { return std::make_unique(42); }; + + std::unique_ptr p = *make_value(); + EXPECT_THAT(*p, Eq(42)); + + auto result = make_value(); + std::unique_ptr p2 = *std::move(result); + EXPECT_THAT(*p2, Eq(42)); +} + +TYPED_TEST(ErrorOrTest, UnprintableValue) { + struct X { + int i; + }; + using TestErrorOr = ErrorOr; + + 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; auto result = []() -> TestErrorOr { diff --git a/common/error_test_helpers.h b/common/error_test_helpers.h index ecc368241566..7cae1b5b9aa6 100644 --- a/common/error_test_helpers.h +++ b/common/error_test_helpers.h @@ -25,7 +25,7 @@ class IsError { auto MatchAndExplain(const ErrorOr& 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(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 auto operator<<(std::ostream& out, const ErrorOr& 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 = ``}}"; + } } else { out << llvm::formatv("ErrorOr{{.error = \"{0}\"}}", error_or.error()); }