clone classes with ReferenceClass members

[jkamins7]

Example:

library(R6)
c1 = setRefClass('Person')
c2 = R6Class(
  cloneable=TRUE,
  public = list(
    member = c1$new()
  )
)
c2$new()$member
c2$new()$clone(TRUE)

Seems like this could be potentially solved by replacing the check for .__enclose_env with a check for inheritance. For example, if we replace lines 90 through 96 of clone.R with

  deep_clone <- function(name, value) {
    # Check if it's an R6 object.
    is.R6 <- function(x) {
      inherits(x, "R6")
    }
    if (is.environment(value) && is.R6(value)) {
      return(value$clone(deep = TRUE))
    }
    value
  }

it at least fixes this issue. I'm not sure if there is a reasonable way of gaining access to is.R6 inside that function, or whether there is another issue that would arise.

[wch]

Can you please describe the problem (in addition to the example)?

[jkamins7]

Sorry, I forgot to include the error. When I run the last line of the example, I get the following error

Error in envRefInferField(x, what, getClass(class(x)), selfEnv) : 
  ‘.__enclos_env__’ is not a valid field or method name for reference class “Person”

[wch]

There's no need for an is.R6 function -- it could just call inherits(x, "R6") there. But then there's another problem: in some cases, R6 objects don't have the R6 class attribute, when the class is created with R6Class(class=FALSE).

Another way is to check if the object is a Reference Class object. As far as I can tell, the way to do it is to use is(x, "refClass"). Unfortunately, is() is slow compared to inherits(). See the benchmark here:

library(R6)
c1 <- setRefClass('Person')
c2 <- R6Class(
  cloneable = TRUE,
  public = list(
    member = c1$new()
  )
)

library(microbenchmark)
person <- c2$new()    # R6 object
rc <- person$member   # Ref class object
e <- environment()    # Plain environment

microbenchmark(
  is(person, "refClass"),
  is(rc, "refClass"),
  is(e, "refClass"),
  inherits(person, "R6"),
  inherits(rc, "R6"),
  inherits(e, "R6")
)
#> Unit: nanoseconds
#>                    expr   min      lq     mean  median      uq   max neval
#>  is(person, "refClass")  8494  8902.5  9585.47  9309.5  9757.0 33708   100
#>      is(rc, "refClass")  5267  5517.0  5885.22  5846.5  6030.0 11566   100
#>       is(e, "refClass") 11393 12009.5 12696.72 12319.5 12583.0 47550   100
#>  inherits(person, "R6")   563   633.0   731.01   690.0   764.5  2494   100
#>      inherits(rc, "R6")   675   780.5   866.08   842.0   908.5  2059   100
#>       inherits(e, "R6")   610   688.0   860.22   744.0   801.0 12130   100

It's on the order of microseconds, which isn't horrible, but it would still be nice to find a faster way of checking if something is a reference class object. Maybe something like isS4(x) && is(x, "refClass") -- in the common case (not a reference class object), that looks to be quite fast.

microbenchmark(
  is(person, "refClass"),
  is(rc, "refClass"),
  is(e, "refClass"),
  isS4(person),
  isS4(rc),
  isS4(e),
  inherits(person, "R6"),
  inherits(rc, "R6"),
  inherits(e, "R6")
)
#> Unit: nanoseconds
#>                    expr   min      lq     mean  median      uq   max neval
#>  is(person, "refClass")  8577  9218.5  9860.36  9574.0  9944.0 35400   100
#>      is(rc, "refClass")  5238  5611.0  5849.47  5850.5  6035.5  8604   100
#>       is(e, "refClass") 11458 11948.0 12783.43 12267.5 12774.0 53596   100
#>            isS4(person)   108   132.0   160.73   151.5   183.0   431   100
#>                isS4(rc)   102   131.5   163.61   157.0   188.5   308   100
#>                 isS4(e)    98   126.0   173.15   154.0   183.0  1540   100
#>  inherits(person, "R6")   573   634.5   724.34   691.0   745.5  2474   100
#>      inherits(rc, "R6")   684   791.0  1017.30   857.0   913.0 16521   100
#>       inherits(e, "R6")   606   719.0   777.43   769.0   832.5  1097   100

[jkamins7]

Wouldn't it be consistent to treat R6 classes with class=FALSE as environments in this case? At least according to the documentation, if you call R6Class with that option, the return is just an environment. If not, seems like it would make sense to have is.R6 return TRUE for them as well.

Also, if we do decide to treat go this route, we don't have to test isS4, because this code will only run on environments (is.environment is faster than isS4 anyway), and S4 classes aren't environments.

As for testing if something is a reference class, something like

!is.null(c1i$.RefClassDef)

would do it, but would be unstable in a similar way to checking .__enclose_env

I've done some benchmarking but nothing satisfying.

[wch]

Hm, maybe you're right that if class=FALSE, the resulting objects should just be treated as environments. So maybe this would be sufficient:

if (inherits(x, "R6")) {
  ...

On the other hand, it's possible that making this change will break existing code for users who rely on the current behavior.

Regarding using isS4(x) && is(x, "refClass"), I just meant that would be one part of a larger conditional, like

if (is.environment(value) &&
    !(isS4(x) && is(x, "refClass")) &&
    !is.null(value$`.__enclos_env__`) {