From maechler at stat.math.ethz.ch  Tue May 16 19:10:30 2017
From: maechler at stat.math.ethz.ch (Martin Maechler)
Date: Tue, 16 May 2017 19:10:30 +0200
Subject: [Rd] stopifnot() does not stop at first non-TRUE argument
In-Reply-To: <alpine.DEB.2.20.1705160942240.3019@luke-Latitude>
References: <64acdaeb-2857-9017-68c9-9da08e29a617@fredhutch.org>
 <E5DADF15-58C6-4F73-BC56-C6404896982F@gmail.com>
 <c2b3cb22-90bd-5372-e8b9-d1ea8b2ab828@fredhutch.org>
 <8945f7e4-99ce-53fa-46ee-cce64078a6a8@fredhutch.org>
 <22809.26921.473223.395550@stat.math.ethz.ch>
 <8fdb66b1-f785-ed55-6e40-a4ef8138e637@insa-toulouse.fr>
 <26bd9210-e40c-0967-7abb-5f724b09c034@insa-toulouse.fr>
 <22809.44830.153073.675527@stat.math.ethz.ch>
 <477f9e1a-4f7c-a902-e744-da4a36eb5a4f@insa-toulouse.fr>
 <22809.52422.219712.66882@stat.math.ethz.ch>
 <alpine.LFD.2.20.1705151236510.27291@itasca.stat.uiowa.edu>
 <3d296d7c-6fc0-0175-7bc0-57638cc3998b@fredhutch.org>
 <22810.50942.577206.452639@stat.math.ethz.ch>
 <alpine.DEB.2.20.1705160942240.3019@luke-Latitude>
Message-ID: <22811.12934.755008.738297@stat.math.ethz.ch>

>>>>>   <luke-tierney at uiowa.edu>
>>>>>     on Tue, 16 May 2017 09:49:56 -0500 writes:

    > On Tue, 16 May 2017, Martin Maechler wrote:
    >>>>>>> Herv? Pag?s <hpages at fredhutch.org>
    >>>>>>> on Mon, 15 May 2017 16:54:46 -0700 writes:
    >> 
    >> > Hi,
    >> > On 05/15/2017 10:41 AM, luke-tierney at uiowa.edu wrote:
    >> >> This is getting pretty convoluted.
    >> >>
    >> >> The current behavior is consistent with the description at the top of
    >> >> the help page -- it does not promise to stop evaluation once the first
    >> >> non-TRUE is found.  That seems OK to me -- if you want sequencing you
    >> >> can use
    >> >>
    >> >> stopifnot(A)
    >> >> stopifnot(B)
    >> >>
    >> >> or
    >> >>
    >> >> stopifnot(A && B)
    >> 
    >> > My main use case for using stopifnot() is argument checking. In that
    >> > context, I like the conciseness of
    >> 
    >> > stopifnot(
    >> > A,
    >> > B,
    >> > ...
    >> > )
    >> 
    >> > I think it's a common use case (and a pretty natural thing to do) to
    >> > order/organize the expressions in a way such that it only makes sense
    >> > to continue evaluating if all was OK so far e.g.
    >> 
    >> > stopifnot(
    >> > is.numeric(x),
    >> > length(x) == 1,
    >> > is.na(x)
    >> > )
    >> 
    >> I agree.  And that's how I have used stopifnot() in many cases
    >> myself, sometimes even more "extremely" than the above example,
    >> using assertions that only make sense if previous assertions
    >> were fulfilled, such as
    >> 
    >> stopifnot(is.numeric(n), length(n) == 1, n == round(n), n >= 0)
    >> 
    >> or in the Matrix package, first checking some class properties
    >> and then things that only make sense for objects with those properties.
    >> 
    >> 
    >> > At least that's how things are organized in the stopifnot() calls that
    >> > accumulated in my code over the years. That's because I was convinced
    >> > that evaluation would stop at the first non-true expression (as
    >> > suggested by the man page). Until recently when I got a warning issued
    >> > by an expression located *after* the first non-true expression. This
    >> > was pretty unexpected/confusing!
    >> 
    >> > If I can't rely on this "sequencing" feature, I guess I can always
    >> > do
    >> 
    >> > stopifnot(A)
    >> > stopifnot(B)
    >> > ...
    >> 
    >> > but I loose the conciseness of calling stopifnot() only once.
    >> > I could also use
    >> 
    >> > stopifnot(A && B && ...)
    >> 
    >> > but then I loose the conciseness of the error message i.e. it's going
    >> > to be something like
    >> 
    >> > Error: A && B && ... is not TRUE
    >> 
    >> > which can be pretty long/noisy compared to the message that reports
    >> > only the 1st error.
    >> 
    >> 
    >> > Conciseness/readability of the single call to stopifnot() and
    >> > conciseness of the error message are the features that made me
    >> > adopt stopifnot() in the 1st place.
    >> 
    >> Yes, and that had been my design goal when I created it.
    >> 
    >> I do tend agree with  Herv? and Serguei here.
    >> 
    >> > If stopifnot() cannot be revisited
    >> > to do "sequencing" then that means I will need to revisit all my calls
    >> > to stopifnot().
    >> 
    >> >>
    >> >> I could see an argument for a change that in the multiple argumetn
    >> >> case reports _all_ that fail; that would seem more useful to me than
    >> >> twisting the code into knots.
    >> 
    >> Interesting... but really differing from the current documentation,
    >> 
    >> > Why not. Still better than the current situation. But only if that
    >> > semantic seems more useful to people. Would be sad if usefulness
    >> > of one semantic or the other was decided based on trickiness of
    >> > implementation.
    >> 
    >> Well, the trickiness  should definitely play a role.
    >> Apart from functionality and semantics, long term maintenance
    >> and code readibility, even elegance have shown to be very
    >> important aspects of good code in ca 30 years of S and R programming.
    >> 
    >> OTOH, as mentioned above, the creation of good error messages
    >> has been an important design goal of  stopifnot()  and hence I'm
    >> willing to accept the extra complexity of "patching up" the call
    >> used in the error / warning messages.
    >> 
    >> Also, as a change to what I posted yesterday, I now plan to follow
    >> Peter Dalgaard's suggestion of using
    >> eval( ..<n> )
    >> instead of   eval(cl[[i]], envir = <parent.frame(.)>)
    >> as there may be cases where the former behaves better in lazy
    >> evaluation situations.
    >> (Other opinions on that ?)

    > If you go this route it would be useful to step back and think about
    > whether there might be some useful primitives to add to make this
    > easier, such as

    > - provide a dotsLength function for computing the number arguments
    > captured in a ... argument

actually my current version did not use that anymore 
(and here we could use   nargs() )

    > - providing a dotsElt function for extracting the i-the element
    > instead of going through the eval(sprintf("..%d", i)) construct.

I've started to do that, ...

    > - maybe something for extracting the expression for the i-th argument.

well, yes, but that seems quite readable here, we use the whole
call anyway and loop, looking at each sequentially --- that part has not
been new!

    > The might be more generally useful and make the code more readable and
    > maintainable.

    > Best,

    > luke

    >> 
    >> Martin
    >> 
    >> > Thanks,
    >> > H.
    >> 
    >> >>
    >> >> Best,
    >> >>
    >> >> luke
    >> >>
    >> >> On Mon, 15 May 2017, Martin Maechler wrote:
    >> >>
    >> >>>>>>>> Serguei Sokol <sokol at insa-toulouse.fr>
    >> >>>>>>>> on Mon, 15 May 2017 16:32:20 +0200 writes:
    >> >>>
    >> >>> > Le 15/05/2017 ? 15:37, Martin Maechler a ?crit :
    >> >>> >>>>>>> Serguei Sokol <sokol at insa-toulouse.fr>
    >> >>> >>>>>>> on Mon, 15 May 2017 13:14:34 +0200 writes:
    >> >>> >> > I see in the archives that the attachment cannot pass.
    >> >>> >> > So, here is the code:
    >> >>> >>
    >> >>> >> [....... MM: I needed to reformat etc to match closely to
    >> >>> >> the current source code which is in
    >> >>> >>
    >> >>> https://urldefense.proofpoint.com/v2/url?u=https-3A__svn.r-2Dproject.org_R_trunk_src_library_base_R_stop.R&d=DwIFAw&c=eRAMFD45gAfqt84VtBcfhQ&r=BK7q3XeAvimeWdGbWY_wJYbW0WYiZvSXAJJKaaPhzWA&m=t9fJDOl9YG2zB-GF0wQXrXJTsW2jxTxMHE-qZfLGzHU&s=KGsvpXrXpHCFTdbLM9ci3sBNO9C3ocsgEqHMvZKvV9I&e=
    >> >>> >> or its corresponding github mirror
    >> >>> >>
    >> >>> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_wch_r-2Dsource_blob_trunk_src_library_base_R_stop.R&d=DwIFAw&c=eRAMFD45gAfqt84VtBcfhQ&r=BK7q3XeAvimeWdGbWY_wJYbW0WYiZvSXAJJKaaPhzWA&m=t9fJDOl9YG2zB-GF0wQXrXJTsW2jxTxMHE-qZfLGzHU&s=7Z5bPVWdGPpY2KLnXQP6c-_8s86CpKe0ZYkCfqjfxY0&e=
    >> >>> >> ]
    >> >>> >>
    >> >>> >> > Best,
    >> >>> >> > Serguei.
    >> >>> >>
    >> >>> >> Yes, something like that seems even simpler than Peter's
    >> >>> >> suggestion...
    >> >>> >>
    >> >>> >> It currently breaks 'make check' in the R sources,
    >> >>> >> specifically in tests/reg-tests-2.R (lines 6574 ff),
    >> >>> >> the new code now gives
    >> >>> >>
    >> >>> >> > ## error messages from (C-level) evalList
    >> >>> >> > tst <- function(y) { stopifnot(is.numeric(y)); y+ 1 }
    >> >>> >> > try(tst())
    >> >>> >> Error in eval(cl.i, pfr) : argument "y" is missing, with no default
    >> >>> >>
    >> >>> >> whereas previously it gave
    >> >>> >>
    >> >>> >> Error in stopifnot(is.numeric(y)) :
    >> >>> >> argument "y" is missing, with no default
    >> >>> >>
    >> >>> >>
    >> >>> >> But I think that change (of call stack in such an error case) is
    >> >>> >> unavoidable and not a big problem.
    >> >>>
    >> >>> > It can be avoided but at price of customizing error() and
    >> >>> warning() calls with something like:
    >> >>> > wrn <- function(w) {w$call <- cl.i; warning(w)}
    >> >>> > err <- function(e) {e$call <- cl.i; stop(e)}
    >> >>> > ...
    >> >>> > tryCatch(r <- eval(cl.i, pfr), warning=wrn, error=err)
    >> >>>
    >> >>> > Serguei.
    >> >>>
    >> >>> Well, a good idea, but the 'warning' case is more complicated
    >> >>> (and the above incorrect): I do want the warning there, but
    >> >>> _not_ return the warning, but rather, the result of eval() :
    >> >>> So this needs even more sophistication, using  withCallingHandlers(.)
    >> >>> and maybe that really get's too sophisticated and no
    >> >>> more "readable" to 99.9% of the R users ... ?
    >> >>>
    >> >>> I now do append my current version -- in case some may want to
    >> >>> comment or improve further.
    >> >>>
    >> >>> Martin
    >> 

    > -- 
    > Luke Tierney
    > Ralph E. Wareham Professor of Mathematical Sciences
    > University of Iowa                  Phone:             319-335-3386
    > Department of Statistics and        Fax:               319-335-3017
    > Actuarial Science
    > 241 Schaeffer Hall                  email:   luke-tierney at uiowa.edu
    > Iowa City, IA 52242                 WWW:  http://www.stat.uiowa.edu



[Rd] stopifnot() does not stop at first non-TRUE argument
Martin Maechler