[Development] Disavowing the Lakos Rule for Q_ASSERT

[Marc Mutz]

Your two examples are quite different, though:

On 26.08.24 22:28, Thiago Macieira wrote:
> On Monday 26 August 2024 12:59:54 GMT-7 Marc Mutz via Development wrote:
>> Part of why I'm sympathetic to the assertions throw mantra is that I
>> used it very successfully in Kleopatra way back when. We could easily
>> take the exception's what() and report back over the UI server channel
>> that an assertion happened and the client would show a message box with
>> the assertion details, copyable from a QMessageBox into a bugreport.
>> T'was a boon for testing.
> 
> Only developers would be interested in seeing exception names. Regular users
> won't understand what that exception means and will just be confused.
> 
> I am also vehemently against catching exceptions you don't have a recovery
> code path for. That destroys the stack trace and state that would have helped
> the developer understand and actually fix the problem.
> 
> I suppose it's possible someone will use preconditions for recoverable
> conditions, but I've never seen that even proposed, for any API. More
> importantly, for the cases we have at hand, like:
> 
>          static Qt::strong_ordering compareThreeWay_helper(const Iterator &lhs,
>                                                            const Iterator &rhs)
> noexcept
>          {
>              Q_ASSERT(lhs.item.d == rhs.item.d);
>              return Qt::compareThreeWay(lhs.item.i, rhs.item.i);
>          }
> 
> The most likely reason for violating this precondition is that the caller is
> already corrupt and totally lost. I don't see how it could recover.

This is an iterator API, and that lhs.item.d == rhs.item.d is a 
precondition of the equality operator, so outside of our control. As I 
mentioned on Gerrit, we can control the width of the contract we choose 
to provide, though: we could make the function have a narrow contract if 
we declared iterators with mimatching item.d as partial_ordering::unordered.

The fundamental problem that iterators into different containers are 
incomparable doesn't go away if you slap noexcept on op==. You have a 
decision to make: UB or unordered? And _that_ informs whether it's 
noexcept or not.

> Another example is:
> QUuid QCborValue::toUuid(const QUuid &defaultValue) const
> {
>      if (!container || !isUuid() || container->elements.size() != 2)
>          return defaultValue;
> 
>      Q_ASSERT(n == -1);
>      const ByteData *byteData = container->byteData(1);
>      if (!byteData)
>          return defaultValue; // UUIDs must always be 16 bytes, so this must be
> invalid
> 
>      return QUuid::fromRfc4122(byteData->asByteArrayView());
> }
> 
> This is verifying an internal invariant state, not a precondition. The only
> two reasons this assertion would trigger are a buggy implementation or
> corrupted memory. Neither of which you can recover from with C++ code.

I agree. This is an internal check about the invariants of the class and 
we are in full control over whether they are maintained or broken. This 
is not a precondition and the function could be noexcept (if the other 
checks pass: no memory allocated and no POSIX cancellation points called).

The distinction between the two is why I a weary about the removal of 
Q_ASSUME. I guess we could resurrect the distinction with other names 
(Q_PRE/Q_POST/Q_INVARIANT)?

Thanks,
Marc

-- 
Marc Mutz <marc.mutz at qt.io> (he/his)
Principal Software Engineer

The Qt Company
Erich-Thilo-Str. 10 12489
Berlin, Germany
www.qt.io

Geschäftsführer: Mika Pälsi, Juha Varelius, Jouni Lintunen
Sitz der Gesellschaft: Berlin,
Registergericht: Amtsgericht Charlottenburg,
HRB 144331 B