From dcfccd31879b698ba57cd84f12ce89d8075ea7ab Mon Sep 17 00:00:00 2001
From: Calvin <clavin@users.noreply.github.com>
Date: Tue, 4 Feb 2025 14:08:43 -0700
Subject: [PATCH] Support references in `ErrorOr` (#4889)

### Context & Motivation

The error handling utilities in `//base/error.h` are very useful for
writing code with strong safety guarantees. While hardening the `Dump`
debug utilities (from review in #4866), I encountered a rough edge with
references and pointers. After a [brief Discord discussion in
#contributing-help](https://discord.com/channels/655572317891461132/1052653651895779359/1334675462877610038),
it was suggested that adding support for references to `ErrorOr` would
be a good candidate to move forward.

Using a reference type with the `ErrorOr` class (e.g. `ErrorOr<Node&>`)
produces two errors:

<ol>
<li><strong><code>variant can not have a reference type as an
alternative</code></strong>
<ul><li>From private field: <code>std::variant&lt;Error, T&gt;
val_;</code></li></ul>
</li>
<li><strong><code>'operator-&gt;' declared as a pointer to a
reference</code></strong>
<ul><li>From member function: <code>auto operator-&gt;() -&gt;
T*</code></li></ul>
</li>
</ol>

### Changes

To support reference types, both errors are resolved:

1. `std::reference_wrapper` is conditionally used for storage when `T`
is a reference type
2. type trait aliases like `using ValueT = std::remove_reference_t<T>`
are used to produce compatible types for methods like `auto operator->()
-> ValueT*`
---
 common/error.h        | 40 +++++++++++++++++++++++++---------------
 common/error_test.cpp |  6 ++++++
 2 files changed, 31 insertions(+), 15 deletions(-)

diff --git a/common/error.h b/common/error.h
index 8b29e66ae41f..ec18eb9d1a2e 100644
--- a/common/error.h
+++ b/common/error.h
@@ -5,7 +5,9 @@
 #ifndef CARBON_COMMON_ERROR_H_
 #define CARBON_COMMON_ERROR_H_
 
+#include <functional>
 #include <string>
+#include <type_traits>
 #include <variant>
 
 #include "common/check.h"
@@ -76,18 +78,29 @@ class [[nodiscard]] Error : public Printable<Error> {
 template <typename T>
 class [[nodiscard]] ErrorOr {
  public:
+  using ValueT = std::remove_reference_t<T>;
+
   // 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)) {}
 
+  // Constructs with a reference.
+  // Implicit for easy construction on returns.
+  // NOLINTNEXTLINE(google-explicit-constructor)
+  ErrorOr(T ref)
+    requires std::is_reference_v<T>
+      : val_(std::ref(ref)) {}
+
   // Constructs with a value.
   // Implicit for easy construction on returns.
   // NOLINTNEXTLINE(google-explicit-constructor)
-  ErrorOr(T val) : val_(std::move(val)) {}
+  ErrorOr(T val)
+    requires(!std::is_reference_v<T>)
+      : val_(std::move(val)) {}
 
   // Returns true for success.
-  auto ok() const -> bool { return std::holds_alternative<T>(val_); }
+  auto ok() const -> bool { return std::holds_alternative<StoredT>(val_); }
 
   // Returns the contained error.
   // REQUIRES: `ok()` is false.
@@ -102,35 +115,32 @@ class [[nodiscard]] ErrorOr {
 
   // Returns the contained value.
   // REQUIRES: `ok()` is true.
-  auto operator*() -> T& {
+  auto operator*() -> ValueT& {
     CARBON_CHECK(ok());
-    return std::get<T>(val_);
+    return std::get<StoredT>(val_);
   }
 
   // Returns the contained value.
   // REQUIRES: `ok()` is true.
-  auto operator*() const -> const T& {
+  auto operator*() const -> const ValueT& {
     CARBON_CHECK(ok());
-    return std::get<T>(val_);
+    return std::get<StoredT>(val_);
   }
 
   // Returns the contained value.
   // REQUIRES: `ok()` is true.
-  auto operator->() -> T* {
-    CARBON_CHECK(ok());
-    return &std::get<T>(val_);
-  }
+  auto operator->() -> ValueT* { return &**this; }
 
   // Returns the contained value.
   // REQUIRES: `ok()` is true.
-  auto operator->() const -> const T* {
-    CARBON_CHECK(ok());
-    return &std::get<T>(val_);
-  }
+  auto operator->() const -> const ValueT* { return &**this; }
 
  private:
+  using StoredT = std::conditional_t<std::is_reference_v<T>,
+                                     std::reference_wrapper<ValueT>, T>;
+
   // Either an error message or a value.
-  std::variant<Error, T> val_;
+  std::variant<Error, StoredT> val_;
 };
 
 // A helper class for accumulating error message and converting to
diff --git a/common/error_test.cpp b/common/error_test.cpp
index ebe56ae194f1..1e5dac824fae 100644
--- a/common/error_test.cpp
+++ b/common/error_test.cpp
@@ -50,6 +50,12 @@ TEST(ErrorTest, ErrorOrArrowOp) {
   EXPECT_EQ(err->val, 1);
 }
 
+TEST(ErrorTest, ErrorOrReference) {
+  Val val = {1};
+  ErrorOr<Val&> maybe_val(val);
+  EXPECT_EQ(maybe_val->val, 1);
+}
+
 auto IndirectErrorOrSuccessTest() -> ErrorOr<Success> { return Success(); }
 
 TEST(ErrorTest, IndirectErrorOrSuccess) {