[Rd] C/C++ 'assert' should not be used in R packages

[Simon Urbanek]

On Nov 10, 2007, at 5:38 PM, Duncan Murdoch wrote:

> On 10/11/2007 4:27 PM, Simon Urbanek wrote:
>> On Nov 10, 2007, at 1:05 PM, Duncan Murdoch wrote:
>>> On 10/11/2007 1:00 PM, Duncan Temple Lang wrote:
>>>> -----BEGIN PGP SIGNED MESSAGE-----
>>>> Hash: SHA1
>>>>
>>>>
>>>>
>>>> Duncan Murdoch wrote:
>>>>> Prof Brian Ripley wrote:
>>>>>> Please don't use 'assert' in R packages.  If called, this  
>>>>>> means  that an
>>>>>> error in your code aborts the whole R process, including your   
>>>>>> user's work.
>>>>>> I see several R packages doing this, and one of them called   
>>>>>> 'assert' on me
>>>>>> earlier in the week.
>>>>>>
>>>>> I partly disagree about this.  If assert() is triggered, it  
>>>>> clearly
>>>>> indicates a bug in the package.  If it just generated an R  
>>>>> error,  most
>>>>> users would ignore it, and not report it to the package  
>>>>> maintainer.
>>>>>
>>>>> It may well be that when an assertion fails, none of the  
>>>>> subsequent
>>>>> calculations are reliable, in which case returning control to  
>>>>> the  user
>>>>> could result in data corruption.  That's worse than losing a   
>>>>> session,
>>>>> because at least when you lose a session, you know it.
>>>>>
>>>>> Could we write our own implementation of assert() that displays  
>>>>> an R
>>>>> error and unloads the package?  I think I could do something  
>>>>> like  that
>>>>> in Windows by calling FreeLibrary to unload the DLL, but I'd   
>>>>> prefer a
>>>>> cross-platform solution.
>>>> I am not sure why you think we need to discard the DLL.
>>> The package author has asserted that it is no longer safe to  
>>> run.   They are shutting down R, and Brian finds that too extreme.
>>>
>> Me too. A package should never intentionally close the R session.  
>> If  it does, it is a very severe design bug in the package  
>> (IMHO).  Anything you use assert for can be gracefully handled (as  
>> opposed to  let's say a segfault), so there is no excuse for  
>> killing the whole  process at all (deliberately wiping all user's  
>> work is rude to put it  mildly). Even if it might be unsafe to run  
>> anything, I'd still want  to attempt to save my data.
>
> Perhaps I use assert() differently than you, but the way I use it  
> is to assert assumptions that I believe will always be true.
>
> If one of those assertions fails, it means that my program is  
> operating in a way that I did not foresee.  I can't recover  
> gracefully from that, because at that point my basic assumptions  
> about the program have been proven to be incorrect.  I have no  
> basis for reasoning.  It is by definition a bug.
>
> Now, it's possible that this bug has corrupted R and it really  
> would be best for the user to discard all his work, but I agree  
> that's unlikely.  That's why I proposed that R should offer a way  
> to handle an assertion failure that limits the shutdown to just my  
> package.
>
> I don't offer a guarantee that my work won't corrupt a user's data,  
> but I do work hard to avoid that.  Allowing my package to tell the  
> user that it is now unsafe to use is one way to do so.

I agree with everything so far...


>   That's what assert() is for.
>

No, that's the part I don't agree with. assert() is for a stand-alone  
program, not for modules. Using assert takes down whole R despite the  
fact that R itself is highly likely fine. It's the same as if you  
proposed that an error in a program should take the whole OS down,  
because you want to really alert the user. (I know, some OSes really  
do this, but not the ones I'd want to use ... :P)


> In a standalone program, an (uncaught) assertion failure will cause  
> the RTL to shut down the process.  I think R should support a  
> version of that, but limit the shutdown to the package that has  
> failed.   The version of assert() that I put together for rgl this  
> morning doesn't attempt to shut down rgl, because that's really  
> something that R should be responsible for.  All it does is issue a  
> dire warning to the user.
>

Again, I fully agree with that.

Cheers,
Simon