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

Can we eliminate next_method()? #110

Closed
hadley opened this issue Nov 8, 2021 · 10 comments · Fixed by #181
Closed

Can we eliminate next_method()? #110

hadley opened this issue Nov 8, 2021 · 10 comments · Fixed by #181

Comments

@hadley
Copy link
Member

hadley commented Nov 8, 2021

(Placeholder issue based on discussion in meeting today).

Or at least reduce the dynamism as much as possible? How does Julia get around it?

@hadley
Copy link
Member Author

hadley commented Nov 13, 2021

Maybe we could make method_call(class) call the method for the specified class?

# S3 method
`[.my_class` <- function(x, i) {
  new_my_class(NextMethod(), x = attr(x, "x"), y = attr(x, "y"))
}

# Equivalent R7 method
method(`[`, my_class) <- function(x, i) {
  new_my_class(method_call(R7_object), x = x@x, y = x@y)
}

(Would need to think about the syntax for multi-dispatch methods; maybe using a named list? Can you supply just one of the arguments and leave the others to be looked up using the usual rules?)

This has the advantage of being crystal clear about which method you want to call, and I think adds relatively little work since when you write NextMethod() you presumably already have some idea of what method you want it to call.

Would need to verify that this also solves #119. I think it might just work because this already works:

library(R7)
foo <- new_generic("foo", signature = "x")
method(foo, "character") <- function(x, y, ...) {
  rlang::enquo(y)
}
foo("z", y = x + 1)
#> <quosure>
#> expr: ^x + 1
#> env:  global

Created on 2021-11-14 by the reprex package (v2.0.1)

(presumably because we're somehow short circuiting a layer of promise creation that's normally a bad idea, but makes sense here)

@lawremi
Copy link
Collaborator

lawremi commented Nov 25, 2021

I think what Jan (or a compiler) would like is to know at the call site which method is being called, so that would be both the generic and the signature. Something like Class::method() in C++ but of course we have to handle multiple dispatch.

Maybe if we had some syntax for up-casting? For example, in S4 one can up-cast (strip off the subclass parts) with new("SuperClass", instance_of_subclass). What would be the R7 equivalent? f we are going to come up with a formal construction syntax anyway, perhaps it could support this type of casting, as well?

@hadley
Copy link
Member Author

hadley commented Nov 25, 2021

@lawremi my concern with replacing NextMethod() with upcasting + re-calling the generic is that it's still pretty painful since you have to explicitly pass along all of the extra arguments:

method(`[`, my_class) <- function(x, i, ..., a = 1, b = 2, c = 3) {
  new_my_class(method_call(R7_object), x = x@x, y = x@y)
}

# vs
method(`[`, my_class) <- function(x, i, ..., a = 1, b = 2, c = 3) {
  parent <- upcast(x, R7_object)
  new_my_class(parent[x, i, ..., a = a, b = b, c = c], x = x@x, y = x@y)
}

(the example doesn't quite work here with [, but hopefully you get the idea)

In my experience, remembering to pass along all the additional arguments is a common source of error. That said, in my approach, it's not clear how you avoiding passing along arguments that only exist on this method, not the parent method, or the generic. Maybe method_call() would only pass along arguments that are part of the formals of the generic? But this also depends on #85, and stuff that's currently in my head and not explained well or implemented.

@lawremi
Copy link
Collaborator

lawremi commented Dec 13, 2021

S4 has a utility called callGeneric() that conveniently recalls the generic using the current values of the arguments (so you could convert x to some other object and recall). While that means less typing and a bit less code to maintain, and particularly that it becomes easier to handle argument missingness, we also found that it becomes more difficult to read (which generic are we in again?) and simple text searches no longer reveal all of the call sites. And then there's the consistency of the formal arguments issue that you mention. So there's some benefit to being explicit and using the full call syntax. Other OOP languages expect arguments to be passed explicitly, so it's one less thing to teach. Implicit argument passing definitely looks strange to someone used to more conventional programming languages.

@hadley
Copy link
Member Author

hadley commented Dec 13, 2021

Yeah, that's a good point. Maybe a better solution would be an RStudio add-in (or similar) that could auto-generate the call for you? (And maybe mark it in some way that it could be later checked for consistency so that if you add a new argument to the method it's harder to forget to update the next method call?)

@hadley
Copy link
Member Author

hadley commented Dec 13, 2021

For the purposes of discussion, I think these are the four options we've described above:

library(R7)

foo <- new_class("foo", "character", properties = c(x = "numeric", y = "numeric"))

# like S3: 100% magical
method("[", "foo") <- function(x, i, ...) {
  result <- next_method()
  foo(result, x = x@x, y = y@y)
}

# Reduce magic by making superclass explicit
method("[", "foo") <- function(x, i, ...) {
  resut <- next_method("character")
  foo(result, x = x@x, y = y@y)
}

# like S4: up cast and then use helper to re-call generic
method("[", "foo") <- function(x, i, ...) {
  result <- call_generic(up_cast(x, "character"))
  foo(result, x = x@x, y = y@y)
}

# 100% explicit: up cast and re-call generic
method("[", "foo") <- function(x, i, ...) {
  result <- up_cast(x, "character")[i = i, ...]
  foo(result, x = x@x, y = y@y)
}

@hadley
Copy link
Member Author

hadley commented Dec 13, 2021

Notes from meeting:

What does up_cast() do? Would want to avoid copy. Could just return an object specifically designed for dispatch? Or could combine in one function:

# 100% explicit: up cast and re-call generic
method("[", "foo") <- function(x, i, ...) {
  result <- next_method(x, "[", "character")(i = i, ...)
  foo(result, x = x@x, y = y@y)
}

Would be useful to look at how CLOO/Dylan do next method.

Need to balance human needs vs implementation needs vs static analysis need.

What happens when class hierarchy changes? Do you want to be forced to reconsider every NextMethod() call? Maybe it's good to be forced to think about it? Maybe its too annoying?

How to handle missing arguments? How to handle tidy eval (#119)?

Would it be better to enforce lock down (sealing) of class hierarchy at some time so that next method could be analysed statically?

@hadley hadley changed the title Can we eliminate next_method() Can we eliminate next_method()? Jan 18, 2022
@hadley
Copy link
Member Author

hadley commented Jan 18, 2022

Notes to self:

Needs to happen after #137. If we don't want to create a copy, can start with something like below that is coupled with special handling in the code that goes from an object to a class string.

up_cast <- function(object, class) {
  # class must be R7.
  # class must be a super class of object's class
  
  # avoid creating a copy
  out <- list(object = object, class = class)
  attr(out, "class") <- "r7_upclass"
  out
} 

@lawremi
Copy link
Collaborator

lawremi commented Jan 20, 2022

As I mentioned on another issue, it's important that the modification to the class attribute only lasts for the first dispatch. If we want to avoid NSE, that probably means storing class on the object somehow and the generic being smart enough to start from that class when dispatching.

@hadley
Copy link
Member Author

hadley commented Feb 17, 2022

I wonder if we should make class default to the parent of the class of the object? That seems like it will be a pretty common scenario.

hadley added a commit that referenced this issue Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants