diff --git a/NEWS.md b/NEWS.md index 0de661bf..fad1b4ea 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,6 +1,5 @@ # S7 (development version) -* Internal changes to support R-devel (4.6) (#592, #593, #598, #600). * New `:=` operator creates and names an object in one step, so `Foo := new_class()` is equivalent to `Foo <- new_class(name = "Foo")` (#658). * Errors thrown by S7 now report the function where they occurred, making it easier to track down the source of a problem (#646). * `class_POSIXct` uses the `tzone` attribute (not `tz`), and allows it to be absent (#401). diff --git a/R/class-spec.R b/R/class-spec.R index 62e2d843..6a0970ff 100644 --- a/R/class-spec.R +++ b/R/class-spec.R @@ -204,7 +204,9 @@ class_validate <- function(class, object) { S7 = class@validator, S7_base = class$validator, S7_S3 = class$validator, - S7_external = class_validate(resolve_external_class_req(class), object), + S7_external = function(object) { + class_validate(resolve_external_class_req(class), object) + }, NULL ) @@ -299,7 +301,13 @@ class_deparse <- function(x) { paste0("new_union(", paste(classes, collapse = ", "), ")") }, S7_S3 = paste0("new_S3_class(", deparse1(x$class), ")"), - S7_external = sprintf("new_external_class(%s, %s)", x$package, x$name), + S7_external = { + args <- c(deparse1(x$package), deparse1(x$name)) + if (!is.null(x$version)) { + args <- c(args, paste0("version = ", deparse1(x$version))) + } + sprintf("new_external_class(%s)", paste(args, collapse = ", ")) + }, ) } @@ -312,16 +320,28 @@ class_inherits <- function(x, what) { S4 = isS4(x) && methods::is(x, what), S7 = inherits(x, "S7_object") && inherits(x, S7_class_name(what)), S7_base = what$class == base_class(x), - S7_union = any(vlapply(what$classes, class_inherits, x = x)), + S7_union = { + for (class in what$classes) { + if (class_inherits(x, class)) { + return(TRUE) + } + } + FALSE + }, S7_S3 = !isS4(x) && class_dispatch_extends(what$class, class(x)), - S7_external = inherits(x, "S7_object") && inherits(x, what$class_name), + S7_external = inherits(x, "S7_object") && + inherits(x, what$class_name) && + (is.null(what$version) || + class_inherits(x, resolve_external_class_req(what))), ) } # Is every instance of `child` guaranteed to also be an instance of `parent`? # Used to check that a child class only narrows the type of a property class_extends <- function(child, parent) { - if (is_class_any(parent) || union_contains_any(parent)) { + if (identical(child, parent)) { + TRUE + } else if (is_class_any(parent) || union_contains_any(parent)) { # as a parent, `class_any` accepts every child class TRUE } else if (is_class_any(child)) { @@ -332,25 +352,34 @@ class_extends <- function(child, parent) { all(vlapply(child$classes, class_extends, parent = parent)) } else if (is_union(parent)) { # A non-union child extends a union parent if it extends any of its members. - any(vlapply(parent$classes, class_extends, child = child)) + for (class in parent$classes) { + if (class_extends(child, class)) { + return(TRUE) + } + } + FALSE } else if (is.null(child) && !is.null(parent)) { # as a child, NULL can only extend NULL FALSE } else if (is.null(parent)) { # as a parent, NULL only accepts NULL is.null(child) - } else if (is_S4_class(child) || is_S4_class(parent)) { - is_S4_class(child) && - is_S4_class(parent) && - methods::extends(child@className, parent@className) } else if (is_class(parent) && parent@name == "S7_object") { - is_class(child) + is_class(child) || is_external_class(child) } else if (is_external_class(child)) { child <- resolve_external_class_req(child) class_extends(child, parent) + } else if (is_class(child) && is_external_class(parent)) { + is_external_class_match(child, parent) && + (is.null(parent$version) || + class_extends(child, resolve_external_class_req(parent))) } else if (is_external_class(parent)) { parent <- resolve_external_class_req(parent) class_extends(child, parent) + } else if (is_S4_class(child) || is_S4_class(parent)) { + is_S4_class(child) && + is_S4_class(parent) && + methods::extends(child@className, parent@className) } else { # handle S7, S3, and base types. class_dispatch_extends(class_dispatch(parent), class_dispatch(child)) diff --git a/R/external-class.R b/R/external-class.R index f102762b..6bd1d7ff 100644 --- a/R/external-class.R +++ b/R/external-class.R @@ -37,9 +37,9 @@ #' @returns An S7 external class, i.e. a list with S3 class `S7_external_class`. #' @export #' @examples -#' # Refer to a class in another package without taking a hard dependency: -#' Tibble <- new_external_class("tibble", "tbl_df") -#' Tibble +#' # Refer to an S7 class in another package without taking a hard dependency: +#' TheirClass <- new_external_class("theirpkg", "TheirClass") +#' TheirClass #' #' # Self-referential class: the `child` property can be another `tree`, #' # or `NULL` to terminate the chain. @@ -71,20 +71,6 @@ is_external_class <- function(x) { inherits(x, "S7_external_class") } -class_has_external_class <- function(x) { - if (is_external_class(x)) { - TRUE - } else if (is_union(x)) { - any(vlapply(x$classes, class_has_external_class)) - } else { - FALSE - } -} - -signature_has_external_class <- function(signature) { - any(vlapply(signature, class_has_external_class)) -} - class_external_deps <- function(x) { if (is_external_class(x)) { list(x) @@ -99,6 +85,12 @@ signature_external_deps <- function(signature) { flatten_external_deps(lapply(signature, class_external_deps)) } +external_deps_resolvable <- function(deps) { + all(vlapply(deps, function(dep) { + dep_available(dep) && !is.null(find_external_class(dep)) + })) +} + flatten_external_deps <- function(x) { unlist(x, recursive = FALSE, use.names = FALSE) } @@ -116,8 +108,11 @@ print.S7_external_class <- function(x, ...) { } dep_available <- function(dep) { - isNamespaceLoaded(dep$package) && - (is.null(dep$version) || getNamespaceVersion(dep$package) >= dep$version) + isNamespaceLoaded(dep$package) && dep_version_ok(dep) +} + +dep_version_ok <- function(dep) { + is.null(dep$version) || getNamespaceVersion(dep$package) >= dep$version } # Make it mockable @@ -125,7 +120,7 @@ getNamespaceVersion <- NULL resolve_signature <- function(signature) { for (i in seq_along(signature)) { - signature[[i]] <- resolve_class_req(signature[[i]]) + signature[i] <- list(resolve_class_req(signature[[i]])) } signature } @@ -142,29 +137,18 @@ resolve_class_req <- function(x) { find_external_class <- function(x) { ns <- asNamespace(x$package) - if (exists(x$name, envir = ns, inherits = FALSE)) { - obj <- get(x$name, envir = ns, inherits = FALSE) - if (is_external_class_match(obj, x)) { - return(obj) - } - } - - # Also consider cases where the constructor isn't named the same as the class - for (name in ls(ns, all.names = TRUE)) { - obj <- get(name, envir = ns, inherits = FALSE) - if (is_external_class_match(obj, x)) { - return(obj) - } + obj <- get0(x$name, envir = ns, inherits = FALSE) + if (is_external_class_match(obj, x)) { + obj + } else { + NULL } - - NULL } is_external_class_match <- function(obj, x) { is_class(obj) && - (identical(S7_class_name(obj), x$class_name) || - (identical(obj@name, x$name) && - (is.null(obj@package) || identical(obj@package, x$package)))) + identical(obj@name, x$name) && + identical(obj@package, x$package) } # Required resolution: errors if the external class can't be resolved (e.g. @@ -172,17 +156,17 @@ is_external_class_match <- function(obj, x) { # or looking up methods, checking property overrides in a subclass, and # constructing or validating an instance. resolve_external_class_req <- function(x) { - prefix <- sprintf("Can't find external class <%s>:\n", x$class_name) + prefix <- sprintf("Can't find external class <%s>:", x$class_name) if (!requireNamespace(x$package, quietly = TRUE)) { stop2( - paste0(prefix, sprintf("* Package '%s' is not installed.", x$package)), + c(prefix, sprintf("* Package '%s' is not installed.", x$package)), call = NULL ) } - if (!is.null(x$version) && getNamespaceVersion(x$package) < x$version) { + if (!dep_version_ok(x)) { stop2( - paste0( + c( prefix, sprintf( "* Package '%s' needs version %s, but only %s is available.", @@ -197,10 +181,20 @@ resolve_external_class_req <- function(x) { class <- find_external_class(x) if (is.null(class)) { + binding <- sprintf( + "`%s` with @name '%s' and @package '%s'", + x$name, + x$name, + x$package + ) stop2( - paste0( + c( prefix, - sprintf("* Packages '%s' doesn't contain '%s'.", x$package, x$name) + sprintf( + "* Package '%s' must bind an S7 class to %s.", + x$package, + binding + ) ), call = NULL ) diff --git a/R/external-generic.R b/R/external-generic.R index 1537620a..a2ddef6c 100644 --- a/R/external-generic.R +++ b/R/external-generic.R @@ -41,7 +41,12 @@ new_external_generic <- function(package, name, dispatch_args, version = NULL) { } as_external_generic <- function(x, env = parent.frame()) { - if (is_S7_generic(x)) { + if (is_generic_sentinel(x)) { + # Sentinels are external generic specs with an extra marker class; keep + # this in sync with generic_sentinel(). + class(x) <- "S7_external_generic" + x + } else if (is_S7_generic(x)) { pkg <- package_name(x) new_external_generic(pkg, x@name, x@dispatch_args) } else if (is_external_generic(x)) { @@ -78,41 +83,47 @@ is_external_generic <- function(x) { external_generic_available <- function(generic) { is_external_generic(generic) && - isNamespaceLoaded(generic$package) && - external_generic_version_ok(generic, asNamespace(generic$package)) -} - -external_generic_version_ok <- function(generic, ns) { - stopifnot(is_external_generic(generic), is.environment(ns)) - - is.null(generic$version) || getNamespaceVersion(ns) >= generic$version + dep_available(generic) } -registrar <- function(deps, generic, signature, method, env) { +registrar <- function(generic, signature, method, env) { # Force all arguments - deps generic signature method env function(...) { - if (!all(vlapply(deps, dep_available))) { + if (!dep_available(generic)) { return(invisible()) } + sig_deps <- signature_external_deps(signature) + if (length(sig_deps)) { + if (!all(vlapply(sig_deps, dep_available))) { + return(invisible()) + } + for (dep in sig_deps) { + resolve_external_class_req(dep) + } + } + generic_fun <- resolve_generic(generic) if (is.null(generic_fun)) { return(invisible()) } - signature <- resolve_signature(signature) - register_method(generic_fun, signature, method, env, package = NULL) + register_method( + generic_fun, + resolve_signature(signature), + method, + env, + package = NULL + ) + invisible() } } -# Collects all external dependencies (the generic + any external classes) -# into a single list. Each entry has at minimum `package` + `version`. method_deps <- function(generic, signature) { c(list(generic), signature_external_deps(signature)) } diff --git a/R/hooks.R b/R/hooks.R index 468d8f3c..48cd2655 100644 --- a/R/hooks.R +++ b/R/hooks.R @@ -48,7 +48,12 @@ methods_register <- function() { } S7_on_load_ <- function(env) { - hooks_set_and_run(packageName(env)) + package <- packageName(env) + + hooks_remove(package) # always start from a clean slate + hooks <- hooks_add(package) + hooks_run_loaded(hooks) # run hooks for loaded packages + invisible() } @@ -64,25 +69,29 @@ S7_on_unload_ <- function(env) { tbl <- S7_methods_table(package) for (x in tbl) { - if (!isNamespaceLoaded(x$generic$package)) { - next - } - - ns <- asNamespace(x$generic$package) - if (!external_generic_version_ok(x$generic, ns)) { + if (!dep_available(x$generic)) { next } - generic <- get0(x$generic$name, envir = ns, inherits = FALSE) + generic <- get0( + x$generic$name, + envir = asNamespace(x$generic$package), + inherits = FALSE + ) if (is.null(generic)) { next } generic <- as_generic(generic) # Methods registered for S3 and S4 generics can't be unregistered yet if (is_S7_generic(generic)) { + signature <- x$signature + deps <- signature_external_deps(signature) + if (length(deps) && external_deps_resolvable(deps)) { + signature <- resolve_signature(signature) + } unregister_own_S7_method( generic, - x$signature, + signature, x$method, package ) @@ -92,57 +101,51 @@ S7_on_unload_ <- function(env) { invisible() } -# Start from a clean slate, then register each method whose dependency packages -# are already loaded, and add a hook so it (re)registers whenever one of those -# packages is loaded in the future. -hooks_set_and_run <- function(package) { - hooks_remove(package) - +# Add a hook for each method that registers it when any of its dependency +# packages are loaded. Returns the added hooks, named by the package they're +# attached to. +hooks_add <- function(package) { + ns <- asNamespace(package) + hooks <- list() pkgs <- character() + for (x in S7_methods_table(package)) { - hook <- hook_add(package, x) - hook$run() - pkgs <- c(pkgs, hook$pkgs) + deps <- method_deps(x$generic, x$signature) + register <- registrar(x$generic, x$signature, x$method, ns) + + for (pkg in method_deps_packages(deps)) { + hook <- S7_hook(register, package) + setHook(packageEvent(pkg, "onLoad"), hook) + append1(hooks) <- hook + append1(pkgs) <- pkg + } } - # Record packages with hooks so we can remove them on unload + names(hooks) <- pkgs hooks_packages(package) <- unique(pkgs) - invisible() + hooks } -# Add a hook that (re)registers method `x` whenever one of its dependency -# packages is loaded, and return its registrar so it can also be run now. -hook_add <- function(package, x) { - ns <- asNamespace(package) - - deps <- method_deps(x$generic, x$signature) - register <- registrar(deps, x$generic, x$signature, x$method, ns) - hook <- S7_hook(register, package) - - pkgs <- method_deps_packages(deps) - for (pkg in pkgs) { - setHook(packageEvent(pkg, "onLoad"), hook) - } - - list(run = register, pkgs = pkgs) -} - -# Remove all of our hooks for `package`. `hooks_packages()` records every -# package event we've added a hook to, so we don't need to re-derive them here. +# Remove our hooks for `package`. hooks_remove <- function(package) { for (pkg in hooks_packages(package)) { - hook_remove(package, pkg) + event <- packageEvent(pkg, "onLoad") + hooks <- getHook(event) + ours <- vlapply(hooks, is_S7_hook, package = package) + if (any(ours)) { + setHook(event, hooks[!ours], action = "replace") + } } hooks_packages(package) <- character() invisible() } -hook_remove <- function(package, pkg) { - event <- packageEvent(pkg, "onLoad") - hooks <- getHook(event) - ours <- vlapply(hooks, is_S7_hook, package = package) - if (any(ours)) { - setHook(event, hooks[!ours], action = "replace") + +hooks_run_loaded <- function(hooks) { + is_loaded <- vlapply(names(hooks), isNamespaceLoaded) + for (hook in hooks[is_loaded]) { + hook() } + invisible() } #' @export diff --git a/R/method-register.R b/R/method-register.R index b334f1f7..e1657455 100644 --- a/R/method-register.R +++ b/R/method-register.R @@ -88,11 +88,20 @@ register_method <- function( # Delay package methods with external classes until onLoad. Outside a package # there is no deferred methods table, so resolve them before registering. - if (signature_has_external_class(signature)) { + deps <- signature_external_deps(signature) + if (length(deps)) { if (is.null(package)) { signature <- resolve_signature(signature) } else { generic_ext <- as_external_generic(generic, env) + if (is_S7_generic(generic)) { + check_method( + method, + generic, + name = method_name(generic, signature), + call = call + ) + } external_methods_add(package, generic_ext, signature, method) if (!is_local_generic(generic, package)) { return(generic_sentinel(generic_ext)) @@ -150,6 +159,11 @@ unregister_method <- function( generic <- as_generic(generic, call = call) signature <- as_signature(signature, generic, call = call) + external <- NULL + if (is_external_generic(generic)) { + external <- as_external_generic(generic, env) + } + if (external_generic_available(generic)) { generic <- as_generic( getFromNamespace(generic$name, generic$package), @@ -157,9 +171,15 @@ unregister_method <- function( ) } + deps <- signature_external_deps(signature) + unregister_signature <- signature + if (length(deps) && (is.null(package) || external_deps_resolvable(deps))) { + unregister_signature <- resolve_signature(signature) + } + # Unregister in current session if (is_S7_generic(generic)) { - unregister_S7_method(generic, signature) + unregister_S7_method(generic, unregister_signature) } else if (is_S3_generic(generic)) { stop2("Can't unregister methods for S3 generics", call = call) } else if (is_S4_generic(generic)) { @@ -168,10 +188,13 @@ unregister_method <- function( # If we're inside a package, also remove from the deferred external # methods table so the method isn't re-registered on package load. - if (!is.null(package) && !is_local_generic(generic, package)) { - external <- as_external_generic(generic) + if (!is.null(package)) { + local <- is_local_generic(generic, package) + external <- external %||% as_external_generic(generic, env) external_methods_remove(package, external, signature) - return(generic_sentinel(external)) + if (!local) { + return(generic_sentinel(external)) + } } invisible(original) diff --git a/man/new_external_class.Rd b/man/new_external_class.Rd index b3e6b616..3a05f3f9 100644 --- a/man/new_external_class.Rd +++ b/man/new_external_class.Rd @@ -47,9 +47,9 @@ External classes can not currently be used as parents in \code{\link[=new_class] We hope to relax that restriction in the near future. } \examples{ -# Refer to a class in another package without taking a hard dependency: -Tibble <- new_external_class("tibble", "tbl_df") -Tibble +# Refer to an S7 class in another package without taking a hard dependency: +TheirClass <- new_external_class("theirpkg", "TheirClass") +TheirClass # Self-referential class: the `child` property can be another `tree`, # or `NULL` to terminate the chain. diff --git a/tests/testthat/_snaps/external-class.md b/tests/testthat/_snaps/external-class.md index a37b1a22..dd27ca79 100644 --- a/tests/testthat/_snaps/external-class.md +++ b/tests/testthat/_snaps/external-class.md @@ -41,7 +41,22 @@ Condition Error: ! Can't find external class : - * Packages 'S7' doesn't contain 'not_a_class'. + * Package 'S7' must bind an S7 class to `not_a_class` with @name 'not_a_class' and @package 'S7'. + +# external class resolution explains class binding contract + + Code + new_class(name = "Holder", properties = list(child = Bar)) + Condition + Error: + ! Can't find external class : + * Package 'dep' must bind an S7 class to `Bar` with @name 'Bar' and @package 'dep'. + Code + new_class(name = "SymbolHolder", properties = list(child = SymbolMismatch)) + Condition + Error: + ! Can't find external class : + * Package 'symbol_mismatch' must bind an S7 class to `Bar` with @name 'Bar' and @package 'symbol_mismatch'. # external class works as a property type for self-reference @@ -52,3 +67,21 @@ ! object properties are invalid: - @child must be or , not +# external class property validation reports validator errors + + Code + Holder(child = invalid) + Condition + Error in `Holder()`: + ! object properties are invalid: + - @child: x must be non-negative + +# versioned external class checks package version + + Code + S7_inherits(versioned_pkg$Foo(), Foo) + Condition + Error: + ! Can't find external class : + * Package 'versioned_pkg' needs version 999.0, but only 0.0.0 is available. + diff --git a/tests/testthat/_snaps/method-register.md b/tests/testthat/_snaps/method-register.md index 3873f0ea..0feca523 100644 --- a/tests/testthat/_snaps/method-register.md +++ b/tests/testthat/_snaps/method-register.md @@ -24,6 +24,13 @@ * An S3 class object (from `new_S3_class()`) * An S4 class object * A base class + Code + local_package("pkg_invalid_deferred_external_class_method", foo := new_generic( + "x"), ext := new_external_class("notloaded.pkg"), method(foo, ext) <- + (function(y) "x")) + Condition + Error in `method<-`: + ! foo() dispatches on `x`, but foo() has arguments `y`. # method unregistration removes an S7 method via NULL assignment diff --git a/tests/testthat/test-class.R b/tests/testthat/test-class.R index 8ae0672a..cd1747e7 100644 --- a/tests/testthat/test-class.R +++ b/tests/testthat/test-class.R @@ -155,6 +155,61 @@ test_that("inheritance lets child properties narrow parent unions that include a )) }) +test_that("inheritance handles external class property specs", { + dep := local_package( + External := new_class() + ) + External <- new_external_class(package = "dep", name = "External") + + ParentObject := new_class( + properties = list(x = S7_object), + package = NULL + ) + ChildObject := new_class( + parent = ParentObject, + properties = list( + x = new_property(class = External, default = quote(dep$External())) + ), + package = NULL + ) + expect_s3_class(ChildObject()@x, "dep::External") + + Ext <- new_external_class(package = "notloaded.pkg", name = "Cls") + prop <- new_property( + class = Ext, + default = quote({ + NULL + }) + ) + ParentSame := new_class( + properties = list(x = prop), + package = NULL + ) + expect_no_error(new_class( + name = "ChildSame", + parent = ParentSame, + properties = list(x = prop), + package = NULL + )) + + Missing <- new_external_class(package = "S7testthatmissing", name = "Missing") + ParentUnion := new_class( + properties = list( + x = new_property( + class = Missing | S7_object, + default = quote(S7_object()) + ) + ), + package = NULL + ) + ChildUnion := new_class( + parent = ParentUnion, + properties = list(x = S7_object), + package = NULL + ) + expect_s3_class(ChildUnion(), "ChildUnion") +}) + test_that("inheritance lets child properties narrow optional union properties with NULL", { Parent <- new_class( "Parent", diff --git a/tests/testthat/test-external-class.R b/tests/testthat/test-external-class.R index d25fd0dc..ffb55fff 100644 --- a/tests/testthat/test-external-class.R +++ b/tests/testthat/test-external-class.R @@ -13,28 +13,12 @@ test_that("print method works", { }) test_that("external class is a valid class spec", { - ec <- new_external_class("foo", "Bar") + ec <- new_external_class(package = "foo", name = "Bar") expect_identical(as_class(ec), ec) - expect_equal(class_type(ec), "S7_external") - expect_equal(class_register(ec), "foo::Bar") - expect_equal(class_desc(ec), "") expect_equal(S7_class_desc(ec), "") }) -test_that("external class resolution uses the S7 class name", { - # The class is bound to `class_renamed`, but its S7 name is "renamed", so - # resolution must find it by scanning for a matching S7 class name. - pkg := local_package( - renamed <- new_class("named") - ) - named := new_external_class("pkg") - resolved <- resolve_external_class_req(named) - - expect_s3_class(resolved, "S7_class") - expect_equal(S7_class_name(resolved), "pkg::named") -}) - test_that("resolve_external_class_req() errors per failure mode", { local_mocked_bindings(getNamespaceVersion = function(package) "1.0.0") expect_snapshot(error = TRUE, { @@ -44,6 +28,33 @@ test_that("resolve_external_class_req() errors per failure mode", { }) }) +test_that("external class resolution explains class binding contract", { + local_package( + "dep", + Foo <- new_class(name = "Bar", package = "dep") + ) + Bar <- new_external_class(package = "dep", name = "Bar") + local_package( + "symbol_mismatch", + Bar <- new_class(name = "Foo", package = "symbol_mismatch") + ) + SymbolMismatch <- new_external_class( + package = "symbol_mismatch", + name = "Bar" + ) + + expect_snapshot(error = TRUE, { + new_class( + name = "Holder", + properties = list(child = Bar) + ) + new_class( + name = "SymbolHolder", + properties = list(child = SymbolMismatch) + ) + }) +}) + test_that("external class can be used as a union arm", { ec <- new_external_class("foo", "Bar") u <- NULL | ec @@ -51,6 +62,14 @@ test_that("external class can be used as a union arm", { expect_length(u$classes, 2) }) +test_that("S7_inherits() matches loaded union arms around unloaded external classes", { + Foo := new_class(package = NULL) + Missing <- new_external_class(package = "S7testthatmissing", name = "Bar") + + expect_true(S7_inherits(Foo(), Foo | Missing)) + expect_true(S7_inherits(Foo(), Missing | Foo)) +}) + test_that("external class works as a property type for self-reference", { Tree := new_class( package = "mypkg", @@ -85,36 +104,46 @@ test_that("external class works for mutually recursive classes", { expect_s3_class(obj@x@y, "mypkg::ClassOne") }) -test_that("class_inherits() works for external class", { - Tree := new_class( - package = "mypkg", - properties = list(child = NULL | new_external_class("mypkg", "Tree")) +test_that("external class property validation reports validator errors", { + dep := local_package( + Ext := new_class( + properties = list(x = class_integer), + validator = function(self) { + if (self@x < 0L) { + "x must be non-negative" + } + } + ) ) - ec <- new_external_class("mypkg", "Tree") - expect_true(class_inherits(Tree(), ec)) - expect_false(class_inherits(1, ec)) - expect_false(class_inherits(NULL, ec)) + Holder := new_class( + properties = list( + child = new_property( + class = new_external_class("dep", "Ext"), + default = quote(dep$Ext(x = 0L)) + ) + ) + ) + + valid <- Holder(child = dep$Ext(x = 1L)) + expect_s3_class(valid@child, "dep::Ext") + + invalid <- valid_implicitly(dep$Ext(x = 1L), function(self) { + self@x <- -1L + self + }) + + expect_snapshot(Holder(child = invalid), error = TRUE) }) -test_that("method_deps() collects the generic and external classes", { - gen <- new_external_generic("foo", "bar", "x") - sig <- list( - new_external_class("baz", "X"), - class_character, - NULL | new_external_class("qux", "Y", version = "1.0") +test_that("versioned external class checks package version", { + versioned_pkg := local_package( + Foo := new_class() + ) + Foo <- new_external_class( + package = "versioned_pkg", + name = "Foo", + version = "999.0" ) - deps <- method_deps(gen, sig) - expect_equal(vcapply(deps, `[[`, "package"), c("foo", "baz", "qux")) - expect_equal(deps[[3]]$version, "1.0") -}) -test_that("dep_available() respects loaded + version", { - # S7 is loaded, so this dep is available - expect_true(dep_available(new_external_generic("S7", "S7_inherits", "x"))) - # version too high → not available - expect_false(dep_available( - new_external_generic("S7", "S7_inherits", "x", version = "999.0") - )) - # unloaded package → not available - expect_false(dep_available(new_external_class("not_a_package", "X"))) + expect_snapshot(error = TRUE, S7_inherits(versioned_pkg$Foo(), Foo)) }) diff --git a/tests/testthat/test-hooks.R b/tests/testthat/test-hooks.R index 1f8a62ce..e065f64e 100644 --- a/tests/testthat/test-hooks.R +++ b/tests/testthat/test-hooks.R @@ -14,20 +14,55 @@ test_that("S7_on_load() doesn't accumulate hooks across repeated loads", { expect_length(package_hooks("upstream"), 1) }) -test_that("S7_on_load() registers methods dispatching on an external class", { - upstream := local_package( - Foo := new_class() +test_that("S7_on_load() waits until all external union arms are available", { + generic_pkg <- local_package( + "external_union_generic", + gen := new_generic("x") ) - downstream := local_package( - own_generic := new_generic("x"), - Foo := new_external_class("upstream"), - method(own_generic, Foo) <- \(x) "from external class" + upstream_a <- local_package("external_union_a", A := new_class()) + downstream <- local_package( + "external_union_downstream", + .onLoad <- function(...) S7_on_load(), + gen <- new_external_generic( + package = "external_union_generic", + name = "gen", + dispatch_args = "x" + ), + A := new_external_class(package = "external_union_a"), + B := new_external_class(package = "external_union_b"), + method(gen, A | B) <- function(x) "union" ) - # The method is deferred (its signature has an external class), not yet live - expect_length(methods(downstream$own_generic), 0) - S7_on_load_(downstream) + downstream$.onLoad() + expect_error( + generic_pkg$gen(upstream_a$A()), + class = "S7_error_method_not_found" + ) + + upstream_b <- local_package("external_union_b", B := new_class()) + downstream$.onLoad() + expect_equal(generic_pkg$gen(upstream_a$A()), "union") + expect_equal(generic_pkg$gen(upstream_b$B()), "union") +}) + +test_that("S7_on_load() and S7_on_unload() handle external classes", { + upstream <- local_package("external_class_unload", Foo := new_class()) + downstream <- local_package( + "downstream_external_class_unload", + .onLoad <- function(...) S7_on_load(), + .onUnload <- function(...) S7_on_unload(), + own_generic := new_generic("x"), + Foo := new_external_class(package = "external_class_unload"), + method(own_generic, Foo) <- function(x) "from external class" + ) + downstream$.onLoad() expect_equal(downstream$own_generic(upstream$Foo()), "from external class") + + downstream$.onUnload() + expect_error( + downstream$own_generic(upstream$Foo()), + class = "S7_error_method_not_found" + ) }) test_that("S7_on_unload() unregisters methods and removes hooks", { diff --git a/tests/testthat/test-method-register.R b/tests/testthat/test-method-register.R index 2fe06880..d530ca86 100644 --- a/tests/testthat/test-method-register.R +++ b/tests/testthat/test-method-register.R @@ -51,6 +51,12 @@ test_that("method registration checks argument types", { x <- 10 method(x, class_character) <- function(x) ... method(foo, 1) <- function(x) ... + local_package( + "pkg_invalid_deferred_external_class_method", + foo := new_generic("x"), + ext := new_external_class("notloaded.pkg"), + method(foo, ext) <- function(y) "x" + ) }) }) @@ -65,16 +71,22 @@ test_that("method registration returns the generic unchanged when not in a packa }) test_that("method registration resolves external classes outside packages", { + dep := local_package( + Ext := new_class() + ) + env <- new.env(parent = baseenv()) env[["method<-"]] <- `method<-` env$g <- new_generic("g", "x") - env$ext <- new_external_class("S7", "S7_object") + env$ext <- new_external_class("dep", "Ext") env$f <- function(x) "external" - expect_null(packageName(env)) evalq(method(g, ext) <- f, env) - expect_equal(env$g(S7_object()), "external") + expect_equal(env$g(dep$Ext()), "external") + + evalq(method(g, ext) <- NULL, env) + expect_length(methods(env$g), 0) }) test_that("method registration returns a strippable sentinel for foreign generics in a package (#364)", { @@ -93,30 +105,27 @@ test_that("method registration returns a strippable sentinel for foreign generic # registered through the same binding (as in the t2 test package) evalq(method(ext, foo2) <- function(x) "y", pkg) expect_s3_class(pkg$ext, "S7_generic_sentinel") - expect_length(S7_methods_table("pkg"), 2) }) -test_that("deferred external-class methods preserve sentinel for foreign generics", { - pkg := local_package( - ext := new_external_class("notloaded.pkg") +test_that("method unregistration removes deferred external-class unions", { + upstream := local_package( + "upstream_external_unregister", + Foo := new_class() ) - - # In a package, `method<-` writes a sentinel back into the binding - evalq(method(sum, ext) <- function(x, ...) "x", pkg) - expect_s3_class(pkg$sum, "S7_generic_sentinel") - expect_s3_class(pkg$sum, "S7_external_generic") - expect_length(S7_methods_table("pkg"), 1) -}) - -test_that("method registration defers external classes in union signatures", { - pkg := local_package( + downstream := local_package( + "downstream_external_unregister", + .onLoad <- function(...) S7_on_load(), foo := new_generic("x"), - ext := new_external_class("notloaded.pkg"), - method(foo, NULL | ext) <- function(x) "x" + Foo := new_external_class(package = "upstream_external_unregister"), + method(foo, NULL | Foo) <- function(x) "x", + method(foo, NULL | Foo) <- NULL ) + downstream$.onLoad() - expect_length(methods(pkg$foo), 0) - expect_length(S7_methods_table("pkg"), 1) + expect_error( + downstream$foo(upstream$Foo()), + class = "S7_error_method_not_found" + ) }) test_that("method unregistration removes an S7 method via NULL assignment", { diff --git a/vignettes/packages.Rmd b/vignettes/packages.Rmd index c58d198d..64361f4f 100644 --- a/vignettes/packages.Rmd +++ b/vignettes/packages.Rmd @@ -58,42 +58,29 @@ If you export a class (i.e. its constructor), you must also set the `package` ar NB: if your package creates a hierarchy of classes, subclasses must be defined _after_ the parent classes. That means if you define the classes in separate files, you will need to use the `DESCRIPTION` Collate field (or the equivalent roxygen2 `@include` tag) to ensure the files are loaded in the correct order. -## Generics +## Generics and methods You should document generics like regular functions (since they are!). If you expect others to create their own methods for your generic, you may want to include a section describing the properties that you expect all methods to have. If you want to list all methods for a generic, you can use the [doclisting](https://doclisting.r-lib.org) package. -## Methods - If you use roxygen2, you can document S7 generics and methods by following the advice in [`vignette("rd-S7", package = "roxygen2")`](https://roxygen2.r-lib.org/articles/rd-S7.html). Note that methods can only be defined after both the class and generic have been defined. If generics/methods/classes live in different files, you will need to use the `DESCRIPTION` Collate field (or the equivalent roxygen2 `@include` tag) to ensure the files are loaded in the correct order. -### Methods for generics in suggested packages +### Methods for suggested packages -If you want to register a method for a generic defined in a package that you only suggest (rather than import), use `new_external_generic()` to refer to the generic without taking a hard dependency: +Use `new_external_generic()` and `new_external_class()` to register methods for S7 generics and classes from suggested packages, without taking a hard dependency: ```{r, eval = FALSE} -# In your package -median <- new_external_generic("stats", "median", "x") -method(median, MyClass) <- function(x, ...) { ... } -``` - -When the suggested package is loaded, S7 will register the method automatically (via `S7_on_load()` as described above). +TheirClass <- new_external_class("theirpkg", "TheirClass") +method(my_generic, TheirClass) <- function(x) { ... } -### Methods for classes in suggested packages - -Conversely, you may want to register a method for one of your own generics, dispatching on a class from a suggested package. Use `new_external_class()` to refer to the class by name: - -```{r, eval = FALSE} -# In your package -my_generic <- new_generic("my_generic", "x") -ggplot <- new_external_class("ggplot2", "ggplot") -method(my_generic, ggplot) <- function(x) { ... } +their_generic <- new_external_generic("theirpkg", "their_generic", "x") +method(their_generic, MyClass) <- function(x) { ... } ``` -When the suggested package is loaded, S7 will register the method automatically (via `S7_on_load()` as described above). +S7 will register these methods automatically when the suggested package is loaded, via `S7_on_load()` as described above. ## Backward compatibility