RConsortium / S7

S7: a new OO system for R
https://rconsortium.github.io/S7
Other
386 stars 32 forks source link

Weird S7/S3 interaction in `$()` #390

Open teunbrand opened 8 months ago

teunbrand commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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)