Skip to content

R 2.5.0 devel try issue in conjuntion with S4 method dispatch

9 messages · Martin Maechler, Matthias Burger, Peter Dalgaard +1 more

#
Hi,

after updating R 2.5.0 devel yesterday we today observed many new
unexpected failures in our daily package build and test system runs,
which can be traced to recent changes in the implementation in try()
(as noted in NEWS).

Investigating this new implementation I come across an issue in
conjuntion with using S4 classes and methods. try(expr) does not return an
object with attribute 'try-error' in case of method dispatch failure
in the wrapped expression which to me seems not
quite correct.

Examples to reproduce the observation:

##  using functions all is well:
f <- function(x) { print(x); ret<-try(stop("forced.")); print(ret)}

f(3)
[1] 3
Error in try(stop("forced.")) : forced.
[1] "Error in try(stop(\"forced.\")) : forced.\n"
attr(,"class")
[1] "try-error"


##  using S4 classes and methods
setClass("fooBase",
         representation("VIRTUAL",
                        width      = "numeric",
                        height     = "numeric"),
         prototype(width      = 1024,
                   height     = 1024),
         validity = NULL,
         where    = .GlobalEnv,
         sealed   = TRUE,
         )

if (!isGeneric("plotObject")) {

  setGeneric("plotObject",
             def=function(x, y, ...) {
               value <- standardGeneric("plotObject")
               return(value)
             },
             where=.GlobalEnv,
             useAsDefault=TRUE
             )
}

setClass("foo",
         representation("fooBase"),
         validity = NULL,
         where    = .GlobalEnv,
         sealed   = TRUE)


plotObject.foo <- function(x, y) {
  plot(x,y)
}

setMethod("plotObject", signature=c("foo", "numeric"), plotObject.foo,
where=.GlobalEnv)


fooObject <- new("foo")
##  should fail and return object with attribute 'try-error' set
ret <- try(plotObject(fooObject, character(1)))

Error in as.list(call)[[1]] == "doTryCatch" :
        comparison (1) is possible only for atomic and list types
Error in .class1(object) : object "ret" not found

which I belive is in contradiction to the documentation, where in
Details:

     The value of the expression if 'expr' is evaluated without error,
     but an invisible object of class '"try-error"' containing the
     error message if it fails.

This is crucial for our current implementation of check functions in
package RUnit.

Is this new behaviour 'as intended' and only the documentation has not
caught up?


Regards,

  Matthias
R version 2.5.0 Under development (unstable) (2007-03-13 r40832)
i686-pc-linux-gnu

locale:
C

attached base packages:
[1] "stats"     "graphics"  "grDevices" "datasets"  "utils"     "methods"
[7] "base"

other attached packages:
rcompletion    rcompgen
    "0.1-2"     "0.1-5"
#
ml-it-r-devel at epigenomics.com writes:
We've seen some similar issues but had not had time to track them
down.
It isn't S4 specific.  The issue seems more about anonymous
calls/functions.

   ll = list()
   ll[[1]] = function(x) stop("died")
   
   v = try(do.call(ll[[1]], list(1)), silent=TRUE)
   Error in as.list(call)[[1]] == "doTryCatch" : 
   	comparison (1) is possible only for atomic and list types
   > v
   Error: object "v" not found

I don't fully understand why the call is being computed, but the
following seems to get things going.

+ seth

--- a/src/library/base/R/New-Internal.R
+++ b/src/library/base/R/New-Internal.R
@@ -7,7 +7,8 @@ try <- function(expr, silent = FALSE) {
             ## Patch up the call to produce nicer result for testing as
             ## try(stop(...)).  This will need adjusting if the
             ## implementation of tryCatch changes.
-            if (as.list(call)[[1]] == "doTryCatch")
+            callFun <- as.list(call)[[1]]
+            if (is.name(callFun) && callFun == "doTryCatch")
                 call <- sys.call(-4)
             dcall <- deparse(call)[1]
             prefix <- paste("Error in", dcall, ": ")
#
Seth Falcon wrote:
Good catch, Seth. The code still looks a bit clunky though, and I wonder 
if this wouldn't be nicer:

if (identical(call[[1]], quote(doTryCatch))) call <- sys.call(-4)

I.e., the thing that is clearly wrong is the use of "==" on something 
that is not necessarily a simple name, but the use of as.list seems 
unnecessary and comparisons between strings and names is a bit awkward too.
#

        
PD> Seth Falcon wrote:
>> ml-it-r-devel at epigenomics.com writes:
    >> 
    >>> Investigating this new implementation I come across an issue in
    >>> conjuntion with using S4 classes and methods. try(expr) does not return an
    >>> object with attribute 'try-error' in case of method dispatch failure
    >>> in the wrapped expression which to me seems not
    >>> quite correct.
    >>> 
    >> 
    >> We've seen some similar issues but had not had time to track them
    >> down.
    >> 
    >> 
    >>> Examples to reproduce the observation:
    >>> 
    >> 
    >> It isn't S4 specific.  The issue seems more about anonymous
    >> calls/functions.
    >> 
    >> ll = list()
    >> ll[[1]] = function(x) stop("died")
    >> 
    >> v = try(do.call(ll[[1]], list(1)), silent=TRUE)
    >> Error in as.list(call)[[1]] == "doTryCatch" : 
    >> comparison (1) is possible only for atomic and list types
    >> > v
    >> Error: object "v" not found
    >> 
    >> I don't fully understand why the call is being computed, but the
    >> following seems to get things going.
    >> 
    >> + seth
    >> 
    >> --- a/src/library/base/R/New-Internal.R
    >> +++ b/src/library/base/R/New-Internal.R
    >> @@ -7,7 +7,8 @@ try <- function(expr, silent = FALSE) {
    >> ## Patch up the call to produce nicer result for testing as
    >> ## try(stop(...)).  This will need adjusting if the
    >> ## implementation of tryCatch changes.
    >> -            if (as.list(call)[[1]] == "doTryCatch")
    >> +            callFun <- as.list(call)[[1]]
    >> +            if (is.name(callFun) && callFun == "doTryCatch")
    >> call <- sys.call(-4)
    >> dcall <- deparse(call)[1]
    >> prefix <- paste("Error in", dcall, ": ")
    >> 
    >> 
    PD> Good catch, Seth. 

yes, indeed!

    PD> The code still looks a bit clunky though, and I wonder 
    PD> if this wouldn't be nicer:

    PD> if (identical(call[[1]], quote(doTryCatch))) call <- sys.call(-4)

    PD> I.e., the thing that is clearly wrong is the use of "==" on something 
    PD> that is not necessarily a simple name, but the use of as.list seems 
    PD> unnecessary and comparisons between strings and names is a bit awkward too.

Indeed, I had similar thoughts when reading the part of code
Seth was patching 
{ but would have used the old-fashioned   as.name("doTryCatch")
  instead of the modern  quote(doTryCatch)
  for the only reason that I'm probably slightly ``older fashioned''
  than Peter  ;-)  
}

Martin
#
This is off-topic, but since the discussion moved towards coding
style...  Here are some comments on S4 style.

ml-it-r-devel at epigenomics.com writes:
I think for package code, you don't want to specify the where to be
.GlobalEnv.  If you omit the where argument, the class will be defined
within the package environment which is what one usually wants.
This idiom of conditionally defining an S4 generic is wide-spread and
I suspect was required at some point in time.  However, at this point,
I don't understand why one would do this and it seems that it can only
lead to hard to catch bugs.  I think it should be strongly discouraged.

To define a method on a generic, you need to know what that generic
is.  For example, you need to know the names of the formal arguments.
With conditional definition as above, you risk attempting to define a
method on a generic you know nothing about.

If you want your own generic, define it.  If you want to set a method
on someone else's generic, say so.  For example, you can do:

   setMethod(otherPkg::theirGeneric, ...)
This code is a bit confusing to read since an S3 method for class
"foo" and S3 generic plotObject would be plotObject.foo.  Maybe not
worth worrying about.

Finally, a further subtle point about how the generic was defined in
your example code.  Especially for a standardGeneric, it is best not
to name the result before returning as this can affect when results
get copied.  Here's an illustration:

   setGeneric("frob1", function(x) {
       value <- standardGeneric("frob1")
       value
   })
   
   setGeneric("frob2", function(x) {
       standardGeneric("frob2")
   })
   
   setMethod("frob1", "integer",
             function(x) vector(mode="integer", length=x))
   
   setMethod("frob2", "integer",
             function(x) vector(mode="integer", length=x))


   ###
   
   x1 <- frob1(5L)
   > tracemem(x1)
   [1] "<0x3de8098>"
   > x1[1L] <- 5L
   tracemem[0x3de8098 -> 0x3de80d0]: 
   > 
   > x2 <- frob2(5L)
   > tracemem(x2)
   [1] "<0x3de8140>"
   > x2[1L] <- 5L

Best Wishes,

+ seth
#
Hi Seth,
I applied your patch and the issues seems to be resolved.  Now I just wait
to see if all test case failures related to this disappear.

Thank you for your kind help!

  Matthias
#
Matthias Burger wrote:
....
I have just committed my variation of Seth's patch, so please check the 
current r-devel too.
#
Peter Dalgaard <p.dalgaard at biostat.ku.dk> writes:
Thanks, Peter.  And I agree that your variation is cleaner.
3 days later
#
Peter Dalgaard wrote:
[...]
For the record:
With R 2.5.0 devel (2007-03-18 r40854)
the issue I was concerned about has been resolved.

Thanks to all of you!

Regards,

  Matthias