`not_null`'s evaluation of nullness can lead to *very* cryptic errors. about gsl-lite

[mbeutel]

Thanks – interesting it was! I have been irritated by inadequate provisions for type traits in the past, but rarely have they been as confusing as this one, so it's good to have it documented.

I'll close this after adding the is_nullable<> trait you proposed.

[mbeutel]

Found it. Thanks for mentioning this. If Herb is speaking as Core Guidelines editor, this implies we are non-conforming with regard to not_null<>, which should be mentioned in our (barely existing) documentation.

[mbeutel]

Thank you for the report.

As far as I can tell, GCC does reference the offending line:

...
<source>:11:36:   required from here

as does MSVC:

...
<source>(11): note: see reference to function template instantiation 'gsl::not_null<stlab::copy_on_write<int>> gsl::make_not_null<stlab::copy_on_write<int>>(U)' being compiled

Clang, however, does not, which is puzzling, but ultimately seems to be a shortcoming in the compiler.

---

Of course we could make our nullptr comparisons more contrived, but this would be symptom treatment at best. We shouldn't have gotten as far as instantiating not_null<stlab::copy_on_write<int>> in the first place. The following static_assert() statements should have prevented that:

gsl-lite/include/gsl/gsl-lite.hpp

Lines 2864 to 2866 in a8c7e5b

	static_assert( ! std::is_reference<T>::value, "T may not be a reference type" );
	static_assert( ! std::is_const<T>::value && ! std::is_volatile<T>::value, "T may not be cv-qualified" );
	static_assert( std::is_assignable<T&, std::nullptr_t>::value, "T cannot be assigned nullptr" );

Unfortunately, these assertion statements fail to detect that stlab::copy_on_write<int> is not nullable because stlab::copy_on_write<> defines an under-constrained forwarding constructor and assignment operator. I'd argue that stlab should tighten their constraints:

// stlab/copy_on_write.hpp
...
    template <class U>
-    using disable_copy = std::enable_if_t<!std::is_same<std::decay_t<U>, copy_on_write>::value>*;
+    using constrain_and_disable_copy = std::enable_if_t<std::is_constructible<T, U>::value && !std::is_same<std::decay_t<U>, copy_on_write>::value>*;

    template <typename U>
-    using disable_copy_assign =
-        std::enable_if_t<!std::is_same<std::decay_t<U>, copy_on_write>::value, copy_on_write&>;
+   using constrain_and_disable_copy_assign =
+       std::enable_if_t<std::is_assignable<T, U>::value && !std::is_same<std::decay_t<U>, copy_on_write>::value, copy_on_write&>;
...
    template <class U>
-    copy_on_write(U&& x, disable_copy<U> = nullptr) : _self(new model(std::forward<U>(x))) {}
+    copy_on_write(U&& x, constrain_and_disable_copy<U> = nullptr) : _self(new model(std::forward<U>(x))) {}
...
    template <class U>
-   auto operator=(U&& x) -> disable_copy_assign<U> {
+   auto operator=(U&& x) -> constrain_and_disable_copy_assign<U> {
        if (_self && unique()) {
            _self->_value = std::forward<U>(x);
            return *this;
        }
...

If they did, your code would result in a concise error message (Clang, GCC, MSVC).

---

I'm not very fond of introducing an is_not_null() predicate. There are two idiomatic ways to do a nullptr check: either by relying on the explicit Boolean conversion, if (x), or by stating it explicitly, if (x != nullptr). Given that it would be longer and not idiomatic, I fail to see how if (gsl::is_not_null(x)) would be any better.

I suppose we could introduce an is_nullable<> type trait, along with is_nullable_v<>.

---

Some general remarks, if you allow:

If you aren't using -Dgsl_CONFIG_DEFAULTS_VERSION=1 already, please do; gsl-lite will be more conformant and behave more logically with version-1 defaults. Check out the readme for the semantic differences. If use CMake to consume gsl-lite as a dependency, the easiest way to impose version-1 defaults is to reference the gsl::gsl-lite-v1 target.

Regarding your old code,

        auto x = std::make_shared<int>(42);
        auto y = gsl::make_not_null(x);

I'd like to point out that, with C++17, make_not_null() has become redundant thanks to class template argument deduction,

        auto x = std::make_shared<int>(42);
        auto y = gsl::not_null(x);

and that gsl-lite actually defines its own make_unique<T>() and make_shared<T>() overloads which return gsl::not_null<std::unique_ptr<T>> and gsl::not_null<std::shared_ptr<T>>, respectively, so you could simply have written

        auto y = gsl::make_shared<int>(42);  // not_null<std::shared_ptr<int>>

I really need to document our extensions better.

[BenFrantzDale]

Cool. Feel free to close. It seemed weird and obscure enough that you’d at least find it interesting!

[BenFrantzDale]

I hadn’t realized the not_null c’tor with CTAD was the preferred way. We’ve been using make_not_null.

I also didn’t realize you had the factory functions. We made our own: nn_make_shared and nn_make_unique. I just had an intern remove all uses of std::make_* in favor of these.

this is a wonderful library. I’ve been thinking of putting together a blog post or talk showing how we’ve gone from crash report to bug fix by finding the null dereference, making that not_null then make changes until it compiles. It’s magic! (And the official GSL one is next to useless because their not-null unique-ptr is immovable.)

[mbeutel]

Yes, these things are documented in the release notes but hardly anywhere else. I really need to work on the documentation.

As far as I understand it, the Core Guidelines only specify not_null<> for pointers, where movability is irrelevant. With movable types, we have to make a choice: (1) preserve the not-null class invariant; (2) support movability. Option 1 implies that not_null<> cannot be movable for move-only types like std::unique_ptr<>, whereas option 2 implies that, for movable types, we lose the guarantee that a not_null<> object always holds a not-null value, which means that we need additional runtime checks in accessor methods. I think both are valid choices; M-GSL chose option 1, whereas gsl-lite chose option 2.

Glad to hear you found the library useful. I'd love to see a blog post with a case study!

[BenFrantzDale]

I suggested move support (and function-pointer and std::function support) on the main GSL. Herb Sutter himself shot down movable not_null<unique_ptr>.

[BenFrantzDale]

Yeah. In my opinion, read-from-moved-from is fundamentally different from null dereference. The prior is readily statically checked and locally scoped. Similarly, Sean Parent’s stlab::copy_on_write is a value type with the caveat that read-from-moved-from is UB. Movable nn_unique_ptr is so useful on API boundaries!

[BenFrantzDale]

For your curiosity...

I played with your diff of copy_on_write. I'm not using exactly that version of copy_on_write, so it may not matter in that case, but your diff on my version resulted in the T being fully defined in places where it wasn't required before, because constrain_and_disable_copy needs to evaluate std::is_constructible_v<T, U>. I did it slightly differently and it seemed to work:

template <class U>
using disable_copy = std::enable_if_t<!std::is_same<std::decay_t<U>, copy_on_write>::value>*;

template <class U>
copy_on_write(U&& x, disable_copy<U> = std::invoke([] { static_assert(std::is_constructible_v<T, U&&>); return nullptr; }))

That is: I used an immediately-invoked lambda in the default, pushing the std::is_constructible_v<T, U&&>); the check to the translation unit that's actually instantiating that constructor. I've never seen this technique, of an immediately-invoked lambda with a static_assert in a default, but it seems to do the trick by getting the static_assert to be evaluated only when T already needs to be fully defined and sneak it in before the members are initialized (where the cryptic error emerges).

I guess this works too probably:

template <class U>
copy_on_write(U&& x, disable_copy<U> = nullptr) 
    : _self{new model{
        std::invoke([&]() -> decltype(auto) {
            static_assert(std::is_constructible_v<T, U&&>);
            return std::forward<U>(x);
        }} {}

[mbeutel]

Interesting technique. However, assuming that the problem you're running into is caused by the evaluation of std::is_constructible<> for the copy_on_write<> type itself which is currently being defined—which I think is ill-formed, and indeed an oversight of mine—, then you can probably avoid this by re-ordering constraints and using std::conjunction<> and std::negation<>:

    template <class U>
-    using constrain_and_disable_copy = std::enable_if_t<std::is_constructible<T, U>::value && !std::is_same<std::decay_t<U>, copy_on_write>::value>*;
+    using constrain_and_disable_copy = std::enable_if_t<std::conjunction_v<std::negation<std::is_same<std::decay_t<U>, copy_on_write>>, std::is_constructible<T, U>>::value>*;

    template <typename U>
   using constrain_and_disable_copy_assign =
-       std::enable_if_t<std::is_assignable<T, U>::value && !std::is_same<std::decay_t<U>, copy_on_write>::value, copy_on_write&>;
+       std::enable_if_t<std::conjunction_v<std::negation<std::is_same<std::decay_t<U>, copy_on_write>>, std::is_assignable<T, U>, copy_on_write&>;