Prevent calling make_optional<int&>(i) or make_optional<const int&>(i)

.github/workflows/ci.yml

@@ -1,5 +1,6 @@
 name: CI Tests
 on:
+  workflow_dispatch:
   push:
     branches: [main]
   pull_request:

.github/workflows/pre-commit.yml

@@ -1,6 +1,7 @@
 name: pre-commit
 
 on:
+  workflow_dispatch:
   pull_request:
   push:
     branches: [main]

include/Beman/Optional26/optional.hpp

@@ -232,30 +232,37 @@ template <class T>
 inline constexpr bool is_optional = false;
 template <class T>
 inline constexpr bool is_optional<optional<T>> = true;
+
+struct specification_barrier {
+    explicit specification_barrier() = default;
+};
 } // namespace detail
 
-template <typename T>
-constexpr optional<std::decay_t<T>>
-make_optional(T&& t) noexcept(std::is_nothrow_constructible_v<optional<std::decay_t<T>>, T>)
+template <detail::specification_barrier = detail::specification_barrier(), class T>
+constexpr optional<std::decay_t<T>> make_optional(T&& t) noexcept(std::is_nothrow_constructible_v<std::decay_t<T>, T>)
     requires std::is_constructible_v<std::decay_t<T>, T>
 {
-    return optional<std::decay_t<T>>{std::forward<T>(t)};
+    return optional<std::decay_t<T>>(std::forward<T>(t));
 }
 
-template <typename T, typename... Args>
+template <class T, class... Args>
 constexpr optional<T> make_optional(Args&&... args) noexcept(std::is_nothrow_constructible_v<T, Args...>)
     requires std::is_constructible_v<T, Args...>
 {
-    return optional<T>{in_place, std::forward<Args>(args)...};
+    static_assert(std::is_object_v<T>, "make_optional's template argument must be an object type");
+    static_assert(!std::is_array_v<T>, "make_optional's template argument must be a non-array object type");
+    return optional<T>(in_place, std::forward<Args>(args)...);
 }
 
-template <typename T, typename _Up, typename... Args>
+template <class T, class U, class... Args>
 constexpr optional<T>
-make_optional(std::initializer_list<_Up> init_list,
-              Args&&... args) noexcept(std::is_nothrow_constructible_v<T, std::initializer_list<_Up>&, Args...>)
-    requires std::is_constructible_v<T, std::initializer_list<_Up>&, Args...>
+make_optional(std::initializer_list<U> il,
+              Args&&... args) noexcept(std::is_nothrow_constructible_v<T, std::initializer_list<U>&, Args...>)
+    requires std::is_constructible_v<T, std::initializer_list<U>&, Args...>
 {
-    return optional<T>{in_place, init_list, std::forward<Args>(args)...};
+    static_assert(std::is_object_v<T>, "make_optional's template argument must be an object type");
+    static_assert(!std::is_array_v<T>, "make_optional's template argument must be a non-array object type");
+    return optional<T>(in_place, il, std::forward<Args>(args)...);
 }
 
 template <class T, class U>
@@ -339,7 +346,7 @@ class optional {
         }
     }
 
-    constexpr optional(const optional&)
+    optional(const optional&)
         requires std::is_copy_constructible_v<T> && std::is_trivially_copy_constructible_v<T>
     = default;
 
@@ -352,7 +359,7 @@ class optional {
         }
     }
 
-    constexpr optional(optional&&)
+    optional(optional&&)
         requires std::is_move_constructible_v<T> && std::is_trivially_move_constructible_v<T>
     = default;
 
@@ -430,7 +437,7 @@ class optional {
         return *this;
     }
 
-    constexpr optional& operator=(const optional&)
+    optional& operator=(const optional&)
         requires std::is_copy_constructible_v<T> && std::is_copy_assignable_v<T> &&
                      std::is_trivially_copy_constructible_v<T> && std::is_trivially_copy_assignable_v<T>
     = default;
@@ -448,7 +455,7 @@ class optional {
         return *this;
     }
 
-    constexpr optional& operator=(optional&&)
+    optional& operator=(optional&&)
         requires std::is_move_constructible_v<T> && std::is_move_assignable_v<T> &&
                      std::is_trivially_move_constructible_v<T> && std::is_trivially_move_assignable_v<T>
     = default;
@@ -939,6 +946,43 @@ auto optional_map_impl(Opt&& opt, F&& f)
 /* optional<T&> */
 /****************/
 
+namespace detail {
+
+#ifdef __cpp_lib_reference_from_temporary
+using std::reference_constructs_from_temporary_v;
+using std::reference_converts_from_temporary_v;
+#else
+template <class To, class From>
+concept reference_converts_from_temporary_v =
+    std::is_reference_v<To> &&
+    (
+        // A prvalue of a type similar to To, so that we're binding directly to the materialized prvalue of type From
+        (!std::is_reference_v<From> && std::is_convertible_v<std::remove_cvref_t<From>*, std::remove_cvref_t<To>*>) ||
+        // A value of an unrelated type, convertible to To, but only by materializing a To and binding a const
+        // reference; if we were trying to bind a non-const reference, we'd be unable to. (This is not quite exhaustive
+        // of the problem cases, but I think it's fairly close in practice.)
+        (std::is_lvalue_reference_v<To> && std::is_const_v<std::remove_reference_t<To>> &&
+         std::is_convertible_v<From, const std::remove_cvref_t<To>&&> &&
+         !std::is_convertible_v<From, std::remove_cvref_t<To>&>));
+
+template <class To, class From>
+concept reference_constructs_from_temporary_v =
+    // This is close in practice, because cases where conversion and construction differ in semantics are rare.
+    reference_converts_from_temporary_v<To, From>;
+#endif
+
+template <class To, class From>
+concept safely_convertible = std::is_convertible_v<From, To> && !reference_converts_from_temporary_v<To, From>;
+
+template <class To, class From>
+concept safely_constructible_not_convertible = std::is_constructible_v<To, From> && !std::is_convertible_v<From, To> &&
+                                               !reference_constructs_from_temporary_v<To, From>;
+
+template <class To, class From>
+concept safely_constructible = std::is_constructible_v<To, From> && !reference_constructs_from_temporary_v<To, From>;
+
+} // namespace detail
+
 template <class T>
 class optional<T&> {
   public:
@@ -1016,37 +1060,57 @@ class optional<T&> {
     // constexpr void reset() noexcept;
 
   private:
-    T* value_; // exposition only
+    T* value_ = nullptr; // exposition only
 
   public:
     //  \rSec3[optional.ctor]{Constructors}
 
-    constexpr optional() noexcept : value_(nullptr) {}
+    optional() = default;
 
     constexpr optional(nullopt_t) noexcept : value_(nullptr) {}
 
-    constexpr optional(const optional& rhs) noexcept = default;
-    constexpr optional(optional&& rhs) noexcept      = default;
+    optional(const optional& rhs) = default;
+    optional(optional&& rhs)      = default;
 
-    template <class U = T>
-        requires(!detail::is_optional<std::decay_t<U>>)
-    constexpr explicit(!std::is_convertible_v<U, T>) optional(U&& u) noexcept : value_(std::addressof(u)) {
-        static_assert(std::is_constructible_v<std::add_lvalue_reference_t<T>, U>, "Must be able to bind U to T&");
-        static_assert(std::is_lvalue_reference<U>::value, "U must be an lvalue");
-    }
+    template <class U>
+    constexpr explicit optional(U&& u) noexcept(std::is_nothrow_constructible_v<U&&, T&>)
+        requires detail::safely_constructible_not_convertible<T&, U&&>
+        : value_(std::addressof(static_cast<T&>(std::forward<U>(u)))) {}
 
     template <class U>
-    constexpr explicit(!std::is_convertible_v<U, T>) optional(const optional<U>& rhs) noexcept {
-        static_assert(std::is_constructible_v<std::add_lvalue_reference_t<T>, U>, "Must be able to bind U to T&");
-        if (rhs.has_value())
-            value_ = std::to_address(rhs);
-        else
-            value_ = nullptr;
-    }
+    constexpr optional(U&& u) noexcept(std::is_nothrow_convertible_v<U&&, T&>)
+        requires detail::safely_convertible<T&, U&&>
+        : value_(std::addressof(static_cast<T&>(std::forward<U>(u)))) {}
+
+    template <class U>
+    constexpr explicit optional(const optional<U>& rhs) noexcept(std::is_nothrow_constructible_v<T&, const U&>)
+        requires detail::safely_constructible_not_convertible<T&, const U&>
+        : value_(rhs ? std::addressof(static_cast<T&>(*rhs)) : nullptr) {}
+
+    template <class U>
+    constexpr optional(const optional<U>& rhs) noexcept(std::is_nothrow_convertible_v<const U&, T&>)
+        requires detail::safely_convertible<T&, const U&>
+        : value_(rhs ? std::addressof(static_cast<T&>(*rhs)) : nullptr) {}
+
+    template <class U>
+    constexpr explicit optional(optional<U>& rhs) noexcept(std::is_nothrow_constructible_v<T&, U&>)
+        requires detail::safely_constructible_not_convertible<T&, U&>
+        : value_(rhs ? std::addressof(static_cast<T&>(*rhs)) : nullptr) {}
+
+    template <class U>
+    constexpr optional(optional<U>& rhs) noexcept(std::is_nothrow_convertible_v<U&, T&>)
+        requires detail::safely_convertible<T&, U&>
+        : value_(rhs ? std::addressof(static_cast<T&>(*rhs)) : nullptr) {}
+
+    /// Binds the stored reference in-place using the given argument.
+    template <class U>
+    constexpr optional(in_place_t, U&& u)
+        requires detail::safely_constructible<T&, U&&>
+        : value_(std::addressof(static_cast<T&>(std::forward<U>(u)))) {}
 
     //  \rSec3[optional.dtor]{Destructor}
 
-    constexpr ~optional() = default;
+    ~optional() = default;
 
     // \rSec3[optional.assign]{Assignment}
 
@@ -1055,35 +1119,14 @@ class optional<T&> {
         return *this;
     }
 
-    constexpr optional& operator=(const optional& rhs) noexcept = default;
-    constexpr optional& operator=(optional&& rhs) noexcept      = default;
-
-    template <class U = T>
-        requires(!detail::is_optional<std::decay_t<U>>)
-    constexpr optional& operator=(U&& u) {
-        static_assert(std::is_constructible_v<std::add_lvalue_reference_t<T>, U>, "Must be able to bind U to T&");
-        static_assert(std::is_lvalue_reference<U>::value, "U must be an lvalue");
-        value_ = std::addressof(u);
-        return *this;
-    }
-
-    template <class U>
-    constexpr optional& operator=(const optional<U>& rhs) noexcept {
-        static_assert(std::is_constructible_v<std::add_lvalue_reference_t<T>, U>, "Must be able to bind U to T&");
-        if (rhs.has_value())
-            value_ = std::to_address(rhs);
-        else
-            value_ = nullptr;
-        return *this;
-    }
-
-    template <class U>
-    constexpr optional& operator=(optional<U>&& rhs) = delete;
+    optional& operator=(const optional&) = default;
+    optional& operator=(optional&&)      = default;
 
     template <class U>
-        requires(!detail::is_optional<std::decay_t<U>>)
-    constexpr optional& emplace(U&& u) noexcept {
-        return *this = std::forward<U>(u);
+        requires detail::safely_constructible<T&, U&&>
+    constexpr T& emplace(U&& u) {
+        value_ = std::addressof(static_cast<T&>(std::forward<U>(u)));
+        return *value_;
     }
 
     //   \rSec3[optional.swap]{Swap}

src/Beman/Optional26/tests/optional.t.cpp

@@ -118,12 +118,45 @@ TEST(OptionalTest, NonDefaultConstruct) {
     std::ignore = v2;
 }
 
+TEST(OptionalTest, OptionalOfOptional) {
+    using O = beman::optional26::optional<int>;
+    O                              o;
+    beman::optional26::optional<O> oo1 = o;
+    EXPECT_TRUE(oo1.has_value());
+    oo1 = o;
+    EXPECT_TRUE(oo1.has_value());
+    EXPECT_FALSE(oo1->has_value());
+
+    beman::optional26::optional<O> oo2 = std::move(o);
+    EXPECT_TRUE(oo2.has_value());
+    oo2 = o;
+    EXPECT_TRUE(oo2.has_value());
+    EXPECT_FALSE(oo2->has_value());
+
+    // emplace constructs the inner optional
+    oo2.emplace(beman::optional26::nullopt);
+    EXPECT_TRUE(oo2.has_value());
+    EXPECT_FALSE(oo2->has_value());
+    oo2.emplace(beman::optional26::in_place, 41);
+    EXPECT_TRUE(oo2.has_value());
+    EXPECT_TRUE(oo2.value() == 41);
+    oo2.emplace(42);
+    EXPECT_TRUE(oo2.has_value());
+    EXPECT_TRUE(oo2.value() == 42);
+    oo2.emplace(o);
+    EXPECT_TRUE(oo2.has_value());
+    EXPECT_FALSE(oo2->has_value());
+    oo2.emplace(O(43));
+    EXPECT_TRUE(oo2.has_value());
+    EXPECT_TRUE(oo2.value() == 43);
+}
+
 TEST(OptionalTest, AssignmentValue) {
     beman::optional26::optional<int> o1 = 42;
     beman::optional26::optional<int> o2 = 12;
     beman::optional26::optional<int> o3;
 
-    o1 = o1;
+    o1 = static_cast<beman::optional26::optional<int>&>(o1);
     EXPECT_TRUE(*o1 == 42);
 
     o1 = o2;
@@ -338,7 +371,7 @@ TEST(OptionalTest, MakeOptional) {
     EXPECT_TRUE(std::get<1>(o5->t) == 3);
 
     auto i  = 42;
-    auto o6 = beman::optional26::make_optional<int&>(i);
+    auto o6 = beman::optional26::make_optional(i);
     static_assert(std::is_same<decltype(o6), beman::optional26::optional<int>>::value);
 
     EXPECT_TRUE((std::is_same<decltype(o6), beman::optional26::optional<int>>::value));

src/Beman/Optional26/tests/optional_constexpr.t.cpp

@@ -109,6 +109,7 @@ class NoDefault {
 
   public:
     constexpr NoDefault(int v) : v_(v) {}
+    constexpr int value() const { return v_; }
 };
 } // namespace
 
@@ -126,7 +127,7 @@ consteval bool testConstexprAssignmentValue() {
     beman::optional26::optional<int> o2     = 12;
     beman::optional26::optional<int> o3;
 
-    o1 = o1;
+    o1 = static_cast<beman::optional26::optional<int>&>(o1);
     retval &= (*o1 == 42);
 
     o1 = o2;
@@ -240,10 +241,9 @@ TEST(OptionalConstexprTest, MakeOptional) {
     EXPECT_TRUE(std::get<1>(o5->t) == 3);
 
     static constexpr auto i  = 42;
-    constexpr auto        o6 = beman::optional26::make_optional<const int&>(i);
-    static_assert(std::is_same<decltype(o6), const beman::optional26::optional<int>>::value);
+    constexpr auto        o6 = beman::optional26::make_optional(i);
+    static_assert(std::is_same_v<decltype(o6), const beman::optional26::optional<int>>);
 
-    EXPECT_TRUE((std::is_same<decltype(o6), const beman::optional26::optional<int>>::value));
     EXPECT_TRUE(o6);
     EXPECT_TRUE(*o6 == 42);
 }