RConsortium / S7

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

Can't register S3 generic for S7 classes when using package namespacing in S7 class name #501

Open hafen opened 1 week ago

hafen commented 1 week ago

When using S7 classes in a package, there are times when I want to be able to register an S3 generic for an S7 class. When not using package namespacing in the S7 class name (new_class(..., package = NULL, ...)), this works as expected.

For example, here I'm registering a print method:

range <- S7::new_class("range",
  package = NULL,
  properties = list(
    start = S7::class_double,
    end = S7::class_double
  )
)

#' @export
print.range <- function(x, ...) {
  message("range(", x@start, ", ", x@end, ")")
}

x <- range(start = 1, end = 10)

class(x)
#> [1] "range"     "S7_object"

x
#> range(1, 10)

However, I don't know how to do this when using the package namespacing:

range <- S7::new_class("range",
  package = "mypackage", # to simulate this being defined in a package called "mypackage"
  properties = list(
    start = S7::class_double,
    end = S7::class_double
  )
)

x <- range(start = 1, end = 10)

class(x)
#> [1] "mypackage::range" "S7_object"

I can't define an s3 method print.mypackage::range <- function(x, ...) {} - R is not a fan of the ::.

Is there another way to define the method? If so, it would be good to document here. If not, perhaps the way the namespaced class name gets created should be reconsidered.

t-kalinowski commented 1 week ago

Thanks for opening, I agree it would be good to update the docs there and include an example showing a namespace class.

One way to create non-syntatic names like this is w/ the backtick:

`print.mypackage::Range` <- function(x, ...) {}

But note, this is only necessary in exceptional circumstances, as method<- can handle S3 methods just fine.

method(print, Range) <- function(x, ...) {}
hafen commented 1 week ago

Thanks! I don't know why I didn't think of the backtick - must have gotten into tunnel vision from debugging some issues that stemmed from this.

Regarding using method(), however, I have noticed that when using you use method() with an S3 generic in a package, it breaks other uses of that S3 generic outside S7.

For example, suppose I have an R package with the following:

#' @export
Range <- S7::new_class("Range",
  properties = list(
    start = S7::class_double,
    end = S7::class_double
  )
)

S7::method(print, Range) <- function(x, ...) {
  message("Range(", x@start, ", ", x@end, ")")
}

#' @export
myclass <- function() {
  structure(list(), class = "myclass")
}

#' @export
print.myclass <- function(x, ...) {
  message("This is my class...")
}

Now if I instantiate these objects and try to print them:

library(s7test)

x <- Range(start = 1, end = 10)
x
#> Range(1, 10)

y <- myclass()
y
#> list()
#> attr(,"class")
#> [1] "myclass"

The S3 object y doesn't get dispatched to the S3 print method we defined for it. This only happens when the first code snippet is defined in an R package (if you just run it in your console then things in the second snippet will print fine). If I define the print method for Range using print.s7test::Range instead of method(), it will work. I have created a package for reference here: https://github.com/hafen/s7test.

This can be a surprising unintended consequence as there can often be S3 objects interspersed in a package that also has S7 objects that both want to register a method for the same generic. It's possible that there is something obvious here I have missed as well.

hadley commented 1 week ago

@hafen that works for me? Are you sure you have latest S7 (I think this was a bug we fixed in the latest release)

hafen commented 1 week ago

Interesting. I have S7 0.2.0 installed from CRAN.

install.packages("S7")
remotes::install_github("hafen/s7test")

library(s7test)

x <- Range(start = 1, end = 10)
x
#> range(1, 10)

y <- myclass()
y
#> list()
#> attr(,"class")
#> [1] "myclass"

More information about my environment:

sessionInfo()
#> R version 4.4.1 (2024-06-14)
#> Platform: aarch64-apple-darwin20
#> Running under: macOS 15.1.1
#> 
#> Matrix products: default
#> BLAS:   /System/Library/Frameworks/Accelerate.framework/Versions/A/Frameworks/vecLib.framework/Versions/A/libBLAS.dylib 
#> LAPACK: /Library/Frameworks/R.framework/Versions/4.4-arm64/Resources/lib/libRlapack.dylib;  LAPACK version 3.12.0
#> 
#> locale:
#> [1] en_US.UTF-8/en_US.UTF-8/en_US.UTF-8/C/en_US.UTF-8/en_US.UTF-8
#> 
#> time zone: America/Los_Angeles
#> tzcode source: internal
#> 
#> attached base packages:
#> [1] stats     graphics  grDevices utils     datasets  methods   base     
#> 
#> other attached packages:
#> [1] s7test_0.0.0.9000
#> 
#> loaded via a namespace (and not attached):
#> [1] compiler_4.4.1 cli_3.6.3      S7_0.2.0       jsonlite_1.8.9 rlang_1.1.4
hadley commented 1 week ago

Hmmmm, weird. I see the same thing as you now.

t-kalinowski commented 1 week ago

I think it's a bug in R and/or roxygen2.

method<- binds the print symbol in the package namespace with the value of base::print. Then, loadNamespace() sees the S3method(print,myclass) directive, sees that print is a binding to a generic in the package namespace, assumes the generic has the same namespace as the package, and registers the s3 method in the package namespace, rather than the actual namespace of the generic (base).

One way to fix/workaround is to replace the @export roxygen directive with this:

#' @rawNamespace S3method(base::print, myclass)
print.myclass <- function(x, ...) {
  message("This is my class...")
}
hadley commented 1 week ago

It's also an instance of #364