From b2ab53e49c7f3f5a49af2f715ad874e9ff8f58ae Mon Sep 17 00:00:00 2001 From: Chandler Carruth Date: Wed, 21 Jan 2026 09:41:46 -0800 Subject: [PATCH] Fix an incompatiblitiy between our YAML and `ErrorOr` test helpers (#6636) The YAML test helpers didn't use the `Printable` abstraction in one place and instead directly used `<<` with a `std::ostream`. This matches the `require`s expression in the `error_test_helpers.h` printing logic for `ErrorOr`, but fails to provide the necessary implementation for `llvm::formatv` to succeed with the `Yaml::Value` type. The main fix is to use `Printable` and to define the `Print` method in terms of `llvm::raw_ostream`. We already have all the mapping hooks in place to also support `std::ostream` when needed based on that definition. This also adds some constraints to the printing in `error_test_helpers.h` so it is a bit less under-constrained and more understandable when it is correctly being used. These are just tidying though, they aren't what makes these headers work together. I've added a test to try and make sure these test helpers compose as well. --- common/BUILD | 1 + common/error_test_helpers.h | 5 +++++ toolchain/testing/BUILD | 1 + toolchain/testing/yaml_test_helpers.cpp | 14 +++++++++----- toolchain/testing/yaml_test_helpers.h | 6 ++++-- toolchain/testing/yaml_test_helpers_test.cpp | 18 ++++++++++++++++++ 6 files changed, 38 insertions(+), 7 deletions(-) diff --git a/common/BUILD b/common/BUILD index 890e8a2bba72..ca417f1e6b0b 100644 --- a/common/BUILD +++ b/common/BUILD @@ -199,6 +199,7 @@ cc_library( hdrs = ["error_test_helpers.h"], deps = [ ":error", + ":ostream", "@googletest//:gtest", ], ) diff --git a/common/error_test_helpers.h b/common/error_test_helpers.h index 9348831394d2..886d3188ebbb 100644 --- a/common/error_test_helpers.h +++ b/common/error_test_helpers.h @@ -7,7 +7,10 @@ #include +#include + #include "common/error.h" +#include "common/ostream.h" namespace Carbon::Testing { @@ -122,6 +125,8 @@ namespace Carbon { // Supports printing `ErrorOr` to `std::ostream` in tests. template + requires(std::same_as || + std::derived_from>) auto operator<<(std::ostream& out, const ErrorOr& error_or) -> std::ostream& { if (error_or.ok()) { diff --git a/toolchain/testing/BUILD b/toolchain/testing/BUILD index 7f81fc70568f..7b3ac6012795 100644 --- a/toolchain/testing/BUILD +++ b/toolchain/testing/BUILD @@ -98,6 +98,7 @@ cc_test( srcs = ["yaml_test_helpers_test.cpp"], deps = [ ":yaml_test_helpers", + "//common:error_test_helpers", "//testing/base:gtest_main", "@googletest//:gtest", ], diff --git a/toolchain/testing/yaml_test_helpers.cpp b/toolchain/testing/yaml_test_helpers.cpp index 036430c2c35b..d3bfc3a6a0ce 100644 --- a/toolchain/testing/yaml_test_helpers.cpp +++ b/toolchain/testing/yaml_test_helpers.cpp @@ -88,13 +88,17 @@ auto Value::FromText(llvm::StringRef text) -> ErrorOr { return result; } -auto operator<<(std::ostream& os, const Value& v) -> std::ostream& { +auto Value::Print(llvm::raw_ostream& os) const -> void { // Variant visitor that prints the value in the form of code to recreate the // value. struct Printer { auto operator()(NullValue /*v*/) -> void { out << "Yaml::NullValue()"; } auto operator()(AliasValue /*v*/) -> void { out << "Yaml::AliasValue()"; } - auto operator()(const ScalarValue& v) -> void { out << std::quoted(v); } + auto operator()(const ScalarValue& v) -> void { + out << "\""; + out.write_escaped(v); + out << "\""; + } auto operator()(const MappingValue& v) -> void { out << "Yaml::MappingValue{"; bool first = true; @@ -122,10 +126,10 @@ auto operator<<(std::ostream& os, const Value& v) -> std::ostream& { out << "}"; } - std::ostream& out; + llvm::raw_ostream& out; }; - std::visit(Printer{.out = os}, v); - return os; + + std::visit(Printer{.out = os}, *this); } } // namespace Carbon::Testing::Yaml diff --git a/toolchain/testing/yaml_test_helpers.h b/toolchain/testing/yaml_test_helpers.h index 5249b35bac56..5efa419e786b 100644 --- a/toolchain/testing/yaml_test_helpers.h +++ b/toolchain/testing/yaml_test_helpers.h @@ -55,6 +55,7 @@ #include "absl/strings/str_replace.h" #include "common/error.h" +#include "common/ostream.h" #include "llvm/ADT/StringRef.h" namespace Carbon::Testing::Yaml { @@ -81,12 +82,13 @@ struct AliasValue : EmptyComparable {}; // A thin wrapper around a variant of possible YAML value types. This type // intentionally provides no additional encapsulation or invariants beyond // those of the variant. -struct Value : std::variant, + std::variant { using variant::variant; // Prints the Value in the form of code to recreate the value. - friend auto operator<<(std::ostream& os, const Value& v) -> std::ostream&; + auto Print(llvm::raw_ostream& os) const -> void; // Parses a sequence of YAML documents from the given YAML text. static auto FromText(llvm::StringRef text) -> ErrorOr; diff --git a/toolchain/testing/yaml_test_helpers_test.cpp b/toolchain/testing/yaml_test_helpers_test.cpp index 4e704be651f9..612c6fe6f7ad 100644 --- a/toolchain/testing/yaml_test_helpers_test.cpp +++ b/toolchain/testing/yaml_test_helpers_test.cpp @@ -7,6 +7,8 @@ #include #include +#include "common/error_test_helpers.h" + namespace Carbon::Testing { namespace { @@ -28,5 +30,21 @@ TEST(YamlTestHelpersTest, InvalidYaml) { EXPECT_THAT(result, Not(Yaml::IsYaml(_))); } +TEST(YamlTestHelpersTest, ComposeWithErrorOr) { + auto helper = []() -> ErrorOr { + auto result = Yaml::Value::FromText("[foo, bar]"); + if (!result.ok()) { + return std::move(result).error(); + } + return {*std::move(result)}; + }; + + // Make sure this works correctly with the generic `ErrorOr` test helper as + // well. Note that `FromText` always produces a sequence of its own, so there + // are two layers of nested sequence here. + EXPECT_THAT(helper(), IsSuccess(Yaml::Sequence(ElementsAre( + Yaml::Sequence(ElementsAre("foo", "bar")))))); +} + } // namespace } // namespace Carbon::Testing