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

super() while implementing a method on a class union #493

Open
mjskay opened this issue Nov 12, 2024 · 5 comments
Open

super() while implementing a method on a class union #493

mjskay opened this issue Nov 12, 2024 · 5 comments

Comments

@mjskay
Copy link

mjskay commented Nov 12, 2024

I'm not sure how to use super() when implementing a method for a class union. e.g.:

A = new_class("A")
A_Child = new_class("A_Child", parent = A)
B = new_class("B")
B_Child = new_class("B_Child", parent = B)

f = new_generic("f", "x")
method(f, A) = \(x) cat("A\n")
method(f, B) = \(x) cat("B\n")
method(f, A_Child | B_Child) = \(x) {
  cat("child\n")
  # do some stuff, then...
  # what to put here?
  f(super(x, ???))
}

I think I would have to implement f() twice, once for A_child and once for B_child (which feels like it defeats the purpose of class unions), or use a condition to pick the second argument to super() based on the class of x (which feels clumsy).

Is there a more elegant existing solution? If not, perhaps syntax like f(super(x, A | B))? To maintain the explicitness that motivated super() it could require there to be exactly one class in the intersection between the to argument to super() and the ancestors of x.

(P.S. thanks for all of this, it is very fun to play with!)

@t-kalinowski
Copy link
Member

Currently, super() requires that the dispatched method is known when you write it. This is an intentional design decision, as the dynamism of NextMethod() often makes larger codebases hard to reason about. (If, with time, this restriction proves too onerous, it could potentially be relaxed to allow a union.)

However, you can still achieve what you want by calling method() and/or convert() to perform dispatch "manually":

# Approach 1: using `method()`
method(f, A_Child | B_Child) = \(x) {
  parent_method <- method(f, S7_class(x)@parent) # Get method for A or B
  parent_method(x)
}

# Approach 2: using `convert()`
method(f, A_Child | B_Child) = \(x) {
  # parent_instance is an A or B
  parent_instance <- convert(x, S7_class(x)@parent)
  f(parent_instance) # Dynamically dispatch to method for A or B
  
  ## Return f(parent_instance), or
  ## maybe update x with f() parent method updated props
  #   props(x) <- props(f(parent_instance)) 
}

@lawremi
Copy link
Collaborator

lawremi commented Nov 16, 2024

Note that defining a method on a union is just a shortcut for defining a method for each class. Another way to solve this would be to define a separate method for each and then factor out the common part into an ordinary function. That feels a bit more future proof to me, because super() is introducing some heavy coupling, and then you're doing that across multiple classes.

@mjskay
Copy link
Author

mjskay commented Nov 17, 2024

@t-kalinowski thanks for the help --- However, I'm not sure if using S7_class(x)@parent solves the general problem. If A_Child itself had a child class A_child_child, then wouldn't S7_class(x)@parent return A_child and cause me to re-call the implementation of f that I am already in?

@lawremi's comment about heavy coupling echoes some of the feelings I am having while using super() to implement some classes that make extensive use of binary operators (think: set algebra). It feels like I am introducing a lot of heavy coupling. And if I imagine other folks building on my codebase, it becomes worse. Say I have a class c("A3", "A2", "A1") and someone in another package implements a method on A2, if I later change its inheritance structure to c("A3", "A2", "A1.5", "A1"), now their calls to super(x, A1) skip A1.5 (also, I would have to do an audit through my codebase for all existing calls to super(..., A1) to change them to super(..., A1.5).

FWIW, while I have been implementing binary operators, the two forms of super() that have been helpful to me are "dispatch on the same class again" or "dispatch on the next class up". To make this more concrete, imagine a generic binary operator op where I implement a method like:

method(op, list(A2, A2)) = \(x, y) {
  # do some stuff, and then...
  op(super(x, A2), super(y, A1))
}

To me, super(x, A2) does not introduce heavy coupling because I know I am in an implementation of a method for A2, but super(y, A1) does because its correctness relies on the class hierarchy of y, which a downstream implementor should not have to know. I think something closer to the semantics of "dispatch on the next class up from class X", maybe something like super(y, A2, -1) instead of super(y, A1), would induce less coupling while still being explicit enough to solve the multiple-dispatch ambiguities inherent to callNextMethod() in S4. With a union this would also still be explicit, given semantics like "dispatch on the next class up from class X or Y", producing an error if both X and Y are ancestors to the class.

@mjskay
Copy link
Author

mjskay commented Nov 17, 2024

Just realized in the non-union case what I think I'm looking for is super(x, A2@parent)!

In the case of unions it seems to me this would still be useful to have. However, at least this gives a path forward for me in the short term, as I should be able to write a simple wrapper around super() to do what I need in the union case.

@mjskay
Copy link
Author

mjskay commented Nov 17, 2024

In case it's helpful, this solution seems to work for my cases (doesn't handle non-S7 classes but that's not a problem for my use case, and could be fixed):

#' Get the single class in `to` (if it is a union) that is in the
#' class hierarchy of `from`
#' @noRd
as_single_ancestor = \(to, from) {
  if (inherits(to, "S7_union")) {
    is_ancestor = vapply(to$classes, S7_inherits, NA, x = from)
    if (sum(is_ancestor) != 1) {
      stop("Cannot dispatch on a class union unless object inherits from exactly one class in the union.")
    }
    to$classes[[which(is_ancestor)]]
  } else {
    to
  }
}

#' Alternative to super() that supports unions
#' @param from object to dispatch on
#' @param to class or class union to use for dispatch --- if it is a union,
#' then there must be exactly one class in the union in the class hierarchy
#' of `from`
#' @noRd
dispatch = \(from, to) {
  super(from, as_single_ancestor(to, from))
}

#' Alternative to super() that looks one class up
#' @param from object to dispatch on
#' @param to class or class union to use for dispatch --- if it is a union,
#' then there must be exactly one class in the union in the class hierarchy
#' of `from`. Dispatches on the parent to that class.
#' @noRd
parent = \(from, to) {
  super(from, as_single_ancestor(to, from)@parent)
}

Now for "dispatch on this specific class" I can use dispatch(x, class) and for "dispatch on the parent" I can use parent(x, class) where class may be a union.

(Incidentally, splitting this into two different operations made me wonder if super() is slightly misnamed, since its name is closer to what I have as parent() but what it actually does is set the dispatch to the provided class).

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

3 participants