Weird S7/S3 interaction

[teunbrand]

I found that defining a $-method for an S7 class negates another $-method for an S3 class.
I could not reproduce this in an interactive session, so I build a small package to demonstrate the weird interaction.
Please see the README of https://github.com/teunbrand/s7bugreport/ for details.

[hadley]

Oooh I wonder if this is #389 — it might be but that PR alone is not enough (and also it seems to cause a problem when unregistering methods with pkgload).

But running rm(`$`) after the method<- call fixes the problem, so that's almostly certainly the root cause.

You also get the correct result with evalq(bar()$x, asNamespace("s7bugreport")), suggesting that the root cause is that the S3method() directive in the namespace ends up registering the method for the wrong generic.

[teunbrand]

I quickly tried that PR and it doesn't seem to solve the interaction.
If I replace the .onLoad of that package to explicitly register the S3 method:

.onLoad <- function(...) {
  methods_register()
  registerS3method("$", "bar", `$.bar`)
}

Then it behaves as it should, but I don't know enough about the innards of S7 to speculate why this works.

the root cause is that the S3method() directive in the namespace ends up registering the method for the wrong generic.

That does seem consistent with the observed behaviour.

[hadley]

Ok so it looks like the mere presence of $ (even though after #389 it's no longer a function) in the package causes S3method() to register the method in the wrong environment 😬

Minimal reprex of the S3 issue:

#' A foo object
#' @export
foo <- function(x) {
  structure(list(), class = "foo")
}

#' @export
`mean.foo` <- function(...) {
  "FOO"
}

`mean` <- NULL

That means back to the drawing board.

[t-kalinowski]

This is similar to the problem we encountered with @ and S3 methods for @ being in the S7 namespace. We worked around it by being more specific in NAMESPACE:

S3method(base::`@`, S7_object)

[teunbrand]

FWIW it looks like I can work around this bug by using S7::method<- as usual, and using the following roxygen comment at the S3 method:

#' @rawNamespace S3method(base::`$`, S3_class_name)

[teunbrand]

I've found that this issue is not limited to just $, but essentially when both S7 and S3 methods are written for an S3 generic in a package. For example using print(), I run into a similar bug:

https://github.com/teunbrand/s7bugreport/tree/demonstrate-print

[hadley]

@teunbrand if you need a short-term work around, you can use rm(print) etc after each method creation.

And maybe we can generalise this by having method()<- return a sentinel object that we later delete. Unfortunately I think .onLoad() will be too late, because IIRC there you can redirect existing bindings but you can't add or delete.

[teunbrand]

@schloerke attended us on the use of local() to avoid attaching the function in tidyverse/ggplot2#6522. The benefit over rm() is that it doesn't throw a warning when this bug would get fixed.