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

Inherited class should be filled at first object creation, not class creation #12

Closed
wch opened this issue Aug 5, 2014 · 15 comments

Comments

@wch
Copy link
Member

commented Aug 5, 2014

If an R6 class (e.g., in pkgB) is created in a package with an inherit class (from pkgA), then that inherited superclass will be filled with the package is built. This means that if the user updates pkgA, then the R6 class in pkgB won't get the updated version of the superclass -- it will retain the original version that was present at package build time.

@gaborcsardi

This comment has been minimized.

Copy link
Member

commented Aug 5, 2014

Isn't it enough to have a constructor call that is kept as an unevaluated expression? It would go in the built package un-evaluated, and would be evaluated only (and every time) when an object is created with class$new()

I think this is possible.

@wch

This comment has been minimized.

Copy link
Member Author

commented Aug 5, 2014

I think that is possible... I like to avoid using special evaluation, but it might be acceptable in this case.

@gaborcsardi

This comment has been minimized.

Copy link
Member

commented Aug 5, 2014

I am not sure if you need special evaluation for this. What if R6Class returns a function? Then in pkgA this function goes into the package at build time, unevaluated, of course. And you could just evaluate it when a new object is created, either from class AC, or class BC in pkgB.

Even the syntax could stay the same, by adding a class to the function, and defining print and $ for it, you could still say AC$new().

But fixme if I am wrong, I am not totally convinced myself.

@gaborcsardi

This comment has been minimized.

Copy link
Member

commented Aug 5, 2014

Well, I guess this is probably stupid, because you want to keep R6 classes and objects as environments. Sorry for the noise.

@gaborcsardi

This comment has been minimized.

Copy link
Member

commented Aug 5, 2014

Actually, maybe this is not completely stupid. See e.g. a toy implementation here:
https://gist.github.com/gaborcsardi/2cae849128964044df65

It does not actually have any kind of inheritance, maybe not even possible to add it. But the point is that objects that are functions with enclosed environments still have reference semantics.

@wch

This comment has been minimized.

Copy link
Member Author

commented Aug 6, 2014

Yeah, defining a $ method lets you do a lot of tricky things.

If R6Class() returns a function, isn't there still the same problem? When pkgB is built, BC would get a copy of this function stored inside of it, and updating pkgA::AC wouldn't change what's stored in BC

@gaborcsardi

This comment has been minimized.

Copy link
Member

commented Aug 6, 2014

I think the trick is that BC does not evaluate inherit until an object it created, it is kept as a promise. When you create an object from class BC, then it evaluates inherit, and the new object gets the data from the most recent pkgA::AC.

This is simple to do if the class is a function object instead of an environment, because function argument are kept as promises by default. But it might be possible to keep the class as an environment and use delayedAssign. Then you don't have to redefine $, which is a big plus I guess. (But actually the real solution would be to fix the slow $ in R, not hacking around it.)

Still haven't tried this, and there are many places where it could go wrong. I put my experiments here: https://github.com/gaborcsardi/closure (Sorry, it seemed easier than hacking into R6.) Planning to add inheritance later today.

@gaborcsardi

This comment has been minimized.

Copy link
Member

commented Aug 6, 2014

Just a note. It seems that a promise can be saved to a file, so it should also be kept as a promise in installed packages. This means that you can put inherit into an environment as a promise, and then evaluate it when an object is created from the class. E.g.

env <- new.env()
f <- function(x, env) {
  delayedAssign("x", x, assign.env = env)
}
what <- "foo"
f(what, env)
ls(env)
# [1] "x"
save(env, file="/tmp/env.rda")
rm(list=ls())
load("/tmp/env.rda")
ls()
# [1] "env"
what <- "bar"
env$x
# [1] "bar"

I am not sure how exactly this is done, maybe the evaluation environment of the promise is saved, too.

@wch

This comment has been minimized.

Copy link
Member Author

commented Aug 6, 2014

Interesting, I didn't know that.

However, I still have reservations about saving a promise, because it's kind of fragile. It seems more robust to get the unevaluated expression (using substitute()) and the calling environment, and then do the evaluation each time an object is instantiated.

Here's an example of the fragility. Suppose that in pkgB, we create classB which inherits from pkgA::classA, and also inside the package, we instantiate the class with classB$new(). The promise gets evaluated, and returns the classA object. Then pkgB is built, and the classA generator object gets saved inside of classB.

To avoid this problem, I think that the classB generator must not keep any direct references to the classA object. (A string like "pkgA::classA" is not a direct reference; an unevaluated promise is not a direct reference; but a regular variable that resolves to the object is a direct reference.)

Reference classes register themselves by name in some sort of lookup table, and I'd guess that there's a similar reason behind it.

@gaborcsardi

This comment has been minimized.

Copy link
Member

commented Aug 6, 2014

However, I still have reservations about saving a promise, because it's kind of fragile. It seems more robust to get the unevaluated expression (using substitute()) and the calling environment, and then do the evaluation each time an object is instantiated.

Yes, I think this is essentially the same as R6Class() returning functions that are called when they are instantiate objects. Except that you can keep everything as an environments, which is good.

Here's an example of the fragility. Suppose that in pkgB, we create classB which inherits from pkgA::classA, and also inside the package, we instantiate the class with classB$new(). The promise gets evaluated, and returns the classA object. Then pkgB is built, and the classA generator object gets saved inside of classB.

Hmmm, yes, that might be a problem with delayedAssign() I guess.

To avoid this problem, I think that the classB generator must not keep any direct references to the classA object. (A string like "pkgA::classA" is not a direct reference; an unevaluated promise is not a direct reference; but a regular variable that resolves to the object is a direct reference.)

I don't really see the big difference between "pkgA::classA" and expression(pkgA::classA) and substitute(pkgA::classA).

I personally don't like "pkg::classA", because I think that writing R code in strings is just not right. But that might be just me.

@gaborcsardi

This comment has been minimized.

Copy link
Member

commented Aug 6, 2014

Btw. even writing AC <- R6Class("AC",... seems wrong to me. Why does one need the string? You don't have to "name" any object in R, why do you need to name a class? A class should be just like any other object.

EDIT: I mean, I am not asking you in particular, just asking a philosophical question....:)

@wch

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2014

Regarding AC <- R6Class("AC",..., it's needed for the class attribute for S3 method dispatch.

The really correct way to capture the expression unevaluated would be something like inherit=quote(pkgA::classA), but that's really clunky.

@gaborcsardi

This comment has been minimized.

Copy link
Member

commented Aug 7, 2014

Ah, yes, forgot the S3 dispatch. Although you might say that the user should add that by hand if she wants, and only add an R6ClassGenerator class. The obj$print() method (or a default print method) could still be used for printing.

I guess my frustration is that R has now 4+ class/object systems and all of them were introduced without changing the syntax of the language. Which sounds cool, but in reality it means that all of the class systems suck. R6 sucks the least so far. :)

@wch

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2014

Thanks. :) You can pretend that S4 and reference classes don't exist, so then there's only two class systems.

The generator only needs the "R6ClassGenerator" class. The instances in your example have "AC" and "R6" as their classes. I think S3 is useful enough that it makes sense to keep that as the default.

@gaborcsardi

This comment has been minimized.

Copy link
Member

commented Aug 7, 2014

Yes, I meant R6 class for objects, sorry.

Btw. I like it that you can say A <- R6Class() and then it does not have a name. It would be cool if you would not need the name for inheritance, either, and you could just inherit form an R6ClassGenerator object.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.