From e99448eecf4bb30c450135bb05fcdabefc3ad006 Mon Sep 17 00:00:00 2001 From: Chandler Carruth Date: Wed, 23 Jul 2025 16:15:41 -0700 Subject: [PATCH] Add support for a custom error type in `ErrorOr` (#5834) This doesn't split apart the current error type into one that tracks location and one that doesn't, although that might be easier to do once we have this. Instead, this is primarily intended to support custom error types that lazily materialize the error message in case that can be avoided by completely handling the error. For example, many file system operations are *expected* to produce errors even in the hot path and we don't want to render `ENOENT` (for example) to a pretty string and instead will directly query the error to understand and handle it in code. The type parameter ordering isn't the most obvious, but helpfully allows us to default the error type in a useful way. --------- Co-authored-by: Jon Ross-Perkins --- common/error.h | 79 +++++++++-- common/error_test.cpp | 259 +++++++++++++++++++++++++----------- common/error_test_helpers.h | 22 +-- 3 files changed, 263 insertions(+), 97 deletions(-) diff --git a/common/error.h b/common/error.h index 72486e5821e1..641f77fa771f 100644 --- a/common/error.h +++ b/common/error.h @@ -5,9 +5,11 @@ #ifndef CARBON_COMMON_ERROR_H_ #define CARBON_COMMON_ERROR_H_ +#include #include #include #include +#include #include #include "common/check.h" @@ -71,11 +73,49 @@ class [[nodiscard]] Error : public Printable { std::string message_; }; -// Holds a value of type `T`, or an Error explaining why the value is +// A common base class that custom error types should derive from. +// +// This combines the ability to be printed with the ability to convert the error +// to a string and in turn to a non-customized `Error` type by rendering into a +// string. +// +// The goal is that custom error types can be used for errors that are common +// and/or would have cost to fully render the error message to a string. A +// custom type can then be used to allow custom, light-weight handling of errors +// when appropriate. But to avoid these custom types being excessively viral, we +// ensure they can be converted to normal `Error` types when needed by rendering +// fully to a string. +template +class [[nodiscard]] ErrorBase : public Printable { + public: + ErrorBase(const ErrorBase&) = delete; + auto operator=(const ErrorBase&) -> ErrorBase& = delete; + + auto ToString() const -> std::string { + RawStringOstream os; + static_cast(this)->Print(os); + return os.TakeStr(); + } + auto ToError() const -> Error { return Error(this->ToString()); } + + protected: + ErrorBase() = default; + ErrorBase(ErrorBase&&) noexcept = default; + auto operator=(ErrorBase&&) noexcept -> ErrorBase& = default; +}; + +// Holds a value of type `T`, or an `ErrorT` type explaining why the value is // unavailable. // +// The `ErrorT` type defaults to `Error` but can be customized where desired +// with a type that derives from `ErrorBase` above. See the documentation for +// `ErrorBase` to understand the expected contract of custom error types. +// // This is nodiscard to enforce error handling prior to destruction. -template +template + requires(!std::is_reference_v && + (std::same_as || + std::derived_from>)) class [[nodiscard]] ErrorOr { public: using ValueT = std::remove_reference_t; @@ -83,7 +123,30 @@ class [[nodiscard]] ErrorOr { // Constructs with an error; the error must not be Error::Success(). // Implicit for easy construction on returns. // NOLINTNEXTLINE(google-explicit-constructor) - ErrorOr(Error err) : val_(std::move(err)) {} + ErrorOr(ErrorT err) : val_(std::move(err)) {} + + // Constructs from a custom error type derived from `ErrorBase` into an + // `ErrorOr` for `Error` to facilitate returning errors transparently. + template + requires(std::same_as && + std::derived_from>) + // Implicit for easy construction on returns. + // NOLINTNEXTLINE(google-explicit-constructor) + ErrorOr(OtherErrorT other_err) : val_(other_err.ToError()) {} + + // Constructs with any convertible error type, necessary for return statements + // that are already converting to the `ErrorOr` wrapper. + // + // This supports *explicitly* conversions, not just implicit, which is + // important to make common patterns of returning and adjusting the error + // type without each error type conversion needing to be implicit. + template + requires(std::constructible_from && + std::derived_from>) + // Implicit for easy construction on returns. + // NOLINTNEXTLINE(google-explicit-constructor) + ErrorOr(OtherErrorT other_err) + : val_(std::in_place_type, std::move(other_err)) {} // Constructs with a reference. // Implicit for easy construction on returns. @@ -104,13 +167,13 @@ class [[nodiscard]] ErrorOr { // Returns the contained error. // REQUIRES: `ok()` is false. - auto error() const& -> const Error& { + auto error() const& -> const ErrorT& { CARBON_CHECK(!ok()); - return std::get(val_); + return std::get(val_); } - auto error() && -> Error { + auto error() && -> ErrorT { CARBON_CHECK(!ok()); - return std::get(std::move(val_)); + return std::get(std::move(val_)); } // Returns the contained value. @@ -140,7 +203,7 @@ class [[nodiscard]] ErrorOr { std::reference_wrapper, T>; // Either an error message or a value. - std::variant val_; + std::variant val_; }; // A helper class for accumulating error message and converting to diff --git a/common/error_test.cpp b/common/error_test.cpp index 1e5dac824fae..67216f033ee0 100644 --- a/common/error_test.cpp +++ b/common/error_test.cpp @@ -6,6 +6,8 @@ #include +#include + #include "common/error_test_helpers.h" #include "common/raw_string_ostream.h" @@ -29,84 +31,6 @@ auto IndirectError() -> Error { return Error("test"); } TEST(ErrorTest, IndirectError) { EXPECT_EQ(IndirectError().message(), "test"); } -TEST(ErrorTest, ErrorOr) { - ErrorOr err(Error("test")); - - EXPECT_THAT(err, IsError("test")); -} - -TEST(ErrorTest, ErrorOrValue) { EXPECT_TRUE(ErrorOr(0).ok()); } - -auto IndirectErrorOrTest() -> ErrorOr { return Error("test"); } - -TEST(ErrorTest, IndirectErrorOr) { EXPECT_FALSE(IndirectErrorOrTest().ok()); } - -struct Val { - int val; -}; - -TEST(ErrorTest, ErrorOrArrowOp) { - ErrorOr err({1}); - EXPECT_EQ(err->val, 1); -} - -TEST(ErrorTest, ErrorOrReference) { - Val val = {1}; - ErrorOr maybe_val(val); - EXPECT_EQ(maybe_val->val, 1); -} - -auto IndirectErrorOrSuccessTest() -> ErrorOr { return Success(); } - -TEST(ErrorTest, IndirectErrorOrSuccess) { - EXPECT_TRUE(IndirectErrorOrSuccessTest().ok()); -} - -TEST(ErrorTest, ReturnIfErrorNoError) { - auto result = []() -> ErrorOr { - CARBON_RETURN_IF_ERROR(ErrorOr(Success())); - CARBON_RETURN_IF_ERROR(ErrorOr(Success())); - return Success(); - }(); - EXPECT_TRUE(result.ok()); -} - -TEST(ErrorTest, ReturnIfErrorHasError) { - auto result = []() -> ErrorOr { - CARBON_RETURN_IF_ERROR(ErrorOr(Success())); - CARBON_RETURN_IF_ERROR(ErrorOr(Error("error"))); - return Success(); - }(); - EXPECT_THAT(result, IsError("error")); -} - -TEST(ErrorTest, AssignOrReturnNoError) { - auto result = []() -> ErrorOr { - CARBON_ASSIGN_OR_RETURN(int a, ErrorOr(1)); - CARBON_ASSIGN_OR_RETURN(const int b, ErrorOr(2)); - int c = 0; - CARBON_ASSIGN_OR_RETURN(c, ErrorOr(3)); - return a + b + c; - }(); - EXPECT_THAT(result, IsSuccess(Eq(6))); -} - -TEST(ErrorTest, AssignOrReturnHasDirectError) { - auto result = []() -> ErrorOr { - CARBON_RETURN_IF_ERROR(ErrorOr(Error("error"))); - return 0; - }(); - EXPECT_THAT(result, IsError("error")); -} - -TEST(ErrorTest, AssignOrReturnHasErrorInExpected) { - auto result = []() -> ErrorOr { - CARBON_ASSIGN_OR_RETURN(int a, ErrorOr(Error("error"))); - return a; - }(); - EXPECT_THAT(result, IsError("error")); -} - TEST(ErrorTest, ErrorBuilderOperatorImplicitCast) { ErrorOr result = ErrorBuilder() << "msg"; EXPECT_THAT(result, IsError("msg")); @@ -119,5 +43,184 @@ TEST(ErrorTest, StreamError) { EXPECT_EQ(result_stream.TakeStr(), "TestFunc: msg"); } +class CustomError : public ErrorBase { + public: + auto Print(llvm::raw_ostream& os) const -> void { + os << "Custom test error!"; + } +}; + +template +class ErrorOrTest : public ::testing::Test { + public: + auto ErrorStr() -> std::string { + if constexpr (std::same_as) { + return "test error"; + } else if constexpr (std::same_as) { + return CustomError().ToString(); + } else { + static_assert(false, "Unsupported custom error type!"); + } + } + + auto MakeError() -> ErrorT { + if constexpr (std::same_as) { + return Error("test error"); + } else if constexpr (std::same_as) { + return CustomError(); + } else { + static_assert(false, "Unsupported custom error type!"); + } + } +}; + +using ErrorOrTestParams = ::testing::Types; +TYPED_TEST_SUITE(ErrorOrTest, ErrorOrTestParams); + +TYPED_TEST(ErrorOrTest, ErrorOr) { + using TestErrorOr = ErrorOr; + TestErrorOr err(this->MakeError()); + + EXPECT_THAT(err, IsError(this->ErrorStr())); +} + +TYPED_TEST(ErrorOrTest, ErrorOrValue) { + using TestErrorOr = ErrorOr; + EXPECT_TRUE(TestErrorOr(0).ok()); +} + +template +auto IndirectErrorOrTest(Fixture& fixture) -> ErrorOr { + return fixture.MakeError(); +} + +TYPED_TEST(ErrorOrTest, IndirectErrorOr) { + EXPECT_FALSE(IndirectErrorOrTest(*this).ok()); +} + +struct Val { + int val; +}; + +TYPED_TEST(ErrorOrTest, ErrorOrArrowOp) { + using TestErrorOr = ErrorOr; + TestErrorOr err({1}); + EXPECT_EQ(err->val, 1); +} + +TYPED_TEST(ErrorOrTest, ErrorOrReference) { + using TestErrorOr = ErrorOr; + Val val = {1}; + TestErrorOr maybe_val(val); + EXPECT_EQ(maybe_val->val, 1); +} + +template +auto IndirectErrorOrSuccessTest() -> ErrorOr { + return Success(); +} + +TYPED_TEST(ErrorOrTest, IndirectErrorOrSuccess) { + EXPECT_TRUE(IndirectErrorOrSuccessTest().ok()); +} + +TYPED_TEST(ErrorOrTest, ReturnIfErrorNoError) { + using TestErrorOr = ErrorOr; + auto result = []() -> TestErrorOr { + CARBON_RETURN_IF_ERROR(TestErrorOr(Success())); + CARBON_RETURN_IF_ERROR(TestErrorOr(Success())); + return Success(); + }(); + EXPECT_TRUE(result.ok()); +} + +TYPED_TEST(ErrorOrTest, ReturnIfErrorHasError) { + using TestErrorOr = ErrorOr; + auto result = [this]() -> TestErrorOr { + CARBON_RETURN_IF_ERROR(TestErrorOr(Success())); + CARBON_RETURN_IF_ERROR(TestErrorOr(this->MakeError())); + return Success(); + }(); + EXPECT_THAT(result, IsError(this->ErrorStr())); +} + +TYPED_TEST(ErrorOrTest, AssignOrReturnNoError) { + using TestErrorOr = ErrorOr; + auto result = []() -> TestErrorOr { + CARBON_ASSIGN_OR_RETURN(int a, TestErrorOr(1)); + CARBON_ASSIGN_OR_RETURN(const int b, TestErrorOr(2)); + int c = 0; + CARBON_ASSIGN_OR_RETURN(c, TestErrorOr(3)); + return a + b + c; + }(); + EXPECT_THAT(result, IsSuccess(Eq(6))); +} + +TYPED_TEST(ErrorOrTest, AssignOrReturnHasDirectError) { + using TestErrorOr = ErrorOr; + auto result = [this]() -> TestErrorOr { + CARBON_RETURN_IF_ERROR(TestErrorOr(this->MakeError())); + return 0; + }(); + EXPECT_THAT(result, IsError(this->ErrorStr())); +} + +TYPED_TEST(ErrorOrTest, AssignOrReturnHasErrorInExpected) { + using TestErrorOr = ErrorOr; + auto result = [this]() -> TestErrorOr { + CARBON_ASSIGN_OR_RETURN(int a, TestErrorOr(this->MakeError())); + return a; + }(); + EXPECT_THAT(result, IsError(this->ErrorStr())); +} + +class AnotherCustomError : public ErrorBase { + public: + auto Print(llvm::raw_ostream& os) const -> void { + os << "Another custom test error!"; + } + + explicit operator CustomError() { return CustomError(); } +}; + +TYPED_TEST(ErrorOrTest, AssignOrReturnNoErrorAcrossErrorTypes) { + using TestErrorOr = ErrorOr; + auto result = []() -> ErrorOr { + CARBON_ASSIGN_OR_RETURN(int a, TestErrorOr(1)); + CARBON_ASSIGN_OR_RETURN(const int b, []() -> TestErrorOr { + CARBON_ASSIGN_OR_RETURN(int inner, (ErrorOr(2))); + return inner; + }()); + int c = 0; + CARBON_ASSIGN_OR_RETURN(c, TestErrorOr(3)); + return a + b + c; + }(); + EXPECT_THAT(result, IsSuccess(Eq(6))); +} + +TYPED_TEST(ErrorOrTest, AssignOrReturnErrorAcrossErrorTypes) { + using TestErrorOr = ErrorOr; + auto result = []() -> ErrorOr { + CARBON_ASSIGN_OR_RETURN(int a, TestErrorOr(1)); + CARBON_ASSIGN_OR_RETURN(const int b, []() -> TestErrorOr { + CARBON_ASSIGN_OR_RETURN( + int inner, (ErrorOr(AnotherCustomError()))); + return inner; + }()); + int c = 0; + CARBON_ASSIGN_OR_RETURN(c, TestErrorOr(3)); + return a + b + c; + }(); + + // When directly using the `Error` type, the explicit custom type above has + // its message preserved. When testing against `CustomError`, that one + // overrides the message. + if constexpr (std::same_as) { + EXPECT_THAT(result, IsError("Another custom test error!")); + } else { + EXPECT_THAT(result, IsError("Custom test error!")); + } +} + } // namespace } // namespace Carbon diff --git a/common/error_test_helpers.h b/common/error_test_helpers.h index 3e87cfa87f6c..ecc368241566 100644 --- a/common/error_test_helpers.h +++ b/common/error_test_helpers.h @@ -21,14 +21,16 @@ class IsError { explicit IsError(::testing::Matcher matcher) : matcher_(std::move(matcher)) {} - template - auto MatchAndExplain(const ErrorOr& result, + template + auto MatchAndExplain(const ErrorOr& result, ::testing::MatchResultListener* listener) const -> bool { if (result.ok()) { *listener->stream() << "is a success"; return false; } else { - return matcher_.MatchAndExplain(result.error().message(), listener); + RawStringOstream os; + os << result.error(); + return matcher_.MatchAndExplain(os.TakeStr(), listener); } } @@ -57,14 +59,13 @@ class IsSuccessMatcher { explicit IsSuccessMatcher(InnerMatcher matcher) : matcher_(std::move(matcher)) {} - template - auto MatchAndExplain(const ErrorOr& result, + template + auto MatchAndExplain(const ErrorOr& result, ::testing::MatchResultListener* listener) const -> bool { if (result.ok()) { return ::testing::Matcher(matcher_).MatchAndExplain(*result, listener); } else { - *listener->stream() << "is an error with `" << result.error().message() - << "`"; + *listener->stream() << "is an error with `" << result.error() << "`"; return false; } } @@ -94,14 +95,13 @@ auto IsSuccess(InnerMatcher matcher) -> IsSuccessMatcher { namespace Carbon { // Supports printing `ErrorOr` to `std::ostream` in tests. -template -auto operator<<(std::ostream& out, const ErrorOr& error_or) +template +auto operator<<(std::ostream& out, const ErrorOr& error_or) -> std::ostream& { if (error_or.ok()) { out << llvm::formatv("ErrorOr{{.value = `{0}`}}", *error_or); } else { - out << llvm::formatv("ErrorOr{{.error = \"{0}\"}}", - error_or.error().message()); + out << llvm::formatv("ErrorOr{{.error = \"{0}\"}}", error_or.error()); } return out; }