r-lib / R6

Encapsulated object-oriented programming for R
https://R6.r-lib.org
Other
405 stars 56 forks source link

Make clone() modifiable to resolve circular dependecies #110

Closed kajomano closed 7 years ago

kajomano commented 7 years ago

Hello!

I ran into a circular dependency with my nested R6 classes when trying to use clone(deep = TRUE) on one of them. I tried to overwrite the clone method to something like this:

clone = function( deep = FALSE ){
  # Resolve/remove problematic pointer
  super$clone( deep = deep )
}

But sadly the clone() method is reserved. Now I understand that the super$clone() part would also not work, but would you consider making the clone() function modifiable so that by default it would call:

clone = function( deep = FALSE ){
  self$.clone( deep = deep )
}

where .clone() is the actually reserved method? If this is not possible, maybe you could make deep_clone() to also run on fields that contain R6 classes if needed?

R6 is an awesome package, thank you for your work!

wch commented 7 years ago

Hm, can you provide a reproducible example that illustrates what you're trying to do? It's not entirely clear to me from just the description.

kajomano commented 7 years ago
library(R6)

child <- R6Class(
  "child",
  public = list(
    parent = NULL,
    initialize = function( parent ){
      self$parent <- parent
    }
  )
)

parent <- R6Class(
  "parent",
  public = list(
    child = NULL, 
    initialize = function(){
      self$child <- child$new( self )
    }
  )
)

dad <- parent$new()
dad$clone( deep = TRUE )

Now this obviously dies from the infinitely recursive reference, but if I could modify the parent's clone():

parent <- R6Class(
  "parent",
  public = list(
    child = NULL, 
    initialize = function(){
      self$child <- child$new( self )
    },
    clone = function( deep = FALSE ){
      if( deep ){
        # Break the circular reference before cloning
        self$child$parent <- NULL
      }

      # Call the reserved function
      cloned <- private$.clone( deep = deep )

      if( deep ){
        # Restore the correct reference in child in both places
        self$child$parent <- self
        cloned$child$parent <- cloned
      }

      return( cloned )
    }
  )
)

This would solve my problem.

wch commented 7 years ago

I looked into implementing a deep_clone method to do what you want, but it still doesn't do the trick:

library(R6)

child <- R6Class(
  "child",
  public = list(
    parent = NULL,
    initialize = function( parent ){
      self$parent <- parent
    }
  )
)

parent <- R6Class(
  "parent",
  public = list(
    child = NULL, 
    initialize = function(){
      self$child <- child$new( self )
    }
  ),
  private = list(
    deep_clone = function(name, value) {
      if (name == "child") {
        new_child <- value$clone(deep = FALSE)
        new_child$parent <- self
        new_child

      } else {
        value
      }
    }
  )
)

dad <- parent$new()
dad2 <- dad$clone(deep = TRUE)

# The dad2$child$parent points to dad, but should point to dad2
identical(dad2$child$parent, dad2)  # FALSE
identical(dad2$child$parent, dad)   # TRUE

The problem is that, when the deep_clone method is invoked, self points to the original object, not the new one. It may be possible to add a new_self that's available in the deep_clone which points to the new object, but that raises new problems. The fields in new_self will not all be populated at the time that deep_clone is called, so it'll only be a partially-complete object, which may not work at all.

kajomano commented 7 years ago

I currently have this workaround:

library(R6)

child <- R6Class(
  "child",
  public = list(
    parent = NULL,
    initialize = function( parent ){
      self$parent <- parent
    },
    copy = function( deep = FALSE ){
      if( deep ){
        parent <- self$parent
        self$parent <- NULL
      }

      cloned <- self$clone( deep )

      if( deep ){
        self$parent <- parent
      }

      cloned
    }
  )
)

parent <- R6Class(
  "parent",
  public = list(
    child = NULL,
    initialize = function(){
      self$child <- child$new( self )
    },
    copy = function( deep = FALSE ){
      cloned <- self$clone( deep )

      if( deep ){
        cloned$child$parent <- cloned
      }

      cloned
    }
  ),
  private = list(
    deep_clone = function(name, value) {
      if (name == "child") {
        value$copy( TRUE )
      } else {
        value
      }
    }
  )
)

dad <- parent$new()
dad2 <- dad$copy(deep = TRUE)

# This works correctly now
identical(dad2$child$parent, dad2)  # TRUE
identical(dad2$child$parent, dad)   # FALSE

In this case you would have to call copy() instead of clone() for the objects, this is why I would rather have clone() modifiable.

wch commented 7 years ago

You can simplify it so that the child class doesn't have a copy method (unless there's some reason you want to call copy on the child objects directly):

library(R6)

child <- R6Class(
  "child",
  public = list(
    parent = NULL,
    initialize = function( parent ){
      self$parent <- parent
    }
  )
)

parent <- R6Class(
  "parent",
  public = list(
    child = NULL,
    initialize = function(){
      self$child <- child$new( self )
    },
    copy = function( deep = FALSE ){
      cloned <- self$clone( deep )

      if( deep ){
        cloned$child$parent <- cloned
      }

      cloned
    }
  ),
  private = list(
    deep_clone = function(name, value) {
      if (name == "child") {
        value$clone(deep = FALSE)
      } else {
        value
      }
    }
  )
)

dad <- parent$new()
dad2 <- dad$copy(deep = TRUE)

# This works correctly now
identical(dad2$child$parent, dad2)  # TRUE
identical(dad2$child$parent, dad)   # FALSE

FWIW, I'm not likely to add a .clone method, and change clone so that it calls .clone.

kajomano commented 7 years ago

In the actual code I have multiple R6 classes within the child also, so I need to call clone( deep = TRUE ) there, that is why I defined the copy() function for the child.

I guess I will have to stick with the workaround then. Anyway, thank you for your help!

wch commented 7 years ago

Sounds good!

mb706 commented 5 years ago

It happened to me multiple times already that I had a few objects that reference each other and need a method to create deep copies. It would be really convenient if this could somehow be implemented where calling the clone method works, because it gets messy having to keep track of which objects require a $copy() and which require $clone(). It makes it harder to interact with other packages that assume R6 objects with a working $clone method, or to provide a consistent UI. A post_clone(old_self) that gets called (in the context of the new object) after the main duty of clone() was done would probably already do the trick for many cases.

dfalbel commented 5 years ago

@mb706 You can actualy modify the clone behavior if you set cloneable=FALSE. See here: https://stackoverflow.com/a/54544769/3297472

mb706 commented 5 years ago

@dfalbel thanks a lot, this was helpful, I wasn't thinking of the possibility of using set (and setting clone in the R6Class call gave me an error). It will probably work for me, although I still think there should be a more supported/documented way to do this.