[boost] [optional] optional(Expr&&) is insufficiently constrained

Subject: [boost] [optional] optional(Expr&&) is insufficiently constrained
From: Stephan T. Lavavej (stl_at_[hidden])
Date: 2016-02-18 17:03:40

Hi,

A user reported a bug (VSO#191303/Connect#2351203, see [1]) using Boost.Optional with MSVC 2015 Update 2. The original test case involved a minor bug in VC's tuple, which I'll look into fixing in the future (specifically, N-tuples recursively construct (N - 1)-tuples, in a way that's visible to overload resolution). However, there is also a bug in boost::optional.

Here's a reduced test case, which contains the relevant parts of boost::optional, and doesn't involve that bug in VC's tuple:

C:\Temp>type meow.cpp
#include <tuple>
#include <type_traits>
#include <utility>
using namespace std;

struct none_t { };

template <typename T> struct optional {
    T thing;

    optional(none_t)
        : thing() { }

    template <typename Expr,
        typename = typename enable_if<
            !is_same<typename decay<Expr>::type, none_t>::value
#ifdef FIX
            && is_constructible<T, Expr>::value
#endif
        >::type>
    explicit optional(Expr&& expr)
        : thing(forward<Expr>(expr)) { }
};

int main() {
    tuple<none_t> one{};

    tuple<optional<double>> two(one);
}

C:\Temp>cl /EHsc /nologo /W4 meow.cpp
meow.cpp
meow.cpp(22): error C2440: 'initializing': cannot convert from 'std::tuple<none_t>' to 'double'
[...context...]

C:\Temp>cl /EHsc /nologo /W4 /DFIX meow.cpp
meow.cpp

C:\Temp>

This is happening because I implemented N4387 (see [2]) and the Proposed Resolution for LWG 2549 (see [3]). The PR is required to implement N4387 safely.

The direct-init of two from one is attempting to select tuple's converting copy ctor (which N4387 marks as implicit, because optional's constructor from none_t is implicit). However, LWG 2549 additionally constrains the converting copy ctor. In English, when the source is tuple<A> and the destination is tuple<B>, LWG 2549 determines whether A is constructible from tuple<B>. If it IS, then the converting copy ctor vanishes, so that tuple's perfect forwarding ctor (tuple(UTypes&&...)) can construct A from tuple<B>. Alternatively, if A ISN'T constructible from tuple<B>, then the perfect forwarding ctor SFINAEs away while the converting copy ctor is permitted to participate, so A is constructed from the B within the tuple<B>.

The boost::optional bug is that optional(Expr&&) isn't sufficiently constrained, so it appears to be constructible (via is_constructible) from tuple<none_t>, even though the definition will explode. So LWG 2549 makes tuple's converting copy ctor vanish, and we end up attempting to construct optional<double> from tuple<none_t> which is doomed.

This is NOT a problem with LWG 2549's Proposed Resolution, NOR is it a problem in VC 2015 Update 2's tuple (which has correctly implemented this, at least for the 1-tuple case, as previously mentioned).

The fix that should be applied to boost::optional is to constrain optional(Expr&&) based on whether its definition will compile. (This is inherently safe, because it just makes is_constructible report accurate answers, and doesn't affect code that previously compiled.) WG21 has (gradually) taught pair/tuple to do this (constructors are now constrained on is_constructible for elements, and additionally is_convertible when they care about implicitness; previously they didn't ask is_constructible).

The relevant code in optional.hpp is:

  template<class Expr>
  explicit optional ( Expr&& expr, 
                      BOOST_DEDUCED_TYPENAME boost::disable_if_c<
                        (boost::is_base_of<optional_detail::optional_tag, BOOST_DEDUCED_TYPENAME boost::decay<Expr>::type>::value) || 
                        boost::is_same<BOOST_DEDUCED_TYPENAME boost::decay<Expr>::type, none_t>::value >::type* = 0 
  ) 
    : base(boost::forward<Expr>(expr),boost::addressof(expr)) 
    {optional_detail::prevent_binding_rvalue_ref_to_optional_lvalue_ref<T, Expr&&>();}

Unlike my reduced repro (where is_constructible<T, Expr> is the constraint corresponding to thing(forward<Expr>(expr))), I can't suggest an exact fix without knowing what optional_base's constructor is going to do with the provided arguments.

As an aside, the "prevent_binding" stuff should probably be expressed as a SFINAE constraint, not a static_assert.

Thanks,
STL

[1] https://connect.microsoft.com/VisualStudio/feedback/details/2351203/vc14-2-ctp1-std-tuple-broken
[2] http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/n4387.html
[3] http://cplusplus.github.io/LWG/lwg-active.html#2549