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

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

Open
hafen opened this issue Nov 22, 2024 · 7 comments

Comments

@hafen
Copy link

hafen commented Nov 22, 2024

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
Copy link
Member

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
Copy link
Author

hafen commented Nov 22, 2024

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
Copy link
Member

hadley commented Nov 22, 2024

@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
Copy link
Author

hafen commented Nov 22, 2024

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
Copy link
Member

hadley commented Nov 22, 2024

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

@t-kalinowski
Copy link
Member

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
Copy link
Member

hadley commented Nov 22, 2024

It's also an instance of #364

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