Should Expects be required to abort?

[JordanMaples]

The guidelines say here that GSL Expects always aborts on failure. http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#SS-assertions

However, Microsoft's GSL implementation has a mode that can alternatively throw on failure. see here microsoft/GSL#775 which prevents making some functions unconditionally noexcept.

[mbeutel]

The point of GSL_THROW_ON_CONTRACT_VIOLATION is testability. You want to be able to test that your function detects a precondition violation, and that is cumbersome if the function aborts the program. With GSL_THROW_ON_CONTRACT_VIOLATION you can write tests like

CHECK_THROWS_AS(expects_positive_number(-42), gsl::fail_fast);

Cf. http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2011/n3248.pdf, http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2011/n3279.pdf, and a discussion at https://quuxplusone.github.io/blog/2018/04/25/the-lakos-rule/.

(Edit: What I meant to say by this: perhaps the Core Guidelines should take the testing scenario into account and formally permit the handling of contract violations to be configured with a compile-time switch.)

[hsutter]

Editors' call: GSL.assert currently requires Expects to terminate the program if the condition is false. This is intended to mean std::terminate which permits a terminate_handler to do things like log some final information before restarting the process to try again. There is nothing in the Guidelines today about Expects throwing on contract violations.

[hsutter]

Clarification: Expects should terminate, not abort, just to be clear.

[aligusnet]

At the moment the guideline allows non-terminating behaviour as far as I understand:

“... Expect in under control of some options (enforcement, error message, alternatives to terminate)”

I think throws fall under “error message, alternatives to terminate”.

Moreover, the contract proposal allows custom handlers of contract violations.

So, I would not insist on removing the throws option.

[reddwarf69]

So,

• In the initial checkin of the Guidelines (https://github.com/isocpp/CppCoreGuidelines/commit/947cf3af#diff-feb71ecadc563b52e66838adbd6b8e30R10906) from 14 Sep 2015 "??? Expect in under control of some options (enforcement, error message, alternatives to terminate)" was added.
• On 8 Oct 2015 Bjarne removed the ??? from it (https://github.com/isocpp/CppCoreGuidelines/commit/91109eb8#diff-feb71ecadc563b52e66838adbd6b8e30R12407).

...

• Based on this issue https://github.com/microsoft/GSL/pull/831 was created. Not only removing the throwing option (GSL_THROW_ON_CONTRACT_VIOLATION), the enforcement option (GSL_UNENFORCED_ON_CONTRACT_VIOLATION) also.

This was done because The goal of this PR was to align the GSL's contract violation behaviors to those outlined in the Core Guidelines. Specifically, in isocpp/CppCoreGuidelines#1512 it was discussed that contract violation should result in terminating behavior. So removal of the alternative contract violation behavior was necessary. Notice it says necessary.

Meanwhile https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#gslassert-assertions is still saying "// Expect in under control of some options (enforcement, error message, alternatives to terminate)".

Are we sure everybody understood each other?

[johnmcfarlane]

I do feel that GSL_UNENFORCED_ON_CONTRACT_VIOLATION should have been retained as a mode for Expects. It's one of the "alternatives to terminate" which works really well under a UB sanitizer.

[MikeGitb]

Only if the violation leads to hard language UB.

[johnmcfarlane]

@MikeGitb ... which it did with GSL_UNENFORCED_ON_CONTRACT_VIOLATION up until this recent policy change. Now we get occasionally slower performance-critical code and analysis tools which are a little less able to do their job. This really feels like a lose-lose to me.

[reddwarf69]

We have an "Editors' call" from Herb and an action based on Jordan's interpretation of it. The only question is whether that interpretation is correct, and @hsutter is the only one that can confirm or deny that.

[MikeGitb]

@MikeGitb ... which it did with GSL_UNENFORCED_ON_CONTRACT_VIOLATION up until this recent policy change.

Are you sure? If I have a 5 element view over a 10 Element array and call view[5], that may be a contract violation in the library level, but it most likely will not be UB as far as the language is concerned.

[mbeutel]

@MikeGitb ... which it did with GSL_UNENFORCED_ON_CONTRACT_VIOLATION up until this recent policy change.

Are you sure? If I have a 5 element view over a 10 Element array and call view[5], that may be a contract violation in the library level, but it most likely will not be UB as far as the language is concerned.

It is UB because a contract violation amounts to Expects(false), and under GSL_UNENFORCED_ON_CONTRACT_VIOLATION this used to be expanded to __assume(false) (MSVC) or false ? static_cast<void>(0) : __builtin_unreachable() (GCC) (reference). According to MSDN, "If the compiler can reach an invalid __assume statement, the program might cause unpredictable and potentially dangerous behavior", which I think describes UB. Likewise, the GCC documentation states: "If control flow reaches the point of the __builtin_unreachable, the program is undefined."

[pgroke-dt]

IMO having terminate as the only option is a big mistake for a library - at least if the intent is that it can be widely adopted in all kinds of applications/projects. Don't get me wrong, I understand the benefits of it, and I'm also a huge fan of using terminate to deal with unexpected errors. I also generally don't like "defensive" programming.

Only there are applications where it's actually preferable to risk something working "not quite right" (or not at all), missing/incomplete/wrong results etc. compared to terminating the application. Throwing an exception on contract violation IMO was a good compromise for such applications. Even ignoring the contract violation as with good-old assert would be preferable for some such applications.

Of course the latter (ignoring violations) would have to be implemented in a way that doesn't add additional UB by means of __builtin_unreachable() & co.

So while I agree that the guideline is good in general, I disagree about removing support for "throw on violation" or "ignore violations" from a library like GSL because of that.

[MikeGitb]

I agree that it should be possible to turn contract checks of.

Also, I'd like to mention that I think it is a misuse of contracts to use them to implement parts of a function's/type's specified behavior. If gsl::span's specification says it will prevent any out of bound access, then passing in an invalid index isn't a contract violation anymore - span::operator[] now simply has a wide contract that says "calls terminate if i>= size()" and that should be exactly how it's implemented.

[beinhaerter]

@hsutter Can you please comment on the replies, especially of aligusnet and reddwarf69? That would help end the discussion (which just revived on the GSL repo). Also it is confusing that the issue is still open after you wrote the editor's call. Was this the final answer or is there still a discussion ongoing from your point of view?