remove null check during access

[ericLemanissier]

based on @xaxxon's work

fixes #649
fixes #604

[annagrin]

thanks @ericLemanissier , I would love to remove the unnecessary null check, but it actually catches a case when a moved-from not null unique_ptr is accessed (see discussion in #674), so will close this for now, unless some other solution is found later.

[xaxxon]

This obviously can't provide guarantees that something externally modified isn't null. I question the usefulness of notifying the user much later after the thing has already become null that it is null.

Instead you're paying a runtime cost that only "helps" when you violate a basic premise of the type. The "help" doesn't give you any actual idea where you messed up, though, so I really don't understand forcing everyone to pay for it all the time.

[xaxxon]

If someone were to dereference the result, isn't it even possible the compiler will elide the check as well since it's not possible to be null in a valid program?

edit: no, that's not possible because it might terminate before it gets to the dereference.

[MikeGitb]

@annagrin: Does not_null now support unique_ptr? That wasn't the case last time I checked (admittedly some time ago).

[MikeGitb]

@xaxxon: As not_null supports move, a pointer like type that becomes null when moved from (like almost all smart pointers) could create a legal program state where a moved from not_null object contains a nullptr.

[xaxxon]

It's "legal" but nonsensical - it violates the entire point of a not_null - that you know at contract time that a variable isn't null. The error produced by accessing it is not helpful as it doesn't tell you WHY you are in this nonsensical state or WHERE in your program caused it. Your program is almost certainly going to crash and it's entirely possible the compiler elides the check in many circumstances if you were to somehow rely on the check.

it gives a false sense of safety, provides very little (if any), and will have a runtime cost to provide that. Its literally worse than useless both logically and for performance.

[MikeGitb]

@xaxxon: I was just answering your question if the compiler could optimize it away and the answer is no. That doesn't mean I like the design, but without language support for destructive move I'm not sure if there are any good solutions here (personally I'd just not support move and live with the consequences).

@annagrin : Are you aware that this check will not only fire for a moved from not_null<std::unique_ptr>, but also for a moved from not_null<std::shared_ptr> or not_null<std::weak_ptr>, which have been supported until this commit: ed3693f last year?

Is defaulting the move constructor of not_null really the right thing to do instead of e.g. making not_null a copy-only type (I find it particularly strange, that the type has a move constructor, but not a move assignment operator)?

// rant on
[ EDIT: Sorry, that was unnecessary. I'm just pretty frustrated with the gsl. ]
// rant off

[MikeGitb]

@annagrin : There is a typo in your last post: I think you wanted to link to #675.

[KVic]

I propose to treat the moved-from not_null is not in the state with a null inside, but in the unspecified and invalid state, like many other objects. So there is no need in the Ensures() check in the get() in the Release build.

Pro.lifetime: Lifetime safety profile or static analyzer checks, such as clang-tidy - bugprone-use-after-move , are one of the solutions.

[MikeGitb]

@KVic: A not_null object, that contains a nullptr is already treated as an invalid state - if it where a valid state, the post-condition of get would not be violated. I think it is a design mistake if it is possible that resonable code could produce such an invalid state without violating any preconditions on not_null (imho it should not be possible at all without going into language UB territory)). It imho greatly reduces the utility of not_null, even if static analyzer checks can mitigate that problem somewhat.

There may be different views on what contract checks are (supposed to be).
My perspective is this: Contract checks are debugging / safety mechanisms that are there to identify contract violations. They are NOT a mechanism to enforce the contract (i.e. if a check fails the contract has been violated even if the safety mechanisms catched it before that violation could cause any more damage). This is fundamentally different, from how the check in at() works where passing an invalid index is not a contract violation.

The reason for post condition checks to exist in the first place is that a) we are not sure if the implementation of a function is bugfree and b) that we might not have stated and checked all explicit and implicit preconditions. In this case, an implicit precondition is that the object is in a valid state. Now sure, we could delegate the responsibility to ensure that not_null is in a valid state to the user, but the nice thing about not_null (prior to ed3693f) that there was simply no sane way to get into such an invalid state in the first place. In fact, if you are willing to rely on a static analyzer, one could go even further and turn not_null into a simple type alias like owning.

[galik]

I also think that having not_null objects that can contain nullptr isn't good. The whole point of not_null is that you don't need to check if it is null before using it. But now you have no idea if you use it will it work or will it throw an exception? Suddenly, instead of simply being able to assume it is valid you have to assume you may have to catch an exception. Now you have to put null pointer exception traps throughout your code. The ability to continue safe in the knowledge you had a valid object is gone.

Maybe you could have two different types? First a not_null that can never be nullptr and is not moveable. Second, for those who want to substitute a well defined exception for undefined behavior a null_checked type that throws on accessing a nullptr which is a bit better then undefined behavior?

[annagrin]

@MikeGitb I would love to hear fustrations with GSL in a separate thread - would you be able to summarize them and send me an email (alternatively, you can start an issue so others can join. "Rant allowed" in the title would help:). I also greatly appreciate suggestions with PRs, since this is a community project. I only have very limited time to work on this, and mostly at my free time, so response and fixing issues can be delayed)

@xaxxon I would love to be able to resolve all the problems at once, but not remove the safety. I would prefer not to rely on static analysis checkers since they are not always used and, to my knowledge, not formally proved to detect all issues related to moved-from objects. If null check is removed, not_null just becomes an annotation, which is exactly what we are currently trying to replace with this type.

@galik @MikeGitb you have a good point about the untrustworthy not_null. Accepting #675 now seems premature - we either need to make not_null not movable or add more null checks (on move, copy and assign) to make at least caller/callee contracts be trustworthy, only leaving a local problem of a moved-from not_null object. As much as I am nor in favor in splitting not_null, that could be the only way to resolve all problems for now. Would you be able to submit a PR?