More Robust GSL Contracts

[suncho]

I was looking at the Microsoft GSL implementation of the Expects and Ensures macros to see how they worked and I discovered that they both do exactly the same thing:

https://github.com/Microsoft/GSL/blob/master/include/gsl/gsl_assert

The behavior I had expected was that the Ensures condition would be evaluated when the block exited. Instead, it's just an on-the-spot assert. This means that to properly use Ensures, you have to slap an Ensures statement at every possible exit point of your function.

So I decided to fix it and implement the behavior I had expected in the first place:

https://github.com/suncho/GSL/blob/c9ecdfcbc170a75b666d8ba42fd7fed5e58df380/include/gsl/gsl_assert

With this implementation, you can now put your Ensures statement at the top of your function body and it will automatically check that the condition is met when the function exits. I also implemented AlwaysEnsures to do the check even if the function exits via exception.

Because some functions simply maintain invariants, expressing that would require Expects and Ensures statements for the same condition. So I also added a Maintains macro and an AlwaysMaintains macro to express that invariants are maintained. These macros aren't strictly necessary, but they seemed like obvious additions because they simplify readability and reduce the chance of bugs that may arise from having to specify the same condition twice.

I had submitted a pull request to the MS GSL implementation, but @neilmacintosh suggested that I raise the issue here first:

https://github.com/Microsoft/GSL/pull/637

He was worried that my implementation didn't match up with the description in the Core Guidelines. Is there any reason why it shouldn't?

If you had been putting Ensures at the end of your function bodies, my implementation should still work the same as the old version.

I think the only time the behavior would change is if you had code where you used Ensures to check a condition and then changed the condition after the Ensures, but before the end of the block. In that case, you weren't properly using Ensures to check post-conditions anyway.

[MikeGitb]

I like the Idea.

I wonder if it has any significant performance impact (calling uncaught_exceptions twice for every small function that has a post condition is probably not all that cheap)

[suncho]

Hmm. Good question. I certainly didn't design this for performance.

That being said, I'm pretty sure uncaught_exceptions is only called once per post condition. And only if the post conditions are enforced, which you might not want on release builds anyway.

AlwaysEnsures technically doesn't have to call uncaught_exceptions at all. When I initially wrote the if statement, it looked like this:

if (always || !GSL_UNCAUGHT_EXCEPTIONS()) asserter();

But that generated a "conditional expression is constant" warning on the Microsoft compiler, so I switched them around. If I just disable the Microsoft warning and switch the order back, then AlwaysEnsures will never call uncaught_exceptions.

We could probably rearrange it so the post condition is tested before uncaught_exceptions is called. The call to uncaught_exceptions would only be called if the post condition fails. We wouldn't call it at all during normal control flow.

[suncho]

K. I've made it so that uncaught_exceptions is called only when the post condition fails:

https://github.com/suncho/GSL/commit/4db6df62d3832a854e7e142a488fc9e08bf8774e

[MikeGitb]

@suncho: I have to admit, I didn't look into your implementation, but in order to reliably implement a on_success functionality (as in only check post-condition when not exiting via exception) you afaik need two calls to std::uncaught_exceptions. One at the start to check if stackunwinding due to exception is already in progress and one on function exit to see if a new one occured. Afaik that is the main reason for std::uncaught_exceptions replacing std::uncaught_exception.

AlwaysEnsures is nice, but of limited use outside of noexcept functions.

Don't get me wrong: I still think it would be worth it. The fact that I have to write the function's post conditions inside a function and maybe even multiple times has annoyed me for some time. Of course that will still be necessary sometimes, but it should reduce the number of occurrences.

[suncho]

@MikeGitb I'm having trouble imagining the implementation you're describing. Why would you need two calls to std::uncaught_exceptions? Can you link me to some code or a description of what you're talking about?

The new version of my Ensures implementation only calls std::uncaught_exceptions at most once and that call takes place immediately before the destructor exits. Because I call it right before the destructor exits, the difference between std::uncaught_exception and std::uncaught_exceptions does not come into play.

It's important to keep in mind the following:

// There are three configuration options for this GSL implementation's behavior
// when pre/post conditions on the GSL types are violated:
//
// 1. GSL_TERMINATE_ON_CONTRACT_VIOLATION: std::terminate will be called (default)
// 2. GSL_THROW_ON_CONTRACT_VIOLATION: a gsl::fail_fast exception will be thrown
// 3. GSL_UNENFORCED_ON_CONTRACT_VIOLATION: nothing happens

What Ensures and AlwaysEnsures do will depend on the configuration option.

If GSL_TERMINATE_ON_CONTRACT_VIOLATION or GSL_THROW_ON_CONTRACT_VIOLATION are set, then the post conditions for a function are tested every time the function exits.

If the condition fails, Ensures will call std::uncaught_exceptions once and silently return if there is an uncaught exception. AlwaysEnsures skips this step.

Next, either we throw a gsl::fail_fast exception or we std::terminate() depending on the configuration option. If we're using AlwaysEnsures, and the function is already exiting via exception, throwing a new exception will end up calling std::terminate() anyway. So the two are roughly equivalent in that case.

Does that make sense?

In what sense is AlwaysEnsures of limited use outside of noexcept functions? It's a promise that a particular post condition always holds even when the function exits via exception. You can use it to ensure that your function isn't going to violate the post condition even when it fails.

Are you worried that the condition test itself might throw an exception? This would cause your function to exit via an exception originating from within the condition test. The condition would never be successfully verified.

If we want to prevent this, we can wrap the AlwaysEnsures condition test in a noexcept lambda. It will ensure that the program terminates any time the condition test itself throws an exception. But it might be overkill since we'd already be dealing with a logic error that should probably be fixed anyway.

Here's the change if we wanted to implement that:

https://github.com/suncho/GSL/commit/dea13c2659cb1e05447f4863fe8e54742301eb73

[MikeGitb]

The question is: How do you distinguish between an exception that has been thrown before the function (for which I specify the post-condition) has been entered and one occuring during it's execution?

The most likely answer is that you don't care most of the time, but if you do, you have to compare the exception count at the start and exit of the function.

[MikeGitb]

The reason, why I think that check_always is of limited use (not no use) is because usually the reason for throwing an exception is precisely, because I can't maintain the post condition of a function.

Anyway, sorry for bringing up this nitpicking in the first place. It's imho really not that important.

[suncho]

I see what you mean now. You're worried that a destructor called by stack unwinding might cause the invocation of a function that has an Ensures statement. A post condition violation would be ignored because std::uncaught_exceptions would be a number other than zero.

That's a reasonable concern.

It's only a problem for Ensures though. It's not a problem for AlwaysEnsures. AlwaysEnsures will always detect a violation. It doesn't call std::uncaught_exceptions.

Hmm.

[suncho]

@MikeGitb I've updated it so that Ensures only ignores a post condition violation if, as you say, the exception count changes between the start and exit of the function:

https://github.com/suncho/GSL/commit/fc04409e724bb9bdb95a2f70bd90c3fc388f42b5

This always works in C++17.

In C++14, which only has std::uncaught_exception, this only works for the first exception. If an exception is thrown inside a function invoked as a result of a destructor called during stack unwinding, C++14 has no way of seeing that. When post conditions are violated in such a case, an Ensures will behave as if it's an AlwaysEnsures.

Importantly, you'll never end up in a situation where a post condition violation is ignored during normal (non-exception) function exit, even during stack unwinding.

I'm assuming the performance isn't great. But std::uncaught_exception is still only called once for each Ensures unless the post condition is violated. And like I said before, you can always set GSL_UNENFORCED_ON_CONTRACT_VIOLATION in release builds.

[hsutter]

Expects and Ensures are to temporarily emulate pre/postconditions until we get language contracts. The editors would like to defer this question until after the contracts feature is voted into the IS.

[HenryMiller1]

Now that http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0542r4.html is accepted for C++20 the deferred label needs to be removed and this revisited.