-
Notifications
You must be signed in to change notification settings - Fork 34
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
Method call frame #56
Comments
What would be the concrete benefits? There are some potential issues, for example, a non-trivial/standard generic might define some variables that would mysteriously show up in the method frame. |
I don't think we need to use the same call frame; we're just likely to need some tricks to pass along unevaluated promises in such a way that |
Just to make this a bit more concrete, I think that this is the main challenge: foo <- function(x) {
UseMethod("foo")
}
foo.numeric <- function(x) {
substitute(x)
}
x <- 1:10
foo(x)
#> x Created on 2020-11-18 by the reprex package (v0.3.0.9001) Because this feature is used by important base functions like |
Agreed, that's an issue. In Bioconductor, we've had to rely on a hack to follow the promise chain in order to get the right frame for deferred evaluation, such as with |
@lawremi we've mostly moved to explicitly reifying promises into quosures (expr + env) and requiring special syntax to pass along to subcalls. I don't think that's a good fit with base R, but I'm sure we can come up with something. |
I also agree that unevaluated promises are an issue, but I am still not entirely convinced that we can live with disagreeing call frames. I feel as though there may be specialized use cases that require the user to directly call print_quick_summary <- function(fit) {
print(mean(coef(fit)$Subject$Days))
}
model <- function(data) {
data$Days <- -data$Days
print_quick_summary(lmer(Reaction ~ Days + (Days | Subject), data = data))
UseMethod("model")
}
model.data.frame <- function(data) {
print_quick_summary(lmer(Reaction ~ Days + (Days | Subject), data = data))
}
library(lme4)
#> Loading required package: Matrix
library(tibble)
data <- new_tibble(sleepstudy, nrow = nrow(sleepstudy))
model(sleepstudy)
#> [1] -10.46729
#> [1] 10.46729 Created on 2020-11-18 by the reprex package (v0.3.0) |
But that's a good point, especially if the user does not define the generic. |
I believe you could use |
True. How much do we care about call frame agreement among child methods? Seems like those are more likely to be user-defined. foo <- function(x) {
print(parent.frame(2))
print(environment())
UseMethod("foo")
}
foo.x <- function(x) {
print(parent.frame(2))
print(environment())
NextMethod()
}
foo.y <- function(x) {
print(parent.frame(2))
print(environment())
}
x <- structure(list(), class = c("x", "y"))
foo(x)
#> <environment: R_GlobalEnv>
#> <environment: 0x7fc7bc3b3b70>
#> <environment: R_GlobalEnv>
#> <environment: 0x7fc7bc3b7f98>
#> <environment: R_GlobalEnv>
#> <environment: 0x7fc7bc3be200> |
Except in the case of calls to the "next" method. |
Yes, child methods can get the frame of the generic but not the frames of the parents. |
@lawremi ah true. In that case, though I'd probably suggest it's time to make the environment an explicit argument to the generic. |
The dispatch document proposes a possible alternative to
UseMethod()
that acts like an ordinary function call. What would it take to implement this without call frame issues? cc @lionel-.I understand that maybe we want parent frames to agree if we want
deparse(substitute(arg))
to work (or if the user just wants to get the parent frame from inside a method). Seems like we could somehow make the actual call frames agree as well. Current S3 behavior:Goal for R7?
I think these same considerations apply to
NextMethod()
and its possible alternatives.The text was updated successfully, but these errors were encountered: