not_null<T> construction from T: is the "explicit" constructor explicit enough?

[petamas]

Problem statement

Last year, I described Dropbox nn's constructor this way (see my comment in issue #200):

• it does not have an nn(T) constructor
• it does have an i_promise_i_checked_for_null_t tag type
• it does have an nn(i_promise_i_checked_for_null_t, T) constructor
• you're urged to create some macro named NN_CHECK (or any name you want to use, like CHECK_NOT_NULL), which checks for null, does whatever error handling you want to if the pointer is null, then calls the tagged constructor

This way, you must be pretty explicit about creating nn objects, but I think it's not much safer to use that version than ours (only advantage is that you can't construct an nn "by accident", using static_casts and such).

Since then, I have revised my opinion: I think it would be better if you couldn't create a not_null from a nullable without explicitly calling make_not_null. The following is the description of the evolution of my company's codebase, highlighting the problem with our current (explicit) constructor, then my proposed solution.

Case study

Original code

We had several old, pre-C++11 factory methods:

gsl::owner<Thing1*> createThing1(); // never returns NULL
gsl::owner<Thing2*> createThing2(); // never returns NULL
gsl::owner<Thing3*> createThing3(); // my return NULL based on configuration

The client code used std::auto_ptr to manage the memory:

std::auto_ptr<Thing1> thing1(createThing1());
std::auto_ptr<Thing2> thing2(createThing2());
std::auto_ptr<Thing3> thing3(createThing3());

Introducing unique_ptr

After migrating to C++11, we changed our auto_ptrs to unique_ptrs, an made our factory methods return unique_ptr:

std::unique_ptr<Thing1> createThing1(); // never returns nullptr
std::unique_ptr<Thing2> createThing2(); // never returns nullptr
std::unique_ptr<Thing3> createThing3(); // may return nullptr based on configuration
// ...
std::unique_ptr<Thing1> thing1(createThing1());
std::unique_ptr<Thing2> thing2(createThing2());
std::unique_ptr<Thing3> thing3(createThing3());

Note that we did the auto_ptr->unique_ptr rather mindlessly, not changing the syntax in the client apart from changing the smart pointer's name.

Introducing not_null

When not_null became able to handle unique_ptr, we changed the return type of the factory functions to nn_unique_ptr<T> (i.e. gsl::not_null<std::unique_ptr<T>>):

nn_unique_ptr<Thing1> createThing1(); // never returns nullptr
nn_unique_ptr<Thing2> createThing2(); // never returns nullptr
std::unique_ptr<Thing3> createThing3(); // my return nullptr based on configuration
// ...
nn_unique_ptr<Thing1> thing1(createThing1());
nn_unique_ptr<Thing2> thing2(createThing2());
nn_unique_ptr<Thing3> thing3(createThing3()); // !!! oooops !!!

During this transition, we made a mistake, and changed the type of thing3 even though it should have been left as a plain old unique_ptr. The above code compiled without errors, and the mistake was only discovered during testing, when createThing3() was configured to return nullptr.

Proposed solution

The events described above led me to believe that not_null<T> should not have a converting constructor from T or any other nullable types; the conversion should always be done via explicitly calling make_not_null(). In that case, nn_unique_ptr<Thing3> thing3(createThing3()); would result in a compile-time error instead of a run-time error.

This could be achieved by stealing the idea from Dropbox nn, and giving it our own spin:

• make the T->not_null<T> constructor private, and give it a tag argument
• make make_not_null() a friend of not_null

This way, only make_not_null() could be used to create not_nulls, while not impacting the converting constructors between different not_null instantiations. Coupled with my proposal in issue #253, it would mean the only transition between nullable- and not_null-land would be through the two named functions make_not_null and as_nullable, making everything explicit and (mostly) mistake-free.

[mbeutel]

Thank you, this is very valuable input.

My main objection would be that, conventionally, make_*() functions are constructor forwarders mostly made obsolete by deduction guides in C++17. Hence I'd find it surprising if in C++17 make_not_null(X) worked but not_null(X) didn't.

I'd prefer to give the not-null "entry point" a different name. Perhaps check_not_null()?

Of course, subsequent objections would be concerned with backward compatibility and general Core Guidelines compliance. But that's a bigger issue. For now I'd be happy to have this change under a config switch so we can experiment with it.

[petamas]

My main objection would be that, conventionally, make_*() functions are constructor forwarders mostly made obsolete by deduction guides in C++17. Hence I'd find it surprising if in C++17 make_not_null(X) worked but not_null(X) didn't.
I'd prefer to give the not-null "entry point" a different name. Perhaps check_not_null()?

I understand and agree; maybe checked_not_null?

Of course, subsequent objections would be concerned with backward compatibility and general Core Guidelines compliance. But that's a bigger issue.

True; this may also warrant discussion with the Core Guidelines team.

For now I'd be happy to have this change under a config switch so we can experiment with it.

I'll prepare a PR. (And leave make_not_null's name until we know more.) Name ideas for the config are welcome, as usual. :) gsl_CONFIG_NOT_NULL_DIRECT_CONSTRUCTOR, with 1 as default?

[mbeutel]

I understand and agree; maybe checked_not_null?

I'd prefer to use verbs to name functions. Why not check_not_null()?

gsl_CONFIG_NOT_NULL_DIRECT_CONSTRUCTOR, with 1 as default?

Well, we already have gsl_CONFIG_NOT_NULL_EXPLICIT_CTOR, so perhaps your switch could just be named gsl_CONFIG_NOT_NULL_CTOR, with 1 as default.

[petamas]

I'd prefer to use verbs to name functions. Why not check_not_null()?

To me, the name check_not_null suggests that the function will check if the argument is null, then report back; instead, the function signals contract violation (by throwing an exception, or terminating, etc.). By calling it checked_not_null, we kinda-sorta shift the responsibility of checking to the caller. (I agree it's still not the best name, but I think it's better than check_not_null.)

Well, we already have gsl_CONFIG_NOT_NULL_EXPLICIT_CTOR, so perhaps your switch could just be named gsl_CONFIG_NOT_NULL_CTOR, with 1 as default.

Calling it gsl_CONFIG_NOT_NULL_CTOR would be misleading in my opinion: even if we hide the "direct" constructor, plenty more are still available (copy, move, converting form other not_nulls, etc.). That's why I'd prefer gsl_CONFIG_NOT_NULL_XXX_CTOR, with a sufficiently chosen XXX.
XXX=CONVERTING is not a good option, since the not_null<U> constructors are usually referred as "converting constructors"; that's why i said XXX=DIRECT, which is not an established name. Or maybe XXX=UNCHECKED?

(I agree with the usage of _CTOR for consistency)

[mbeutel]

By calling it checked_not_null, we kinda-sorta shift the responsibility of checking to the caller.

I think I understand why you find check slightly misleading. This made me remember CHECK() vs. REQUIRE() in Catch2; perhaps require_not_null() would be a better fit?

[petamas]

require_not_null() seems good to me.

[mbeutel]

I now implemented require_not_null() and a switch tentatively called gsl_CONFIG_NOT_NULL_DIRECT_CTOR. I also refactored the CMake build scripts somewhat to make it easier to add more testing configurations, which may help with #264.

However, I've had second thoughts about this. The problem you've run into is a much more general pitfall, and it affects explicit constructors everywhere, not just the not_null<> ones. This is one of the reasons why one should prefer "almost always auto" style.

I'm not saying require_not_null() cannot be a useful addition to gsl-lite; but I'm a little wary of foregoing explicit constructors; this is a general language issue, so perhaps it would be more appropriate to use a linting tool (e.g. clang-tidy with "modernize-use-auto"). This might help identify all the other inadvertently called explicit constructors in addition to the ones from gsl-lite.

In fact this problem (inadvertently called explicit constructors) seems so commonplace that I'd expect Clang or GCC to have a warning for this. I didn't check but it might be worth searching for one.

[mbeutel]

Any comments? Otherwise I'm inclined to factor out the CMake-related changes out of #274 into a separate PR and to close #274.

[mbeutel]

The CMake-related changes are now in #285. I'm closing this issue and #274 for the reasons outlined above (and due to lack of feedback). Please feel free to reopen the issue if you want to discuss this further.