POC: double dispatch for ggplot_add()

[teunbrand]

This PR explores suggestions from #3818 to use double dispatch to + objects to ggplot subclasses. The ultimate goal is to fix #3986 and fix #5536.

Double dispatch is implemented for layers only at the moment, but we can expand that once we agree on the approach, which I'm still happy to change.
It currently borrows from the {vctrs} style of S3 double dispatch, though we could consider using {S7} if we're willing to take on an extra dependency.

Open questions:

• @hadley is this implementation of double dispatch appropriate?
• Should we use double dispatch for all currently supported classes? If not, which ones shouldn't use double dispatch?
• Should we go for S7 generics?

Small demo that a custom method is invoked:

devtools::load_all("~/packages/ggplot2")
#> ℹ Loading ggplot2

# Making a ggplot subclass
p <- ggplot(mpg, aes(displ, cty))
class(p) <- union("foo", class(p))

# Adding custom method for adding layers
ggplot_add.Layer.foo <- function(object, plot, object_name) {
  print("I'm adding a layer to foo")
  ggplot_add.Layer.default(object, plot, object_name)
}

# Should print text as well as the plot
p + geom_point()
#> [1] "I'm adding a layer to foo"

^{Created on 2023-11-23 with reprex v2.0.2}

[hadley]

I don't think it's a good idea to implement custom double dispatch in ggplot2, and it would instead be better to use S7 (which would require switching the ggplot2 base class to a S7, but that in theory should be easy).

[teunbrand]

Thanks for the advice, Hadley! I'll experiment around with S7 for a bit then.

[teunbrand]

Because a ggplot object as S3 class might be baked into many extension assumptions, I haven't converted it to an S7 class (yet).
The double dispatch of S7 is sufficient to make the mechanism work, but without being able to call S7::super() on an S3 object, the extended methods might get slightly awkward.

devtools::load_all("~/packages/ggplot2")
#> ℹ Loading ggplot2
library(S7)

# Making a ggplot subclass
p <- ggplot(mpg, aes(displ, cty))
class(p) <- union("foo", class(p))

# Adding custom method for adding layers
method(ggplot_add, list(object = new_S3_class("Layer"), plot = new_S3_class("foo"))) <-
  function(object, plot, object_name) {
    print("I'm adding a layer to foo")
    
    # This is the awkward part:
    method(ggplot_add, list(object = new_S3_class("Layer"), plot = class_ggplot))(
      object, plot, object_name
    )
  }

# Should print text as well as the plot
p + geom_point()
#> [1] "I'm adding a layer to foo"

^{Created on 2023-11-26 with reprex v2.0.2}

[hadley]

You should be able to create an S7 class with exactly the same S3 class as previously (since S7 is designed to be backward compatible with S3); i.e. you should be able to make the without affecting extensions (unless they use NextMethod() which we should think about at the S7 level too).

[hadley]

I'd be tempted to leave object_name as an optional argument for the methods (i.e. just rely on the default constructor which will also include ..., which I'd now consider to be best practice in a generic. OTOH it might be better to make that change in a separate PR, as I'd probably now call this error_arg, and I'd expect a matching error_call argument in order to apply our current error best practices.

[teunbrand]

Fair point, I'll leave this comment unresolved to not forget about this for now.

[hadley]

Thanks for working on this! I don't know if S7 is ready for use in such a high profile package, but I think it's extremely worthwhile to try it out so we can make changes to S7 if needed.

[teunbrand]

Thanks for pointing out all these improvements Hadley!

i.e. you should be able to make the without affecting extensions

I had a shallow attempt at implementing a ggplot class, but the issue I kept running into is that the rest of the codebase always extracts information with $ instead of @. We can of course fix all this for ggplot2 itself, but it would also mean all extensions that extract information directly (mostly those that reimplement ggplot_build() or ggplot_gtable()) would be affected.

(unless they use NextMethod() which we should think about at the S7 level too)

This would be a neat mechanism to have, or more broadly a 'dispatch on the next s3 class'-mechanism.

I don't know if S7 is ready for use in such a high profile package,

I think it might be good to start using S7 for this double dispatch issue, and leave converting S3 to S7 classes for another day when the need arises.

[hadley]

Could you add a $ method?

[teunbrand]

I've converted this PR to a draft to indicate that it is 'on hold' untill S7 is stable enough.