From 71cd220deaccf28b95b4048d2e1143d0e0159e23 Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Thu, 18 Jun 2026 18:41:33 -0400 Subject: [PATCH 01/42] Address external class review findings --- R/class-spec.R | 12 ++++++------ R/method-register.R | 11 ++++++++--- tests/testthat/test-class.R | 24 ++++++++++++++++++++++++ tests/testthat/test-method-register.R | 12 ++++++++++++ vignettes/packages.Rmd | 14 +++++++++++--- 5 files changed, 61 insertions(+), 12 deletions(-) diff --git a/R/class-spec.R b/R/class-spec.R index 62e2d843..e8c59af5 100644 --- a/R/class-spec.R +++ b/R/class-spec.R @@ -339,18 +339,18 @@ class_extends <- function(child, parent) { } 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) } else if (is_external_class(child)) { child <- resolve_external_class_req(child) class_extends(child, 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 if (is_class(parent) && parent@name == "S7_object") { + is_class(child) } else { # handle S7, S3, and base types. class_dispatch_extends(class_dispatch(parent), class_dispatch(child)) diff --git a/R/method-register.R b/R/method-register.R index b334f1f7..280da03e 100644 --- a/R/method-register.R +++ b/R/method-register.R @@ -168,10 +168,15 @@ 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) + local <- !is.null(package) && is_local_generic(generic, package) + deferred <- !is.null(package) && + (!local || signature_has_external_class(signature)) + if (deferred) { + 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/tests/testthat/test-class.R b/tests/testthat/test-class.R index 8ae0672a..829c3289 100644 --- a/tests/testthat/test-class.R +++ b/tests/testthat/test-class.R @@ -93,6 +93,30 @@ test_that("inheritance lets child properties narrow the parent's type", { )) }) +test_that("inheritance lets child properties narrow S7_object with external classes", { + dep := local_package( + External := new_class() + ) + Parent := new_class( + properties = list(x = S7_object), + package = NULL, + abstract = TRUE + ) + + Child := new_class( + parent = Parent, + properties = list( + x = new_property( + class = new_external_class("dep", "External"), + default = quote(dep$External()) + ) + ), + package = NULL + ) + + expect_s3_class(Child(x = dep$External())@x, "dep::External") +}) + test_that("inheritance lets child properties narrow with S4 inheritance", { S4PropertyParent := local_S4_class(slots = c(x = "numeric")) S4PropertyChild := local_S4_class(contains = "S4PropertyParent") diff --git a/tests/testthat/test-method-register.R b/tests/testthat/test-method-register.R index 2fe06880..857a3572 100644 --- a/tests/testthat/test-method-register.R +++ b/tests/testthat/test-method-register.R @@ -119,6 +119,18 @@ test_that("method registration defers external classes in union signatures", { expect_length(S7_methods_table("pkg"), 1) }) +test_that("method unregistration removes deferred external-class methods", { + pkg := local_package( + foo := new_generic("x"), + ext := new_external_class("notloaded.pkg"), + method(foo, ext) <- function(x) "x" + ) + expect_length(S7_methods_table("pkg"), 1) + + evalq(method(foo, ext) <- NULL, pkg) + expect_length(S7_methods_table("pkg"), 0) +}) + test_that("method unregistration removes an S7 method via NULL assignment", { foo := new_generic("x") method(foo, class_character) <- function(x) "c" diff --git a/vignettes/packages.Rmd b/vignettes/packages.Rmd index c58d198d..2789a7ad 100644 --- a/vignettes/packages.Rmd +++ b/vignettes/packages.Rmd @@ -84,17 +84,25 @@ When the suggested package is loaded, S7 will register the method automatically ### 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: +Conversely, you may want to register a method for one of your own generics, dispatching on an S7 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) { ... } +TheirClass <- new_external_class("theirpkg", "TheirClass") +method(my_generic, TheirClass) <- function(x) { ... } ``` When the suggested package is loaded, S7 will register the method automatically (via `S7_on_load()` as described above). +`new_external_class()` is only for S7 classes. For S3 classes from suggested packages, use `new_S3_class()`: + +```{r, eval = FALSE} +# In your package +ggplot <- new_S3_class("ggplot") +method(my_generic, ggplot) <- function(x) { ... } +``` + ## Backward compatibility ### S3 From f0981bd30ac6de2cc902e7c267c9c267bc583a15 Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Thu, 18 Jun 2026 18:54:20 -0400 Subject: [PATCH 02/42] Fix deferred external class method unregistration --- R/external-generic.R | 34 +++++++++++++++++++++++++++++++++- R/method-register.R | 6 ++---- tests/testthat/test-hooks.R | 35 +++++++++++++++++++++++++++++++++++ 3 files changed, 70 insertions(+), 5 deletions(-) diff --git a/R/external-generic.R b/R/external-generic.R index 1537620a..98745661 100644 --- a/R/external-generic.R +++ b/R/external-generic.R @@ -166,12 +166,44 @@ external_methods_remove <- function(package, generic, signature) { } keep <- !vlapply(tbl, function(x) { - identical(x$generic, generic) && identical(x$signature, signature) + identical(x$generic, generic) && + external_method_signature_matches(x$signature, signature) }) S7_methods_table(package) <- tbl[keep] invisible() } +external_method_signature_matches <- function(x, y) { + if (identical(x, y)) { + return(TRUE) + } + if (length(x) != length(y)) { + return(FALSE) + } + + all(vlapply(seq_along(x), function(i) { + external_method_class_matches(x[[i]], y[[i]]) + })) +} + +external_method_class_matches <- function(x, y) { + if (identical(x, y)) { + return(TRUE) + } + + if (is_external_class(x) && is_class(y)) { + return(is_external_class_match(y, x)) + } + if (is_class(x) && is_external_class(y)) { + return(is_external_class_match(x, y)) + } + if (is_union(x) && is_union(y)) { + return(external_method_signature_matches(x$classes, y$classes)) + } + + FALSE +} + # Store external methods in an attribute of the S3 methods table since # this mutable object is present in all packages. diff --git a/R/method-register.R b/R/method-register.R index 280da03e..713c5444 100644 --- a/R/method-register.R +++ b/R/method-register.R @@ -168,10 +168,8 @@ 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. - local <- !is.null(package) && is_local_generic(generic, package) - deferred <- !is.null(package) && - (!local || signature_has_external_class(signature)) - if (deferred) { + if (!is.null(package)) { + local <- is_local_generic(generic, package) external <- as_external_generic(generic, env) external_methods_remove(package, external, signature) if (!local) { diff --git a/tests/testthat/test-hooks.R b/tests/testthat/test-hooks.R index 1f8a62ce..cb6682cc 100644 --- a/tests/testthat/test-hooks.R +++ b/tests/testthat/test-hooks.R @@ -30,6 +30,41 @@ test_that("S7_on_load() registers methods dispatching on an external class", { expect_equal(downstream$own_generic(upstream$Foo()), "from external class") }) +test_that("method<- NULL removes deferred methods for resolved external classes", { + upstream <- local_package( + "upstream_resolved_external_unregister", + Foo := new_class() + ) + downstream <- local_package( + "downstream_resolved_external_unregister", + .onLoad <- function(...) S7_on_load(), + .onUnload <- function(...) S7_on_unload(), + own_generic := new_generic("x"), + Foo := new_external_class( + package = "upstream_resolved_external_unregister" + ), + method(own_generic, Foo) <- function(x) "from external class" + ) + downstream$.onLoad() + downstream$ResolvedFoo <- upstream$Foo + + expect_equal(downstream$own_generic(upstream$Foo()), "from external class") + expect_length(S7_methods_table("downstream_resolved_external_unregister"), 1) + + evalq(method(own_generic, ResolvedFoo) <- NULL, downstream) + expect_length(S7_methods_table("downstream_resolved_external_unregister"), 0) + expect_error( + downstream$own_generic(upstream$Foo()), + class = "S7_error_method_not_found" + ) + + downstream$.onLoad() + expect_error( + downstream$own_generic(upstream$Foo()), + class = "S7_error_method_not_found" + ) +}) + test_that("S7_on_unload() unregisters methods and removes hooks", { upstream <- local_package("upstream", gen := new_generic("x")) downstream <- local_package( From 0dea2814aad269e39b47c1594c1a746e19592bc7 Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Thu, 18 Jun 2026 19:03:15 -0400 Subject: [PATCH 03/42] Resolve external class signatures on unload --- R/hooks.R | 10 +++++++++- tests/testthat/test-hooks.R | 23 +++++++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/R/hooks.R b/R/hooks.R index 468d8f3c..a251013a 100644 --- a/R/hooks.R +++ b/R/hooks.R @@ -80,9 +80,17 @@ S7_on_unload_ <- function(env) { generic <- as_generic(generic) # Methods registered for S3 and S4 generics can't be unregistered yet if (is_S7_generic(generic)) { + signature <- x$signature + if (signature_has_external_class(signature)) { + deps <- signature_external_deps(signature) + if (!all(vlapply(deps, dep_available))) { + next + } + signature <- resolve_signature(signature) + } unregister_own_S7_method( generic, - x$signature, + signature, x$method, package ) diff --git a/tests/testthat/test-hooks.R b/tests/testthat/test-hooks.R index cb6682cc..efb6f2a4 100644 --- a/tests/testthat/test-hooks.R +++ b/tests/testthat/test-hooks.R @@ -30,6 +30,29 @@ test_that("S7_on_load() registers methods dispatching on an external class", { expect_equal(downstream$own_generic(upstream$Foo()), "from external class") }) +test_that("S7_on_unload() unregisters methods dispatching on an external class", { + upstream <- local_package( + "upstream_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 = "upstream_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("method<- NULL removes deferred methods for resolved external classes", { upstream <- local_package( "upstream_resolved_external_unregister", From 8068cd4c328ae0f91b461c9d2cc73acf574e7082 Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Thu, 18 Jun 2026 19:14:36 -0400 Subject: [PATCH 04/42] Fix external class validation and cleanup --- R/class-spec.R | 5 ++++- R/external-generic.R | 24 +++++++++++++++++++- R/method-register.R | 4 ++++ tests/testthat/_snaps/external-class.md | 9 ++++++++ tests/testthat/_snaps/method-register.md | 24 ++++++++++++++++++++ tests/testthat/test-external-class.R | 28 ++++++++++++++++++++++++ tests/testthat/test-method-register.R | 25 +++++++++++++++++++++ 7 files changed, 117 insertions(+), 2 deletions(-) diff --git a/R/class-spec.R b/R/class-spec.R index e8c59af5..0f536ca6 100644 --- a/R/class-spec.R +++ b/R/class-spec.R @@ -204,7 +204,10 @@ 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 = return(class_validate( + resolve_external_class_req(class), + object + )), NULL ) diff --git a/R/external-generic.R b/R/external-generic.R index 98745661..e404df2d 100644 --- a/R/external-generic.R +++ b/R/external-generic.R @@ -198,12 +198,34 @@ external_method_class_matches <- function(x, y) { return(is_external_class_match(x, y)) } if (is_union(x) && is_union(y)) { - return(external_method_signature_matches(x$classes, y$classes)) + return(external_method_union_matches(x$classes, y$classes)) } FALSE } +external_method_union_matches <- function(x, y) { + if (length(x) != length(y)) { + return(FALSE) + } + + matched <- rep(FALSE, length(y)) + for (xi in x) { + hits <- which( + !matched & + vlapply(y, function(yi) { + external_method_class_matches(xi, yi) + }) + ) + if (length(hits) == 0) { + return(FALSE) + } + matched[[hits[[1]]]] <- TRUE + } + + TRUE +} + # Store external methods in an attribute of the S3 methods table since # this mutable object is present in all packages. diff --git a/R/method-register.R b/R/method-register.R index 713c5444..1970b6b2 100644 --- a/R/method-register.R +++ b/R/method-register.R @@ -157,6 +157,10 @@ unregister_method <- function( ) } + if (is.null(package) && signature_has_external_class(signature)) { + signature <- resolve_signature(signature) + } + # Unregister in current session if (is_S7_generic(generic)) { unregister_S7_method(generic, signature) diff --git a/tests/testthat/_snaps/external-class.md b/tests/testthat/_snaps/external-class.md index a37b1a22..07f61fa5 100644 --- a/tests/testthat/_snaps/external-class.md +++ b/tests/testthat/_snaps/external-class.md @@ -52,3 +52,12 @@ ! 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 + diff --git a/tests/testthat/_snaps/method-register.md b/tests/testthat/_snaps/method-register.md index 3873f0ea..56303fc4 100644 --- a/tests/testthat/_snaps/method-register.md +++ b/tests/testthat/_snaps/method-register.md @@ -25,6 +25,30 @@ * An S4 class object * A base class +# method registration resolves external classes outside packages + + Code + env$g(S7_object()) + Condition + Error: + ! Can't find method for `g()`. + +# method unregistration removes deferred unions regardless of order + + Code + downstream$foo(upstream$Ext()) + Condition + Error: + ! Can't find method for `foo()`. + +--- + + Code + downstream$foo(upstream$Ext()) + Condition + Error: + ! Can't find method for `foo()`. + # method unregistration removes an S7 method via NULL assignment Code diff --git a/tests/testthat/test-external-class.R b/tests/testthat/test-external-class.R index d25fd0dc..ad5a27cf 100644 --- a/tests/testthat/test-external-class.R +++ b/tests/testthat/test-external-class.R @@ -70,6 +70,34 @@ test_that("external class works as a property type for self-reference", { expect_snapshot(error = TRUE, Tree(label = "bad", child = 1)) }) +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" + } + } + ) + ) + Holder := new_class( + properties = list( + child = new_property( + class = new_external_class("dep", "Ext"), + default = quote(dep$Ext(x = 0L)) + ) + ) + ) + + invalid <- valid_implicitly(dep$Ext(x = 1L), function(self) { + self@x <- -1L + self + }) + + expect_snapshot(Holder(child = invalid), error = TRUE) +}) + test_that("external class works for mutually recursive classes", { ClassOne := new_class( package = "mypkg", diff --git a/tests/testthat/test-method-register.R b/tests/testthat/test-method-register.R index 857a3572..e3848ef7 100644 --- a/tests/testthat/test-method-register.R +++ b/tests/testthat/test-method-register.R @@ -75,6 +75,9 @@ test_that("method registration resolves external classes outside packages", { evalq(method(g, ext) <- f, env) expect_equal(env$g(S7_object()), "external") + + evalq(method(g, ext) <- NULL, env) + expect_snapshot(env$g(S7_object()), error = TRUE) }) test_that("method registration returns a strippable sentinel for foreign generics in a package (#364)", { @@ -131,6 +134,28 @@ test_that("method unregistration removes deferred external-class methods", { expect_length(S7_methods_table("pkg"), 0) }) +test_that("method unregistration removes deferred unions regardless of order", { + upstream <- local_package( + "upstream_external_union_unregister", + Ext := new_class() + ) + downstream <- local_package( + "downstream_external_union_unregister", + .onLoad <- function(...) S7_on_load(), + foo := new_generic("x"), + Ext := new_external_class("upstream_external_union_unregister"), + method(foo, NULL | Ext) <- function(x) "external" + ) + downstream$.onLoad() + expect_equal(downstream$foo(upstream$Ext()), "external") + + evalq(method(foo, Ext | NULL) <- NULL, downstream) + expect_snapshot(downstream$foo(upstream$Ext()), error = TRUE) + + downstream$.onLoad() + expect_snapshot(downstream$foo(upstream$Ext()), error = TRUE) +}) + test_that("method unregistration removes an S7 method via NULL assignment", { foo := new_generic("x") method(foo, class_character) <- function(x) "c" From 6e51807dd7b53a53100acf8cee60f34df996c4aa Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Thu, 18 Jun 2026 19:32:12 -0400 Subject: [PATCH 05/42] Fix external-class method hook cleanup --- R/external-generic.R | 4 ++ R/hooks.R | 71 ++++++++++++++++++++++++++++++------ tests/testthat/helper.R | 10 ++++- tests/testthat/test-hooks.R | 73 +++++++++++++++++++++++++++++++++++++ 4 files changed, 144 insertions(+), 14 deletions(-) diff --git a/R/external-generic.R b/R/external-generic.R index e404df2d..28592234 100644 --- a/R/external-generic.R +++ b/R/external-generic.R @@ -169,7 +169,11 @@ external_methods_remove <- function(package, generic, signature) { identical(x$generic, generic) && external_method_signature_matches(x$signature, signature) }) + removed <- tbl[!keep] S7_methods_table(package) <- tbl[keep] + for (x in removed) { + hooks_remove_method(package, x) + } invisible() } diff --git a/R/hooks.R b/R/hooks.R index a251013a..d9c74935 100644 --- a/R/hooks.R +++ b/R/hooks.R @@ -81,11 +81,10 @@ S7_on_unload_ <- function(env) { # Methods registered for S3 and S4 generics can't be unregistered yet if (is_S7_generic(generic)) { signature <- x$signature - if (signature_has_external_class(signature)) { - deps <- signature_external_deps(signature) - if (!all(vlapply(deps, dep_available))) { - next - } + if ( + signature_has_external_class(signature) && + all(vlapply(signature_external_deps(signature), dep_available)) + ) { signature <- resolve_signature(signature) } unregister_own_S7_method( @@ -125,7 +124,7 @@ hook_add <- function(package, x) { deps <- method_deps(x$generic, x$signature) register <- registrar(deps, x$generic, x$signature, x$method, ns) - hook <- S7_hook(register, package) + hook <- S7_hook(register, package, x$generic, x$signature) pkgs <- method_deps_packages(deps) for (pkg in pkgs) { @@ -144,10 +143,25 @@ hooks_remove <- function(package) { hooks_packages(package) <- character() invisible() } -hook_remove <- function(package, pkg) { + +hooks_remove_method <- function(package, x) { + deps <- method_deps(x$generic, x$signature) + for (pkg in method_deps_packages(deps)) { + hook_remove(package, pkg, x$generic, x$signature) + } + invisible() +} + +hook_remove <- function(package, pkg, generic = NULL, signature = NULL) { event <- packageEvent(pkg, "onLoad") hooks <- getHook(event) - ours <- vlapply(hooks, is_S7_hook, package = package) + ours <- vlapply( + hooks, + is_S7_hook, + package = package, + generic = generic, + signature = signature + ) if (any(ours)) { setHook(event, hooks[!ours], action = "replace") } @@ -178,16 +192,34 @@ is_generic_sentinel <- function(x) inherits(x, "S7_generic_sentinel") # Tag our hooks so we can remove later -S7_hook <- function(fun, package) { +S7_hook <- function(fun, package, generic = NULL, signature = NULL) { attr(fun, "S7_package") <- package + attr(fun, "S7_generic") <- generic + attr(fun, "S7_signature") <- signature class(fun) <- "S7_hook" fun } -is_S7_hook <- function(x, package = NULL) { +is_S7_hook <- function(x, package = NULL, generic = NULL, signature = NULL) { if (!inherits(x, "S7_hook")) { return(FALSE) } - is.null(package) || identical(attr(x, "S7_package", TRUE), package) + if (!is.null(package) && !identical(attr(x, "S7_package", TRUE), package)) { + return(FALSE) + } + if (!is.null(generic) && !identical(attr(x, "S7_generic", TRUE), generic)) { + return(FALSE) + } + if (!is.null(signature)) { + hook_signature <- attr(x, "S7_signature", TRUE) + if ( + is.null(hook_signature) || + !external_method_signature_matches(hook_signature, signature) + ) { + return(FALSE) + } + } + + TRUE } hooks_packages <- function(package) { @@ -215,9 +247,24 @@ unregister_own_S7_method <- function( own <- S7_method_for_signature(method, generic, sig, package = package) # Unload only removes the method this package currently owns. It does not # remember or restore any method that was overwritten during loading. - if (identical(current, own)) { + if (identical(current, own) || is_own_S7_method(current, method, package)) { generic_remove_method(generic, sig) } } invisible() } + +is_own_S7_method <- function(current, method, package = NULL) { + if (!is.function(current)) { + return(FALSE) + } + if ( + !is.null(package) && !identical(attr(current, "S7_package", TRUE), package) + ) { + return(FALSE) + } + + current_method <- current + attributes(current_method) <- attributes(method) + identical(current_method, method) +} diff --git a/tests/testthat/helper.R b/tests/testthat/helper.R index 912c657d..5a34c620 100644 --- a/tests/testthat/helper.R +++ b/tests/testthat/helper.R @@ -68,8 +68,14 @@ local_package <- function(name, ..., frame = parent.frame()) { # register namespace so asNamespace(pkg) works internal <- get(".Internal", envir = baseenv()) internal(registerNamespace(name, ns)) - defer(internal(unregisterNamespace(name)), frame = frame) - defer(S7_on_unload_(ns), frame = frame) + defer( + if (isNamespaceLoaded(name)) internal(unregisterNamespace(name)), + frame = frame + ) + defer( + if (isNamespaceLoaded(name)) S7_on_unload_(ns), + frame = frame + ) for (expr in eval(substitute(alist(...)))) { eval(expr, ns) diff --git a/tests/testthat/test-hooks.R b/tests/testthat/test-hooks.R index efb6f2a4..87a55f64 100644 --- a/tests/testthat/test-hooks.R +++ b/tests/testthat/test-hooks.R @@ -53,6 +53,42 @@ test_that("S7_on_unload() unregisters methods dispatching on an external class", ) }) +test_that("S7_on_unload() unregisters external-class methods after class unload", { + generic_pkg <- local_package( + "upstream_external_unloaded_class_generic", + gen := new_generic("x") + ) + class_pkg <- local_package( + "upstream_external_unloaded_class", + Foo := new_class() + ) + downstream <- local_package( + "downstream_external_unloaded_class", + .onLoad <- function(...) S7_on_load(), + .onUnload <- function(...) S7_on_unload(), + gen := new_external_generic( + package = "upstream_external_unloaded_class_generic", + dispatch_args = "x" + ), + Foo := new_external_class( + package = "upstream_external_unloaded_class" + ), + method(gen, Foo) <- function(x) "from external class" + ) + downstream$.onLoad() + + obj <- class_pkg$Foo() + expect_equal(generic_pkg$gen(obj), "from external class") + expect_equal(nrow(S7_methods(generic = generic_pkg$gen)), 1) + + unloadNamespace("upstream_external_unloaded_class") + expect_false(isNamespaceLoaded("upstream_external_unloaded_class")) + expect_equal(generic_pkg$gen(obj), "from external class") + + downstream$.onUnload() + expect_equal(nrow(S7_methods(generic = generic_pkg$gen)), 0) +}) + test_that("method<- NULL removes deferred methods for resolved external classes", { upstream <- local_package( "upstream_resolved_external_unregister", @@ -88,6 +124,43 @@ test_that("method<- NULL removes deferred methods for resolved external classes" ) }) +test_that("method<- NULL removes installed hooks for deferred external-class methods", { + upstream <- local_package( + "upstream_deferred_external_hook", + gen := new_generic("x") + ) + downstream <- local_package( + "downstream_deferred_external_hook", + .onLoad <- function(...) S7_on_load(), + .onUnload <- function(...) S7_on_unload(), + gen := new_external_generic( + package = "upstream_deferred_external_hook", + dispatch_args = "x" + ), + Ext := new_external_class( + package = "upstream_deferred_external_hook_class" + ), + method(gen, Ext) <- function(x) "from stale hook" + ) + downstream$.onLoad() + expect_equal(nrow(S7_methods(generic = upstream$gen)), 0) + + evalq(method(gen, Ext) <- NULL, downstream) + expect_length(S7_methods_table("downstream_deferred_external_hook"), 0) + + ext_pkg <- local_package( + "upstream_deferred_external_hook_class", + Ext := new_class() + ) + for (hook in package_hooks("upstream_deferred_external_hook_class")) { + hook() + } + + expect_equal(nrow(S7_methods(generic = upstream$gen)), 0) + expect_length(package_hooks("upstream_deferred_external_hook_class"), 0) + invisible(ext_pkg) +}) + test_that("S7_on_unload() unregisters methods and removes hooks", { upstream <- local_package("upstream", gen := new_generic("x")) downstream <- local_package( From 1c96c9a88b484dde8d808e7569e458c8d065e115 Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Thu, 18 Jun 2026 19:46:00 -0400 Subject: [PATCH 06/42] Fix external class dispatch handling --- R/class-spec.R | 12 ++++++++++-- tests/testthat/test-class-spec.R | 13 +++++++++++++ tests/testthat/test-external-class.R | 8 ++++++++ tests/testthat/test-inherits.R | 6 ++++++ 4 files changed, 37 insertions(+), 2 deletions(-) diff --git a/R/class-spec.R b/R/class-spec.R index 0f536ca6..99bfdc01 100644 --- a/R/class-spec.R +++ b/R/class-spec.R @@ -302,7 +302,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 = ", ")) + }, ) } @@ -317,7 +323,9 @@ class_inherits <- function(x, what) { S7_base = what$class == base_class(x), S7_union = any(vlapply(what$classes, class_inherits, x = x)), 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) || + class_inherits(x, resolve_external_class_req(what))), ) } diff --git a/tests/testthat/test-class-spec.R b/tests/testthat/test-class-spec.R index 74d77ef2..6e74b4c9 100644 --- a/tests/testthat/test-class-spec.R +++ b/tests/testthat/test-class-spec.R @@ -173,6 +173,19 @@ test_that("can work with S3 classes", { expect_equal(class_inherits(factor(), klass), FALSE) }) +test_that("external classes deparse as executable calls", { + ext <- new_external_class("foo", "Bar") + ext_versioned <- new_external_class("foo", "Bar", version = "1.0") + + expect_equal(class_deparse(ext), 'new_external_class("foo", "Bar")') + expect_equal( + class_deparse(ext_versioned), + 'new_external_class("foo", "Bar", version = "1.0")' + ) + expect_equal(eval(parse(text = class_deparse(ext))), ext) + expect_equal(eval(parse(text = class_deparse(ext_versioned))), ext_versioned) +}) + test_that("class_inherits() requires S3 classes to be contiguous and ordered", { klass <- new_S3_class(c("a", "b")) diff --git a/tests/testthat/test-external-class.R b/tests/testthat/test-external-class.R index ad5a27cf..571dbfe8 100644 --- a/tests/testthat/test-external-class.R +++ b/tests/testthat/test-external-class.R @@ -98,6 +98,14 @@ test_that("external class property validation reports validator errors", { expect_snapshot(Holder(child = invalid), error = TRUE) }) +test_that("external class property validation uses resolved dispatch", { + Holder := new_class( + properties = list(x = new_external_class("S7", "S7_object")) + ) + + expect_s3_class(Holder(x = S7_object())@x, "S7_object") +}) + test_that("external class works for mutually recursive classes", { ClassOne := new_class( package = "mypkg", diff --git a/tests/testthat/test-inherits.R b/tests/testthat/test-inherits.R index 73755a9d..a5550277 100644 --- a/tests/testthat/test-inherits.R +++ b/tests/testthat/test-inherits.R @@ -24,6 +24,12 @@ test_that("accepts any class specification (#556)", { # class_any expect_true(S7_inherits("anything", class_any)) + + # external class + expect_true(S7_inherits( + S7_object(), + new_external_class("S7", "S7_object") + )) }) test_that("checks that input is a class", { From 21083bd960d00a6a874ee93d5e4f95d6a8d0f7bd Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Thu, 18 Jun 2026 20:03:31 -0400 Subject: [PATCH 07/42] Fix deferred external class registration --- R/external-class.R | 6 ++-- R/hooks.R | 6 ++-- R/method-register.R | 8 +++++ man/new_external_class.Rd | 6 ++-- tests/testthat/_snaps/hooks.md | 18 +++++++++++ tests/testthat/_snaps/method-register.md | 10 ++++++ tests/testthat/test-hooks.R | 40 ++++++++++++++++++++++++ tests/testthat/test-method-register.R | 11 +++++++ 8 files changed, 95 insertions(+), 10 deletions(-) create mode 100644 tests/testthat/_snaps/hooks.md diff --git a/R/external-class.R b/R/external-class.R index f102762b..802141bb 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: +#' ExternalObject <- new_external_class("S7", "S7_object") +#' ExternalObject #' #' # Self-referential class: the `child` property can be another `tree`, #' # or `NULL` to terminate the chain. diff --git a/R/hooks.R b/R/hooks.R index d9c74935..9a5432fd 100644 --- a/R/hooks.R +++ b/R/hooks.R @@ -108,12 +108,10 @@ hooks_set_and_run <- function(package) { pkgs <- character() for (x in S7_methods_table(package)) { hook <- hook_add(package, x) + pkgs <- union(pkgs, hook$pkgs) + `hooks_packages<-`(package, pkgs) hook$run() - pkgs <- c(pkgs, hook$pkgs) } - - # Record packages with hooks so we can remove them on unload - hooks_packages(package) <- unique(pkgs) invisible() } diff --git a/R/method-register.R b/R/method-register.R index 1970b6b2..f078bbe9 100644 --- a/R/method-register.R +++ b/R/method-register.R @@ -93,6 +93,14 @@ register_method <- function( 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)) diff --git a/man/new_external_class.Rd b/man/new_external_class.Rd index b3e6b616..75cf4ba8 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: +ExternalObject <- new_external_class("S7", "S7_object") +ExternalObject # Self-referential class: the `child` property can be another `tree`, # or `NULL` to terminate the chain. diff --git a/tests/testthat/_snaps/hooks.md b/tests/testthat/_snaps/hooks.md new file mode 100644 index 00000000..725e663b --- /dev/null +++ b/tests/testthat/_snaps/hooks.md @@ -0,0 +1,18 @@ +# S7_on_load() doesn't duplicate hooks when registrars error + + Code + downstream$.onLoad() + Condition + Error: + ! Can't find external class : + * Packages 'hookclasspkg' doesn't contain 'Missing'. + +--- + + Code + downstream$.onLoad() + Condition + Error: + ! Can't find external class : + * Packages 'hookclasspkg' doesn't contain 'Missing'. + diff --git a/tests/testthat/_snaps/method-register.md b/tests/testthat/_snaps/method-register.md index 56303fc4..d5ef5010 100644 --- a/tests/testthat/_snaps/method-register.md +++ b/tests/testthat/_snaps/method-register.md @@ -33,6 +33,16 @@ Error: ! Can't find method for `g()`. +# method registration validates deferred external-class methods + + 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 deferred unions regardless of order Code diff --git a/tests/testthat/test-hooks.R b/tests/testthat/test-hooks.R index 87a55f64..63a1adf7 100644 --- a/tests/testthat/test-hooks.R +++ b/tests/testthat/test-hooks.R @@ -14,6 +14,46 @@ test_that("S7_on_load() doesn't accumulate hooks across repeated loads", { expect_length(package_hooks("upstream"), 1) }) +test_that("S7_on_load() doesn't duplicate hooks when registrars error", { + upstream_generic <- local_package( + "hookgenericpkg", + gen := new_generic("x") + ) + upstream_class <- local_package( + "hookclasspkg", + Real := new_class() + ) + downstream <- local_package( + "downstreamerrorhook", + .onLoad <- function(...) S7_on_load(), + gen <- new_external_generic( + package = "hookgenericpkg", + name = "gen", + dispatch_args = "x" + ), + Missing <- new_external_class( + package = "hookclasspkg", + name = "Missing" + ), + method(gen, Missing) <- function(x) "dispatched" + ) + + expect_snapshot(downstream$.onLoad(), error = TRUE) + expect_length(package_hooks("hookgenericpkg"), 1) + expect_length(package_hooks("hookclasspkg"), 1) + + expect_snapshot(downstream$.onLoad(), error = TRUE) + expect_length(package_hooks("hookgenericpkg"), 1) + expect_length(package_hooks("hookclasspkg"), 1) + + evalq(method(gen, Missing) <- NULL, downstream) + expect_length(package_hooks("hookgenericpkg"), 0) + expect_length(package_hooks("hookclasspkg"), 0) + + invisible(upstream_generic) + invisible(upstream_class) +}) + test_that("S7_on_load() registers methods dispatching on an external class", { upstream := local_package( Foo := new_class() diff --git a/tests/testthat/test-method-register.R b/tests/testthat/test-method-register.R index e3848ef7..28c9ff5e 100644 --- a/tests/testthat/test-method-register.R +++ b/tests/testthat/test-method-register.R @@ -122,6 +122,17 @@ test_that("method registration defers external classes in union signatures", { expect_length(S7_methods_table("pkg"), 1) }) +test_that("method registration validates deferred external-class methods", { + expect_snapshot(error = TRUE, { + local_package( + "pkg_invalid_deferred_external_class_method", + foo := new_generic("x"), + ext := new_external_class("notloaded.pkg"), + method(foo, ext) <- function(y) "x" + ) + }) +}) + test_that("method unregistration removes deferred external-class methods", { pkg := local_package( foo := new_generic("x"), From 3b7883e25ec8fffd80fd553b4b11f8a7aea2e0b8 Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Thu, 18 Jun 2026 20:19:39 -0400 Subject: [PATCH 08/42] Resolve external signatures on method unregister --- R/external-class.R | 7 +++++++ R/hooks.R | 2 +- R/method-register.R | 12 +++++++++--- tests/testthat/_snaps/method-register.md | 16 ++++++++++++++++ tests/testthat/test-method-register.R | 18 ++++++++++++++++++ 5 files changed, 51 insertions(+), 4 deletions(-) diff --git a/R/external-class.R b/R/external-class.R index 802141bb..1f48b00c 100644 --- a/R/external-class.R +++ b/R/external-class.R @@ -99,6 +99,13 @@ signature_external_deps <- function(signature) { flatten_external_deps(lapply(signature, class_external_deps)) } +signature_external_deps_resolvable <- function(signature) { + deps <- signature_external_deps(signature) + 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) } diff --git a/R/hooks.R b/R/hooks.R index 9a5432fd..e5650183 100644 --- a/R/hooks.R +++ b/R/hooks.R @@ -83,7 +83,7 @@ S7_on_unload_ <- function(env) { signature <- x$signature if ( signature_has_external_class(signature) && - all(vlapply(signature_external_deps(signature), dep_available)) + signature_external_deps_resolvable(signature) ) { signature <- resolve_signature(signature) } diff --git a/R/method-register.R b/R/method-register.R index f078bbe9..0a285277 100644 --- a/R/method-register.R +++ b/R/method-register.R @@ -165,13 +165,19 @@ unregister_method <- function( ) } - if (is.null(package) && signature_has_external_class(signature)) { - signature <- resolve_signature(signature) + unregister_signature <- signature + if (signature_has_external_class(signature)) { + if ( + is.null(package) || + signature_external_deps_resolvable(signature) + ) { + 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)) { diff --git a/tests/testthat/_snaps/method-register.md b/tests/testthat/_snaps/method-register.md index d5ef5010..bb5abe0d 100644 --- a/tests/testthat/_snaps/method-register.md +++ b/tests/testthat/_snaps/method-register.md @@ -43,6 +43,22 @@ Error in `method<-`: ! foo() dispatches on `x`, but foo() has arguments `y`. +# method unregistration resolves loaded external-class methods in packages + + Code + downstream$foo(S7_object()) + Condition + Error: + ! Can't find method for `foo()`. + +--- + + Code + downstream$foo(S7_object()) + Condition + Error: + ! Can't find method for `foo()`. + # method unregistration removes deferred unions regardless of order Code diff --git a/tests/testthat/test-method-register.R b/tests/testthat/test-method-register.R index 28c9ff5e..cfc46e60 100644 --- a/tests/testthat/test-method-register.R +++ b/tests/testthat/test-method-register.R @@ -145,6 +145,24 @@ test_that("method unregistration removes deferred external-class methods", { expect_length(S7_methods_table("pkg"), 0) }) +test_that("method unregistration resolves loaded external-class methods in packages", { + downstream <- local_package( + "downstream_external_unregister_resolve", + .onLoad <- function(...) S7_on_load(), + foo := new_generic("x"), + Ext <- new_external_class("S7", "S7_object"), + method(foo, Ext) <- function(x) "external" + ) + downstream$.onLoad() + expect_equal(downstream$foo(S7_object()), "external") + + evalq(method(foo, Ext) <- NULL, downstream) + expect_snapshot(downstream$foo(S7_object()), error = TRUE) + + downstream$.onLoad() + expect_snapshot(downstream$foo(S7_object()), error = TRUE) +}) + test_that("method unregistration removes deferred unions regardless of order", { upstream <- local_package( "upstream_external_union_unregister", From d306406400e2fd41989f07032a64e1aca343a1a1 Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Thu, 18 Jun 2026 20:33:25 -0400 Subject: [PATCH 09/42] Check external class versions before string match --- R/class-spec.R | 9 +++++++-- tests/testthat/_snaps/external-class.md | 18 ++++++++++++++++++ tests/testthat/test-external-class.R | 18 ++++++++++++++++++ 3 files changed, 43 insertions(+), 2 deletions(-) diff --git a/R/class-spec.R b/R/class-spec.R index 99bfdc01..75b80421 100644 --- a/R/class-spec.R +++ b/R/class-spec.R @@ -324,8 +324,13 @@ class_inherits <- function(x, what) { S7_union = any(vlapply(what$classes, class_inherits, x = x)), S7_S3 = !isS4(x) && class_dispatch_extends(what$class, class(x)), S7_external = inherits(x, "S7_object") && - (inherits(x, what$class_name) || - class_inherits(x, resolve_external_class_req(what))), + { + if (is.null(what$version) && inherits(x, what$class_name)) { + TRUE + } else { + class_inherits(x, resolve_external_class_req(what)) + } + }, ) } diff --git a/tests/testthat/_snaps/external-class.md b/tests/testthat/_snaps/external-class.md index 07f61fa5..a931aa3a 100644 --- a/tests/testthat/_snaps/external-class.md +++ b/tests/testthat/_snaps/external-class.md @@ -61,3 +61,21 @@ ! 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. + +--- + + Code + Holder(x = versioned_pkg$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/test-external-class.R b/tests/testthat/test-external-class.R index 571dbfe8..0994a4eb 100644 --- a/tests/testthat/test-external-class.R +++ b/tests/testthat/test-external-class.R @@ -132,6 +132,24 @@ test_that("class_inherits() works for external class", { expect_false(class_inherits(NULL, ec)) }) +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" + ) + + expect_snapshot(error = TRUE, S7_inherits(versioned_pkg$Foo(), Foo)) + + Holder := new_class( + properties = list(x = NULL | Foo) + ) + expect_snapshot(error = TRUE, Holder(x = versioned_pkg$Foo())) +}) + test_that("method_deps() collects the generic and external classes", { gen <- new_external_generic("foo", "bar", "x") sig <- list( From 79042550b4776b5113bfd7c118a6f4b870cef0cb Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Thu, 18 Jun 2026 22:59:51 -0400 Subject: [PATCH 10/42] Remove stale S7 hooks after failed loads --- R/hooks.R | 13 +++++++++--- tests/testthat/_snaps/hooks.md | 18 ++++++++++++++++ tests/testthat/test-hooks.R | 38 ++++++++++++++++++++++++++++++++++ 3 files changed, 66 insertions(+), 3 deletions(-) diff --git a/R/hooks.R b/R/hooks.R index e5650183..98823bee 100644 --- a/R/hooks.R +++ b/R/hooks.R @@ -132,10 +132,17 @@ hook_add <- function(package, x) { 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 all of our hooks for `package`. Start with the recorded package +# events, then re-derive from deferred methods in case loading failed and R +# discarded our record while leaving the global hooks installed. hooks_remove <- function(package) { - for (pkg in hooks_packages(package)) { + pkgs <- hooks_packages(package) + for (x in S7_methods_table(package)) { + deps <- method_deps(x$generic, x$signature) + pkgs <- union(pkgs, method_deps_packages(deps)) + } + + for (pkg in pkgs) { hook_remove(package, pkg) } hooks_packages(package) <- character() diff --git a/tests/testthat/_snaps/hooks.md b/tests/testthat/_snaps/hooks.md index 725e663b..fe7f1429 100644 --- a/tests/testthat/_snaps/hooks.md +++ b/tests/testthat/_snaps/hooks.md @@ -16,3 +16,21 @@ ! Can't find external class : * Packages 'hookclasspkg' doesn't contain 'Missing'. +# S7_on_load() removes stale hooks when hook records are lost + + Code + downstream$.onLoad() + Condition + Error: + ! Can't find external class : + * Packages 'hooklostclasspkg' doesn't contain 'Missing'. + +--- + + Code + downstream$.onLoad() + Condition + Error: + ! Can't find external class : + * Packages 'hooklostclasspkg' doesn't contain 'Missing'. + diff --git a/tests/testthat/test-hooks.R b/tests/testthat/test-hooks.R index 63a1adf7..2191cc5f 100644 --- a/tests/testthat/test-hooks.R +++ b/tests/testthat/test-hooks.R @@ -54,6 +54,44 @@ test_that("S7_on_load() doesn't duplicate hooks when registrars error", { invisible(upstream_class) }) +test_that("S7_on_load() removes stale hooks when hook records are lost", { + upstream_generic <- local_package( + "hooklostgenericpkg", + gen := new_generic("x") + ) + upstream_class <- local_package( + "hooklostclasspkg", + Real := new_class() + ) + downstream <- local_package( + "hooklostdownstream", + .onLoad <- function(...) S7_on_load(), + gen <- new_external_generic( + package = "hooklostgenericpkg", + name = "gen", + dispatch_args = "x" + ), + Missing <- new_external_class( + package = "hooklostclasspkg", + name = "Missing" + ), + method(gen, Missing) <- function(x) "dispatched" + ) + + expect_snapshot(downstream$.onLoad(), error = TRUE) + expect_length(package_hooks("hooklostgenericpkg"), 1) + expect_length(package_hooks("hooklostclasspkg"), 1) + + `hooks_packages<-`("hooklostdownstream", character()) + + expect_snapshot(downstream$.onLoad(), error = TRUE) + expect_length(package_hooks("hooklostgenericpkg"), 1) + expect_length(package_hooks("hooklostclasspkg"), 1) + + invisible(upstream_generic) + invisible(upstream_class) +}) + test_that("S7_on_load() registers methods dispatching on an external class", { upstream := local_package( Foo := new_class() From 9969cc72126d0792327225802ecc0eeb7cfd9032 Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Thu, 18 Jun 2026 23:14:38 -0400 Subject: [PATCH 11/42] Reject package-less external class matches --- R/external-class.R | 5 ++++- tests/testthat/_snaps/external-class.md | 9 +++++++++ tests/testthat/test-external-class.R | 9 +++++++++ 3 files changed, 22 insertions(+), 1 deletion(-) diff --git a/R/external-class.R b/R/external-class.R index 1f48b00c..1bb38d30 100644 --- a/R/external-class.R +++ b/R/external-class.R @@ -170,8 +170,11 @@ find_external_class <- function(x) { is_external_class_match <- function(obj, x) { is_class(obj) && (identical(S7_class_name(obj), x$class_name) || + (identical(x$package, "S7") && + identical(x$name, "S7_object") && + identical(obj, S7_object)) || (identical(obj@name, x$name) && - (is.null(obj@package) || identical(obj@package, x$package)))) + identical(obj@package, x$package))) } # Required resolution: errors if the external class can't be resolved (e.g. diff --git a/tests/testthat/_snaps/external-class.md b/tests/testthat/_snaps/external-class.md index a931aa3a..68d0d285 100644 --- a/tests/testthat/_snaps/external-class.md +++ b/tests/testthat/_snaps/external-class.md @@ -22,6 +22,15 @@ Output foo::Bar (>= 1.0) +# external class resolution rejects package-less classes + + Code + resolve_external_class_req(Foo) + Condition + Error: + ! Can't find external class : + * Packages 'pkg' doesn't contain 'Foo'. + # resolve_external_class_req() errors per failure mode Code diff --git a/tests/testthat/test-external-class.R b/tests/testthat/test-external-class.R index 0994a4eb..f9bf2f81 100644 --- a/tests/testthat/test-external-class.R +++ b/tests/testthat/test-external-class.R @@ -35,6 +35,15 @@ test_that("external class resolution uses the S7 class name", { expect_equal(S7_class_name(resolved), "pkg::named") }) +test_that("external class resolution rejects package-less classes", { + pkg := local_package( + Foo := new_class(package = NULL) + ) + Foo := new_external_class("pkg") + + expect_snapshot(error = TRUE, resolve_external_class_req(Foo)) +}) + test_that("resolve_external_class_req() errors per failure mode", { local_mocked_bindings(getNamespaceVersion = function(package) "1.0.0") expect_snapshot(error = TRUE, { From 609e234c4b34718fc996b9e335b30505e39f1363 Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Thu, 18 Jun 2026 23:29:03 -0400 Subject: [PATCH 12/42] Normalize external generic sentinels --- R/external-generic.R | 5 ++++- tests/testthat/test-method-register.R | 17 +++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/R/external-generic.R b/R/external-generic.R index 28592234..d4938c51 100644 --- a/R/external-generic.R +++ b/R/external-generic.R @@ -41,7 +41,10 @@ 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)) { + 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)) { diff --git a/tests/testthat/test-method-register.R b/tests/testthat/test-method-register.R index cfc46e60..cff52ebd 100644 --- a/tests/testthat/test-method-register.R +++ b/tests/testthat/test-method-register.R @@ -111,6 +111,23 @@ test_that("deferred external-class methods preserve sentinel for foreign generic expect_length(S7_methods_table("pkg"), 1) }) +test_that("deferred external-class methods match sentinel foreign generics", { + pkg := local_package( + gen := new_external_generic("notloaded.pkg", "x"), + ext := new_external_class("notloaded.pkg") + ) + + evalq(method(gen, ext) <- function(x) "first", pkg) + expect_s3_class(pkg$gen, "S7_generic_sentinel") + + evalq(method(gen, ext) <- function(x) "second", pkg) + expect_length(S7_methods_table("pkg"), 1) + expect_equal(S7_methods_table("pkg")[[1]]$method(NULL), "second") + + evalq(method(gen, ext) <- NULL, pkg) + expect_length(S7_methods_table("pkg"), 0) +}) + test_that("method registration defers external classes in union signatures", { pkg := local_package( foo := new_generic("x"), From 3ce2305a159946d0d39c0dcc81b345d12b6d618a Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Thu, 18 Jun 2026 23:48:14 -0400 Subject: [PATCH 13/42] Register runtime external-class methods --- R/hooks.R | 28 ++++++++++++++++++----- R/method-register.R | 6 +++++ tests/testthat/test-hooks.R | 44 +++++++++++++++++++++++++++++++++++++ 3 files changed, 72 insertions(+), 6 deletions(-) diff --git a/R/hooks.R b/R/hooks.R index 98823bee..4ff12af4 100644 --- a/R/hooks.R +++ b/R/hooks.R @@ -104,17 +104,20 @@ S7_on_unload_ <- function(env) { # packages is loaded in the future. hooks_set_and_run <- function(package) { hooks_remove(package) + `hooks_active<-`(package, TRUE) - pkgs <- character() for (x in S7_methods_table(package)) { - hook <- hook_add(package, x) - pkgs <- union(pkgs, hook$pkgs) - `hooks_packages<-`(package, pkgs) - hook$run() + hook_set_and_run(package, x) } invisible() } +hook_set_and_run <- function(package, x) { + hook <- hook_add(package, x) + `hooks_packages<-`(package, union(hooks_packages(package), hook$pkgs)) + hook$run() +} + # 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) { @@ -145,7 +148,8 @@ hooks_remove <- function(package) { for (pkg in pkgs) { hook_remove(package, pkg) } - hooks_packages(package) <- character() + `hooks_packages<-`(package, character()) + `hooks_active<-`(package, FALSE) invisible() } @@ -239,6 +243,18 @@ hooks_packages <- function(package) { invisible() } +hooks_active <- function(package) { + ns <- asNamespace(package) + tbl <- ns[[".__S3MethodsTable__."]] + isTRUE(attr(tbl, "S7hooks_active")) +} +`hooks_active<-` <- function(package, value) { + ns <- asNamespace(package) + tbl <- ns[[".__S3MethodsTable__."]] + attr(tbl, "S7hooks_active") <- value + invisible() +} + unregister_own_S7_method <- function( generic, signature, diff --git a/R/method-register.R b/R/method-register.R index 0a285277..bdfb4e9e 100644 --- a/R/method-register.R +++ b/R/method-register.R @@ -102,6 +102,12 @@ register_method <- function( ) } external_methods_add(package, generic_ext, signature, method) + if (hooks_active(package)) { + hook_set_and_run( + package, + list(generic = generic_ext, signature = signature, method = method) + ) + } if (!is_local_generic(generic, package)) { return(generic_sentinel(generic_ext)) } diff --git a/tests/testthat/test-hooks.R b/tests/testthat/test-hooks.R index 2191cc5f..89a52a7f 100644 --- a/tests/testthat/test-hooks.R +++ b/tests/testthat/test-hooks.R @@ -108,6 +108,50 @@ test_that("S7_on_load() registers methods dispatching on an external class", { expect_equal(downstream$own_generic(upstream$Foo()), "from external class") }) +test_that("method<- updates loaded external-class methods after S7_on_load()", { + upstream <- local_package( + "upstream_runtime_external_loaded", + Foo := new_class() + ) + downstream <- local_package( + "downstream_runtime_external_loaded", + .onLoad <- function(...) S7_on_load(), + own_generic := new_generic("x"), + Foo := new_external_class("upstream_runtime_external_loaded"), + method(own_generic, Foo) <- function(x) "first" + ) + downstream$.onLoad() + expect_equal(downstream$own_generic(upstream$Foo()), "first") + + expect_message( + evalq(method(own_generic, Foo) <- function(x) "second", downstream), + "Overwriting method" + ) + expect_equal(downstream$own_generic(upstream$Foo()), "second") +}) + +test_that("method<- hooks unloaded external-class methods after S7_on_load()", { + downstream <- local_package( + "downstream_runtime_external_unloaded", + .onLoad <- function(...) S7_on_load(), + own_generic := new_generic("x"), + Foo := new_external_class("upstream_runtime_external_unloaded") + ) + downstream$.onLoad() + + evalq(method(own_generic, Foo) <- function(x) "runtime", downstream) + expect_length(package_hooks("upstream_runtime_external_unloaded"), 1) + + upstream <- local_package( + "upstream_runtime_external_unloaded", + Foo := new_class() + ) + for (hook in package_hooks("upstream_runtime_external_unloaded")) { + hook() + } + expect_equal(downstream$own_generic(upstream$Foo()), "runtime") +}) + test_that("S7_on_unload() unregisters methods dispatching on an external class", { upstream <- local_package( "upstream_external_class_unload", From fe9bd61e462d72f82b50335a159e445332f88669 Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Fri, 19 Jun 2026 00:08:22 -0400 Subject: [PATCH 14/42] Fix deferred external class method registration --- R/external-class.R | 2 +- R/external-generic.R | 26 +++++++--- R/method-register.R | 9 +++- tests/testthat/test-hooks.R | 70 +++++++++++++++++++++++++++ tests/testthat/test-method-register.R | 35 ++++++++++++++ 5 files changed, 133 insertions(+), 9 deletions(-) diff --git a/R/external-class.R b/R/external-class.R index 1bb38d30..ff8027e2 100644 --- a/R/external-class.R +++ b/R/external-class.R @@ -132,7 +132,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 } diff --git a/R/external-generic.R b/R/external-generic.R index d4938c51..7fda65e6 100644 --- a/R/external-generic.R +++ b/R/external-generic.R @@ -100,7 +100,7 @@ registrar <- function(deps, generic, signature, method, env) { env function(...) { - if (!all(vlapply(deps, dep_available))) { + if (!dep_available(generic)) { return(invisible()) } @@ -109,8 +109,17 @@ registrar <- function(deps, generic, signature, method, env) { return(invisible()) } - signature <- resolve_signature(signature) - register_method(generic_fun, signature, method, env, package = NULL) + for (sig in flatten_signature(signature)) { + deps <- signature_external_deps(sig) + if (!all(vlapply(deps, dep_available))) { + next + } + + sig <- resolve_signature(sig) + register_method(generic_fun, sig, method, env, package = NULL) + } + + invisible() } } @@ -147,7 +156,7 @@ external_methods_add <- function( method ) { # Remove any existing entries - external_methods_remove(package, generic, signature) + removed <- external_methods_remove(package, generic, signature) entry <- list( generic = generic, @@ -159,13 +168,13 @@ external_methods_add <- function( append1(tbl) <- entry S7_methods_table(package) <- tbl - invisible() + invisible(removed) } external_methods_remove <- function(package, generic, signature) { tbl <- S7_methods_table(package) if (length(tbl) == 0) { - return(invisible()) + return(invisible(list())) } keep <- !vlapply(tbl, function(x) { @@ -177,7 +186,7 @@ external_methods_remove <- function(package, generic, signature) { for (x in removed) { hooks_remove_method(package, x) } - invisible() + invisible(removed) } external_method_signature_matches <- function(x, y) { @@ -204,6 +213,9 @@ external_method_class_matches <- function(x, y) { if (is_class(x) && is_external_class(y)) { return(is_external_class_match(x, y)) } + if (is_external_class(x) && is_external_class(y)) { + return(identical(x$class_name, y$class_name)) + } if (is_union(x) && is_union(y)) { return(external_method_union_matches(x$classes, y$classes)) } diff --git a/R/method-register.R b/R/method-register.R index bdfb4e9e..ad1db61b 100644 --- a/R/method-register.R +++ b/R/method-register.R @@ -101,7 +101,14 @@ register_method <- function( call = call ) } - external_methods_add(package, generic_ext, signature, method) + removed <- external_methods_add(package, generic_ext, signature, method) + if ( + is_S7_generic(generic) && !signature_external_deps_resolvable(signature) + ) { + for (x in removed) { + unregister_own_S7_method(generic, x$signature, x$method, package) + } + } if (hooks_active(package)) { hook_set_and_run( package, diff --git a/tests/testthat/test-hooks.R b/tests/testthat/test-hooks.R index 89a52a7f..b2c4f1cd 100644 --- a/tests/testthat/test-hooks.R +++ b/tests/testthat/test-hooks.R @@ -130,6 +130,37 @@ test_that("method<- updates loaded external-class methods after S7_on_load()", { expect_equal(downstream$own_generic(upstream$Foo()), "second") }) +test_that("method<- clears stale external-class methods before deferring", { + upstream <- local_package( + "upstream_runtime_external_replaced_unloaded", + Foo := new_class() + ) + downstream <- local_package( + "downstream_runtime_external_replaced_unloaded", + .onLoad <- function(...) S7_on_load(), + own_generic := new_generic(dispatch_args = "x"), + Foo := new_external_class( + package = "upstream_runtime_external_replaced_unloaded" + ), + method(own_generic, Foo) <- function(x) "first" + ) + downstream$.onLoad() + expect_equal(downstream$own_generic(upstream$Foo()), "first") + + unloadNamespace("upstream_runtime_external_replaced_unloaded") + expect_false(isNamespaceLoaded("upstream_runtime_external_replaced_unloaded")) + + evalq(method(own_generic, Foo) <- function(x) "second", downstream) + expect_equal(nrow(S7_methods(generic = downstream$own_generic)), 0) + + upstream <- local_package( + "upstream_runtime_external_replaced_unloaded", + Foo := new_class() + ) + downstream$.onLoad() + expect_equal(downstream$own_generic(upstream$Foo()), "second") +}) + test_that("method<- hooks unloaded external-class methods after S7_on_load()", { downstream <- local_package( "downstream_runtime_external_unloaded", @@ -152,6 +183,45 @@ test_that("method<- hooks unloaded external-class methods after S7_on_load()", { expect_equal(downstream$own_generic(upstream$Foo()), "runtime") }) +test_that("S7_on_load() registers available union arms independently", { + generic_pkg <- local_package( + "upstream_external_union_partial_generic", + gen := new_generic(dispatch_args = "x") + ) + upstream_a <- local_package( + "upstream_external_union_partial_a", + A := new_class() + ) + downstream <- local_package( + "downstream_external_union_partial", + .onLoad <- function(...) S7_on_load(), + gen <- new_external_generic( + package = "upstream_external_union_partial_generic", + name = "gen", + dispatch_args = "x" + ), + A := new_external_class( + package = "upstream_external_union_partial_a" + ), + B := new_external_class( + package = "upstream_external_union_partial_b" + ), + method(gen, A | B) <- function(x) "union" + ) + + downstream$.onLoad() + expect_equal(generic_pkg$gen(upstream_a$A()), "union") + expect_equal(nrow(S7_methods(generic = generic_pkg$gen)), 1) + + upstream_b <- local_package( + "upstream_external_union_partial_b", + B := new_class() + ) + downstream$.onLoad() + expect_equal(generic_pkg$gen(upstream_b$B()), "union") + expect_equal(nrow(S7_methods(generic = generic_pkg$gen)), 2) +}) + test_that("S7_on_unload() unregisters methods dispatching on an external class", { upstream <- local_package( "upstream_external_class_unload", diff --git a/tests/testthat/test-method-register.R b/tests/testthat/test-method-register.R index cff52ebd..37114a18 100644 --- a/tests/testthat/test-method-register.R +++ b/tests/testthat/test-method-register.R @@ -180,6 +180,41 @@ test_that("method unregistration resolves loaded external-class methods in packa expect_snapshot(downstream$foo(S7_object()), error = TRUE) }) +test_that("method unregistration ignores external class version constraints", { + upstream <- local_package( + "upstream_external_versioned_unregister", + Ext := new_class() + ) + downstream <- local_package( + "downstream_external_versioned_unregister", + .onLoad <- function(...) S7_on_load(), + foo := new_generic(dispatch_args = "x"), + ExtVersioned <- new_external_class( + package = "upstream_external_versioned_unregister", + name = "Ext", + version = "0.0.0" + ), + method(foo, ExtVersioned) <- function(x) "versioned" + ) + downstream$.onLoad() + expect_equal(downstream$foo(upstream$Ext()), "versioned") + + evalq( + { + Ext <- new_external_class( + package = "upstream_external_versioned_unregister", + name = "Ext" + ) + method(foo, Ext) <- NULL + }, + downstream + ) + expect_equal(nrow(S7_methods(generic = downstream$foo)), 0) + + downstream$.onLoad() + expect_equal(nrow(S7_methods(generic = downstream$foo)), 0) +}) + test_that("method unregistration removes deferred unions regardless of order", { upstream <- local_package( "upstream_external_union_unregister", From 11fad44ab1c914dd1bf9cdf2a5e7724dfb52939d Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Fri, 19 Jun 2026 00:18:45 -0400 Subject: [PATCH 15/42] Fix external generic hook replacement --- R/method-register.R | 6 ++++++ tests/testthat/test-hooks.R | 28 ++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/R/method-register.R b/R/method-register.R index ad1db61b..4f1e13ba 100644 --- a/R/method-register.R +++ b/R/method-register.R @@ -154,6 +154,12 @@ register_method <- function( # when the package is loaded if (!is.null(package) && !is_local_generic(generic, package)) { external_methods_add(package, external, signature, method) + if (hooks_active(package)) { + hook_set_and_run( + package, + list(generic = external, signature = signature, method = method) + ) + } return(generic_sentinel(external)) } diff --git a/tests/testthat/test-hooks.R b/tests/testthat/test-hooks.R index b2c4f1cd..e1c31b5a 100644 --- a/tests/testthat/test-hooks.R +++ b/tests/testthat/test-hooks.R @@ -183,6 +183,34 @@ test_that("method<- hooks unloaded external-class methods after S7_on_load()", { expect_equal(downstream$own_generic(upstream$Foo()), "runtime") }) +test_that("method<- rehooks unloaded external-generic methods after S7_on_load()", { + downstream <- local_package( + "downstream_runtime_external_generic_replaced", + .onLoad <- function(...) S7_on_load(), + Foo := new_class(), + gen <- new_external_generic( + package = "upstream_runtime_external_generic_replaced", + name = "gen", + dispatch_args = "x" + ), + method(gen, Foo) <- function(x) "first" + ) + downstream$.onLoad() + expect_length(package_hooks("upstream_runtime_external_generic_replaced"), 1) + + evalq(method(gen, Foo) <- function(x) "second", downstream) + expect_length(package_hooks("upstream_runtime_external_generic_replaced"), 1) + + upstream <- local_package( + "upstream_runtime_external_generic_replaced", + gen := new_generic(dispatch_args = "x") + ) + for (hook in package_hooks("upstream_runtime_external_generic_replaced")) { + hook() + } + expect_equal(upstream$gen(downstream$Foo()), "second") +}) + test_that("S7_on_load() registers available union arms independently", { generic_pkg <- local_package( "upstream_external_union_partial_generic", From ef9714a03c7fa8905ca885f97b686f95b9c89148 Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Fri, 19 Jun 2026 00:36:28 -0400 Subject: [PATCH 16/42] Short-circuit union class checks --- R/class-spec.R | 16 ++++++++++++++-- tests/testthat/test-class.R | 16 ++++++++++++++++ tests/testthat/test-external-class.R | 7 +++++++ 3 files changed, 37 insertions(+), 2 deletions(-) diff --git a/R/class-spec.R b/R/class-spec.R index 75b80421..1b2d6dbb 100644 --- a/R/class-spec.R +++ b/R/class-spec.R @@ -321,7 +321,14 @@ 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") && { @@ -348,7 +355,12 @@ 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 diff --git a/tests/testthat/test-class.R b/tests/testthat/test-class.R index 829c3289..99c2e47e 100644 --- a/tests/testthat/test-class.R +++ b/tests/testthat/test-class.R @@ -179,6 +179,22 @@ test_that("inheritance lets child properties narrow parent unions that include a )) }) +test_that("inheritance short-circuits matching union parent properties", { + Parent := new_class( + properties = list( + x = S7_object | new_external_class("S7testthatmissing", "Ext") + ), + package = NULL + ) + + expect_no_error(new_class( + "Child", + Parent, + properties = list(x = S7_object), + package = NULL + )) +}) + 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 f9bf2f81..abf17fe6 100644 --- a/tests/testthat/test-external-class.R +++ b/tests/testthat/test-external-class.R @@ -60,6 +60,13 @@ test_that("external class can be used as a union arm", { expect_length(u$classes, 2) }) +test_that("S7_inherits() short-circuits external union classes", { + Foo := new_class(package = NULL) + union <- Foo | new_external_class("S7testthatmissing", "Bar") + + expect_true(S7_inherits(Foo(), union)) +}) + test_that("external class works as a property type for self-reference", { Tree := new_class( package = "mypkg", From d910c296464263a336afeaf3b3abb18b5148aa1d Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Fri, 19 Jun 2026 00:52:14 -0400 Subject: [PATCH 17/42] Address external method registration reviews --- R/class-spec.R | 2 +- R/external-class.R | 10 +++++++++ R/external-generic.R | 10 ++++++--- tests/testthat/_snaps/hooks.md | 17 +++++++++++++++ tests/testthat/test-hooks.R | 36 ++++++++++++++++++++++++++++++++ tests/testthat/test-introspect.R | 12 +++++++++++ 6 files changed, 83 insertions(+), 4 deletions(-) diff --git a/R/class-spec.R b/R/class-spec.R index 1b2d6dbb..f159235a 100644 --- a/R/class-spec.R +++ b/R/class-spec.R @@ -282,7 +282,7 @@ class_register <- function(x) { S7 = S7_class_name(x), S7_base = x$class, S7_S3 = x$class[[1]], - S7_external = x$class_name, + S7_external = external_class_register(x), stop2("Unsupported class type.", call = NULL) ) } diff --git a/R/external-class.R b/R/external-class.R index ff8027e2..4e8953db 100644 --- a/R/external-class.R +++ b/R/external-class.R @@ -122,6 +122,16 @@ print.S7_external_class <- function(x, ...) { invisible(x) } +external_class_register <- function(x) { + stopifnot(is_external_class(x)) + + if (identical(x$package, "S7") && identical(x$name, "S7_object")) { + "S7_object" + } else { + x$class_name + } +} + dep_available <- function(dep) { isNamespaceLoaded(dep$package) && (is.null(dep$version) || getNamespaceVersion(dep$package) >= dep$version) diff --git a/R/external-generic.R b/R/external-generic.R index 7fda65e6..a733020e 100644 --- a/R/external-generic.R +++ b/R/external-generic.R @@ -109,13 +109,17 @@ registrar <- function(deps, generic, signature, method, env) { return(invisible()) } + signatures <- list() for (sig in flatten_signature(signature)) { - deps <- signature_external_deps(sig) - if (!all(vlapply(deps, dep_available))) { + sig_deps <- signature_external_deps(sig) + if (!all(vlapply(sig_deps, dep_available))) { next } - sig <- resolve_signature(sig) + append1(signatures) <- resolve_signature(sig) + } + + for (sig in signatures) { register_method(generic_fun, sig, method, env, package = NULL) } diff --git a/tests/testthat/_snaps/hooks.md b/tests/testthat/_snaps/hooks.md index fe7f1429..47387a7d 100644 --- a/tests/testthat/_snaps/hooks.md +++ b/tests/testthat/_snaps/hooks.md @@ -34,3 +34,20 @@ ! Can't find external class : * Packages 'hooklostclasspkg' doesn't contain 'Missing'. +# S7_on_load() does not partially register unions when an arm errors + + Code + downstream$.onLoad() + Condition + Error: + ! Can't find external class : + * Packages 'upstream_external_union_error_b' doesn't contain 'B'. + +--- + + Code + generic_pkg$gen(upstream_a$A()) + Condition + Error: + ! Can't find method for `gen()`. + diff --git a/tests/testthat/test-hooks.R b/tests/testthat/test-hooks.R index e1c31b5a..853a4643 100644 --- a/tests/testthat/test-hooks.R +++ b/tests/testthat/test-hooks.R @@ -250,6 +250,42 @@ test_that("S7_on_load() registers available union arms independently", { expect_equal(nrow(S7_methods(generic = generic_pkg$gen)), 2) }) +test_that("S7_on_load() does not partially register unions when an arm errors", { + generic_pkg <- local_package( + "upstream_external_union_error_generic", + gen := new_generic(dispatch_args = "x") + ) + upstream_a <- local_package( + "upstream_external_union_error_a", + A := new_class() + ) + local_package( + "upstream_external_union_error_b", + Other := new_class() + ) + downstream <- local_package( + "downstream_external_union_error", + .onLoad <- function(...) S7_on_load(), + gen <- new_external_generic( + package = "upstream_external_union_error_generic", + name = "gen", + dispatch_args = "x" + ), + A := new_external_class( + package = "upstream_external_union_error_a" + ), + B <- new_external_class( + package = "upstream_external_union_error_b", + name = "B" + ), + method(gen, A | B) <- function(x) "union" + ) + + expect_snapshot(error = TRUE, downstream$.onLoad()) + expect_equal(nrow(S7_methods(generic = generic_pkg$gen)), 0) + expect_snapshot(generic_pkg$gen(upstream_a$A()), error = TRUE) +}) + test_that("S7_on_unload() unregisters methods dispatching on an external class", { upstream <- local_package( "upstream_external_class_unload", diff --git a/tests/testthat/test-introspect.R b/tests/testthat/test-introspect.R index d3f756d3..fd48ae17 100644 --- a/tests/testthat/test-introspect.R +++ b/tests/testthat/test-introspect.R @@ -97,6 +97,18 @@ test_that("S7_methods(class) scans attached generics", { ) }) +test_that("S7_methods(class) uses real key for external S7_object", { + gen_name <- "S7_introspect_s7_object_xyzzy" + gen <- new_generic(name = gen_name, dispatch_args = "x") + method(gen, S7_object) <- function(x) "s7" + + assign(gen_name, gen, envir = globalenv()) + defer(rm(list = gen_name, envir = globalenv())) + + res <- S7_methods(class = new_external_class("S7", "S7_object")) + expect_equal(res$generic[res$generic == gen_name], gen_name) +}) + test_that("S7_methods() reports the generic's package", { Foo <- new_class("Foo", package = NULL) gen <- new_generic("gen", "x") From a3c361718e438ccc6c47c3635895dad2cf47cd36 Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Fri, 19 Jun 2026 01:18:03 -0400 Subject: [PATCH 18/42] Fix external class union handling --- R/class-spec.R | 4 +- R/external-generic.R | 138 +++++++++++++++++++++-- R/hooks.R | 17 ++- tests/testthat/_snaps/method-register.md | 24 ++++ tests/testthat/test-external-class.R | 7 ++ tests/testthat/test-method-register.R | 33 ++++++ 6 files changed, 209 insertions(+), 14 deletions(-) diff --git a/R/class-spec.R b/R/class-spec.R index f159235a..09ba335c 100644 --- a/R/class-spec.R +++ b/R/class-spec.R @@ -332,7 +332,9 @@ class_inherits <- function(x, what) { S7_S3 = !isS4(x) && class_dispatch_extends(what$class, class(x)), S7_external = inherits(x, "S7_object") && { - if (is.null(what$version) && inherits(x, what$class_name)) { + if (!inherits(x, external_class_register(what))) { + FALSE + } else if (is.null(what$version)) { TRUE } else { class_inherits(x, resolve_external_class_req(what)) diff --git a/R/external-generic.R b/R/external-generic.R index a733020e..979d9816 100644 --- a/R/external-generic.R +++ b/R/external-generic.R @@ -176,23 +176,143 @@ external_methods_add <- function( } external_methods_remove <- function(package, generic, signature) { - tbl <- S7_methods_table(package) + package_name <- force(package) + generic <- force(generic) + signature <- force(signature) + + tbl <- S7_methods_table(package_name) if (length(tbl) == 0) { return(invisible(list())) } - keep <- !vlapply(tbl, function(x) { - identical(x$generic, generic) && - external_method_signature_matches(x$signature, signature) - }) - removed <- tbl[!keep] - S7_methods_table(package) <- tbl[keep] - for (x in removed) { - hooks_remove_method(package, x) + active <- hooks_active(package_name) + new_tbl <- list() + removed <- list() + unhook <- list() + rehook <- list() + + for (x in tbl) { + if (!identical(x$generic, generic)) { + append1(new_tbl) <- x + next + } + + change <- external_method_signature_remove(x$signature, signature) + if (is.null(change)) { + append1(new_tbl) <- x + next + } + + removed_x <- x + removed_x$signature <- change$removed + append1(removed) <- removed_x + append1(unhook) <- x + + if (!is.null(change$remaining)) { + x$signature <- change$remaining + append1(new_tbl) <- x + append1(rehook) <- x + } + } + + `S7_methods_table<-`(package_name, new_tbl) + for (x in unhook) { + hooks_remove_method(package_name, x) + } + if (length(rehook) > 0 && active) { + for (x in rehook) { + hook_set_and_run(package_name, x) + } } + invisible(removed) } +external_method_signature_remove <- function(x, y) { + if (length(x) != length(y)) { + return(NULL) + } + + if (external_method_signature_matches(x, y)) { + return(list(removed = y, remaining = NULL)) + } + + candidates <- which(vlapply(seq_along(x), function(i) { + external_method_signature_arm_matches(x, y, i) + })) + if (length(candidates) != 1) { + return(NULL) + } + + i <- candidates[[1]] + change <- external_method_union_remove(x[[i]], y[[i]]) + removed <- x + removed[i] <- list(external_method_classes_to_class(change$removed)) + + if (length(change$remaining) == 0) { + remaining <- NULL + } else { + remaining <- x + remaining[i] <- list(external_method_classes_to_class(change$remaining)) + remaining <- new_signature(remaining) + } + + list( + removed = new_signature(removed), + remaining = remaining + ) +} + +external_method_signature_arm_matches <- function(x, y, i) { + if (!is_union(x[[i]])) { + return(FALSE) + } + if (is.null(external_method_union_remove(x[[i]], y[[i]]))) { + return(FALSE) + } + + other <- setdiff(seq_along(x), i) + all(vlapply(other, function(j) { + external_method_class_matches(x[[j]], y[[j]]) + })) +} + +external_method_union_remove <- function(x, y) { + stopifnot(is_union(x)) + + remove <- if (is_union(y)) y$classes else list(y) + removed <- list() + remaining <- list() + + for (class in x$classes) { + if ( + any(vlapply(remove, function(y) { + external_method_class_matches(class, y) + })) + ) { + removed[length(removed) + 1L] <- list(class) + } else { + remaining[length(remaining) + 1L] <- list(class) + } + } + + if (length(removed) == 0) { + return(NULL) + } + + list(removed = removed, remaining = remaining) +} + +external_method_classes_to_class <- function(x) { + stopifnot(length(x) > 0) + + if (length(x) == 1) { + x[[1]] + } else { + do.call(new_union, x) + } +} + external_method_signature_matches <- function(x, y) { if (identical(x, y)) { return(TRUE) diff --git a/R/hooks.R b/R/hooks.R index 4ff12af4..81fe2e04 100644 --- a/R/hooks.R +++ b/R/hooks.R @@ -113,19 +113,28 @@ hooks_set_and_run <- function(package) { } hook_set_and_run <- function(package, x) { - hook <- hook_add(package, x) - `hooks_packages<-`(package, union(hooks_packages(package), hook$pkgs)) + package_name <- force(package) + x <- force(x) + + hook <- hook_add(package_name, x) + `hooks_packages<-`( + package_name, + union(hooks_packages(package_name), hook$pkgs) + ) hook$run() } # 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) + package_name <- force(package) + x <- force(x) + + ns <- asNamespace(package_name) deps <- method_deps(x$generic, x$signature) register <- registrar(deps, x$generic, x$signature, x$method, ns) - hook <- S7_hook(register, package, x$generic, x$signature) + hook <- S7_hook(register, package_name, x$generic, x$signature) pkgs <- method_deps_packages(deps) for (pkg in pkgs) { diff --git a/tests/testthat/_snaps/method-register.md b/tests/testthat/_snaps/method-register.md index bb5abe0d..2662dd1d 100644 --- a/tests/testthat/_snaps/method-register.md +++ b/tests/testthat/_snaps/method-register.md @@ -75,6 +75,30 @@ Error: ! Can't find method for `foo()`. +# method unregistration removes deferred unions by concrete arm + + Code + upstream$gen(upstream$Ext()) + Condition + Error: + ! Can't find method for `gen()`. + +--- + + Code + upstream$gen(upstream$Ext()) + Condition + Error: + ! Can't find method for `gen()`. + +--- + + Code + upstream$gen(NULL) + Condition + Error: + ! Can't find method for `gen()`. + # method unregistration removes an S7 method via NULL assignment Code diff --git a/tests/testthat/test-external-class.R b/tests/testthat/test-external-class.R index abf17fe6..9ff235bb 100644 --- a/tests/testthat/test-external-class.R +++ b/tests/testthat/test-external-class.R @@ -67,6 +67,13 @@ test_that("S7_inherits() short-circuits external union classes", { expect_true(S7_inherits(Foo(), union)) }) +test_that("S7_inherits() skips non-matching external union classes", { + Foo := new_class(package = NULL) + union <- new_external_class("S7testthatmissing", "Bar") | Foo + + expect_true(S7_inherits(Foo(), union)) +}) + test_that("external class works as a property type for self-reference", { Tree := new_class( package = "mypkg", diff --git a/tests/testthat/test-method-register.R b/tests/testthat/test-method-register.R index 37114a18..d06a7283 100644 --- a/tests/testthat/test-method-register.R +++ b/tests/testthat/test-method-register.R @@ -237,6 +237,39 @@ test_that("method unregistration removes deferred unions regardless of order", { expect_snapshot(downstream$foo(upstream$Ext()), error = TRUE) }) +test_that("method unregistration removes deferred unions by concrete arm", { + upstream <- local_package( + "upstream_external_union_single_unregister", + gen := new_generic("x"), + Ext := new_class() + ) + downstream <- local_package( + "downstream_external_union_single_unregister", + .onLoad <- function(...) S7_on_load(), + .onUnload <- function(...) S7_on_unload(), + gen := new_external_generic( + package = "upstream_external_union_single_unregister", + dispatch_args = "x" + ), + Ext := new_external_class("upstream_external_union_single_unregister"), + method(gen, NULL | Ext) <- function(x) "external" + ) + downstream$.onLoad() + expect_equal(upstream$gen(upstream$Ext()), "external") + expect_equal(upstream$gen(NULL), "external") + + evalq(method(gen, Ext) <- NULL, downstream) + expect_snapshot(upstream$gen(upstream$Ext()), error = TRUE) + expect_equal(upstream$gen(NULL), "external") + + downstream$.onLoad() + expect_snapshot(upstream$gen(upstream$Ext()), error = TRUE) + expect_equal(upstream$gen(NULL), "external") + + downstream$.onUnload() + expect_snapshot(upstream$gen(NULL), error = TRUE) +}) + test_that("method unregistration removes an S7 method via NULL assignment", { foo := new_generic("x") method(foo, class_character) <- function(x) "c" From 5c916a6c8f91e4c9b5ba8c75b380094b6477b6cf Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Fri, 19 Jun 2026 08:41:29 -0400 Subject: [PATCH 19/42] Remove stale deferred external methods --- R/external-generic.R | 30 ++++++++++++++++++++---- tests/testthat/_snaps/method-register.md | 16 +++++++++++++ tests/testthat/test-method-register.R | 22 +++++++++++++++++ 3 files changed, 63 insertions(+), 5 deletions(-) diff --git a/R/external-generic.R b/R/external-generic.R index 979d9816..057835a3 100644 --- a/R/external-generic.R +++ b/R/external-generic.R @@ -245,7 +245,7 @@ external_method_signature_remove <- function(x, y) { } i <- candidates[[1]] - change <- external_method_union_remove(x[[i]], y[[i]]) + change <- external_method_class_remove(x[[i]], y[[i]]) removed <- x removed[i] <- list(external_method_classes_to_class(change$removed)) @@ -264,10 +264,7 @@ external_method_signature_remove <- function(x, y) { } external_method_signature_arm_matches <- function(x, y, i) { - if (!is_union(x[[i]])) { - return(FALSE) - } - if (is.null(external_method_union_remove(x[[i]], y[[i]]))) { + if (is.null(external_method_class_remove(x[[i]], y[[i]]))) { return(FALSE) } @@ -277,6 +274,29 @@ external_method_signature_arm_matches <- function(x, y, i) { })) } +external_method_class_remove <- function(x, y) { + if (is_union(x)) { + return(external_method_union_remove(x, y)) + } + if (is_union(y)) { + return(external_method_concrete_remove(x, y)) + } + + NULL +} + +external_method_concrete_remove <- function(x, y) { + stopifnot(is_union(y)) + + for (class in y$classes) { + if (external_method_class_matches(x, class)) { + return(list(removed = list(x), remaining = list())) + } + } + + NULL +} + external_method_union_remove <- function(x, y) { stopifnot(is_union(x)) diff --git a/tests/testthat/_snaps/method-register.md b/tests/testthat/_snaps/method-register.md index 2662dd1d..ddd443c0 100644 --- a/tests/testthat/_snaps/method-register.md +++ b/tests/testthat/_snaps/method-register.md @@ -99,6 +99,22 @@ Error: ! Can't find method for `gen()`. +# method unregistration removes deferred concrete methods by union + + Code + downstream$foo(upstream$Ext()) + Condition + Error: + ! Can't find method for `foo()`. + +--- + + Code + downstream$foo(upstream$Ext()) + Condition + Error: + ! Can't find method for `foo()`. + # method unregistration removes an S7 method via NULL assignment Code diff --git a/tests/testthat/test-method-register.R b/tests/testthat/test-method-register.R index d06a7283..15813649 100644 --- a/tests/testthat/test-method-register.R +++ b/tests/testthat/test-method-register.R @@ -270,6 +270,28 @@ test_that("method unregistration removes deferred unions by concrete arm", { expect_snapshot(upstream$gen(NULL), error = TRUE) }) +test_that("method unregistration removes deferred concrete methods by union", { + upstream <- local_package( + "upstream_external_concrete_union_unregister", + Ext := new_class() + ) + downstream <- local_package( + "downstream_external_concrete_union_unregister", + .onLoad <- function(...) S7_on_load(), + foo := new_generic("x"), + Ext := new_external_class("upstream_external_concrete_union_unregister"), + method(foo, Ext) <- function(x) "external" + ) + downstream$.onLoad() + expect_equal(downstream$foo(upstream$Ext()), "external") + + evalq(method(foo, Ext | NULL) <- NULL, downstream) + expect_snapshot(downstream$foo(upstream$Ext()), error = TRUE) + + downstream$.onLoad() + expect_snapshot(downstream$foo(upstream$Ext()), error = TRUE) +}) + test_that("method unregistration removes an S7 method via NULL assignment", { foo := new_generic("x") method(foo, class_character) <- function(x) "c" From 65fa60da502edae65e64d1d2113df234819502ad Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Fri, 19 Jun 2026 09:00:50 -0400 Subject: [PATCH 20/42] Fix external method unregister review cases --- R/class.R | 4 + R/external-generic.R | 112 ++++------------------- tests/testthat/_snaps/method-register.md | 20 ++++ tests/testthat/test-class.R | 42 +++++++++ tests/testthat/test-method-register.R | 42 +++++++++ 5 files changed, 127 insertions(+), 93 deletions(-) diff --git a/R/class.R b/R/class.R index 549f0e25..1e1c327a 100644 --- a/R/class.R +++ b/R/class.R @@ -463,6 +463,10 @@ check_prop_overrides <- function( child_class <- child_prop$class parent_class <- parent_props[[prop]]$class + if (identical(child_class, parent_class)) { + next + } + if (!class_extends(child_class, parent_class)) { child_desc <- paste0("<", name, ">") parent_desc <- class_desc(parent) diff --git a/R/external-generic.R b/R/external-generic.R index 057835a3..fadca5b6 100644 --- a/R/external-generic.R +++ b/R/external-generic.R @@ -203,15 +203,18 @@ external_methods_remove <- function(package, generic, signature) { next } - removed_x <- x - removed_x$signature <- change$removed - append1(removed) <- removed_x + for (sig in change$removed) { + removed_x <- x + removed_x$signature <- sig + append1(removed) <- removed_x + } append1(unhook) <- x - if (!is.null(change$remaining)) { - x$signature <- change$remaining - append1(new_tbl) <- x - append1(rehook) <- x + for (sig in change$remaining) { + remaining_x <- x + remaining_x$signature <- sig + append1(new_tbl) <- remaining_x + append1(rehook) <- remaining_x } } @@ -233,86 +236,19 @@ external_method_signature_remove <- function(x, y) { return(NULL) } - if (external_method_signature_matches(x, y)) { - return(list(removed = y, remaining = NULL)) - } - - candidates <- which(vlapply(seq_along(x), function(i) { - external_method_signature_arm_matches(x, y, i) - })) - if (length(candidates) != 1) { - return(NULL) - } - - i <- candidates[[1]] - change <- external_method_class_remove(x[[i]], y[[i]]) - removed <- x - removed[i] <- list(external_method_classes_to_class(change$removed)) - - if (length(change$remaining) == 0) { - remaining <- NULL - } else { - remaining <- x - remaining[i] <- list(external_method_classes_to_class(change$remaining)) - remaining <- new_signature(remaining) - } - - list( - removed = new_signature(removed), - remaining = remaining - ) -} - -external_method_signature_arm_matches <- function(x, y, i) { - if (is.null(external_method_class_remove(x[[i]], y[[i]]))) { - return(FALSE) - } - - other <- setdiff(seq_along(x), i) - all(vlapply(other, function(j) { - external_method_class_matches(x[[j]], y[[j]]) - })) -} - -external_method_class_remove <- function(x, y) { - if (is_union(x)) { - return(external_method_union_remove(x, y)) - } - if (is_union(y)) { - return(external_method_concrete_remove(x, y)) - } - - NULL -} - -external_method_concrete_remove <- function(x, y) { - stopifnot(is_union(y)) - - for (class in y$classes) { - if (external_method_class_matches(x, class)) { - return(list(removed = list(x), remaining = list())) - } - } - - NULL -} - -external_method_union_remove <- function(x, y) { - stopifnot(is_union(x)) - - remove <- if (is_union(y)) y$classes else list(y) + x_signatures <- flatten_signature(x) + y_signatures <- flatten_signature(y) removed <- list() remaining <- list() - for (class in x$classes) { - if ( - any(vlapply(remove, function(y) { - external_method_class_matches(class, y) - })) - ) { - removed[length(removed) + 1L] <- list(class) + for (x_sig in x_signatures) { + matched <- any(vlapply(y_signatures, function(y_sig) { + external_method_signature_matches(x_sig, y_sig) + })) + if (matched) { + append1(removed) <- new_signature(x_sig) } else { - remaining[length(remaining) + 1L] <- list(class) + append1(remaining) <- new_signature(x_sig) } } @@ -323,16 +259,6 @@ external_method_union_remove <- function(x, y) { list(removed = removed, remaining = remaining) } -external_method_classes_to_class <- function(x) { - stopifnot(length(x) > 0) - - if (length(x) == 1) { - x[[1]] - } else { - do.call(new_union, x) - } -} - external_method_signature_matches <- function(x, y) { if (identical(x, y)) { return(TRUE) diff --git a/tests/testthat/_snaps/method-register.md b/tests/testthat/_snaps/method-register.md index ddd443c0..73ccbf2d 100644 --- a/tests/testthat/_snaps/method-register.md +++ b/tests/testthat/_snaps/method-register.md @@ -115,6 +115,26 @@ Error: ! Can't find method for `foo()`. +# method unregistration splits deferred multidispatch unions + + Code + upstream$gen(upstream$A(), upstream$C()) + Condition + Error: + ! Can't find method for generic `gen(x, y)`: + - x: + - y: + +--- + + Code + upstream$gen(upstream$A(), upstream$C()) + Condition + Error: + ! Can't find method for generic `gen(x, y)`: + - x: + - y: + # method unregistration removes an S7 method via NULL assignment Code diff --git a/tests/testthat/test-class.R b/tests/testthat/test-class.R index 99c2e47e..1e0ee580 100644 --- a/tests/testthat/test-class.R +++ b/tests/testthat/test-class.R @@ -179,6 +179,48 @@ test_that("inheritance lets child properties narrow parent unions that include a )) }) +test_that("inheritance lets child properties keep external class specs", { + Ext <- new_external_class("notloaded.pkg", "Cls") + + x <- new_property( + Ext, + default = quote({ + NULL + }) + ) + child_x <- new_property( + Ext, + default = quote({ + NULL + }) + ) + Parent <- new_class("Parent", properties = list(x = x), package = NULL) + expect_no_error(new_class("Child", Parent, properties = list(x = child_x))) + + optional_x <- new_property( + NULL | Ext, + default = quote({ + NULL + }) + ) + optional_child_x <- new_property( + NULL | Ext, + default = quote({ + NULL + }) + ) + OptionalParent <- new_class( + "OptionalParent", + properties = list(x = optional_x), + package = NULL + ) + expect_no_error(new_class( + "OptionalChild", + OptionalParent, + properties = list(x = optional_child_x) + )) +}) + test_that("inheritance short-circuits matching union parent properties", { Parent := new_class( properties = list( diff --git a/tests/testthat/test-method-register.R b/tests/testthat/test-method-register.R index 15813649..9aa444cc 100644 --- a/tests/testthat/test-method-register.R +++ b/tests/testthat/test-method-register.R @@ -292,6 +292,48 @@ test_that("method unregistration removes deferred concrete methods by union", { expect_snapshot(downstream$foo(upstream$Ext()), error = TRUE) }) +test_that("method unregistration splits deferred multidispatch unions", { + upstream <- local_package( + "upstream_external_multi_union_unregister", + gen := new_generic(c("x", "y")), + A := new_class(), + B := new_class(), + C := new_class(), + D := new_class() + ) + downstream <- local_package( + "downstream_external_multi_union_unregister", + .onLoad <- function(...) S7_on_load(), + gen <- new_external_generic( + package = "upstream_external_multi_union_unregister", + name = "gen", + dispatch_args = c("x", "y") + ), + A := new_external_class("upstream_external_multi_union_unregister"), + B := new_external_class("upstream_external_multi_union_unregister"), + C := new_external_class("upstream_external_multi_union_unregister"), + D := new_external_class("upstream_external_multi_union_unregister"), + method(gen, list(A | B, C | D)) <- function(x, y) "external" + ) + downstream$.onLoad() + expect_equal(upstream$gen(upstream$A(), upstream$C()), "external") + expect_equal(upstream$gen(upstream$A(), upstream$D()), "external") + expect_equal(upstream$gen(upstream$B(), upstream$C()), "external") + expect_equal(upstream$gen(upstream$B(), upstream$D()), "external") + + evalq(method(gen, list(A, C)) <- NULL, downstream) + expect_snapshot(upstream$gen(upstream$A(), upstream$C()), error = TRUE) + expect_equal(upstream$gen(upstream$A(), upstream$D()), "external") + expect_equal(upstream$gen(upstream$B(), upstream$C()), "external") + expect_equal(upstream$gen(upstream$B(), upstream$D()), "external") + + downstream$.onLoad() + expect_snapshot(upstream$gen(upstream$A(), upstream$C()), error = TRUE) + expect_equal(upstream$gen(upstream$A(), upstream$D()), "external") + expect_equal(upstream$gen(upstream$B(), upstream$C()), "external") + expect_equal(upstream$gen(upstream$B(), upstream$D()), "external") +}) + test_that("method unregistration removes an S7 method via NULL assignment", { foo := new_generic("x") method(foo, class_character) <- function(x) "c" From f43a9885fc07c729bd464a9950251df0319afe86 Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Fri, 19 Jun 2026 09:18:42 -0400 Subject: [PATCH 21/42] Fix deferred union registration --- R/class-spec.R | 5 +++- R/external-generic.R | 23 ++++++++++++++++- R/hooks.R | 22 ++++++++++++++-- tests/testthat/test-class.R | 3 ++- tests/testthat/test-hooks.R | 50 +++++++++++++++++++++++++++++++++++++ 5 files changed, 98 insertions(+), 5 deletions(-) diff --git a/R/class-spec.R b/R/class-spec.R index 09ba335c..6e802ea7 100644 --- a/R/class-spec.R +++ b/R/class-spec.R @@ -357,7 +357,10 @@ 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. - for (class in parent$classes) { + classes <- parent$classes + external <- vlapply(classes, is_external_class) + classes <- c(classes[!external], classes[external]) + for (class in classes) { if (class_extends(child, class)) { return(TRUE) } diff --git a/R/external-generic.R b/R/external-generic.R index fadca5b6..7b317cdd 100644 --- a/R/external-generic.R +++ b/R/external-generic.R @@ -91,13 +91,21 @@ external_generic_version_ok <- function(generic, ns) { is.null(generic$version) || getNamespaceVersion(ns) >= generic$version } -registrar <- function(deps, generic, signature, method, env) { +registrar <- function( + deps, + generic, + signature, + method, + env, + on_load_package = NULL +) { # Force all arguments deps generic signature method env + on_load_package function(...) { if (!dep_available(generic)) { @@ -111,6 +119,10 @@ registrar <- function(deps, generic, signature, method, env) { signatures <- list() for (sig in flatten_signature(signature)) { + if (!registrar_signature_needs_package(sig, generic, on_load_package)) { + next + } + sig_deps <- signature_external_deps(sig) if (!all(vlapply(sig_deps, dep_available))) { next @@ -127,6 +139,15 @@ registrar <- function(deps, generic, signature, method, env) { } } +registrar_signature_needs_package <- function(signature, generic, package) { + if (is.null(package) || identical(package, generic$package)) { + return(TRUE) + } + + deps <- signature_external_deps(signature) + any(vlapply(deps, function(dep) identical(dep$package, package))) +} + # 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) { diff --git a/R/hooks.R b/R/hooks.R index 81fe2e04..adee845a 100644 --- a/R/hooks.R +++ b/R/hooks.R @@ -134,16 +134,34 @@ hook_add <- function(package, x) { deps <- method_deps(x$generic, x$signature) register <- registrar(deps, x$generic, x$signature, x$method, ns) - hook <- S7_hook(register, package_name, x$generic, x$signature) pkgs <- method_deps_packages(deps) for (pkg in pkgs) { - setHook(packageEvent(pkg, "onLoad"), hook) + hook_add_package(package_name, x, deps, ns, pkg) } list(run = register, pkgs = pkgs) } +hook_add_package <- function(package, x, deps, ns, on_load_package) { + package_name <- force(package) + x <- force(x) + deps <- force(deps) + ns <- force(ns) + on_load_package <- force(on_load_package) + + hook_register <- registrar( + deps, + x$generic, + x$signature, + x$method, + ns, + on_load_package = on_load_package + ) + hook <- S7_hook(hook_register, package_name, x$generic, x$signature) + setHook(packageEvent(on_load_package, "onLoad"), hook) +} + # Remove all of our hooks for `package`. Start with the recorded package # events, then re-derive from deferred methods in case loading failed and R # discarded our record while leaving the global hooks installed. diff --git a/tests/testthat/test-class.R b/tests/testthat/test-class.R index 1e0ee580..12086f76 100644 --- a/tests/testthat/test-class.R +++ b/tests/testthat/test-class.R @@ -222,9 +222,10 @@ test_that("inheritance lets child properties keep external class specs", { }) test_that("inheritance short-circuits matching union parent properties", { + Ext <- new_external_class("S7testthatmissing", "Ext") Parent := new_class( properties = list( - x = S7_object | new_external_class("S7testthatmissing", "Ext") + x = new_property(Ext | S7_object, default = quote(S7_object())) ), package = NULL ) diff --git a/tests/testthat/test-hooks.R b/tests/testthat/test-hooks.R index 853a4643..e2d17efe 100644 --- a/tests/testthat/test-hooks.R +++ b/tests/testthat/test-hooks.R @@ -250,6 +250,56 @@ test_that("S7_on_load() registers available union arms independently", { expect_equal(nrow(S7_methods(generic = generic_pkg$gen)), 2) }) +test_that("external-class hooks only register arms for the loaded package", { + generic_pkg <- local_package( + "upstream.external.union.hook.generic", + gen := new_generic(dispatch_args = "x") + ) + upstream_a <- local_package( + "upstream.external.union.hook.a", + A := new_class() + ) + downstream <- local_package( + "downstream.external.union.hook", + .onLoad <- function(...) S7_on_load(), + gen <- new_external_generic( + package = "upstream.external.union.hook.generic", + name = "gen", + dispatch_args = "x" + ), + A := new_external_class( + package = "upstream.external.union.hook.a" + ), + B := new_external_class( + package = "upstream.external.union.hook.b" + ), + method(gen, A | B) <- function(x) "union" + ) + + downstream$.onLoad() + expect_equal(generic_pkg$gen(upstream_a$A()), "union") + + gen <- generic_pkg$gen + expect_message( + method(gen, upstream_a$A) <- function(x) "specific", + "Overwriting method" + ) + expect_equal(generic_pkg$gen(upstream_a$A()), "specific") + + upstream_b <- local_package( + "upstream.external.union.hook.b", + B := new_class() + ) + expect_no_message({ + for (hook in package_hooks("upstream.external.union.hook.b")) { + hook() + } + }) + + expect_equal(generic_pkg$gen(upstream_a$A()), "specific") + expect_equal(generic_pkg$gen(upstream_b$B()), "union") +}) + test_that("S7_on_load() does not partially register unions when an arm errors", { generic_pkg <- local_package( "upstream_external_union_error_generic", From cf4b7678791ebb54d55c2b664cbd30158cd21ce6 Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Fri, 19 Jun 2026 09:34:41 -0400 Subject: [PATCH 22/42] Preserve versioned external generic on unregister --- R/method-register.R | 7 +++++- tests/testthat/test-method-register.R | 32 +++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/R/method-register.R b/R/method-register.R index 4f1e13ba..6295793c 100644 --- a/R/method-register.R +++ b/R/method-register.R @@ -177,6 +177,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), @@ -207,7 +212,7 @@ unregister_method <- function( # methods table so the method isn't re-registered on package load. if (!is.null(package)) { local <- is_local_generic(generic, package) - external <- as_external_generic(generic, env) + external <- external %||% as_external_generic(generic, env) external_methods_remove(package, external, signature) if (!local) { return(generic_sentinel(external)) diff --git a/tests/testthat/test-method-register.R b/tests/testthat/test-method-register.R index 9aa444cc..fd964fc3 100644 --- a/tests/testthat/test-method-register.R +++ b/tests/testthat/test-method-register.R @@ -215,6 +215,38 @@ test_that("method unregistration ignores external class version constraints", { expect_equal(nrow(S7_methods(generic = downstream$foo)), 0) }) +test_that("method unregistration preserves external generic version constraints", { + downstream <- local_package( + "downstream_versioned_external_generic_unregister", + .onLoad <- function(...) S7_on_load(), + gen := new_external_generic( + package = "upstream_versioned_external_generic_unregister", + dispatch_args = "x", + version = "0.0.0" + ), + Ext := new_external_class("upstream_versioned_external_generic_unregister"), + method(gen, Ext) <- function(x) "external" + ) + upstream <- local_package( + "upstream_versioned_external_generic_unregister", + gen := new_generic("x"), + Ext := new_class() + ) + + downstream$.onLoad() + expect_equal(upstream$gen(upstream$Ext()), "external") + + evalq(method(gen, Ext) <- NULL, downstream) + expect_equal(nrow(S7_methods(generic = upstream$gen)), 0) + expect_length( + S7_methods_table("downstream_versioned_external_generic_unregister"), + 0 + ) + + downstream$.onLoad() + expect_equal(nrow(S7_methods(generic = upstream$gen)), 0) +}) + test_that("method unregistration removes deferred unions regardless of order", { upstream <- local_package( "upstream_external_union_unregister", From a34553edd3352748130491b0d69317ab965dae83 Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Fri, 19 Jun 2026 09:51:19 -0400 Subject: [PATCH 23/42] Allow external classes to narrow S7_object --- R/class-spec.R | 4 ++-- tests/testthat/test-class.R | 23 +++++++++++++++++++++++ 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/R/class-spec.R b/R/class-spec.R index 6e802ea7..b7fd2cd4 100644 --- a/R/class-spec.R +++ b/R/class-spec.R @@ -372,6 +372,8 @@ class_extends <- function(child, parent) { } else if (is.null(parent)) { # as a parent, NULL only accepts NULL is.null(child) + } else if (is_class(parent) && parent@name == "S7_object") { + is_class(child) || is_external_class(child) } else if (is_external_class(child)) { child <- resolve_external_class_req(child) class_extends(child, parent) @@ -382,8 +384,6 @@ class_extends <- function(child, 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) } else { # handle S7, S3, and base types. class_dispatch_extends(class_dispatch(parent), class_dispatch(child)) diff --git a/tests/testthat/test-class.R b/tests/testthat/test-class.R index 12086f76..a7fa2e3e 100644 --- a/tests/testthat/test-class.R +++ b/tests/testthat/test-class.R @@ -117,6 +117,29 @@ test_that("inheritance lets child properties narrow S7_object with external clas expect_s3_class(Child(x = dep$External())@x, "dep::External") }) +test_that("inheritance lets child properties narrow S7_object with unloaded external classes", { + Ext <- new_external_class("notloaded.pkg", "Cls") + Parent := new_class( + properties = list(x = S7_object), + package = NULL, + abstract = TRUE + ) + + expect_no_error(new_class( + "Child", + parent = Parent, + properties = list( + x = new_property( + Ext, + default = quote({ + NULL + }) + ) + ), + package = NULL + )) +}) + test_that("inheritance lets child properties narrow with S4 inheritance", { S4PropertyParent := local_S4_class(slots = c(x = "numeric")) S4PropertyChild := local_S4_class(contains = "S4PropertyParent") From 798b32f9887df1e773e4c73b4f75f0f84d0f126e Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Fri, 19 Jun 2026 12:00:11 -0400 Subject: [PATCH 24/42] Simplify external class matching --- R/class-spec.R | 14 +++++++---- R/external-generic.R | 43 ++++------------------------------ tests/testthat/_snaps/hooks.md | 1 - tests/testthat/test-class.R | 24 +++++++++++++++++++ 4 files changed, 39 insertions(+), 43 deletions(-) diff --git a/R/class-spec.R b/R/class-spec.R index b7fd2cd4..0acbd98f 100644 --- a/R/class-spec.R +++ b/R/class-spec.R @@ -357,10 +357,7 @@ 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. - classes <- parent$classes - external <- vlapply(classes, is_external_class) - classes <- c(classes[!external], classes[external]) - for (class in classes) { + for (class in parent$classes) { if (class_extends(child, class)) { return(TRUE) } @@ -377,6 +374,15 @@ class_extends <- function(child, parent) { } 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)) { + if (!is_external_class_match(child, parent)) { + FALSE + } else if (is.null(parent$version)) { + TRUE + } else { + parent <- resolve_external_class_req(parent) + class_extends(child, parent) + } } else if (is_external_class(parent)) { parent <- resolve_external_class_req(parent) class_extends(child, parent) diff --git a/R/external-generic.R b/R/external-generic.R index 7b317cdd..9b575c64 100644 --- a/R/external-generic.R +++ b/R/external-generic.R @@ -197,19 +197,14 @@ external_methods_add <- function( } external_methods_remove <- function(package, generic, signature) { - package_name <- force(package) - generic <- force(generic) - signature <- force(signature) - - tbl <- S7_methods_table(package_name) + tbl <- S7_methods_table(package) if (length(tbl) == 0) { return(invisible(list())) } - active <- hooks_active(package_name) + active <- hooks_active(package) new_tbl <- list() removed <- list() - unhook <- list() rehook <- list() for (x in tbl) { @@ -229,7 +224,7 @@ external_methods_remove <- function(package, generic, signature) { removed_x$signature <- sig append1(removed) <- removed_x } - append1(unhook) <- x + hooks_remove_method(package, x) for (sig in change$remaining) { remaining_x <- x @@ -239,13 +234,10 @@ external_methods_remove <- function(package, generic, signature) { } } - `S7_methods_table<-`(package_name, new_tbl) - for (x in unhook) { - hooks_remove_method(package_name, x) - } + `S7_methods_table<-`(package, new_tbl) if (length(rehook) > 0 && active) { for (x in rehook) { - hook_set_and_run(package_name, x) + hook_set_and_run(package, x) } } @@ -307,35 +299,10 @@ external_method_class_matches <- function(x, y) { if (is_external_class(x) && is_external_class(y)) { return(identical(x$class_name, y$class_name)) } - if (is_union(x) && is_union(y)) { - return(external_method_union_matches(x$classes, y$classes)) - } FALSE } -external_method_union_matches <- function(x, y) { - if (length(x) != length(y)) { - return(FALSE) - } - - matched <- rep(FALSE, length(y)) - for (xi in x) { - hits <- which( - !matched & - vlapply(y, function(yi) { - external_method_class_matches(xi, yi) - }) - ) - if (length(hits) == 0) { - return(FALSE) - } - matched[[hits[[1]]]] <- TRUE - } - - TRUE -} - # Store external methods in an attribute of the S3 methods table since # this mutable object is present in all packages. diff --git a/tests/testthat/_snaps/hooks.md b/tests/testthat/_snaps/hooks.md index 47387a7d..e81ca740 100644 --- a/tests/testthat/_snaps/hooks.md +++ b/tests/testthat/_snaps/hooks.md @@ -50,4 +50,3 @@ Condition Error: ! Can't find method for `gen()`. - diff --git a/tests/testthat/test-class.R b/tests/testthat/test-class.R index a7fa2e3e..3659dbf3 100644 --- a/tests/testthat/test-class.R +++ b/tests/testthat/test-class.R @@ -261,6 +261,30 @@ test_that("inheritance short-circuits matching union parent properties", { )) }) +test_that("inheritance skips non-matching external union parent properties", { + dep := local_package( + External := new_class() + ) + Missing <- new_external_class("S7testthatmissing", "Missing") + External <- new_external_class("dep", "External") + + Parent := new_class( + properties = list( + x = new_property(Missing | External, default = quote(dep$External())) + ), + package = NULL + ) + + expect_no_error(new_class( + "Child", + Parent, + properties = list( + x = new_property(External, default = quote(dep$External())) + ), + package = NULL + )) +}) + test_that("inheritance lets child properties narrow optional union properties with NULL", { Parent <- new_class( "Parent", From 42ee22d27b3dafe37cbeb6a29c97d9f5aef3895b Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Fri, 19 Jun 2026 12:12:17 -0400 Subject: [PATCH 25/42] Simplify external class cleanup --- R/class-spec.R | 23 ++++++----------------- R/external-generic.R | 2 -- R/hooks.R | 8 +++----- R/method-register.R | 12 +++++------- 4 files changed, 14 insertions(+), 31 deletions(-) diff --git a/R/class-spec.R b/R/class-spec.R index 0acbd98f..35bc4a87 100644 --- a/R/class-spec.R +++ b/R/class-spec.R @@ -331,15 +331,9 @@ class_inherits <- function(x, what) { }, S7_S3 = !isS4(x) && class_dispatch_extends(what$class, class(x)), S7_external = inherits(x, "S7_object") && - { - if (!inherits(x, external_class_register(what))) { - FALSE - } else if (is.null(what$version)) { - TRUE - } else { - class_inherits(x, resolve_external_class_req(what)) - } - }, + inherits(x, external_class_register(what)) && + (is.null(what$version) || + class_inherits(x, resolve_external_class_req(what))), ) } @@ -375,14 +369,9 @@ class_extends <- function(child, parent) { child <- resolve_external_class_req(child) class_extends(child, parent) } else if (is_class(child) && is_external_class(parent)) { - if (!is_external_class_match(child, parent)) { - FALSE - } else if (is.null(parent$version)) { - TRUE - } else { - parent <- resolve_external_class_req(parent) - class_extends(child, 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) diff --git a/R/external-generic.R b/R/external-generic.R index 9b575c64..6218d579 100644 --- a/R/external-generic.R +++ b/R/external-generic.R @@ -92,7 +92,6 @@ external_generic_version_ok <- function(generic, ns) { } registrar <- function( - deps, generic, signature, method, @@ -100,7 +99,6 @@ registrar <- function( on_load_package = NULL ) { # Force all arguments - deps generic signature method diff --git a/R/hooks.R b/R/hooks.R index adee845a..6c31cb5a 100644 --- a/R/hooks.R +++ b/R/hooks.R @@ -133,25 +133,23 @@ hook_add <- function(package, x) { ns <- asNamespace(package_name) deps <- method_deps(x$generic, x$signature) - register <- registrar(deps, x$generic, x$signature, x$method, ns) + register <- registrar(x$generic, x$signature, x$method, ns) pkgs <- method_deps_packages(deps) for (pkg in pkgs) { - hook_add_package(package_name, x, deps, ns, pkg) + hook_add_package(package_name, x, ns, pkg) } list(run = register, pkgs = pkgs) } -hook_add_package <- function(package, x, deps, ns, on_load_package) { +hook_add_package <- function(package, x, ns, on_load_package) { package_name <- force(package) x <- force(x) - deps <- force(deps) ns <- force(ns) on_load_package <- force(on_load_package) hook_register <- registrar( - deps, x$generic, x$signature, x$method, diff --git a/R/method-register.R b/R/method-register.R index 6295793c..0b2f9313 100644 --- a/R/method-register.R +++ b/R/method-register.R @@ -190,13 +190,11 @@ unregister_method <- function( } unregister_signature <- signature - if (signature_has_external_class(signature)) { - if ( - is.null(package) || - signature_external_deps_resolvable(signature) - ) { - unregister_signature <- resolve_signature(signature) - } + if ( + signature_has_external_class(signature) && + (is.null(package) || signature_external_deps_resolvable(signature)) + ) { + unregister_signature <- resolve_signature(signature) } # Unregister in current session From 53f09a082f96d5e403effec689cf9c123d047bfa Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Fri, 19 Jun 2026 12:56:19 -0400 Subject: [PATCH 26/42] Simplify external class registration --- R/external-generic.R | 161 ++-------- R/hooks.R | 160 +++------- R/method-register.R | 21 +- tests/testthat/_snaps/external-class.md | 1 - tests/testthat/_snaps/hooks.md | 52 ---- tests/testthat/_snaps/method-register.md | 92 ------ tests/testthat/test-external-class.R | 23 -- tests/testthat/test-hooks.R | 359 +---------------------- tests/testthat/test-method-register.R | 206 +------------ 9 files changed, 81 insertions(+), 994 deletions(-) delete mode 100644 tests/testthat/_snaps/hooks.md diff --git a/R/external-generic.R b/R/external-generic.R index 6218d579..1cdf97d5 100644 --- a/R/external-generic.R +++ b/R/external-generic.R @@ -91,63 +91,42 @@ external_generic_version_ok <- function(generic, ns) { is.null(generic$version) || getNamespaceVersion(ns) >= generic$version } -registrar <- function( - generic, - signature, - method, - env, - on_load_package = NULL -) { +registrar <- function(generic, signature, method, env) { # Force all arguments generic signature method env - on_load_package function(...) { if (!dep_available(generic)) { return(invisible()) } - generic_fun <- resolve_generic(generic) - if (is.null(generic_fun)) { + sig_deps <- signature_external_deps(signature) + if (!all(vlapply(sig_deps, dep_available))) { return(invisible()) } - - signatures <- list() - for (sig in flatten_signature(signature)) { - if (!registrar_signature_needs_package(sig, generic, on_load_package)) { - next - } - - sig_deps <- signature_external_deps(sig) - if (!all(vlapply(sig_deps, dep_available))) { - next - } - - append1(signatures) <- resolve_signature(sig) + for (dep in sig_deps) { + resolve_external_class_req(dep) } - for (sig in signatures) { - register_method(generic_fun, sig, method, env, package = NULL) + generic_fun <- resolve_generic(generic) + if (is.null(generic_fun)) { + return(invisible()) } + register_method( + generic_fun, + resolve_signature(signature), + method, + env, + package = NULL + ) invisible() } } -registrar_signature_needs_package <- function(signature, generic, package) { - if (is.null(package) || identical(package, generic$package)) { - return(TRUE) - } - - deps <- signature_external_deps(signature) - any(vlapply(deps, function(dep) identical(dep$package, package))) -} - -# 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)) } @@ -179,7 +158,7 @@ external_methods_add <- function( method ) { # Remove any existing entries - removed <- external_methods_remove(package, generic, signature) + external_methods_remove(package, generic, signature) entry <- list( generic = generic, @@ -191,114 +170,20 @@ external_methods_add <- function( append1(tbl) <- entry S7_methods_table(package) <- tbl - invisible(removed) + invisible() } external_methods_remove <- function(package, generic, signature) { tbl <- S7_methods_table(package) if (length(tbl) == 0) { - return(invisible(list())) - } - - active <- hooks_active(package) - new_tbl <- list() - removed <- list() - rehook <- list() - - for (x in tbl) { - if (!identical(x$generic, generic)) { - append1(new_tbl) <- x - next - } - - change <- external_method_signature_remove(x$signature, signature) - if (is.null(change)) { - append1(new_tbl) <- x - next - } - - for (sig in change$removed) { - removed_x <- x - removed_x$signature <- sig - append1(removed) <- removed_x - } - hooks_remove_method(package, x) - - for (sig in change$remaining) { - remaining_x <- x - remaining_x$signature <- sig - append1(new_tbl) <- remaining_x - append1(rehook) <- remaining_x - } + return(invisible()) } - `S7_methods_table<-`(package, new_tbl) - if (length(rehook) > 0 && active) { - for (x in rehook) { - hook_set_and_run(package, x) - } - } - - invisible(removed) -} - -external_method_signature_remove <- function(x, y) { - if (length(x) != length(y)) { - return(NULL) - } - - x_signatures <- flatten_signature(x) - y_signatures <- flatten_signature(y) - removed <- list() - remaining <- list() - - for (x_sig in x_signatures) { - matched <- any(vlapply(y_signatures, function(y_sig) { - external_method_signature_matches(x_sig, y_sig) - })) - if (matched) { - append1(removed) <- new_signature(x_sig) - } else { - append1(remaining) <- new_signature(x_sig) - } - } - - if (length(removed) == 0) { - return(NULL) - } - - list(removed = removed, remaining = remaining) -} - -external_method_signature_matches <- function(x, y) { - if (identical(x, y)) { - return(TRUE) - } - if (length(x) != length(y)) { - return(FALSE) - } - - all(vlapply(seq_along(x), function(i) { - external_method_class_matches(x[[i]], y[[i]]) - })) -} - -external_method_class_matches <- function(x, y) { - if (identical(x, y)) { - return(TRUE) - } - - if (is_external_class(x) && is_class(y)) { - return(is_external_class_match(y, x)) - } - if (is_class(x) && is_external_class(y)) { - return(is_external_class_match(x, y)) - } - if (is_external_class(x) && is_external_class(y)) { - return(identical(x$class_name, y$class_name)) - } - - FALSE + keep <- !vlapply(tbl, function(x) { + identical(x$generic, generic) && identical(x$signature, signature) + }) + S7_methods_table(package) <- tbl[keep] + invisible() } # Store external methods in an attribute of the S3 methods table since diff --git a/R/hooks.R b/R/hooks.R index 6c31cb5a..c2b6029f 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() } @@ -99,108 +104,53 @@ 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) - `hooks_active<-`(package, TRUE) +# 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_set_and_run(package, x) - } - invisible() -} - -hook_set_and_run <- function(package, x) { - package_name <- force(package) - x <- force(x) - - hook <- hook_add(package_name, x) - `hooks_packages<-`( - package_name, - union(hooks_packages(package_name), hook$pkgs) - ) - hook$run() -} - -# 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) { - package_name <- force(package) - x <- force(x) - - ns <- asNamespace(package_name) - - deps <- method_deps(x$generic, x$signature) - register <- registrar(x$generic, x$signature, x$method, ns) + deps <- method_deps(x$generic, x$signature) + register <- registrar(x$generic, x$signature, x$method, ns) - pkgs <- method_deps_packages(deps) - for (pkg in pkgs) { - hook_add_package(package_name, x, ns, pkg) + for (pkg in method_deps_packages(deps)) { + hook <- S7_hook(register, package) + setHook(packageEvent(pkg, "onLoad"), hook) + append1(hooks) <- hook + append1(pkgs) <- pkg + } } - list(run = register, pkgs = pkgs) + names(hooks) <- pkgs + hooks_packages(package) <- unique(pkgs) + hooks } -hook_add_package <- function(package, x, ns, on_load_package) { - package_name <- force(package) - x <- force(x) - ns <- force(ns) - on_load_package <- force(on_load_package) - - hook_register <- registrar( - x$generic, - x$signature, - x$method, - ns, - on_load_package = on_load_package - ) - hook <- S7_hook(hook_register, package_name, x$generic, x$signature) - setHook(packageEvent(on_load_package, "onLoad"), hook) -} - -# Remove all of our hooks for `package`. Start with the recorded package -# events, then re-derive from deferred methods in case loading failed and R -# discarded our record while leaving the global hooks installed. +# Remove our hooks for `package`. hooks_remove <- function(package) { - pkgs <- hooks_packages(package) - for (x in S7_methods_table(package)) { - deps <- method_deps(x$generic, x$signature) - pkgs <- union(pkgs, method_deps_packages(deps)) - } - - for (pkg in pkgs) { - hook_remove(package, pkg) + for (pkg in hooks_packages(package)) { + 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()) - `hooks_active<-`(package, FALSE) + hooks_packages(package) <- character() invisible() } -hooks_remove_method <- function(package, x) { - deps <- method_deps(x$generic, x$signature) - for (pkg in method_deps_packages(deps)) { - hook_remove(package, pkg, x$generic, x$signature) +hooks_run_loaded <- function(hooks) { + is_loaded <- vlapply(names(hooks), isNamespaceLoaded) + for (hook in hooks[is_loaded]) { + hook() } invisible() } -hook_remove <- function(package, pkg, generic = NULL, signature = NULL) { - event <- packageEvent(pkg, "onLoad") - hooks <- getHook(event) - ours <- vlapply( - hooks, - is_S7_hook, - package = package, - generic = generic, - signature = signature - ) - if (any(ours)) { - setHook(event, hooks[!ours], action = "replace") - } -} - #' @export #' @rdname S7_on_load S7_on_build <- function() { @@ -226,34 +176,16 @@ is_generic_sentinel <- function(x) inherits(x, "S7_generic_sentinel") # Tag our hooks so we can remove later -S7_hook <- function(fun, package, generic = NULL, signature = NULL) { +S7_hook <- function(fun, package) { attr(fun, "S7_package") <- package - attr(fun, "S7_generic") <- generic - attr(fun, "S7_signature") <- signature class(fun) <- "S7_hook" fun } -is_S7_hook <- function(x, package = NULL, generic = NULL, signature = NULL) { +is_S7_hook <- function(x, package = NULL) { if (!inherits(x, "S7_hook")) { return(FALSE) } - if (!is.null(package) && !identical(attr(x, "S7_package", TRUE), package)) { - return(FALSE) - } - if (!is.null(generic) && !identical(attr(x, "S7_generic", TRUE), generic)) { - return(FALSE) - } - if (!is.null(signature)) { - hook_signature <- attr(x, "S7_signature", TRUE) - if ( - is.null(hook_signature) || - !external_method_signature_matches(hook_signature, signature) - ) { - return(FALSE) - } - } - - TRUE + is.null(package) || identical(attr(x, "S7_package", TRUE), package) } hooks_packages <- function(package) { @@ -268,18 +200,6 @@ hooks_packages <- function(package) { invisible() } -hooks_active <- function(package) { - ns <- asNamespace(package) - tbl <- ns[[".__S3MethodsTable__."]] - isTRUE(attr(tbl, "S7hooks_active")) -} -`hooks_active<-` <- function(package, value) { - ns <- asNamespace(package) - tbl <- ns[[".__S3MethodsTable__."]] - attr(tbl, "S7hooks_active") <- value - invisible() -} - unregister_own_S7_method <- function( generic, signature, diff --git a/R/method-register.R b/R/method-register.R index 0b2f9313..ff4a2f4e 100644 --- a/R/method-register.R +++ b/R/method-register.R @@ -101,20 +101,7 @@ register_method <- function( call = call ) } - removed <- external_methods_add(package, generic_ext, signature, method) - if ( - is_S7_generic(generic) && !signature_external_deps_resolvable(signature) - ) { - for (x in removed) { - unregister_own_S7_method(generic, x$signature, x$method, package) - } - } - if (hooks_active(package)) { - hook_set_and_run( - package, - list(generic = generic_ext, signature = signature, method = method) - ) - } + external_methods_add(package, generic_ext, signature, method) if (!is_local_generic(generic, package)) { return(generic_sentinel(generic_ext)) } @@ -154,12 +141,6 @@ register_method <- function( # when the package is loaded if (!is.null(package) && !is_local_generic(generic, package)) { external_methods_add(package, external, signature, method) - if (hooks_active(package)) { - hook_set_and_run( - package, - list(generic = external, signature = signature, method = method) - ) - } return(generic_sentinel(external)) } diff --git a/tests/testthat/_snaps/external-class.md b/tests/testthat/_snaps/external-class.md index 68d0d285..a200fe1f 100644 --- a/tests/testthat/_snaps/external-class.md +++ b/tests/testthat/_snaps/external-class.md @@ -87,4 +87,3 @@ 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/hooks.md b/tests/testthat/_snaps/hooks.md deleted file mode 100644 index e81ca740..00000000 --- a/tests/testthat/_snaps/hooks.md +++ /dev/null @@ -1,52 +0,0 @@ -# S7_on_load() doesn't duplicate hooks when registrars error - - Code - downstream$.onLoad() - Condition - Error: - ! Can't find external class : - * Packages 'hookclasspkg' doesn't contain 'Missing'. - ---- - - Code - downstream$.onLoad() - Condition - Error: - ! Can't find external class : - * Packages 'hookclasspkg' doesn't contain 'Missing'. - -# S7_on_load() removes stale hooks when hook records are lost - - Code - downstream$.onLoad() - Condition - Error: - ! Can't find external class : - * Packages 'hooklostclasspkg' doesn't contain 'Missing'. - ---- - - Code - downstream$.onLoad() - Condition - Error: - ! Can't find external class : - * Packages 'hooklostclasspkg' doesn't contain 'Missing'. - -# S7_on_load() does not partially register unions when an arm errors - - Code - downstream$.onLoad() - Condition - Error: - ! Can't find external class : - * Packages 'upstream_external_union_error_b' doesn't contain 'B'. - ---- - - Code - generic_pkg$gen(upstream_a$A()) - Condition - Error: - ! Can't find method for `gen()`. diff --git a/tests/testthat/_snaps/method-register.md b/tests/testthat/_snaps/method-register.md index 73ccbf2d..71959452 100644 --- a/tests/testthat/_snaps/method-register.md +++ b/tests/testthat/_snaps/method-register.md @@ -43,98 +43,6 @@ Error in `method<-`: ! foo() dispatches on `x`, but foo() has arguments `y`. -# method unregistration resolves loaded external-class methods in packages - - Code - downstream$foo(S7_object()) - Condition - Error: - ! Can't find method for `foo()`. - ---- - - Code - downstream$foo(S7_object()) - Condition - Error: - ! Can't find method for `foo()`. - -# method unregistration removes deferred unions regardless of order - - Code - downstream$foo(upstream$Ext()) - Condition - Error: - ! Can't find method for `foo()`. - ---- - - Code - downstream$foo(upstream$Ext()) - Condition - Error: - ! Can't find method for `foo()`. - -# method unregistration removes deferred unions by concrete arm - - Code - upstream$gen(upstream$Ext()) - Condition - Error: - ! Can't find method for `gen()`. - ---- - - Code - upstream$gen(upstream$Ext()) - Condition - Error: - ! Can't find method for `gen()`. - ---- - - Code - upstream$gen(NULL) - Condition - Error: - ! Can't find method for `gen()`. - -# method unregistration removes deferred concrete methods by union - - Code - downstream$foo(upstream$Ext()) - Condition - Error: - ! Can't find method for `foo()`. - ---- - - Code - downstream$foo(upstream$Ext()) - Condition - Error: - ! Can't find method for `foo()`. - -# method unregistration splits deferred multidispatch unions - - Code - upstream$gen(upstream$A(), upstream$C()) - Condition - Error: - ! Can't find method for generic `gen(x, y)`: - - x: - - y: - ---- - - Code - upstream$gen(upstream$A(), upstream$C()) - Condition - Error: - ! Can't find method for generic `gen(x, y)`: - - x: - - y: - # method unregistration removes an S7 method via NULL assignment Code diff --git a/tests/testthat/test-external-class.R b/tests/testthat/test-external-class.R index 9ff235bb..ccd89b29 100644 --- a/tests/testthat/test-external-class.R +++ b/tests/testthat/test-external-class.R @@ -172,26 +172,3 @@ test_that("versioned external class checks package version", { ) expect_snapshot(error = TRUE, Holder(x = versioned_pkg$Foo())) }) - -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") - ) - 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"))) -}) diff --git a/tests/testthat/test-hooks.R b/tests/testthat/test-hooks.R index e2d17efe..aadb717f 100644 --- a/tests/testthat/test-hooks.R +++ b/tests/testthat/test-hooks.R @@ -14,84 +14,6 @@ test_that("S7_on_load() doesn't accumulate hooks across repeated loads", { expect_length(package_hooks("upstream"), 1) }) -test_that("S7_on_load() doesn't duplicate hooks when registrars error", { - upstream_generic <- local_package( - "hookgenericpkg", - gen := new_generic("x") - ) - upstream_class <- local_package( - "hookclasspkg", - Real := new_class() - ) - downstream <- local_package( - "downstreamerrorhook", - .onLoad <- function(...) S7_on_load(), - gen <- new_external_generic( - package = "hookgenericpkg", - name = "gen", - dispatch_args = "x" - ), - Missing <- new_external_class( - package = "hookclasspkg", - name = "Missing" - ), - method(gen, Missing) <- function(x) "dispatched" - ) - - expect_snapshot(downstream$.onLoad(), error = TRUE) - expect_length(package_hooks("hookgenericpkg"), 1) - expect_length(package_hooks("hookclasspkg"), 1) - - expect_snapshot(downstream$.onLoad(), error = TRUE) - expect_length(package_hooks("hookgenericpkg"), 1) - expect_length(package_hooks("hookclasspkg"), 1) - - evalq(method(gen, Missing) <- NULL, downstream) - expect_length(package_hooks("hookgenericpkg"), 0) - expect_length(package_hooks("hookclasspkg"), 0) - - invisible(upstream_generic) - invisible(upstream_class) -}) - -test_that("S7_on_load() removes stale hooks when hook records are lost", { - upstream_generic <- local_package( - "hooklostgenericpkg", - gen := new_generic("x") - ) - upstream_class <- local_package( - "hooklostclasspkg", - Real := new_class() - ) - downstream <- local_package( - "hooklostdownstream", - .onLoad <- function(...) S7_on_load(), - gen <- new_external_generic( - package = "hooklostgenericpkg", - name = "gen", - dispatch_args = "x" - ), - Missing <- new_external_class( - package = "hooklostclasspkg", - name = "Missing" - ), - method(gen, Missing) <- function(x) "dispatched" - ) - - expect_snapshot(downstream$.onLoad(), error = TRUE) - expect_length(package_hooks("hooklostgenericpkg"), 1) - expect_length(package_hooks("hooklostclasspkg"), 1) - - `hooks_packages<-`("hooklostdownstream", character()) - - expect_snapshot(downstream$.onLoad(), error = TRUE) - expect_length(package_hooks("hooklostgenericpkg"), 1) - expect_length(package_hooks("hooklostclasspkg"), 1) - - invisible(upstream_generic) - invisible(upstream_class) -}) - test_that("S7_on_load() registers methods dispatching on an external class", { upstream := local_package( Foo := new_class() @@ -108,232 +30,43 @@ test_that("S7_on_load() registers methods dispatching on an external class", { expect_equal(downstream$own_generic(upstream$Foo()), "from external class") }) -test_that("method<- updates loaded external-class methods after S7_on_load()", { - upstream <- local_package( - "upstream_runtime_external_loaded", - Foo := new_class() - ) - downstream <- local_package( - "downstream_runtime_external_loaded", - .onLoad <- function(...) S7_on_load(), - own_generic := new_generic("x"), - Foo := new_external_class("upstream_runtime_external_loaded"), - method(own_generic, Foo) <- function(x) "first" - ) - downstream$.onLoad() - expect_equal(downstream$own_generic(upstream$Foo()), "first") - - expect_message( - evalq(method(own_generic, Foo) <- function(x) "second", downstream), - "Overwriting method" - ) - expect_equal(downstream$own_generic(upstream$Foo()), "second") -}) - -test_that("method<- clears stale external-class methods before deferring", { - upstream <- local_package( - "upstream_runtime_external_replaced_unloaded", - Foo := new_class() - ) - downstream <- local_package( - "downstream_runtime_external_replaced_unloaded", - .onLoad <- function(...) S7_on_load(), - own_generic := new_generic(dispatch_args = "x"), - Foo := new_external_class( - package = "upstream_runtime_external_replaced_unloaded" - ), - method(own_generic, Foo) <- function(x) "first" - ) - downstream$.onLoad() - expect_equal(downstream$own_generic(upstream$Foo()), "first") - - unloadNamespace("upstream_runtime_external_replaced_unloaded") - expect_false(isNamespaceLoaded("upstream_runtime_external_replaced_unloaded")) - - evalq(method(own_generic, Foo) <- function(x) "second", downstream) - expect_equal(nrow(S7_methods(generic = downstream$own_generic)), 0) - - upstream <- local_package( - "upstream_runtime_external_replaced_unloaded", - Foo := new_class() - ) - downstream$.onLoad() - expect_equal(downstream$own_generic(upstream$Foo()), "second") -}) - -test_that("method<- hooks unloaded external-class methods after S7_on_load()", { - downstream <- local_package( - "downstream_runtime_external_unloaded", - .onLoad <- function(...) S7_on_load(), - own_generic := new_generic("x"), - Foo := new_external_class("upstream_runtime_external_unloaded") - ) - downstream$.onLoad() - - evalq(method(own_generic, Foo) <- function(x) "runtime", downstream) - expect_length(package_hooks("upstream_runtime_external_unloaded"), 1) - - upstream <- local_package( - "upstream_runtime_external_unloaded", - Foo := new_class() - ) - for (hook in package_hooks("upstream_runtime_external_unloaded")) { - hook() - } - expect_equal(downstream$own_generic(upstream$Foo()), "runtime") -}) - -test_that("method<- rehooks unloaded external-generic methods after S7_on_load()", { - downstream <- local_package( - "downstream_runtime_external_generic_replaced", - .onLoad <- function(...) S7_on_load(), - Foo := new_class(), - gen <- new_external_generic( - package = "upstream_runtime_external_generic_replaced", - name = "gen", - dispatch_args = "x" - ), - method(gen, Foo) <- function(x) "first" - ) - downstream$.onLoad() - expect_length(package_hooks("upstream_runtime_external_generic_replaced"), 1) - - evalq(method(gen, Foo) <- function(x) "second", downstream) - expect_length(package_hooks("upstream_runtime_external_generic_replaced"), 1) - - upstream <- local_package( - "upstream_runtime_external_generic_replaced", - gen := new_generic(dispatch_args = "x") - ) - for (hook in package_hooks("upstream_runtime_external_generic_replaced")) { - hook() - } - expect_equal(upstream$gen(downstream$Foo()), "second") -}) - -test_that("S7_on_load() registers available union arms independently", { +test_that("S7_on_load() waits until all external union arms are available", { generic_pkg <- local_package( - "upstream_external_union_partial_generic", + "upstream_external_union_generic", gen := new_generic(dispatch_args = "x") ) upstream_a <- local_package( - "upstream_external_union_partial_a", + "upstream_external_union_a", A := new_class() ) downstream <- local_package( - "downstream_external_union_partial", + "downstream_external_union", .onLoad <- function(...) S7_on_load(), gen <- new_external_generic( - package = "upstream_external_union_partial_generic", + package = "upstream_external_union_generic", name = "gen", dispatch_args = "x" ), A := new_external_class( - package = "upstream_external_union_partial_a" + package = "upstream_external_union_a" ), B := new_external_class( - package = "upstream_external_union_partial_b" + package = "upstream_external_union_b" ), method(gen, A | B) <- function(x) "union" ) downstream$.onLoad() - expect_equal(generic_pkg$gen(upstream_a$A()), "union") - expect_equal(nrow(S7_methods(generic = generic_pkg$gen)), 1) + expect_equal(nrow(S7_methods(generic = generic_pkg$gen)), 0) upstream_b <- local_package( - "upstream_external_union_partial_b", + "upstream_external_union_b", B := new_class() ) - downstream$.onLoad() - expect_equal(generic_pkg$gen(upstream_b$B()), "union") - expect_equal(nrow(S7_methods(generic = generic_pkg$gen)), 2) -}) - -test_that("external-class hooks only register arms for the loaded package", { - generic_pkg <- local_package( - "upstream.external.union.hook.generic", - gen := new_generic(dispatch_args = "x") - ) - upstream_a <- local_package( - "upstream.external.union.hook.a", - A := new_class() - ) - downstream <- local_package( - "downstream.external.union.hook", - .onLoad <- function(...) S7_on_load(), - gen <- new_external_generic( - package = "upstream.external.union.hook.generic", - name = "gen", - dispatch_args = "x" - ), - A := new_external_class( - package = "upstream.external.union.hook.a" - ), - B := new_external_class( - package = "upstream.external.union.hook.b" - ), - method(gen, A | B) <- function(x) "union" - ) - downstream$.onLoad() expect_equal(generic_pkg$gen(upstream_a$A()), "union") - - gen <- generic_pkg$gen - expect_message( - method(gen, upstream_a$A) <- function(x) "specific", - "Overwriting method" - ) - expect_equal(generic_pkg$gen(upstream_a$A()), "specific") - - upstream_b <- local_package( - "upstream.external.union.hook.b", - B := new_class() - ) - expect_no_message({ - for (hook in package_hooks("upstream.external.union.hook.b")) { - hook() - } - }) - - expect_equal(generic_pkg$gen(upstream_a$A()), "specific") expect_equal(generic_pkg$gen(upstream_b$B()), "union") -}) - -test_that("S7_on_load() does not partially register unions when an arm errors", { - generic_pkg <- local_package( - "upstream_external_union_error_generic", - gen := new_generic(dispatch_args = "x") - ) - upstream_a <- local_package( - "upstream_external_union_error_a", - A := new_class() - ) - local_package( - "upstream_external_union_error_b", - Other := new_class() - ) - downstream <- local_package( - "downstream_external_union_error", - .onLoad <- function(...) S7_on_load(), - gen <- new_external_generic( - package = "upstream_external_union_error_generic", - name = "gen", - dispatch_args = "x" - ), - A := new_external_class( - package = "upstream_external_union_error_a" - ), - B <- new_external_class( - package = "upstream_external_union_error_b", - name = "B" - ), - method(gen, A | B) <- function(x) "union" - ) - - expect_snapshot(error = TRUE, downstream$.onLoad()) - expect_equal(nrow(S7_methods(generic = generic_pkg$gen)), 0) - expect_snapshot(generic_pkg$gen(upstream_a$A()), error = TRUE) + expect_equal(nrow(S7_methods(generic = generic_pkg$gen)), 2) }) test_that("S7_on_unload() unregisters methods dispatching on an external class", { @@ -395,78 +128,6 @@ test_that("S7_on_unload() unregisters external-class methods after class unload" expect_equal(nrow(S7_methods(generic = generic_pkg$gen)), 0) }) -test_that("method<- NULL removes deferred methods for resolved external classes", { - upstream <- local_package( - "upstream_resolved_external_unregister", - Foo := new_class() - ) - downstream <- local_package( - "downstream_resolved_external_unregister", - .onLoad <- function(...) S7_on_load(), - .onUnload <- function(...) S7_on_unload(), - own_generic := new_generic("x"), - Foo := new_external_class( - package = "upstream_resolved_external_unregister" - ), - method(own_generic, Foo) <- function(x) "from external class" - ) - downstream$.onLoad() - downstream$ResolvedFoo <- upstream$Foo - - expect_equal(downstream$own_generic(upstream$Foo()), "from external class") - expect_length(S7_methods_table("downstream_resolved_external_unregister"), 1) - - evalq(method(own_generic, ResolvedFoo) <- NULL, downstream) - expect_length(S7_methods_table("downstream_resolved_external_unregister"), 0) - expect_error( - downstream$own_generic(upstream$Foo()), - class = "S7_error_method_not_found" - ) - - downstream$.onLoad() - expect_error( - downstream$own_generic(upstream$Foo()), - class = "S7_error_method_not_found" - ) -}) - -test_that("method<- NULL removes installed hooks for deferred external-class methods", { - upstream <- local_package( - "upstream_deferred_external_hook", - gen := new_generic("x") - ) - downstream <- local_package( - "downstream_deferred_external_hook", - .onLoad <- function(...) S7_on_load(), - .onUnload <- function(...) S7_on_unload(), - gen := new_external_generic( - package = "upstream_deferred_external_hook", - dispatch_args = "x" - ), - Ext := new_external_class( - package = "upstream_deferred_external_hook_class" - ), - method(gen, Ext) <- function(x) "from stale hook" - ) - downstream$.onLoad() - expect_equal(nrow(S7_methods(generic = upstream$gen)), 0) - - evalq(method(gen, Ext) <- NULL, downstream) - expect_length(S7_methods_table("downstream_deferred_external_hook"), 0) - - ext_pkg <- local_package( - "upstream_deferred_external_hook_class", - Ext := new_class() - ) - for (hook in package_hooks("upstream_deferred_external_hook_class")) { - hook() - } - - expect_equal(nrow(S7_methods(generic = upstream$gen)), 0) - expect_length(package_hooks("upstream_deferred_external_hook_class"), 0) - invisible(ext_pkg) -}) - test_that("S7_on_unload() unregisters methods and removes hooks", { upstream <- local_package("upstream", gen := new_generic("x")) downstream <- local_package( diff --git a/tests/testthat/test-method-register.R b/tests/testthat/test-method-register.R index fd964fc3..d855528d 100644 --- a/tests/testthat/test-method-register.R +++ b/tests/testthat/test-method-register.R @@ -162,208 +162,16 @@ test_that("method unregistration removes deferred external-class methods", { expect_length(S7_methods_table("pkg"), 0) }) -test_that("method unregistration resolves loaded external-class methods in packages", { - downstream <- local_package( - "downstream_external_unregister_resolve", - .onLoad <- function(...) S7_on_load(), - foo := new_generic("x"), - Ext <- new_external_class("S7", "S7_object"), - method(foo, Ext) <- function(x) "external" - ) - downstream$.onLoad() - expect_equal(downstream$foo(S7_object()), "external") - - evalq(method(foo, Ext) <- NULL, downstream) - expect_snapshot(downstream$foo(S7_object()), error = TRUE) - - downstream$.onLoad() - expect_snapshot(downstream$foo(S7_object()), error = TRUE) -}) - -test_that("method unregistration ignores external class version constraints", { - upstream <- local_package( - "upstream_external_versioned_unregister", - Ext := new_class() - ) - downstream <- local_package( - "downstream_external_versioned_unregister", - .onLoad <- function(...) S7_on_load(), - foo := new_generic(dispatch_args = "x"), - ExtVersioned <- new_external_class( - package = "upstream_external_versioned_unregister", - name = "Ext", - version = "0.0.0" - ), - method(foo, ExtVersioned) <- function(x) "versioned" - ) - downstream$.onLoad() - expect_equal(downstream$foo(upstream$Ext()), "versioned") - - evalq( - { - Ext <- new_external_class( - package = "upstream_external_versioned_unregister", - name = "Ext" - ) - method(foo, Ext) <- NULL - }, - downstream - ) - expect_equal(nrow(S7_methods(generic = downstream$foo)), 0) - - downstream$.onLoad() - expect_equal(nrow(S7_methods(generic = downstream$foo)), 0) -}) - -test_that("method unregistration preserves external generic version constraints", { - downstream <- local_package( - "downstream_versioned_external_generic_unregister", - .onLoad <- function(...) S7_on_load(), - gen := new_external_generic( - package = "upstream_versioned_external_generic_unregister", - dispatch_args = "x", - version = "0.0.0" - ), - Ext := new_external_class("upstream_versioned_external_generic_unregister"), - method(gen, Ext) <- function(x) "external" - ) - upstream <- local_package( - "upstream_versioned_external_generic_unregister", - gen := new_generic("x"), - Ext := new_class() - ) - - downstream$.onLoad() - expect_equal(upstream$gen(upstream$Ext()), "external") - - evalq(method(gen, Ext) <- NULL, downstream) - expect_equal(nrow(S7_methods(generic = upstream$gen)), 0) - expect_length( - S7_methods_table("downstream_versioned_external_generic_unregister"), - 0 - ) - - downstream$.onLoad() - expect_equal(nrow(S7_methods(generic = upstream$gen)), 0) -}) - -test_that("method unregistration removes deferred unions regardless of order", { - upstream <- local_package( - "upstream_external_union_unregister", - Ext := new_class() - ) - downstream <- local_package( - "downstream_external_union_unregister", - .onLoad <- function(...) S7_on_load(), - foo := new_generic("x"), - Ext := new_external_class("upstream_external_union_unregister"), - method(foo, NULL | Ext) <- function(x) "external" - ) - downstream$.onLoad() - expect_equal(downstream$foo(upstream$Ext()), "external") - - evalq(method(foo, Ext | NULL) <- NULL, downstream) - expect_snapshot(downstream$foo(upstream$Ext()), error = TRUE) - - downstream$.onLoad() - expect_snapshot(downstream$foo(upstream$Ext()), error = TRUE) -}) - -test_that("method unregistration removes deferred unions by concrete arm", { - upstream <- local_package( - "upstream_external_union_single_unregister", - gen := new_generic("x"), - Ext := new_class() - ) - downstream <- local_package( - "downstream_external_union_single_unregister", - .onLoad <- function(...) S7_on_load(), - .onUnload <- function(...) S7_on_unload(), - gen := new_external_generic( - package = "upstream_external_union_single_unregister", - dispatch_args = "x" - ), - Ext := new_external_class("upstream_external_union_single_unregister"), - method(gen, NULL | Ext) <- function(x) "external" - ) - downstream$.onLoad() - expect_equal(upstream$gen(upstream$Ext()), "external") - expect_equal(upstream$gen(NULL), "external") - - evalq(method(gen, Ext) <- NULL, downstream) - expect_snapshot(upstream$gen(upstream$Ext()), error = TRUE) - expect_equal(upstream$gen(NULL), "external") - - downstream$.onLoad() - expect_snapshot(upstream$gen(upstream$Ext()), error = TRUE) - expect_equal(upstream$gen(NULL), "external") - - downstream$.onUnload() - expect_snapshot(upstream$gen(NULL), error = TRUE) -}) - -test_that("method unregistration removes deferred concrete methods by union", { - upstream <- local_package( - "upstream_external_concrete_union_unregister", - Ext := new_class() - ) - downstream <- local_package( - "downstream_external_concrete_union_unregister", - .onLoad <- function(...) S7_on_load(), +test_that("method unregistration removes deferred external-class unions", { + pkg := local_package( foo := new_generic("x"), - Ext := new_external_class("upstream_external_concrete_union_unregister"), - method(foo, Ext) <- function(x) "external" + ext := new_external_class("notloaded.pkg"), + method(foo, NULL | ext) <- function(x) "x" ) - downstream$.onLoad() - expect_equal(downstream$foo(upstream$Ext()), "external") - - evalq(method(foo, Ext | NULL) <- NULL, downstream) - expect_snapshot(downstream$foo(upstream$Ext()), error = TRUE) - - downstream$.onLoad() - expect_snapshot(downstream$foo(upstream$Ext()), error = TRUE) -}) + expect_length(S7_methods_table("pkg"), 1) -test_that("method unregistration splits deferred multidispatch unions", { - upstream <- local_package( - "upstream_external_multi_union_unregister", - gen := new_generic(c("x", "y")), - A := new_class(), - B := new_class(), - C := new_class(), - D := new_class() - ) - downstream <- local_package( - "downstream_external_multi_union_unregister", - .onLoad <- function(...) S7_on_load(), - gen <- new_external_generic( - package = "upstream_external_multi_union_unregister", - name = "gen", - dispatch_args = c("x", "y") - ), - A := new_external_class("upstream_external_multi_union_unregister"), - B := new_external_class("upstream_external_multi_union_unregister"), - C := new_external_class("upstream_external_multi_union_unregister"), - D := new_external_class("upstream_external_multi_union_unregister"), - method(gen, list(A | B, C | D)) <- function(x, y) "external" - ) - downstream$.onLoad() - expect_equal(upstream$gen(upstream$A(), upstream$C()), "external") - expect_equal(upstream$gen(upstream$A(), upstream$D()), "external") - expect_equal(upstream$gen(upstream$B(), upstream$C()), "external") - expect_equal(upstream$gen(upstream$B(), upstream$D()), "external") - - evalq(method(gen, list(A, C)) <- NULL, downstream) - expect_snapshot(upstream$gen(upstream$A(), upstream$C()), error = TRUE) - expect_equal(upstream$gen(upstream$A(), upstream$D()), "external") - expect_equal(upstream$gen(upstream$B(), upstream$C()), "external") - expect_equal(upstream$gen(upstream$B(), upstream$D()), "external") - - downstream$.onLoad() - expect_snapshot(upstream$gen(upstream$A(), upstream$C()), error = TRUE) - expect_equal(upstream$gen(upstream$A(), upstream$D()), "external") - expect_equal(upstream$gen(upstream$B(), upstream$C()), "external") - expect_equal(upstream$gen(upstream$B(), upstream$D()), "external") + evalq(method(foo, NULL | ext) <- NULL, pkg) + expect_length(S7_methods_table("pkg"), 0) }) test_that("method unregistration removes an S7 method via NULL assignment", { From a8a030c5bf3259d973a1f6a05476f1340cbc06cd Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Fri, 19 Jun 2026 13:18:58 -0400 Subject: [PATCH 27/42] Trim external class changes --- NEWS.md | 1 - R/class-spec.R | 4 +- R/class.R | 4 -- tests/testthat/_snaps/class.md | 9 --- tests/testthat/_snaps/external-class.md | 38 ----------- tests/testthat/test-class.R | 52 +++++---------- tests/testthat/test-external-class.R | 86 ++++--------------------- tests/testthat/test-method-register.R | 57 +++++++--------- vignettes/packages.Rmd | 29 ++------- 9 files changed, 62 insertions(+), 218 deletions(-) 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 35bc4a87..6f889e3a 100644 --- a/R/class-spec.R +++ b/R/class-spec.R @@ -340,7 +340,9 @@ class_inherits <- function(x, 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)) { diff --git a/R/class.R b/R/class.R index 1e1c327a..549f0e25 100644 --- a/R/class.R +++ b/R/class.R @@ -463,10 +463,6 @@ check_prop_overrides <- function( child_class <- child_prop$class parent_class <- parent_props[[prop]]$class - if (identical(child_class, parent_class)) { - next - } - if (!class_extends(child_class, parent_class)) { child_desc <- paste0("<", name, ">") parent_desc <- class_desc(parent) diff --git a/tests/testthat/_snaps/class.md b/tests/testthat/_snaps/class.md index 5e44a215..941c207a 100644 --- a/tests/testthat/_snaps/class.md +++ b/tests/testthat/_snaps/class.md @@ -149,15 +149,6 @@ - @x is . - @x is . -# subclassing an external class requires its package to be loaded - - Code - new_class("Child", Parent, properties = list(x = Ext)) - Condition - Error: - ! Can't find external class : - * Package 'notloaded.pkg' is not installed. - # abstract classes can't be instantiated Code diff --git a/tests/testthat/_snaps/external-class.md b/tests/testthat/_snaps/external-class.md index a200fe1f..cb9085ad 100644 --- a/tests/testthat/_snaps/external-class.md +++ b/tests/testthat/_snaps/external-class.md @@ -22,36 +22,6 @@ Output foo::Bar (>= 1.0) -# external class resolution rejects package-less classes - - Code - resolve_external_class_req(Foo) - Condition - Error: - ! Can't find external class : - * Packages 'pkg' doesn't contain 'Foo'. - -# resolve_external_class_req() errors per failure mode - - Code - resolve_external_class_req(new_external_class("not_a_pkg", "X")) - Condition - Error: - ! Can't find external class : - * Package 'not_a_pkg' is not installed. - Code - resolve_external_class_req(new_external_class("S7", "S7_object", "2.0.0")) - Condition - Error: - ! Can't find external class : - * Package 'S7' needs version 2.0.0, but only 1.0.0 is available. - Code - resolve_external_class_req(new_external_class("S7", "not_a_class")) - Condition - Error: - ! Can't find external class : - * Packages 'S7' doesn't contain 'not_a_class'. - # external class works as a property type for self-reference Code @@ -79,11 +49,3 @@ ! Can't find external class : * Package 'versioned_pkg' needs version 999.0, but only 0.0.0 is available. ---- - - Code - Holder(x = versioned_pkg$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/test-class.R b/tests/testthat/test-class.R index 3659dbf3..d1d6f09e 100644 --- a/tests/testthat/test-class.R +++ b/tests/testthat/test-class.R @@ -115,18 +115,10 @@ test_that("inheritance lets child properties narrow S7_object with external clas ) expect_s3_class(Child(x = dep$External())@x, "dep::External") -}) -test_that("inheritance lets child properties narrow S7_object with unloaded external classes", { Ext <- new_external_class("notloaded.pkg", "Cls") - Parent := new_class( - properties = list(x = S7_object), - package = NULL, - abstract = TRUE - ) - expect_no_error(new_class( - "Child", + "UnloadedChild", parent = Parent, properties = list( x = new_property( @@ -244,31 +236,27 @@ test_that("inheritance lets child properties keep external class specs", { )) }) -test_that("inheritance short-circuits matching union parent properties", { - Ext <- new_external_class("S7testthatmissing", "Ext") - Parent := new_class( +test_that("inheritance matches available union parent properties", { + dep := local_package( + External := new_class() + ) + Missing <- new_external_class(package = "S7testthatmissing", name = "Missing") + External <- new_external_class(package = "dep", name = "External") + + Parent1 := new_class( properties = list( - x = new_property(Ext | S7_object, default = quote(S7_object())) + x = new_property(Missing | S7_object, default = quote(S7_object())) ), package = NULL ) - expect_no_error(new_class( - "Child", - Parent, + "Child1", + Parent1, properties = list(x = S7_object), package = NULL )) -}) - -test_that("inheritance skips non-matching external union parent properties", { - dep := local_package( - External := new_class() - ) - Missing <- new_external_class("S7testthatmissing", "Missing") - External <- new_external_class("dep", "External") - Parent := new_class( + Parent2 := new_class( properties = list( x = new_property(Missing | External, default = quote(dep$External())) ), @@ -276,8 +264,8 @@ test_that("inheritance skips non-matching external union parent properties", { ) expect_no_error(new_class( - "Child", - Parent, + "Child2", + Parent2, properties = list( x = new_property(External, default = quote(dep$External())) ), @@ -324,16 +312,6 @@ test_that("inheritance doesn't let child properties widen or change the parent's }) -test_that("subclassing an external class requires its package to be loaded", { - Ext <- new_external_class("notloaded.pkg", "Cls") - Parent := new_class(properties = list(x = NULL | Ext), package = NULL) - - expect_snapshot( - new_class("Child", Parent, properties = list(x = Ext)), - error = TRUE - ) -}) - test_that("inheritance lets dynamic child properties override any parent type", { foo1 <- new_class("foo1", properties = list(x = class_integer)) readonly <- new_property(class_character, getter = function(self) "x") diff --git a/tests/testthat/test-external-class.R b/tests/testthat/test-external-class.R index ccd89b29..68df08bf 100644 --- a/tests/testthat/test-external-class.R +++ b/tests/testthat/test-external-class.R @@ -13,65 +13,34 @@ 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 + # The class is bound to `renamed`, but its S7 name is "named", 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("external class resolution rejects package-less classes", { - pkg := local_package( - Foo := new_class(package = NULL) + Named <- new_external_class(package = "pkg", name = "named") + Holder := new_class( + properties = list( + x = new_property(class = Named, default = quote(pkg$renamed())) + ) ) - Foo := new_external_class("pkg") - - expect_snapshot(error = TRUE, resolve_external_class_req(Foo)) -}) - -test_that("resolve_external_class_req() errors per failure mode", { - local_mocked_bindings(getNamespaceVersion = function(package) "1.0.0") - expect_snapshot(error = TRUE, { - resolve_external_class_req(new_external_class("not_a_pkg", "X")) - resolve_external_class_req(new_external_class("S7", "S7_object", "2.0.0")) - resolve_external_class_req(new_external_class("S7", "not_a_class")) - }) -}) - -test_that("external class can be used as a union arm", { - ec <- new_external_class("foo", "Bar") - u <- NULL | ec - expect_s3_class(u, "S7_union") - expect_length(u$classes, 2) -}) - -test_that("S7_inherits() short-circuits external union classes", { - Foo := new_class(package = NULL) - union <- Foo | new_external_class("S7testthatmissing", "Bar") - expect_true(S7_inherits(Foo(), union)) + expect_s3_class(Holder(x = pkg$renamed())@x, "pkg::named") }) -test_that("S7_inherits() skips non-matching external union classes", { +test_that("S7_inherits() matches loaded union arms around unloaded external classes", { Foo := new_class(package = NULL) - union <- new_external_class("S7testthatmissing", "Bar") | Foo + Missing <- new_external_class(package = "S7testthatmissing", name = "Bar") - expect_true(S7_inherits(Foo(), union)) + 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", { @@ -129,32 +98,6 @@ test_that("external class property validation uses resolved dispatch", { expect_s3_class(Holder(x = S7_object())@x, "S7_object") }) -test_that("external class works for mutually recursive classes", { - ClassOne := new_class( - package = "mypkg", - properties = list(x = NULL | new_external_class("mypkg", "ClassTwo")) - ) - ClassTwo := new_class( - package = "mypkg", - properties = list(y = NULL | new_external_class("mypkg", "ClassOne")) - ) - - obj <- ClassOne(x = ClassTwo(y = ClassOne())) - expect_s3_class(obj@x, "mypkg::ClassTwo") - 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")) - ) - ec <- new_external_class("mypkg", "Tree") - expect_true(class_inherits(Tree(), ec)) - expect_false(class_inherits(1, ec)) - expect_false(class_inherits(NULL, ec)) -}) - test_that("versioned external class checks package version", { versioned_pkg := local_package( Foo := new_class() @@ -166,9 +109,4 @@ test_that("versioned external class checks package version", { ) expect_snapshot(error = TRUE, S7_inherits(versioned_pkg$Foo(), Foo)) - - Holder := new_class( - properties = list(x = NULL | Foo) - ) - expect_snapshot(error = TRUE, Holder(x = versioned_pkg$Foo())) }) diff --git a/tests/testthat/test-method-register.R b/tests/testthat/test-method-register.R index d855528d..06ab4829 100644 --- a/tests/testthat/test-method-register.R +++ b/tests/testthat/test-method-register.R @@ -71,7 +71,6 @@ test_that("method registration resolves external classes outside packages", { env$ext <- new_external_class("S7", "S7_object") env$f <- function(x) "external" - expect_null(packageName(env)) evalq(method(g, ext) <- f, env) expect_equal(env$g(S7_object()), "external") @@ -96,7 +95,6 @@ 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", { @@ -108,24 +106,23 @@ test_that("deferred external-class methods preserve sentinel for foreign generic 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("deferred external-class methods match sentinel foreign generics", { +test_that("deferred external-class methods can reuse sentinel foreign generics", { pkg := local_package( - gen := new_external_generic("notloaded.pkg", "x"), - ext := new_external_class("notloaded.pkg") + gen := new_external_generic( + package = "notloaded.pkg", + dispatch_args = "x" + ), + Ext1 := new_external_class(package = "notloaded.pkg"), + Ext2 := new_external_class(package = "notloaded.pkg") ) - evalq(method(gen, ext) <- function(x) "first", pkg) + evalq(method(gen, Ext1) <- function(x) "first", pkg) expect_s3_class(pkg$gen, "S7_generic_sentinel") - evalq(method(gen, ext) <- function(x) "second", pkg) - expect_length(S7_methods_table("pkg"), 1) - expect_equal(S7_methods_table("pkg")[[1]]$method(NULL), "second") - - evalq(method(gen, ext) <- NULL, pkg) - expect_length(S7_methods_table("pkg"), 0) + expect_no_error(evalq(method(gen, Ext2) <- function(x) "second", pkg)) + expect_s3_class(pkg$gen, "S7_generic_sentinel") }) test_that("method registration defers external classes in union signatures", { @@ -136,7 +133,6 @@ test_that("method registration defers external classes in union signatures", { ) expect_length(methods(pkg$foo), 0) - expect_length(S7_methods_table("pkg"), 1) }) test_that("method registration validates deferred external-class methods", { @@ -150,28 +146,25 @@ test_that("method registration validates deferred external-class methods", { }) }) -test_that("method unregistration removes deferred external-class methods", { - pkg := local_package( - foo := new_generic("x"), - ext := new_external_class("notloaded.pkg"), - method(foo, ext) <- function(x) "x" - ) - expect_length(S7_methods_table("pkg"), 1) - - evalq(method(foo, ext) <- NULL, pkg) - expect_length(S7_methods_table("pkg"), 0) -}) - test_that("method unregistration removes deferred external-class unions", { - pkg := local_package( + upstream := local_package( + "upstream_external_unregister", + Foo := new_class() + ) + 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 ) - expect_length(S7_methods_table("pkg"), 1) + downstream$.onLoad() - evalq(method(foo, NULL | ext) <- NULL, pkg) - expect_length(S7_methods_table("pkg"), 0) + 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 2789a7ad..ad1e45a3 100644 --- a/vignettes/packages.Rmd +++ b/vignettes/packages.Rmd @@ -58,47 +58,32 @@ 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()` or `new_external_class()` when you want to register a method involving an S7 generic or class from a suggested package, 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). - -### Methods for classes in suggested packages - -Conversely, you may want to register a method for one of your own generics, dispatching on an S7 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") TheirClass <- new_external_class("theirpkg", "TheirClass") method(my_generic, TheirClass) <- function(x) { ... } -``` -When the suggested package is loaded, S7 will register the method automatically (via `S7_on_load()` as described above). +median <- new_external_generic("stats", "median", "x") +method(median, MyClass) <- function(x, ...) { ... } +``` -`new_external_class()` is only for S7 classes. For S3 classes from suggested packages, use `new_S3_class()`: +S7 will register these methods automatically when the suggested package is loaded, via `S7_on_load()` as described above. Use `new_S3_class()` for S3 classes from suggested packages: ```{r, eval = FALSE} -# In your package ggplot <- new_S3_class("ggplot") method(my_generic, ggplot) <- function(x) { ... } ``` From 970d20db6de6b96e1764addc30a90491a7b2e613 Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Fri, 19 Jun 2026 13:56:44 -0400 Subject: [PATCH 28/42] Trim external class cleanup --- R/external-class.R | 25 +---- R/external-generic.R | 12 ++- R/hooks.R | 6 +- R/method-register.R | 9 +- tests/testthat/test-class.R | 137 ++++++++------------------ tests/testthat/test-method-register.R | 10 -- 6 files changed, 57 insertions(+), 142 deletions(-) diff --git a/R/external-class.R b/R/external-class.R index 4e8953db..5e0e8bd5 100644 --- a/R/external-class.R +++ b/R/external-class.R @@ -71,45 +71,26 @@ 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) } else if (is_union(x)) { - flatten_external_deps(lapply(x$classes, class_external_deps)) + unlist(lapply(x$classes, class_external_deps), recursive = FALSE) } else { list() } } signature_external_deps <- function(signature) { - flatten_external_deps(lapply(signature, class_external_deps)) + unlist(lapply(signature, class_external_deps), recursive = FALSE) } -signature_external_deps_resolvable <- function(signature) { - deps <- signature_external_deps(signature) +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) -} - #' @export print.S7_external_class <- function(x, ...) { cat( diff --git a/R/external-generic.R b/R/external-generic.R index 1cdf97d5..7a73e72e 100644 --- a/R/external-generic.R +++ b/R/external-generic.R @@ -104,11 +104,13 @@ registrar <- function(generic, signature, method, env) { } sig_deps <- signature_external_deps(signature) - if (!all(vlapply(sig_deps, dep_available))) { - return(invisible()) - } - for (dep in sig_deps) { - resolve_external_class_req(dep) + 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) diff --git a/R/hooks.R b/R/hooks.R index c2b6029f..7a7dc7ac 100644 --- a/R/hooks.R +++ b/R/hooks.R @@ -86,10 +86,8 @@ S7_on_unload_ <- function(env) { # Methods registered for S3 and S4 generics can't be unregistered yet if (is_S7_generic(generic)) { signature <- x$signature - if ( - signature_has_external_class(signature) && - signature_external_deps_resolvable(signature) - ) { + deps <- signature_external_deps(signature) + if (length(deps) && external_deps_resolvable(deps)) { signature <- resolve_signature(signature) } unregister_own_S7_method( diff --git a/R/method-register.R b/R/method-register.R index ff4a2f4e..e1657455 100644 --- a/R/method-register.R +++ b/R/method-register.R @@ -88,7 +88,8 @@ 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 { @@ -170,11 +171,9 @@ unregister_method <- function( ) } + deps <- signature_external_deps(signature) unregister_signature <- signature - if ( - signature_has_external_class(signature) && - (is.null(package) || signature_external_deps_resolvable(signature)) - ) { + if (length(deps) && (is.null(package) || external_deps_resolvable(deps))) { unregister_signature <- resolve_signature(signature) } diff --git a/tests/testthat/test-class.R b/tests/testthat/test-class.R index d1d6f09e..8fd8434a 100644 --- a/tests/testthat/test-class.R +++ b/tests/testthat/test-class.R @@ -93,45 +93,6 @@ test_that("inheritance lets child properties narrow the parent's type", { )) }) -test_that("inheritance lets child properties narrow S7_object with external classes", { - dep := local_package( - External := new_class() - ) - Parent := new_class( - properties = list(x = S7_object), - package = NULL, - abstract = TRUE - ) - - Child := new_class( - parent = Parent, - properties = list( - x = new_property( - class = new_external_class("dep", "External"), - default = quote(dep$External()) - ) - ), - package = NULL - ) - - expect_s3_class(Child(x = dep$External())@x, "dep::External") - - Ext <- new_external_class("notloaded.pkg", "Cls") - expect_no_error(new_class( - "UnloadedChild", - parent = Parent, - properties = list( - x = new_property( - Ext, - default = quote({ - NULL - }) - ) - ), - package = NULL - )) -}) - test_that("inheritance lets child properties narrow with S4 inheritance", { S4PropertyParent := local_S4_class(slots = c(x = "numeric")) S4PropertyChild := local_S4_class(contains = "S4PropertyParent") @@ -194,81 +155,65 @@ test_that("inheritance lets child properties narrow parent unions that include a )) }) -test_that("inheritance lets child properties keep external class specs", { - Ext <- new_external_class("notloaded.pkg", "Cls") - - x <- new_property( - Ext, - default = quote({ - NULL - }) - ) - child_x <- new_property( - Ext, - default = quote({ - NULL - }) - ) - Parent <- new_class("Parent", properties = list(x = x), package = NULL) - expect_no_error(new_class("Child", Parent, properties = list(x = child_x))) - - optional_x <- new_property( - NULL | Ext, - default = quote({ - NULL - }) - ) - optional_child_x <- new_property( - NULL | Ext, - default = quote({ - NULL - }) - ) - OptionalParent <- new_class( - "OptionalParent", - properties = list(x = optional_x), - package = NULL - ) - expect_no_error(new_class( - "OptionalChild", - OptionalParent, - properties = list(x = optional_child_x) - )) -}) - -test_that("inheritance matches available union parent properties", { +test_that("inheritance handles external class property specs", { dep := local_package( External := new_class() ) - Missing <- new_external_class(package = "S7testthatmissing", name = "Missing") External <- new_external_class(package = "dep", name = "External") + Missing <- new_external_class(package = "S7testthatmissing", name = "Missing") - Parent1 := new_class( + 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())@x, "dep::External") + + Ext <- new_external_class(package = "notloaded.pkg", name = "Cls") + ParentSame := new_class( properties = list( - x = new_property(Missing | S7_object, default = quote(S7_object())) + x = new_property( + class = Ext, + default = quote({ + NULL + }) + ) ), package = NULL ) expect_no_error(new_class( - "Child1", - Parent1, - properties = list(x = S7_object), + name = "ChildSame", + parent = ParentSame, + properties = list( + x = new_property( + class = Ext, + default = quote({ + NULL + }) + ) + ), package = NULL )) - Parent2 := new_class( + ParentUnion := new_class( properties = list( - x = new_property(Missing | External, default = quote(dep$External())) + x = new_property( + class = Missing | S7_object, + default = quote(S7_object()) + ) ), package = NULL ) - expect_no_error(new_class( - "Child2", - Parent2, - properties = list( - x = new_property(External, default = quote(dep$External())) - ), + name = "ChildUnion", + parent = ParentUnion, + properties = list(x = S7_object), package = NULL )) }) diff --git a/tests/testthat/test-method-register.R b/tests/testthat/test-method-register.R index 06ab4829..3a92b73d 100644 --- a/tests/testthat/test-method-register.R +++ b/tests/testthat/test-method-register.R @@ -125,16 +125,6 @@ test_that("deferred external-class methods can reuse sentinel foreign generics", expect_s3_class(pkg$gen, "S7_generic_sentinel") }) -test_that("method registration defers external classes in union signatures", { - pkg := local_package( - foo := new_generic("x"), - ext := new_external_class("notloaded.pkg"), - method(foo, NULL | ext) <- function(x) "x" - ) - - expect_length(methods(pkg$foo), 0) -}) - test_that("method registration validates deferred external-class methods", { expect_snapshot(error = TRUE, { local_package( From 8a0b675816a5543ccadfed226c8d66d167ee647f Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Fri, 19 Jun 2026 14:31:03 -0400 Subject: [PATCH 29/42] Simplify external class resolution --- R/external-class.R | 27 +++++++-------------- R/hooks.R | 17 +------------ tests/testthat/helper.R | 10 ++------ tests/testthat/test-external-class.R | 16 ------------- tests/testthat/test-hooks.R | 36 ---------------------------- 5 files changed, 11 insertions(+), 95 deletions(-) diff --git a/R/external-class.R b/R/external-class.R index 5e0e8bd5..0ef09951 100644 --- a/R/external-class.R +++ b/R/external-class.R @@ -140,30 +140,19 @@ 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(x$package, "S7") && - identical(x$name, "S7_object") && - identical(obj, S7_object)) || + ((identical(x$package, "S7") && + identical(x$name, "S7_object") && + identical(obj, S7_object)) || (identical(obj@name, x$name) && identical(obj@package, x$package))) } diff --git a/R/hooks.R b/R/hooks.R index 7a7dc7ac..b7849bb8 100644 --- a/R/hooks.R +++ b/R/hooks.R @@ -211,24 +211,9 @@ unregister_own_S7_method <- function( own <- S7_method_for_signature(method, generic, sig, package = package) # Unload only removes the method this package currently owns. It does not # remember or restore any method that was overwritten during loading. - if (identical(current, own) || is_own_S7_method(current, method, package)) { + if (identical(current, own)) { generic_remove_method(generic, sig) } } invisible() } - -is_own_S7_method <- function(current, method, package = NULL) { - if (!is.function(current)) { - return(FALSE) - } - if ( - !is.null(package) && !identical(attr(current, "S7_package", TRUE), package) - ) { - return(FALSE) - } - - current_method <- current - attributes(current_method) <- attributes(method) - identical(current_method, method) -} diff --git a/tests/testthat/helper.R b/tests/testthat/helper.R index 5a34c620..912c657d 100644 --- a/tests/testthat/helper.R +++ b/tests/testthat/helper.R @@ -68,14 +68,8 @@ local_package <- function(name, ..., frame = parent.frame()) { # register namespace so asNamespace(pkg) works internal <- get(".Internal", envir = baseenv()) internal(registerNamespace(name, ns)) - defer( - if (isNamespaceLoaded(name)) internal(unregisterNamespace(name)), - frame = frame - ) - defer( - if (isNamespaceLoaded(name)) S7_on_unload_(ns), - frame = frame - ) + defer(internal(unregisterNamespace(name)), frame = frame) + defer(S7_on_unload_(ns), frame = frame) for (expr in eval(substitute(alist(...)))) { eval(expr, ns) diff --git a/tests/testthat/test-external-class.R b/tests/testthat/test-external-class.R index 68df08bf..6f6d1ee9 100644 --- a/tests/testthat/test-external-class.R +++ b/tests/testthat/test-external-class.R @@ -19,22 +19,6 @@ test_that("external class is a valid class spec", { expect_equal(S7_class_desc(ec), "") }) -test_that("external class resolution uses the S7 class name", { - # The class is bound to `renamed`, but its S7 name is "named", so - # resolution must find it by scanning for a matching S7 class name. - pkg := local_package( - renamed <- new_class("named") - ) - Named <- new_external_class(package = "pkg", name = "named") - Holder := new_class( - properties = list( - x = new_property(class = Named, default = quote(pkg$renamed())) - ) - ) - - expect_s3_class(Holder(x = pkg$renamed())@x, "pkg::named") -}) - 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") diff --git a/tests/testthat/test-hooks.R b/tests/testthat/test-hooks.R index aadb717f..fa2c2538 100644 --- a/tests/testthat/test-hooks.R +++ b/tests/testthat/test-hooks.R @@ -92,42 +92,6 @@ test_that("S7_on_unload() unregisters methods dispatching on an external class", ) }) -test_that("S7_on_unload() unregisters external-class methods after class unload", { - generic_pkg <- local_package( - "upstream_external_unloaded_class_generic", - gen := new_generic("x") - ) - class_pkg <- local_package( - "upstream_external_unloaded_class", - Foo := new_class() - ) - downstream <- local_package( - "downstream_external_unloaded_class", - .onLoad <- function(...) S7_on_load(), - .onUnload <- function(...) S7_on_unload(), - gen := new_external_generic( - package = "upstream_external_unloaded_class_generic", - dispatch_args = "x" - ), - Foo := new_external_class( - package = "upstream_external_unloaded_class" - ), - method(gen, Foo) <- function(x) "from external class" - ) - downstream$.onLoad() - - obj <- class_pkg$Foo() - expect_equal(generic_pkg$gen(obj), "from external class") - expect_equal(nrow(S7_methods(generic = generic_pkg$gen)), 1) - - unloadNamespace("upstream_external_unloaded_class") - expect_false(isNamespaceLoaded("upstream_external_unloaded_class")) - expect_equal(generic_pkg$gen(obj), "from external class") - - downstream$.onUnload() - expect_equal(nrow(S7_methods(generic = generic_pkg$gen)), 0) -}) - test_that("S7_on_unload() unregisters methods and removes hooks", { upstream <- local_package("upstream", gen := new_generic("x")) downstream <- local_package( From bdf637c53aabdb0bcb27613f4c5c25caed0a073c Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Fri, 19 Jun 2026 14:38:29 -0400 Subject: [PATCH 30/42] Simplify external dependency checks --- R/external-class.R | 9 ++++++--- R/external-generic.R | 9 +-------- R/hooks.R | 13 ++++++------- tests/testthat/test-hooks.R | 9 ++++----- tests/testthat/test-method-register.R | 28 --------------------------- 5 files changed, 17 insertions(+), 51 deletions(-) diff --git a/R/external-class.R b/R/external-class.R index 0ef09951..1ee0e2a8 100644 --- a/R/external-class.R +++ b/R/external-class.R @@ -114,8 +114,11 @@ external_class_register <- 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 @@ -170,7 +173,7 @@ resolve_external_class_req <- function(x) { ) } - if (!is.null(x$version) && getNamespaceVersion(x$package) < x$version) { + if (!dep_version_ok(x)) { stop2( paste0( prefix, diff --git a/R/external-generic.R b/R/external-generic.R index 7a73e72e..b1601b4c 100644 --- a/R/external-generic.R +++ b/R/external-generic.R @@ -81,14 +81,7 @@ 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(generic, signature, method, env) { diff --git a/R/hooks.R b/R/hooks.R index b7849bb8..48cd2655 100644 --- a/R/hooks.R +++ b/R/hooks.R @@ -69,16 +69,15 @@ S7_on_unload_ <- function(env) { tbl <- S7_methods_table(package) for (x in tbl) { - if (!isNamespaceLoaded(x$generic$package)) { + if (!dep_available(x$generic)) { next } - ns <- asNamespace(x$generic$package) - if (!external_generic_version_ok(x$generic, ns)) { - 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 } diff --git a/tests/testthat/test-hooks.R b/tests/testthat/test-hooks.R index fa2c2538..5bc722aa 100644 --- a/tests/testthat/test-hooks.R +++ b/tests/testthat/test-hooks.R @@ -23,9 +23,6 @@ test_that("S7_on_load() registers methods dispatching on an external class", { Foo := new_external_class("upstream"), method(own_generic, Foo) <- \(x) "from external class" ) - # The method is deferred (its signature has an external class), not yet live - expect_length(methods(downstream$own_generic), 0) - S7_on_load_(downstream) expect_equal(downstream$own_generic(upstream$Foo()), "from external class") }) @@ -57,7 +54,10 @@ test_that("S7_on_load() waits until all external union arms are available", { ) downstream$.onLoad() - expect_equal(nrow(S7_methods(generic = generic_pkg$gen)), 0) + expect_error( + generic_pkg$gen(upstream_a$A()), + class = "S7_error_method_not_found" + ) upstream_b <- local_package( "upstream_external_union_b", @@ -66,7 +66,6 @@ test_that("S7_on_load() waits until all external union arms are available", { downstream$.onLoad() expect_equal(generic_pkg$gen(upstream_a$A()), "union") expect_equal(generic_pkg$gen(upstream_b$B()), "union") - expect_equal(nrow(S7_methods(generic = generic_pkg$gen)), 2) }) test_that("S7_on_unload() unregisters methods dispatching on an external class", { diff --git a/tests/testthat/test-method-register.R b/tests/testthat/test-method-register.R index 3a92b73d..2b0a7816 100644 --- a/tests/testthat/test-method-register.R +++ b/tests/testthat/test-method-register.R @@ -97,34 +97,6 @@ test_that("method registration returns a strippable sentinel for foreign generic expect_s3_class(pkg$ext, "S7_generic_sentinel") }) -test_that("deferred external-class methods preserve sentinel for foreign generics", { - pkg := local_package( - ext := new_external_class("notloaded.pkg") - ) - - # 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") -}) - -test_that("deferred external-class methods can reuse sentinel foreign generics", { - pkg := local_package( - gen := new_external_generic( - package = "notloaded.pkg", - dispatch_args = "x" - ), - Ext1 := new_external_class(package = "notloaded.pkg"), - Ext2 := new_external_class(package = "notloaded.pkg") - ) - - evalq(method(gen, Ext1) <- function(x) "first", pkg) - expect_s3_class(pkg$gen, "S7_generic_sentinel") - - expect_no_error(evalq(method(gen, Ext2) <- function(x) "second", pkg)) - expect_s3_class(pkg$gen, "S7_generic_sentinel") -}) - test_that("method registration validates deferred external-class methods", { expect_snapshot(error = TRUE, { local_package( From 0b8a8d5f0f542cfc1b9e33919b9d6a46255afc43 Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Fri, 19 Jun 2026 14:41:48 -0400 Subject: [PATCH 31/42] Use closure for external class validation --- R/class-spec.R | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/R/class-spec.R b/R/class-spec.R index 6f889e3a..50369e98 100644 --- a/R/class-spec.R +++ b/R/class-spec.R @@ -204,10 +204,9 @@ class_validate <- function(class, object) { S7 = class@validator, S7_base = class$validator, S7_S3 = class$validator, - S7_external = return(class_validate( - resolve_external_class_req(class), - object - )), + S7_external = function(object) { + class_validate(resolve_external_class_req(class), object) + }, NULL ) From 5495aa9098ecd34e3249d82a8ca31003758c5c5f Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Fri, 19 Jun 2026 14:52:09 -0400 Subject: [PATCH 32/42] Remove external S7_object special case --- R/class-spec.R | 4 ++-- R/external-class.R | 21 ++------------------- man/new_external_class.Rd | 4 ---- tests/testthat/_snaps/method-register.md | 4 ++-- tests/testthat/test-external-class.R | 12 ++++++++++-- tests/testthat/test-inherits.R | 7 +++++-- tests/testthat/test-introspect.R | 12 ------------ tests/testthat/test-method-register.R | 10 +++++++--- 8 files changed, 28 insertions(+), 46 deletions(-) diff --git a/R/class-spec.R b/R/class-spec.R index 50369e98..6a0970ff 100644 --- a/R/class-spec.R +++ b/R/class-spec.R @@ -281,7 +281,7 @@ class_register <- function(x) { S7 = S7_class_name(x), S7_base = x$class, S7_S3 = x$class[[1]], - S7_external = external_class_register(x), + S7_external = x$class_name, stop2("Unsupported class type.", call = NULL) ) } @@ -330,7 +330,7 @@ class_inherits <- function(x, what) { }, S7_S3 = !isS4(x) && class_dispatch_extends(what$class, class(x)), S7_external = inherits(x, "S7_object") && - inherits(x, external_class_register(what)) && + inherits(x, what$class_name) && (is.null(what$version) || class_inherits(x, resolve_external_class_req(what))), ) diff --git a/R/external-class.R b/R/external-class.R index 1ee0e2a8..2c22a247 100644 --- a/R/external-class.R +++ b/R/external-class.R @@ -37,10 +37,6 @@ #' @returns An S7 external class, i.e. a list with S3 class `S7_external_class`. #' @export #' @examples -#' # Refer to an S7 class in another package without taking a hard dependency: -#' ExternalObject <- new_external_class("S7", "S7_object") -#' ExternalObject -#' #' # Self-referential class: the `child` property can be another `tree`, #' # or `NULL` to terminate the chain. #' tree_stub <- new_external_class("mypkg", "tree") @@ -103,16 +99,6 @@ print.S7_external_class <- function(x, ...) { invisible(x) } -external_class_register <- function(x) { - stopifnot(is_external_class(x)) - - if (identical(x$package, "S7") && identical(x$name, "S7_object")) { - "S7_object" - } else { - x$class_name - } -} - dep_available <- function(dep) { isNamespaceLoaded(dep$package) && dep_version_ok(dep) } @@ -153,11 +139,8 @@ find_external_class <- function(x) { is_external_class_match <- function(obj, x) { is_class(obj) && - ((identical(x$package, "S7") && - identical(x$name, "S7_object") && - identical(obj, S7_object)) || - (identical(obj@name, x$name) && - 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. diff --git a/man/new_external_class.Rd b/man/new_external_class.Rd index 75cf4ba8..2a12089c 100644 --- a/man/new_external_class.Rd +++ b/man/new_external_class.Rd @@ -47,10 +47,6 @@ 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 an S7 class in another package without taking a hard dependency: -ExternalObject <- new_external_class("S7", "S7_object") -ExternalObject - # Self-referential class: the `child` property can be another `tree`, # or `NULL` to terminate the chain. tree_stub <- new_external_class("mypkg", "tree") diff --git a/tests/testthat/_snaps/method-register.md b/tests/testthat/_snaps/method-register.md index 71959452..b49712b4 100644 --- a/tests/testthat/_snaps/method-register.md +++ b/tests/testthat/_snaps/method-register.md @@ -28,10 +28,10 @@ # method registration resolves external classes outside packages Code - env$g(S7_object()) + env$g(dep$Ext()) Condition Error: - ! Can't find method for `g()`. + ! Can't find method for `g()`. # method registration validates deferred external-class methods diff --git a/tests/testthat/test-external-class.R b/tests/testthat/test-external-class.R index 6f6d1ee9..26687a27 100644 --- a/tests/testthat/test-external-class.R +++ b/tests/testthat/test-external-class.R @@ -75,11 +75,19 @@ test_that("external class property validation reports validator errors", { }) test_that("external class property validation uses resolved dispatch", { + dep := local_package( + Ext := new_class() + ) Holder := new_class( - properties = list(x = new_external_class("S7", "S7_object")) + properties = list( + x = new_property( + class = new_external_class("dep", "Ext"), + default = quote(dep$Ext()) + ) + ) ) - expect_s3_class(Holder(x = S7_object())@x, "S7_object") + expect_s3_class(Holder(x = dep$Ext())@x, "dep::Ext") }) test_that("versioned external class checks package version", { diff --git a/tests/testthat/test-inherits.R b/tests/testthat/test-inherits.R index a5550277..6d9eb02f 100644 --- a/tests/testthat/test-inherits.R +++ b/tests/testthat/test-inherits.R @@ -26,9 +26,12 @@ test_that("accepts any class specification (#556)", { expect_true(S7_inherits("anything", class_any)) # external class + dep := local_package( + Ext := new_class() + ) expect_true(S7_inherits( - S7_object(), - new_external_class("S7", "S7_object") + dep$Ext(), + new_external_class("dep", "Ext") )) }) diff --git a/tests/testthat/test-introspect.R b/tests/testthat/test-introspect.R index fd48ae17..d3f756d3 100644 --- a/tests/testthat/test-introspect.R +++ b/tests/testthat/test-introspect.R @@ -97,18 +97,6 @@ test_that("S7_methods(class) scans attached generics", { ) }) -test_that("S7_methods(class) uses real key for external S7_object", { - gen_name <- "S7_introspect_s7_object_xyzzy" - gen <- new_generic(name = gen_name, dispatch_args = "x") - method(gen, S7_object) <- function(x) "s7" - - assign(gen_name, gen, envir = globalenv()) - defer(rm(list = gen_name, envir = globalenv())) - - res <- S7_methods(class = new_external_class("S7", "S7_object")) - expect_equal(res$generic[res$generic == gen_name], gen_name) -}) - test_that("S7_methods() reports the generic's package", { Foo <- new_class("Foo", package = NULL) gen <- new_generic("gen", "x") diff --git a/tests/testthat/test-method-register.R b/tests/testthat/test-method-register.R index 2b0a7816..acb8478e 100644 --- a/tests/testthat/test-method-register.R +++ b/tests/testthat/test-method-register.R @@ -65,18 +65,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" 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_snapshot(env$g(S7_object()), error = TRUE) + expect_snapshot(env$g(dep$Ext()), error = TRUE) }) test_that("method registration returns a strippable sentinel for foreign generics in a package (#364)", { From d8c83fbb7e3d2b0f60320cc5927e3e481dd895e4 Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Fri, 19 Jun 2026 14:58:45 -0400 Subject: [PATCH 33/42] Document generic sentinel invariant --- R/external-generic.R | 2 ++ 1 file changed, 2 insertions(+) diff --git a/R/external-generic.R b/R/external-generic.R index b1601b4c..a2ddef6c 100644 --- a/R/external-generic.R +++ b/R/external-generic.R @@ -42,6 +42,8 @@ new_external_generic <- function(package, name, dispatch_args, version = NULL) { as_external_generic <- function(x, env = parent.frame()) { 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)) { From 886451c410091b1281946ccd0e946dd170d06436 Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Fri, 19 Jun 2026 15:46:12 -0400 Subject: [PATCH 34/42] Trim-external-class-tests --- tests/testthat/_snaps/external-class.md | 1 - tests/testthat/_snaps/method-introspect.md | 6 --- tests/testthat/_snaps/method-register.md | 11 ----- tests/testthat/test-class-spec.R | 13 ------ tests/testthat/test-class.R | 34 ++++++--------- tests/testthat/test-external-class.R | 19 ++------- tests/testthat/test-hooks.R | 48 +++++----------------- tests/testthat/test-inherits.R | 9 ---- tests/testthat/test-method-introspect.R | 1 - tests/testthat/test-method-register.R | 19 ++++----- 10 files changed, 34 insertions(+), 127 deletions(-) diff --git a/tests/testthat/_snaps/external-class.md b/tests/testthat/_snaps/external-class.md index cb9085ad..87412d97 100644 --- a/tests/testthat/_snaps/external-class.md +++ b/tests/testthat/_snaps/external-class.md @@ -48,4 +48,3 @@ 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-introspect.md b/tests/testthat/_snaps/method-introspect.md index 72924a45..e6eeddcf 100644 --- a/tests/testthat/_snaps/method-introspect.md +++ b/tests/testthat/_snaps/method-introspect.md @@ -68,12 +68,6 @@ Error: ! Can't find external class : * Package 'not_a_package' is not installed. - Code - method_explain(foo, class = ext) - Condition - Error: - ! Can't find external class : - * Package 'not_a_package' is not installed. # method explanation shows all possible methods along with matches diff --git a/tests/testthat/_snaps/method-register.md b/tests/testthat/_snaps/method-register.md index b49712b4..0feca523 100644 --- a/tests/testthat/_snaps/method-register.md +++ b/tests/testthat/_snaps/method-register.md @@ -24,17 +24,6 @@ * An S3 class object (from `new_S3_class()`) * An S4 class object * A base class - -# method registration resolves external classes outside packages - - Code - env$g(dep$Ext()) - Condition - Error: - ! Can't find method for `g()`. - -# method registration validates deferred external-class methods - Code local_package("pkg_invalid_deferred_external_class_method", foo := new_generic( "x"), ext := new_external_class("notloaded.pkg"), method(foo, ext) <- diff --git a/tests/testthat/test-class-spec.R b/tests/testthat/test-class-spec.R index 6e74b4c9..74d77ef2 100644 --- a/tests/testthat/test-class-spec.R +++ b/tests/testthat/test-class-spec.R @@ -173,19 +173,6 @@ test_that("can work with S3 classes", { expect_equal(class_inherits(factor(), klass), FALSE) }) -test_that("external classes deparse as executable calls", { - ext <- new_external_class("foo", "Bar") - ext_versioned <- new_external_class("foo", "Bar", version = "1.0") - - expect_equal(class_deparse(ext), 'new_external_class("foo", "Bar")') - expect_equal( - class_deparse(ext_versioned), - 'new_external_class("foo", "Bar", version = "1.0")' - ) - expect_equal(eval(parse(text = class_deparse(ext))), ext) - expect_equal(eval(parse(text = class_deparse(ext_versioned))), ext_versioned) -}) - test_that("class_inherits() requires S3 classes to be contiguous and ordered", { klass <- new_S3_class(c("a", "b")) diff --git a/tests/testthat/test-class.R b/tests/testthat/test-class.R index 8fd8434a..8bdfbebd 100644 --- a/tests/testthat/test-class.R +++ b/tests/testthat/test-class.R @@ -160,7 +160,6 @@ test_that("inheritance handles external class property specs", { External := new_class() ) External <- new_external_class(package = "dep", name = "External") - Missing <- new_external_class(package = "S7testthatmissing", name = "Missing") ParentObject := new_class( properties = list(x = S7_object), @@ -173,34 +172,27 @@ test_that("inheritance handles external class property specs", { ), package = NULL ) - expect_s3_class(ChildObject(x = dep$External())@x, "dep::External") + 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 = new_property( - class = Ext, - default = quote({ - NULL - }) - ) - ), + properties = list(x = prop), package = NULL ) expect_no_error(new_class( name = "ChildSame", parent = ParentSame, - properties = list( - x = new_property( - class = Ext, - default = quote({ - NULL - }) - ) - ), + properties = list(x = prop), package = NULL )) + Missing <- new_external_class(package = "S7testthatmissing", name = "Missing") ParentUnion := new_class( properties = list( x = new_property( @@ -210,12 +202,12 @@ test_that("inheritance handles external class property specs", { ), package = NULL ) - expect_no_error(new_class( - name = "ChildUnion", + 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", { diff --git a/tests/testthat/test-external-class.R b/tests/testthat/test-external-class.R index 26687a27..3eedb694 100644 --- a/tests/testthat/test-external-class.R +++ b/tests/testthat/test-external-class.R @@ -66,6 +66,9 @@ test_that("external class property validation reports validator errors", { ) ) + 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 @@ -74,22 +77,6 @@ test_that("external class property validation reports validator errors", { expect_snapshot(Holder(child = invalid), error = TRUE) }) -test_that("external class property validation uses resolved dispatch", { - dep := local_package( - Ext := new_class() - ) - Holder := new_class( - properties = list( - x = new_property( - class = new_external_class("dep", "Ext"), - default = quote(dep$Ext()) - ) - ) - ) - - expect_s3_class(Holder(x = dep$Ext())@x, "dep::Ext") -}) - test_that("versioned external class checks package version", { versioned_pkg := local_package( Foo := new_class() diff --git a/tests/testthat/test-hooks.R b/tests/testthat/test-hooks.R index 5bc722aa..e065f64e 100644 --- a/tests/testthat/test-hooks.R +++ b/tests/testthat/test-hooks.R @@ -14,42 +14,22 @@ 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() - ) - downstream := local_package( - own_generic := new_generic("x"), - Foo := new_external_class("upstream"), - method(own_generic, Foo) <- \(x) "from external class" - ) - S7_on_load_(downstream) - expect_equal(downstream$own_generic(upstream$Foo()), "from external class") -}) - test_that("S7_on_load() waits until all external union arms are available", { generic_pkg <- local_package( - "upstream_external_union_generic", - gen := new_generic(dispatch_args = "x") - ) - upstream_a <- local_package( - "upstream_external_union_a", - A := new_class() + "external_union_generic", + gen := new_generic("x") ) + upstream_a <- local_package("external_union_a", A := new_class()) downstream <- local_package( - "downstream_external_union", + "external_union_downstream", .onLoad <- function(...) S7_on_load(), gen <- new_external_generic( - package = "upstream_external_union_generic", + package = "external_union_generic", name = "gen", dispatch_args = "x" ), - A := new_external_class( - package = "upstream_external_union_a" - ), - B := new_external_class( - package = "upstream_external_union_b" - ), + A := new_external_class(package = "external_union_a"), + B := new_external_class(package = "external_union_b"), method(gen, A | B) <- function(x) "union" ) @@ -59,26 +39,20 @@ test_that("S7_on_load() waits until all external union arms are available", { class = "S7_error_method_not_found" ) - upstream_b <- local_package( - "upstream_external_union_b", - B := new_class() - ) + 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_unload() unregisters methods dispatching on an external class", { - upstream <- local_package( - "upstream_external_class_unload", - Foo := new_class() - ) +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 = "upstream_external_class_unload"), + Foo := new_external_class(package = "external_class_unload"), method(own_generic, Foo) <- function(x) "from external class" ) downstream$.onLoad() diff --git a/tests/testthat/test-inherits.R b/tests/testthat/test-inherits.R index 6d9eb02f..73755a9d 100644 --- a/tests/testthat/test-inherits.R +++ b/tests/testthat/test-inherits.R @@ -24,15 +24,6 @@ test_that("accepts any class specification (#556)", { # class_any expect_true(S7_inherits("anything", class_any)) - - # external class - dep := local_package( - Ext := new_class() - ) - expect_true(S7_inherits( - dep$Ext(), - new_external_class("dep", "Ext") - )) }) test_that("checks that input is a class", { diff --git a/tests/testthat/test-method-introspect.R b/tests/testthat/test-method-introspect.R index f1a68067..b8c17764 100644 --- a/tests/testthat/test-method-introspect.R +++ b/tests/testthat/test-method-introspect.R @@ -42,7 +42,6 @@ test_that("method introspection requires external class's package to be loaded", ext <- new_external_class("not_a_package", "X") expect_snapshot(error = TRUE, { method(foo, class = ext) - method_explain(foo, class = ext) }) }) diff --git a/tests/testthat/test-method-register.R b/tests/testthat/test-method-register.R index acb8478e..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" + ) }) }) @@ -80,7 +86,7 @@ test_that("method registration resolves external classes outside packages", { expect_equal(env$g(dep$Ext()), "external") evalq(method(g, ext) <- NULL, env) - expect_snapshot(env$g(dep$Ext()), error = TRUE) + expect_length(methods(env$g), 0) }) test_that("method registration returns a strippable sentinel for foreign generics in a package (#364)", { @@ -101,17 +107,6 @@ test_that("method registration returns a strippable sentinel for foreign generic expect_s3_class(pkg$ext, "S7_generic_sentinel") }) -test_that("method registration validates deferred external-class methods", { - expect_snapshot(error = TRUE, { - local_package( - "pkg_invalid_deferred_external_class_method", - foo := new_generic("x"), - ext := new_external_class("notloaded.pkg"), - method(foo, ext) <- function(y) "x" - ) - }) -}) - test_that("method unregistration removes deferred external-class unions", { upstream := local_package( "upstream_external_unregister", From 1403a8a1f2eee6caf76839930dad76048694acc1 Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Fri, 19 Jun 2026 15:53:54 -0400 Subject: [PATCH 35/42] Clarify-external-package-vignette --- vignettes/packages.Rmd | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/vignettes/packages.Rmd b/vignettes/packages.Rmd index ad1e45a3..64361f4f 100644 --- a/vignettes/packages.Rmd +++ b/vignettes/packages.Rmd @@ -70,23 +70,17 @@ Note that methods can only be defined after both the class and generic have been ### Methods for suggested packages -Use `new_external_generic()` or `new_external_class()` when you want to register a method involving an S7 generic or class from a suggested package, 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 TheirClass <- new_external_class("theirpkg", "TheirClass") method(my_generic, TheirClass) <- function(x) { ... } -median <- new_external_generic("stats", "median", "x") -method(median, MyClass) <- function(x, ...) { ... } +their_generic <- new_external_generic("theirpkg", "their_generic", "x") +method(their_generic, MyClass) <- function(x) { ... } ``` -S7 will register these methods automatically when the suggested package is loaded, via `S7_on_load()` as described above. Use `new_S3_class()` for S3 classes from suggested packages: - -```{r, eval = FALSE} -ggplot <- new_S3_class("ggplot") -method(my_generic, ggplot) <- function(x) { ... } -``` +S7 will register these methods automatically when the suggested package is loaded, via `S7_on_load()` as described above. ## Backward compatibility From 00bdcf2be0fc143d527b0fbe9be180691f49110d Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Fri, 19 Jun 2026 16:26:43 -0400 Subject: [PATCH 36/42] Clarify external class resolution errors --- R/external-class.R | 11 ++++++++++- tests/testthat/_snaps/external-class.md | 10 ++++++++++ tests/testthat/test-external-class.R | 15 +++++++++++++++ 3 files changed, 35 insertions(+), 1 deletion(-) diff --git a/R/external-class.R b/R/external-class.R index 2c22a247..c7aef7ad 100644 --- a/R/external-class.R +++ b/R/external-class.R @@ -176,7 +176,16 @@ resolve_external_class_req <- function(x) { stop2( paste0( prefix, - sprintf("* Packages '%s' doesn't contain '%s'.", x$package, x$name) + sprintf( + paste0( + "* Package '%s' must bind an S7 class to `%s` with ", + "@name '%s' and @package '%s'." + ), + x$package, + x$name, + x$name, + x$package + ) ), call = NULL ) diff --git a/tests/testthat/_snaps/external-class.md b/tests/testthat/_snaps/external-class.md index 87412d97..76a966e6 100644 --- a/tests/testthat/_snaps/external-class.md +++ b/tests/testthat/_snaps/external-class.md @@ -22,6 +22,15 @@ Output foo::Bar (>= 1.0) +# 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'. + # external class works as a property type for self-reference Code @@ -48,3 +57,4 @@ 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/test-external-class.R b/tests/testthat/test-external-class.R index 3eedb694..5ff36473 100644 --- a/tests/testthat/test-external-class.R +++ b/tests/testthat/test-external-class.R @@ -19,6 +19,21 @@ test_that("external class is a valid class spec", { expect_equal(S7_class_desc(ec), "") }) +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") + + expect_snapshot(error = TRUE, { + new_class( + name = "Holder", + properties = list(child = Bar) + ) + }) +}) + 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") From f8659c434b4fe6da48d2dc83188d193d73237a0b Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Fri, 19 Jun 2026 16:47:51 -0400 Subject: [PATCH 37/42] Restore external class docs example --- R/external-class.R | 4 ++++ man/new_external_class.Rd | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/R/external-class.R b/R/external-class.R index c7aef7ad..6f429700 100644 --- a/R/external-class.R +++ b/R/external-class.R @@ -37,6 +37,10 @@ #' @returns An S7 external class, i.e. a list with S3 class `S7_external_class`. #' @export #' @examples +#' # 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. #' tree_stub <- new_external_class("mypkg", "tree") diff --git a/man/new_external_class.Rd b/man/new_external_class.Rd index 2a12089c..3a05f3f9 100644 --- a/man/new_external_class.Rd +++ b/man/new_external_class.Rd @@ -47,6 +47,10 @@ 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 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. tree_stub <- new_external_class("mypkg", "tree") From a1fea9a63f568cfebc0c6fb4da5c2dceffce45de Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Fri, 19 Jun 2026 16:50:05 -0400 Subject: [PATCH 38/42] Restore external dependency flattening helper --- R/external-class.R | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/R/external-class.R b/R/external-class.R index 6f429700..7011a7aa 100644 --- a/R/external-class.R +++ b/R/external-class.R @@ -75,14 +75,14 @@ class_external_deps <- function(x) { if (is_external_class(x)) { list(x) } else if (is_union(x)) { - unlist(lapply(x$classes, class_external_deps), recursive = FALSE) + flatten_external_deps(lapply(x$classes, class_external_deps)) } else { list() } } signature_external_deps <- function(signature) { - unlist(lapply(signature, class_external_deps), recursive = FALSE) + flatten_external_deps(lapply(signature, class_external_deps)) } external_deps_resolvable <- function(deps) { @@ -91,6 +91,10 @@ external_deps_resolvable <- function(deps) { })) } +flatten_external_deps <- function(x) { + unlist(x, recursive = FALSE, use.names = FALSE) +} + #' @export print.S7_external_class <- function(x, ...) { cat( From 4aee542931d7926a0f6157c7d0b8eaaaa774f2d6 Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Fri, 19 Jun 2026 16:52:19 -0400 Subject: [PATCH 39/42] Simplify external class error construction --- R/external-class.R | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/R/external-class.R b/R/external-class.R index 7011a7aa..6bd1d7ff 100644 --- a/R/external-class.R +++ b/R/external-class.R @@ -156,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 (!dep_version_ok(x)) { stop2( - paste0( + c( prefix, sprintf( "* Package '%s' needs version %s, but only %s is available.", @@ -181,18 +181,19 @@ 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( - paste0( - "* Package '%s' must bind an S7 class to `%s` with ", - "@name '%s' and @package '%s'." - ), + "* Package '%s' must bind an S7 class to %s.", x$package, - x$name, - x$name, - x$package + binding ) ), call = NULL From d0b8bee900df75f27c658c9d98bae4776255fb39 Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Fri, 19 Jun 2026 17:08:58 -0400 Subject: [PATCH 40/42] Snapshot external class binding mismatches --- tests/testthat/_snaps/external-class.md | 6 ++++++ tests/testthat/test-external-class.R | 12 ++++++++++++ 2 files changed, 18 insertions(+) diff --git a/tests/testthat/_snaps/external-class.md b/tests/testthat/_snaps/external-class.md index 76a966e6..dd3ac557 100644 --- a/tests/testthat/_snaps/external-class.md +++ b/tests/testthat/_snaps/external-class.md @@ -30,6 +30,12 @@ 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 diff --git a/tests/testthat/test-external-class.R b/tests/testthat/test-external-class.R index 5ff36473..6283fb78 100644 --- a/tests/testthat/test-external-class.R +++ b/tests/testthat/test-external-class.R @@ -25,12 +25,24 @@ test_that("external class resolution explains class binding contract", { 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) + ) }) }) From 3e0f377d2ffeb8dc542fc3446e3a4abec79a47f7 Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Fri, 19 Jun 2026 17:26:26 -0400 Subject: [PATCH 41/42] Restore external class default snapshot --- tests/testthat/_snaps/class.md | 9 +++++++++ tests/testthat/test-class.R | 10 ++++++++++ 2 files changed, 19 insertions(+) diff --git a/tests/testthat/_snaps/class.md b/tests/testthat/_snaps/class.md index 941c207a..606087f9 100644 --- a/tests/testthat/_snaps/class.md +++ b/tests/testthat/_snaps/class.md @@ -125,6 +125,15 @@ * An S4 class object * A base class +# external class property defaults require loaded packages + + Code + new_class("Child", Parent, properties = list(x = Ext)) + Condition + Error: + ! Can't find external class : + * Package 'notloaded.pkg' is not installed. + # inheritance doesn't let child properties widen or change the parent's type Code diff --git a/tests/testthat/test-class.R b/tests/testthat/test-class.R index 8bdfbebd..3ba46167 100644 --- a/tests/testthat/test-class.R +++ b/tests/testthat/test-class.R @@ -210,6 +210,16 @@ test_that("inheritance handles external class property specs", { expect_s3_class(ChildUnion(), "ChildUnion") }) +test_that("external class property defaults require loaded packages", { + Ext <- new_external_class("notloaded.pkg", "Cls") + Parent := new_class(properties = list(x = NULL | Ext), package = NULL) + + expect_snapshot( + new_class("Child", Parent, properties = list(x = Ext)), + error = TRUE + ) +}) + test_that("inheritance lets child properties narrow optional union properties with NULL", { Parent <- new_class( "Parent", From 37ffd2d4328c39f90b85623870811ce8720557f0 Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Fri, 19 Jun 2026 17:33:02 -0400 Subject: [PATCH 42/42] Reduce external class snapshot churn --- tests/testthat/_snaps/class.md | 18 ++++++------- tests/testthat/_snaps/external-class.md | 21 +++++++++++++++ tests/testthat/_snaps/method-introspect.md | 6 +++++ tests/testthat/test-class.R | 20 +++++++------- tests/testthat/test-external-class.R | 31 ++++++++++++++++++++++ tests/testthat/test-method-introspect.R | 1 + 6 files changed, 78 insertions(+), 19 deletions(-) diff --git a/tests/testthat/_snaps/class.md b/tests/testthat/_snaps/class.md index 606087f9..5e44a215 100644 --- a/tests/testthat/_snaps/class.md +++ b/tests/testthat/_snaps/class.md @@ -125,15 +125,6 @@ * An S4 class object * A base class -# external class property defaults require loaded packages - - Code - new_class("Child", Parent, properties = list(x = Ext)) - Condition - Error: - ! Can't find external class : - * Package 'notloaded.pkg' is not installed. - # inheritance doesn't let child properties widen or change the parent's type Code @@ -158,6 +149,15 @@ - @x is . - @x is . +# subclassing an external class requires its package to be loaded + + Code + new_class("Child", Parent, properties = list(x = Ext)) + Condition + Error: + ! Can't find external class : + * Package 'notloaded.pkg' is not installed. + # abstract classes can't be instantiated Code diff --git a/tests/testthat/_snaps/external-class.md b/tests/testthat/_snaps/external-class.md index dd3ac557..dd27ca79 100644 --- a/tests/testthat/_snaps/external-class.md +++ b/tests/testthat/_snaps/external-class.md @@ -22,6 +22,27 @@ Output foo::Bar (>= 1.0) +# resolve_external_class_req() errors per failure mode + + Code + resolve_external_class_req(new_external_class("not_a_pkg", "X")) + Condition + Error: + ! Can't find external class : + * Package 'not_a_pkg' is not installed. + Code + resolve_external_class_req(new_external_class("S7", "S7_object", "2.0.0")) + Condition + Error: + ! Can't find external class : + * Package 'S7' needs version 2.0.0, but only 1.0.0 is available. + Code + resolve_external_class_req(new_external_class("S7", "not_a_class")) + Condition + Error: + ! Can't find external 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 diff --git a/tests/testthat/_snaps/method-introspect.md b/tests/testthat/_snaps/method-introspect.md index e6eeddcf..72924a45 100644 --- a/tests/testthat/_snaps/method-introspect.md +++ b/tests/testthat/_snaps/method-introspect.md @@ -68,6 +68,12 @@ Error: ! Can't find external class : * Package 'not_a_package' is not installed. + Code + method_explain(foo, class = ext) + Condition + Error: + ! Can't find external class : + * Package 'not_a_package' is not installed. # method explanation shows all possible methods along with matches diff --git a/tests/testthat/test-class.R b/tests/testthat/test-class.R index 3ba46167..cd1747e7 100644 --- a/tests/testthat/test-class.R +++ b/tests/testthat/test-class.R @@ -210,16 +210,6 @@ test_that("inheritance handles external class property specs", { expect_s3_class(ChildUnion(), "ChildUnion") }) -test_that("external class property defaults require loaded packages", { - Ext <- new_external_class("notloaded.pkg", "Cls") - Parent := new_class(properties = list(x = NULL | Ext), package = NULL) - - expect_snapshot( - new_class("Child", Parent, properties = list(x = Ext)), - error = TRUE - ) -}) - test_that("inheritance lets child properties narrow optional union properties with NULL", { Parent <- new_class( "Parent", @@ -259,6 +249,16 @@ test_that("inheritance doesn't let child properties widen or change the parent's }) +test_that("subclassing an external class requires its package to be loaded", { + Ext <- new_external_class("notloaded.pkg", "Cls") + Parent := new_class(properties = list(x = NULL | Ext), package = NULL) + + expect_snapshot( + new_class("Child", Parent, properties = list(x = Ext)), + error = TRUE + ) +}) + test_that("inheritance lets dynamic child properties override any parent type", { foo1 <- new_class("foo1", properties = list(x = class_integer)) readonly <- new_property(class_character, getter = function(self) "x") diff --git a/tests/testthat/test-external-class.R b/tests/testthat/test-external-class.R index 6283fb78..ffb55fff 100644 --- a/tests/testthat/test-external-class.R +++ b/tests/testthat/test-external-class.R @@ -19,6 +19,15 @@ test_that("external class is a valid class spec", { expect_equal(S7_class_desc(ec), "") }) +test_that("resolve_external_class_req() errors per failure mode", { + local_mocked_bindings(getNamespaceVersion = function(package) "1.0.0") + expect_snapshot(error = TRUE, { + resolve_external_class_req(new_external_class("not_a_pkg", "X")) + resolve_external_class_req(new_external_class("S7", "S7_object", "2.0.0")) + resolve_external_class_req(new_external_class("S7", "not_a_class")) + }) +}) + test_that("external class resolution explains class binding contract", { local_package( "dep", @@ -46,6 +55,13 @@ test_that("external class resolution explains class binding contract", { }) }) +test_that("external class can be used as a union arm", { + ec <- new_external_class("foo", "Bar") + u <- NULL | ec + expect_s3_class(u, "S7_union") + 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") @@ -73,6 +89,21 @@ test_that("external class works as a property type for self-reference", { expect_snapshot(error = TRUE, Tree(label = "bad", child = 1)) }) +test_that("external class works for mutually recursive classes", { + ClassOne := new_class( + package = "mypkg", + properties = list(x = NULL | new_external_class("mypkg", "ClassTwo")) + ) + ClassTwo := new_class( + package = "mypkg", + properties = list(y = NULL | new_external_class("mypkg", "ClassOne")) + ) + + obj <- ClassOne(x = ClassTwo(y = ClassOne())) + expect_s3_class(obj@x, "mypkg::ClassTwo") + expect_s3_class(obj@x@y, "mypkg::ClassOne") +}) + test_that("external class property validation reports validator errors", { dep := local_package( Ext := new_class( diff --git a/tests/testthat/test-method-introspect.R b/tests/testthat/test-method-introspect.R index b8c17764..f1a68067 100644 --- a/tests/testthat/test-method-introspect.R +++ b/tests/testthat/test-method-introspect.R @@ -42,6 +42,7 @@ test_that("method introspection requires external class's package to be loaded", ext <- new_external_class("not_a_package", "X") expect_snapshot(error = TRUE, { method(foo, class = ext) + method_explain(foo, class = ext) }) })