[swift-evolution] Asserts should not cause undefined behaviour

[Joseph Lord]

> On Jan 2, 2016, at 5:39 PM, Dave Abrahams via swift-evolution <swift-evolution at swift.org> wrote:
> 
> 
>>> On Dec 31, 2015, at 1:56 PM, Dave Abrahams via swift-evolution <swift-evolution at swift.org> wrote:
>>> 
>>> 
>>> On Dec 31, 2015, at 12:27 PM, Chris Lattner via swift-evolution <swift-evolution at swift.org> wrote:
>>> 
>>> On Dec 28, 2015, at 5:48 AM, Joseph Lord via swift-evolution <swift-evolution at swift.org> wrote:
>>>> 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.
>>> 
>>> Right.
>>> 
>>>> 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.
>>> 
>>> Only in cases where the assertion would have failed, right?  The point of -Ounchecked is that - if your code was correct with the checks - that it will still be correct.  Disabling overflow and array bounds checks is far more dangerous than the assertion behavior you cite here.
>>> 
>>>> 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.
>>> 
>>> Right.
>>> 
>>>> [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.
>>> 
>>> Why? :
>>> 
>>>> 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.
>>> 
>>> This is the C model, but as you know, there is a whole field of custom assertions libraries that people have developed.  I don’t think there is anything like consensus on this topic.
>>> 
>>>> 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.
>>> 
>>> This only affects code built with -Ounchecked, which is definitely not a safe mode to build your code.  The intention of this mode is that you can use it to get a performance boost, if you believe your code to be sufficiently tested.  This mode, which isn’t the default in any way, intentionally takes the guard rails off to get better performance.
>>> 
>>> If you don’t like that model, don’t use this mode.
>> 
>> Let’s just consider -O; I think I understand Joseph’s objection here, and it seems valid.
>> 
>> Normally in -O, we say that if you stay in the “safe subset” of Swift code, you never get undefined behavior, even if there’s a bug in your code.  You might get *unpredictable* behavior of course, but presumably guaranteeing no undefined behavior rules out large classes of problems, including many security holes.  Now suppose you decide to be responsible and add some asserts to help you catch bugs during development.  Hopefully they help you catch all the bugs, but what if they don’t?  All of a sudden, if you still have a bug when you ship, you now have undefined behavior.  As much as I’m a fan of assertions having optimization benefits, It seems a little perverse that using them could make shipping code less secure.
> 
> Wait a sec; I just read the doc comments for assert over again.  They don’t say there’s undefined behavior in -O if the condition isn’t satisfied.  So now I don’t understand what Joseph is complaining about.  assert in -O is documented to act exactly as C’s assert would with NDEBUG #defined.
> 
> -Dave

Thanks for the responses. 

The behaviour and documentation at -O is fine. My worry is only for unchecked builds. 

My concern is only for the unchecked behaviour. I still think it is surprising that adding an assert can substantively change the behaviour (undefined if false condition occurs). If assert was a brand new idea that did not exist in other languages It would be absolutely appropriate behaviour for the word as it is now but with the history of assert in other languages I still have two problems based on my understanding of asserts across languages. 

1) Adding asserts should never be able to add bugs (which they can do in unchecked code).
2) The stdlib assert cannot be used to handle any errors that may potentially occur and are handled but that you want to be informed of during development if there is any possibility that the code will ever be compiled in unchecked mode. 

The quote I put in the original email about having asserts and handling the error was to show that it is not a practice unique to myself. 

An example of where I use asserts is when parsing JSON I will always conditionally handle all potential results of invalid content (missing elements or wrong datatypes) and will usually just ignore the item and fail gracefully but will put asserts so that in development I know that either the data is malformed or I have a bug and am not handling all the reasonable inputs. If this code was compiled in unchecked mode with stdlib asserts it is likely to expose crashes and potentially be exploitable. 

Having said all of that there hasn't been much interest on the list so maybe my feeling about assert isn't ideally shared. 

Joseph