Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make clone() modifiable to resolve circular dependecies #110

Closed
kajomano opened this issue Jan 4, 2017 · 10 comments
Closed

Make clone() modifiable to resolve circular dependecies #110

kajomano opened this issue Jan 4, 2017 · 10 comments

Comments

@kajomano
Copy link

kajomano commented Jan 4, 2017

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
Copy link
Member

wch commented Jan 4, 2017

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
Copy link
Author

kajomano commented Jan 4, 2017

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
Copy link
Member

wch commented Jan 4, 2017

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 kajomano changed the title Make clone() modifiable Make clone() modifiable to resolve circular dependecies Jan 5, 2017
@kajomano
Copy link
Author

kajomano commented Jan 5, 2017

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
Copy link
Member

wch commented Jan 5, 2017

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
Copy link
Author

kajomano commented Jan 6, 2017

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
Copy link
Member

wch commented Jan 6, 2017

Sounds good!

@wch wch closed this as completed Jan 6, 2017
@mb706
Copy link

mb706 commented Feb 6, 2019

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
Copy link

dfalbel commented Feb 6, 2019

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

@mb706
Copy link

mb706 commented Feb 6, 2019

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants