[Support] Require llvm::Error passed to formatv() to be wrapped in fmt_consume()

Summary:
Someone must be responsible for handling an Error. When formatv takes
ownership of an Error, the formatv_object destructor must take care of this.

Passing an error by value to formatv() is not considered explicit enough to mark
the error as handled (see D49013), so we require callers to use a format adapter
to confirm this intent.

Reviewers: zturner

Subscribers: llvm-commits, lhames

Differential Revision: https://reviews.llvm.org/D49170

llvm-svn: 336888

3 files changed, 33 insertions, 1 deletions

diff --git a/llvm/include/llvm/Support/FormatAdapters.h b/llvm/include/llvm/Support/FormatAdapters.h
index 197beb7363d..8320eaad39a 100644
--- a/llvm/include/llvm/Support/FormatAdapters.h
+++ b/llvm/include/llvm/Support/FormatAdapters.h
@@ -12,6 +12,7 @@
 
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/FormatCommon.h"
 #include "llvm/Support/FormatVariadicDetails.h"
 #include "llvm/Support/raw_ostream.h"
@@ -19,7 +20,7 @@
 namespace llvm {
 template <typename T> class FormatAdapter : public detail::format_adapter {
 protected:
-  explicit FormatAdapter(T &&Item) : Item(Item) {}
+  explicit FormatAdapter(T &&Item) : Item(std::forward<T>(Item)) {}
 
   T Item;
 };
@@ -71,6 +72,14 @@ public:
     }
   }
 };
+
+class ErrorAdapter : public FormatAdapter<Error> {
+public:
+  ErrorAdapter(Error &&Item) : FormatAdapter(std::move(Item)) {}
+  ErrorAdapter(ErrorAdapter &&) = default;
+  ~ErrorAdapter() { consumeError(std::move(Item)); }
+  void format(llvm::raw_ostream &Stream, StringRef Style) { Stream << Item; }
+};
 }
 
 template <typename T>
@@ -88,6 +97,13 @@ template <typename T>
 detail::RepeatAdapter<T> fmt_repeat(T &&Item, size_t Count) {
   return detail::RepeatAdapter<T>(std::forward<T>(Item), Count);
 }
+
+// llvm::Error values must be consumed before being destroyed.
+// Wrapping an error in fmt_consume explicitly indicates that the formatv_object
+// should take ownership and consume it.
+inline detail::ErrorAdapter fmt_consume(Error &&Item) {
+  return detail::ErrorAdapter(std::move(Item));
+}
 }
 
 #endif
diff --git a/llvm/include/llvm/Support/FormatVariadicDetails.h b/llvm/include/llvm/Support/FormatVariadicDetails.h
index 394decf4b3d..56dda430efd 100644
--- a/llvm/include/llvm/Support/FormatVariadicDetails.h
+++ b/llvm/include/llvm/Support/FormatVariadicDetails.h
@@ -17,6 +17,7 @@
 
 namespace llvm {
 template <typename T, typename Enable = void> struct format_provider {};
+class Error;
 
 namespace detail {
 class format_adapter {
@@ -141,6 +142,12 @@ template <typename T>
 typename std::enable_if<uses_stream_operator<T>::value,
                         stream_operator_format_adapter<T>>::type
 build_format_adapter(T &&Item) {
+  // If the caller passed an Error by value, then stream_operator_format_adapter
+  // would be responsible for consuming it.
+  // Make the caller opt into this by calling fmt_consume().
+  static_assert(
+      !std::is_same<llvm::Error, typename std::remove_cv<T>::type>::value,
+      "llvm::Error-by-value must be wrapped in fmt_consume() for formatv");
   return stream_operator_format_adapter<T>(std::forward<T>(Item));
 }
 
diff --git a/llvm/unittests/Support/FormatVariadicTest.cpp b/llvm/unittests/Support/FormatVariadicTest.cpp
index 6d621464c0e..91a44bae3a9 100644
--- a/llvm/unittests/Support/FormatVariadicTest.cpp
+++ b/llvm/unittests/Support/FormatVariadicTest.cpp
@@ -8,6 +8,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/FormatAdapters.h"
 #include "gtest/gtest.h"
 
@@ -680,3 +681,11 @@ TEST(FormatVariadicTest, FormatStreamable) {
   adl::X X;
   EXPECT_EQ("X", formatv("{0}", X).str());
 }
+
+TEST(FormatVariadicTest, FormatError) {
+  auto E1 = make_error<StringError>("X", inconvertibleErrorCode());
+  EXPECT_EQ("X", formatv("{0}", E1).str());
+  EXPECT_TRUE(E1.isA<StringError>()); // not consumed
+  EXPECT_EQ("X", formatv("{0}", fmt_consume(std::move(E1))).str());
+  EXPECT_FALSE(E1.isA<StringError>()); // consumed
+}