[swift-evolution] Asserts should not cause undefined behaviour

[Joseph Lord]

I propose that assert and assertionFailure should be no-ops (with branch 
hints) in unchecked builds as they are in normal release builds rather 
than resulting in undefined behaviour in the failure condition.

I would like to kick off a discussion of this. I found the proposal 
template useful for setting out my thoughts clearly to trigger the 
discussion but I'm not trying jump the discussion phase. I'm open to 
radical rewrites or to not proceed if there is opposition. Also if my 
understanding of the current code is wrong please let me know.

[Current Situation]

The documented behaviour of assert and assertionFailure in "disable 
safety checks" builds (still documented as -Ounchecked) is that the 
compiler "may assume that it would evaluate to true" or in the 
assertionFailure case never be called.

This documented behaviour would allow the compiler to completely 
eliminate tests and branches before or after the assertion and take the 
operation deep into undefined behaviour.

https://github.com/apple/swift/blob/cf8baedee2b09c9dd2d9c5519bf61629d1f6ebc8/stdlib/public/core/Assert.swift 
(latest commit to this file at time of writing)

[NOTE - Actual current behaviour]

It appears from the code as if the assumption is not currently applied 
on the assert method although it is on the assertionFailure case by 
means of the _conditionallyUnreachable() call. assert seems to be a 
no-op in both normal release and disable safety checks build modes.

It also appears that precondition does not apply the permitted 
assumption when in _isFastAssertConfiguration mode.

There also appears to be code in assert to hint the compiler that the 
assert will likely be true. This was something that I was planning to 
suggest and means that code containing asserts can be faster than the 
same code without the assert in release mode.

[Proposed Change]

Change the documentation for assert and assertionFailure so that 
behaviour in unchecked mode is the same as in normal release - no 
evaluation and no effect.

Change the behaviour of assertionFailure to match the updated documentation.

[Motivation]

1) Expected behaviour based on other languages is for assert to have no 
effect in release. (If current behaviour is highly desired another 
function name should be used). Having potential dangerous behaviour from 
a function that is familiar across languages and is regarded as a safety 
feature is undesirable.

2) Adding asserts to code should not make the code more dangerous 
whatever the build. Assuming the truth of the assert may lead to runtime 
safety checks being skipped and undefined behaviour when a no-op would 
be a safe behaviour.

3) "For highly robust code assert and then handle the error anyway" 
[Code Complete 2nd Edition section 8.2] While the designed expectation 
from the Swift team is to use them for checks against internal 
programming errors within a module in a number of cases I use assertions 
and assertionFailure calls while processing input data. The code without 
the assertion would simply fail to read the input gracefully but as the 
developer I want to know immediately if there are realistic cases that 
have not been handled so use assertions to flag it to myself that a case 
may need better handling. As such there are assertions in my code that 
are in error cases that I expect not to occur. I do not want the runtime 
safety checks being optimised out. [Being aware of this issue I use 
custom assertions but am therefore missing out on the branch hinting of 
the stdlib version and others may want similar behaviour and may not 
fully read the documentation].

[Impact on existing code]

Loss of performance in cases where assertionFailure is used. Loss of 
potential performance improvement option in assert usage. In most cases 
this performance loss will be small but there is potential where the 
assumption of the value allows large code blocks (to evaluate the 
condition) to be eliminated for significant effects but I suspect that 
these cases are rare and preconditionFailure could be used instead to 
get the performance in unchecked builds (at the cost of release 
performance) or an additional method could be added.

[Alternatives]

Renaming assert so that the behaviour is not assumed by users familiar 
with other languages. Function name suggestion:

assume/assumeUnreachable

This could either behaviour as the current assert/assertionFailure 
documentation or possibly allow the assumption to be made in normal 
release mode not just the unchecked. It should be a fatal error in debug 
builds if assumption is incorrect.

[Note about precondition]

precondition/preconditionFailure also have undefined behaviour in 
unchecked mode but this is not a problem for me for a couple of reasons:

1) I don't have the same prior understanding about what they do.

2) It is clear from release build behaviour that hitting the condition 
is always an error.