From 18d7bf061bc8db3e2a790809179de11272d02b84 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Mon, 21 Feb 2022 17:13:34 -0600 Subject: [PATCH 01/18] Require explicit registration for S4 Rather calling `setOldClass()` during method registeration (which feels too magical) or on class creation (which feels to eager), we now require the user to explicitly call S4_register(). --- NAMESPACE | 1 + R/S4.R | 16 ++++++++++++++++ R/method-register.R | 16 +++++++++++----- _pkgdown.yml | 1 + man/S4_register.Rd | 17 +++++++++++++++++ tests/testthat/_snaps/method-register.md | 4 ++++ tests/testthat/test-method-register.R | 9 ++++++--- 7 files changed, 56 insertions(+), 8 deletions(-) create mode 100644 man/S4_register.Rd diff --git a/NAMESPACE b/NAMESPACE index 67ab37c3..03bf7b0e 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -27,6 +27,7 @@ export("prop<-") export("props<-") export(R7_data) export(R7_object) +export(S4_register) export(any_class) export(as_class) export(external_methods_register) diff --git a/R/S4.R b/R/S4.R index a7ea76b0..2676a99c 100644 --- a/R/S4.R +++ b/R/S4.R @@ -1,3 +1,19 @@ +#' Register an R7 class with S4 +#' +#' If you want to use [method<-] to register an method for an S4 generic with +#' an R7 class, you need to call `S4_register()` once. +#' +#' @param class An R7 class created with [new_class()]. +#' @param env Expert use only. Environment where S4 class will be registered. +#' @export +S4_register <- function(class, env = parent.frame()) { + if (!is_class(class)) { + msg <- sprintf("`class` must be an R7 class, not a %s", obj_desc(class)) + } + + methods::setOldClass(class_dispatch(class), where = topenv(env)) +} + is_S4_class <- function(x) inherits(x, "classRepresentation") S4_to_R7_class <- function(x, error_base = "") { diff --git a/R/method-register.R b/R/method-register.R index 5a999631..de0decdc 100644 --- a/R/method-register.R +++ b/R/method-register.R @@ -217,10 +217,8 @@ check_method <- function(method, generic, name = paste0(generic@name, "(???)")) register_S4_method <- function(generic, signature, method, env = parent.frame()) { S4_env <- topenv(env) - S4_signature <- lapply(signature, S4_class, S4_env = S4_env) methods::setMethod(generic, S4_signature, method, where = S4_env) - } S4_class <- function(x, S4_env) { if (is_base_class(x)) { @@ -228,9 +226,17 @@ S4_class <- function(x, S4_env) { } else if (is_S4_class(x)) { x } else if (is_class(x) || is_S3_class(x)) { - class <- class_dispatch(x) - methods::setOldClass(class, where = S4_env) - methods::getClass(class) + class <- tryCatch(methods::getClass(class_register(x)), error = function(err) NULL) + if (is.null(class)) { + msg <- sprintf( + "Class has not been registered with S4; please call S4_register(%s)", + class_deparse(x) + ) + stop(msg, call. = FALSE) + } + class + } else { + stop("Unsupported") } } diff --git a/_pkgdown.yml b/_pkgdown.yml index b242c191..bcc2d35e 100644 --- a/_pkgdown.yml +++ b/_pkgdown.yml @@ -27,6 +27,7 @@ reference: - new_S3_class - missing_class - object_class + - S4_register articles: - title: Learn R7 diff --git a/man/S4_register.Rd b/man/S4_register.Rd new file mode 100644 index 00000000..75b51a0e --- /dev/null +++ b/man/S4_register.Rd @@ -0,0 +1,17 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/S4.R +\name{S4_register} +\alias{S4_register} +\title{Register an R7 class with S4} +\usage{ +S4_register(class, env = parent.frame()) +} +\arguments{ +\item{class}{An R7 class created with \code{\link[=new_class]{new_class()}}.} + +\item{env}{Expert use only. Environment where S4 class will be registered.} +} +\description{ +If you want to use \link{method<-} to register an method for an S4 generic with +an R7 class, you need to call \code{S4_register()} once. +} diff --git a/tests/testthat/_snaps/method-register.md b/tests/testthat/_snaps/method-register.md index d5c60201..78756653 100644 --- a/tests/testthat/_snaps/method-register.md +++ b/tests/testthat/_snaps/method-register.md @@ -5,6 +5,10 @@ Error When registering methods for S3 generic sum(), signature must be an R7 class, not an S3 class. +# method registration: can register R7 method for S4 generic + + Class has not been registered with S4; please call S4_register(S4foo) + # method registration: checks argument types Code diff --git a/tests/testthat/test-method-register.R b/tests/testthat/test-method-register.R index e3d7d961..8b3b09b3 100644 --- a/tests/testthat/test-method-register.R +++ b/tests/testthat/test-method-register.R @@ -54,10 +54,13 @@ describe("method registration", { it("can register R7 method for S4 generic", { methods::setGeneric("bar", function(x) standardGeneric("bar")) - foo <- new_class("foo") - method(bar, foo) <- function(x) "foo" + S4foo <- new_class("S4foo") + + expect_snapshot_error(method(bar, S4foo) <- function(x) "foo") - expect_equal(bar(foo()), "foo") + S4_register(S4foo) + method(bar, S4foo) <- function(x) "foo" + expect_equal(bar(S4foo()), "foo") }) it("checks argument types", { From cf55ffd73bedaeeefad9985f336431e74875178b Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 22 Mar 2022 12:50:11 -0500 Subject: [PATCH 02/18] Create full S4 class --- R/S4.R | 6 +++++- tests/testthat/test-S4.R | 15 +++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/R/S4.R b/R/S4.R index b3a39e82..d17308c1 100644 --- a/R/S4.R +++ b/R/S4.R @@ -11,7 +11,11 @@ S4_register <- function(class, env = parent.frame()) { msg <- sprintf("`class` must be an R7 class, not a %s", obj_desc(class)) } - methods::setOldClass(class_dispatch(class), where = topenv(env)) + name <- class@name + contains <- setdiff(class_dispatch(class), "ANY")[-1] + + methods::setClass(name, contains = contains, where = topenv(env)) + methods::setOldClass(name, S4Class = name, where = topenv(env)) } is_S4_class <- function(x) inherits(x, "classRepresentation") diff --git a/tests/testthat/test-S4.R b/tests/testthat/test-S4.R index 564e94c0..69fb4239 100644 --- a/tests/testthat/test-S4.R +++ b/tests/testthat/test-S4.R @@ -108,3 +108,18 @@ describe("S4_class_dispatch", { expect_equal(S4_class_dispatch("Foo1"), "S4/mypkg::Foo1") }) }) + +describe("S4 registration", { + + it("can register simple class hierarchy", { + foo <- new_class("foo") + foo2 <- new_class("foo2", foo) + + S4_register(foo) + S4_register(foo2) + + expect_s4_class(getClass("foo"), "classRepresentation") + expect_s4_class(getClass("foo2"), "classRepresentation") + }) + +}) From 0034882708cd11019773e4e8b798e62be222aa40 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 22 Mar 2022 12:55:48 -0500 Subject: [PATCH 03/18] More work --- R/S4.R | 2 ++ tests/testthat/test-S4.R | 22 ++++++++++++++++------ 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/R/S4.R b/R/S4.R index d17308c1..239f30aa 100644 --- a/R/S4.R +++ b/R/S4.R @@ -9,12 +9,14 @@ S4_register <- function(class, env = parent.frame()) { if (!is_class(class)) { msg <- sprintf("`class` must be an R7 class, not a %s", obj_desc(class)) + stop(msg) } name <- class@name contains <- setdiff(class_dispatch(class), "ANY")[-1] methods::setClass(name, contains = contains, where = topenv(env)) + methods::setValidity(name, function(object) validate(object), where = topenv(env)) methods::setOldClass(name, S4Class = name, where = topenv(env)) } diff --git a/tests/testthat/test-S4.R b/tests/testthat/test-S4.R index 69fb4239..2e5c4d64 100644 --- a/tests/testthat/test-S4.R +++ b/tests/testthat/test-S4.R @@ -112,14 +112,24 @@ describe("S4_class_dispatch", { describe("S4 registration", { it("can register simple class hierarchy", { - foo <- new_class("foo") - foo2 <- new_class("foo2", foo) + sfoo <- new_class("sfoo") + sfoo2 <- new_class("sfoo2", sfoo) - S4_register(foo) - S4_register(foo2) + S4_register(sfoo) + S4_register(sfoo2) + + expect_s4_class(getClass("sfoo"), "classRepresentation") + expect_s4_class(getClass("sfoo2"), "classRepresentation") + }) + + test_that("S4 validation triggers R7 validation", { + sfoo3 <- new_class("sfoo3", parent = class_character) + S4_register(sfoo3) + + # # Create invalid object + # obj <- foo3() + # mode(obj) <- "integer" - expect_s4_class(getClass("foo"), "classRepresentation") - expect_s4_class(getClass("foo2"), "classRepresentation") }) }) From d22838d4495c3c221b9d111b12caad667b9e9634 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 22 Mar 2022 13:14:39 -0500 Subject: [PATCH 04/18] Test validity --- R/S4.R | 2 +- tests/testthat/test-S4.R | 30 +++++++++++++++++++----------- 2 files changed, 20 insertions(+), 12 deletions(-) diff --git a/R/S4.R b/R/S4.R index 239f30aa..f14e1f42 100644 --- a/R/S4.R +++ b/R/S4.R @@ -17,7 +17,7 @@ S4_register <- function(class, env = parent.frame()) { methods::setClass(name, contains = contains, where = topenv(env)) methods::setValidity(name, function(object) validate(object), where = topenv(env)) - methods::setOldClass(name, S4Class = name, where = topenv(env)) + methods::setOldClass(c(name, contains), S4Class = name, where = topenv(env)) } is_S4_class <- function(x) inherits(x, "classRepresentation") diff --git a/tests/testthat/test-S4.R b/tests/testthat/test-S4.R index 2e5c4d64..d9a35e82 100644 --- a/tests/testthat/test-S4.R +++ b/tests/testthat/test-S4.R @@ -112,24 +112,32 @@ describe("S4_class_dispatch", { describe("S4 registration", { it("can register simple class hierarchy", { - sfoo <- new_class("sfoo") - sfoo2 <- new_class("sfoo2", sfoo) + foo <- new_class("foo") + foo2 <- new_class("foo2", foo) - S4_register(sfoo) - S4_register(sfoo2) + S4_register(foo) + S4_register(foo2) - expect_s4_class(getClass("sfoo"), "classRepresentation") - expect_s4_class(getClass("sfoo2"), "classRepresentation") + expect_s4_class(getClass("foo"), "classRepresentation") + expect_s4_class(getClass("foo2"), "classRepresentation") }) test_that("S4 validation triggers R7 validation", { - sfoo3 <- new_class("sfoo3", parent = class_character) - S4_register(sfoo3) + foo3 <- new_class("foo3", + parent = class_integer, + validator = function(self) { + if (R7_data(self) < 0) "Must be positive" + } + ) + # Create invalid object + R7_obj <- foo3(1L) + R7_obj[[1]] <- -1L - # # Create invalid object - # obj <- foo3() - # mode(obj) <- "integer" + S4_register(foo3) + Foo <- setClass("Foo", slots = list(x = "foo3")) + S4_obj <- Foo(x = R7_obj) + expect_error(validObject(S4_obj, complete = TRUE), "Must be positive") }) }) From e43a926ce6dbdf2d0df56910e14419b0be3b5dfe Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 22 Mar 2022 13:16:23 -0500 Subject: [PATCH 05/18] Paper over double <-> numeric diff --- R/S4.R | 1 + tests/testthat/test-S4.R | 8 +++++++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/R/S4.R b/R/S4.R index f14e1f42..0e10786b 100644 --- a/R/S4.R +++ b/R/S4.R @@ -14,6 +14,7 @@ S4_register <- function(class, env = parent.frame()) { name <- class@name contains <- setdiff(class_dispatch(class), "ANY")[-1] + contains[contains == "double"] <- "numeric" methods::setClass(name, contains = contains, where = topenv(env)) methods::setValidity(name, function(object) validate(object), where = topenv(env)) diff --git a/tests/testthat/test-S4.R b/tests/testthat/test-S4.R index d9a35e82..189c3028 100644 --- a/tests/testthat/test-S4.R +++ b/tests/testthat/test-S4.R @@ -110,7 +110,6 @@ describe("S4_class_dispatch", { }) describe("S4 registration", { - it("can register simple class hierarchy", { foo <- new_class("foo") foo2 <- new_class("foo2", foo) @@ -122,6 +121,13 @@ describe("S4 registration", { expect_s4_class(getClass("foo2"), "classRepresentation") }) + test_that("can register class that inherits from double", { + foo <- new_class("foo", parent = class_double) + S4_register(foo) + + expect_type(new("foo"), "double") + }) + test_that("S4 validation triggers R7 validation", { foo3 <- new_class("foo3", parent = class_integer, From 0bae16cc07dfd4859785d5c016898a4aee4945bc Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 22 Mar 2022 14:19:30 -0500 Subject: [PATCH 06/18] Rough pass of slots conversion --- R/S4.R | 9 ++++++++- tests/testthat/test-S4.R | 10 ++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/R/S4.R b/R/S4.R index 0e10786b..80e27448 100644 --- a/R/S4.R +++ b/R/S4.R @@ -16,7 +16,14 @@ S4_register <- function(class, env = parent.frame()) { contains <- setdiff(class_dispatch(class), "ANY")[-1] contains[contains == "double"] <- "numeric" - methods::setClass(name, contains = contains, where = topenv(env)) + props <- class@properties + if (is_class(class@parent) && class@parent@name != "R7_object") { + parent_props <- class@parent@properties + props <- props[setdiff(names(props), names(parent_props))] + } + slots <- lapply(props, function(x) class_register(x$class)) + + methods::setClass(name, contains = contains, slots = slots, where = topenv(env)) methods::setValidity(name, function(object) validate(object), where = topenv(env)) methods::setOldClass(c(name, contains), S4Class = name, where = topenv(env)) } diff --git a/tests/testthat/test-S4.R b/tests/testthat/test-S4.R index 189c3028..9a8bb00a 100644 --- a/tests/testthat/test-S4.R +++ b/tests/testthat/test-S4.R @@ -146,4 +146,14 @@ describe("S4 registration", { expect_error(validObject(S4_obj, complete = TRUE), "Must be positive") }) + test_that("can register slots", { + foo <- new_class("foo", properties = list(x = class_integer)) + foo2 <- new_class("foo2", foo, properties = list(y = class_character)) + + S4_register(foo) + S4_register(foo2) + expect_equal(getClass("foo")@slots$x, structure("integer", package = "methods")) + expect_equal(getClass("foo2")@slots$x, structure("integer", package = "methods")) + expect_equal(getClass("foo2")@slots$y, structure("character", package = "methods")) + }) }) From bf8bfb0261b4719a902a16075f5068dd6e0f851e Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 22 Mar 2022 14:26:08 -0500 Subject: [PATCH 07/18] Test doubles in properties --- R/S4.R | 23 ++++++++++++++++++++--- tests/testthat/test-S4.R | 19 ++++++++++++------- 2 files changed, 32 insertions(+), 10 deletions(-) diff --git a/R/S4.R b/R/S4.R index 80e27448..b53758d9 100644 --- a/R/S4.R +++ b/R/S4.R @@ -13,15 +13,14 @@ S4_register <- function(class, env = parent.frame()) { } name <- class@name - contains <- setdiff(class_dispatch(class), "ANY")[-1] - contains[contains == "double"] <- "numeric" + contains <- double_to_numeric(setdiff(class_dispatch(class), "ANY")[-1]) props <- class@properties if (is_class(class@parent) && class@parent@name != "R7_object") { parent_props <- class@parent@properties props <- props[setdiff(names(props), names(parent_props))] } - slots <- lapply(props, function(x) class_register(x$class)) + slots <- lapply(props, function(x) R7_to_S4_class(x$class)) methods::setClass(name, contains = contains, slots = slots, where = topenv(env)) methods::setValidity(name, function(object) validate(object), where = topenv(env)) @@ -65,6 +64,24 @@ S4_to_R7_class <- function(x, error_base = "") { } } +R7_to_S4_class <- function(x) { + switch(class_type(x), + NULL = "NULL", + S4 = S4_class_name(x), + R7 = R7_class_name(x), + R7_base = double_to_numeric(x$class), + R7_S3 = x$class[[1]], + R7_union = "ANY", + stop("Unsupported") + ) +} + +# S4 uniformly uses numeric to mean double +double_to_numeric <- function(x) { + x[x == "double"] <- "numeric" + x +} + S4_base_classes <- function() { list( NULL = NULL, diff --git a/tests/testthat/test-S4.R b/tests/testthat/test-S4.R index 9a8bb00a..357275ac 100644 --- a/tests/testthat/test-S4.R +++ b/tests/testthat/test-S4.R @@ -121,13 +121,6 @@ describe("S4 registration", { expect_s4_class(getClass("foo2"), "classRepresentation") }) - test_that("can register class that inherits from double", { - foo <- new_class("foo", parent = class_double) - S4_register(foo) - - expect_type(new("foo"), "double") - }) - test_that("S4 validation triggers R7 validation", { foo3 <- new_class("foo3", parent = class_integer, @@ -156,4 +149,16 @@ describe("S4 registration", { expect_equal(getClass("foo2")@slots$x, structure("integer", package = "methods")) expect_equal(getClass("foo2")@slots$y, structure("character", package = "methods")) }) + + test_that("can handle doubles correct", { + foo <- new_class("foo", + parent = class_double, + properties = list(x = class_double) + ) + S4_register(foo) + + obj <- new("foo") + expect_type(obj, "double") + expect_type(slot(obj, "x"), "double") + }) }) From a24f0a8e9b16ccfc8b088bcd8b4e87297c9d4a57 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 22 Mar 2022 14:30:38 -0500 Subject: [PATCH 08/18] Tweak tests to use unique class names --- tests/testthat/test-S4.R | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/tests/testthat/test-S4.R b/tests/testthat/test-S4.R index 357275ac..550afcf4 100644 --- a/tests/testthat/test-S4.R +++ b/tests/testthat/test-S4.R @@ -122,6 +122,8 @@ describe("S4 registration", { }) test_that("S4 validation triggers R7 validation", { + on.exit(S4_remove_classes("Foo")) + foo3 <- new_class("foo3", parent = class_integer, validator = function(self) { @@ -140,24 +142,24 @@ describe("S4 registration", { }) test_that("can register slots", { - foo <- new_class("foo", properties = list(x = class_integer)) - foo2 <- new_class("foo2", foo, properties = list(y = class_character)) - - S4_register(foo) - S4_register(foo2) - expect_equal(getClass("foo")@slots$x, structure("integer", package = "methods")) - expect_equal(getClass("foo2")@slots$x, structure("integer", package = "methods")) - expect_equal(getClass("foo2")@slots$y, structure("character", package = "methods")) + foo4 <- new_class("foo4", properties = list(x = class_integer)) + foo5 <- new_class("foo5", foo4, properties = list(y = class_character)) + + S4_register(foo4) + S4_register(foo5) + expect_equal(getClass("foo4")@slots$x, structure("integer", package = "methods")) + expect_equal(getClass("foo5")@slots$x, structure("integer", package = "methods")) + expect_equal(getClass("foo5")@slots$y, structure("character", package = "methods")) }) test_that("can handle doubles correct", { - foo <- new_class("foo", + foo6 <- new_class("foo6", parent = class_double, properties = list(x = class_double) ) - S4_register(foo) + S4_register(foo6) - obj <- new("foo") + obj <- new("foo6") expect_type(obj, "double") expect_type(slot(obj, "x"), "double") }) From ade4371e8c968039512d639b838b59879c0b4382 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 22 Mar 2022 14:33:22 -0500 Subject: [PATCH 09/18] Improve docs --- R/S4.R | 16 +++++++++++++++- man/S4_register.Rd | 14 +++++++++++++- 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/R/S4.R b/R/S4.R index b53758d9..331e04e5 100644 --- a/R/S4.R +++ b/R/S4.R @@ -1,7 +1,19 @@ #' Register an R7 class with S4 #' +#' @description #' If you want to use [method<-] to register an method for an S4 generic with -#' an R7 class, you need to call `S4_register()` once. +#' an R7 class, you need to call `S4_register()` once. This generates a full +#' S4 class specification that: +#' +#' * Matches +#' * Uses [validate()] as the validity method. +#' * Defines formal S4 slots to match R7 properties. The slot types are +#' matched to the R7 property types, with the exception of R7 unions, +#' which are unchecked (due to the challenges of converting R7 unions to +#' S4 unions). +#' +#' When registering a class that extends R7 class or specifies an R7 class for +#' a property, you must register those classes first. #' #' @param class An R7 class created with [new_class()]. #' @param env Expert use only. Environment where S4 class will be registered. @@ -15,6 +27,7 @@ S4_register <- function(class, env = parent.frame()) { name <- class@name contains <- double_to_numeric(setdiff(class_dispatch(class), "ANY")[-1]) + # S4 inherits slots from parent class, so props <- class@properties if (is_class(class@parent) && class@parent@name != "R7_object") { parent_props <- class@parent@properties @@ -67,6 +80,7 @@ S4_to_R7_class <- function(x, error_base = "") { R7_to_S4_class <- function(x) { switch(class_type(x), NULL = "NULL", + any = "ANY", S4 = S4_class_name(x), R7 = R7_class_name(x), R7_base = double_to_numeric(x$class), diff --git a/man/S4_register.Rd b/man/S4_register.Rd index 75b51a0e..e78df235 100644 --- a/man/S4_register.Rd +++ b/man/S4_register.Rd @@ -13,5 +13,17 @@ S4_register(class, env = parent.frame()) } \description{ If you want to use \link{method<-} to register an method for an S4 generic with -an R7 class, you need to call \code{S4_register()} once. +an R7 class, you need to call \code{S4_register()} once. This generates a full +S4 class specification that: +\itemize{ +\item Matches +\item Uses \code{\link[=validate]{validate()}} as the validity method. +\item Defines formal S4 slots to match R7 properties. The slot types are +matched to the R7 property types, with the exception of R7 unions, +which are unchecked (due to the challenges of converting R7 unions to +S4 unions). +} + +When registering a class that extends R7 class or specifies an R7 class for +a property, you must register those classes first. } From 28837629ed2e998e8b754bdd66f353e36ec940eb Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 22 Mar 2022 14:34:40 -0500 Subject: [PATCH 10/18] Tweak test names --- tests/testthat/test-S4.R | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/testthat/test-S4.R b/tests/testthat/test-S4.R index 550afcf4..844ad812 100644 --- a/tests/testthat/test-S4.R +++ b/tests/testthat/test-S4.R @@ -121,7 +121,7 @@ describe("S4 registration", { expect_s4_class(getClass("foo2"), "classRepresentation") }) - test_that("S4 validation triggers R7 validation", { + it("ties S4 validation to R7 validation", { on.exit(S4_remove_classes("Foo")) foo3 <- new_class("foo3", @@ -141,7 +141,7 @@ describe("S4 registration", { expect_error(validObject(S4_obj, complete = TRUE), "Must be positive") }) - test_that("can register slots", { + it("can register slots", { foo4 <- new_class("foo4", properties = list(x = class_integer)) foo5 <- new_class("foo5", foo4, properties = list(y = class_character)) @@ -152,7 +152,7 @@ describe("S4 registration", { expect_equal(getClass("foo5")@slots$y, structure("character", package = "methods")) }) - test_that("can handle doubles correct", { + it("translates double to numeric", { foo6 <- new_class("foo6", parent = class_double, properties = list(x = class_double) @@ -163,4 +163,6 @@ describe("S4 registration", { expect_type(obj, "double") expect_type(slot(obj, "x"), "double") }) + + }) From 7af0a8ba044661bf7e3eceefadb32079f27888cc Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 22 Mar 2022 14:35:57 -0500 Subject: [PATCH 11/18] Add test for input checking --- tests/testthat/_snaps/S4.md | 7 +++++++ tests/testthat/test-S4.R | 4 +++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/tests/testthat/_snaps/S4.md b/tests/testthat/_snaps/S4.md index fa02a921..2ddbbb12 100644 --- a/tests/testthat/_snaps/S4.md +++ b/tests/testthat/_snaps/S4.md @@ -5,3 +5,10 @@ Error Unsupported S4 object: must be a class generator or a class definition, not a . +# S4 registration: checks it's inputs + + Code + S4_register("x") + Error + `class` must be an R7 class, not a + diff --git a/tests/testthat/test-S4.R b/tests/testthat/test-S4.R index 844ad812..ae5522b4 100644 --- a/tests/testthat/test-S4.R +++ b/tests/testthat/test-S4.R @@ -164,5 +164,7 @@ describe("S4 registration", { expect_type(slot(obj, "x"), "double") }) - + it("checks it's inputs", { + expect_snapshot(S4_register("x"), error = TRUE) + }) }) From ea8f87ed4b7f495cb6e5c7fcdc4bb2c66e9a73aa Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Wed, 23 Mar 2022 12:49:11 -0500 Subject: [PATCH 12/18] Tweak docs --- R/S4.R | 15 ++++++++------- man/S4_register.Rd | 12 ++++++------ 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/R/S4.R b/R/S4.R index 331e04e5..208010d4 100644 --- a/R/S4.R +++ b/R/S4.R @@ -1,19 +1,19 @@ #' Register an R7 class with S4 #' #' @description -#' If you want to use [method<-] to register an method for an S4 generic with -#' an R7 class, you need to call `S4_register()` once. This generates a full -#' S4 class specification that: +#' If you want to use and R7 class with S4 (e.g. to use [method<-] to register an +#' method for an S4 generic with an R7 class) you need to call `S4_register()` +#' once. This generates a full S4 class specification that: #' -#' * Matches +#' * Matches class name and inheritance hierarchy. #' * Uses [validate()] as the validity method. #' * Defines formal S4 slots to match R7 properties. The slot types are #' matched to the R7 property types, with the exception of R7 unions, #' which are unchecked (due to the challenges of converting R7 unions to #' S4 unions). #' -#' When registering a class that extends R7 class or specifies an R7 class for -#' a property, you must register those classes first. +#' If `class` extends another R7 class or has a property restricted to an +#' R7 class, you you must register those classes first. #' #' @param class An R7 class created with [new_class()]. #' @param env Expert use only. Environment where S4 class will be registered. @@ -27,7 +27,7 @@ S4_register <- function(class, env = parent.frame()) { name <- class@name contains <- double_to_numeric(setdiff(class_dispatch(class), "ANY")[-1]) - # S4 inherits slots from parent class, so + # S4 classes inherits slots from parent but R7 classes flatten props <- class@properties if (is_class(class@parent) && class@parent@name != "R7_object") { parent_props <- class@parent@properties @@ -38,6 +38,7 @@ S4_register <- function(class, env = parent.frame()) { methods::setClass(name, contains = contains, slots = slots, where = topenv(env)) methods::setValidity(name, function(object) validate(object), where = topenv(env)) methods::setOldClass(c(name, contains), S4Class = name, where = topenv(env)) + invisible() } is_S4_class <- function(x) inherits(x, "classRepresentation") diff --git a/man/S4_register.Rd b/man/S4_register.Rd index e78df235..efb74665 100644 --- a/man/S4_register.Rd +++ b/man/S4_register.Rd @@ -12,11 +12,11 @@ S4_register(class, env = parent.frame()) \item{env}{Expert use only. Environment where S4 class will be registered.} } \description{ -If you want to use \link{method<-} to register an method for an S4 generic with -an R7 class, you need to call \code{S4_register()} once. This generates a full -S4 class specification that: +If you want to use and R7 class with S4 (e.g. to use \link{method<-} to register an +method for an S4 generic with an R7 class) you need to call \code{S4_register()} +once. This generates a full S4 class specification that: \itemize{ -\item Matches +\item Matches class name and inheritance hierarchy. \item Uses \code{\link[=validate]{validate()}} as the validity method. \item Defines formal S4 slots to match R7 properties. The slot types are matched to the R7 property types, with the exception of R7 unions, @@ -24,6 +24,6 @@ which are unchecked (due to the challenges of converting R7 unions to S4 unions). } -When registering a class that extends R7 class or specifies an R7 class for -a property, you must register those classes first. +If \code{class} extends another R7 class or has a property restricted to an +R7 class, you you must register those classes first. } From 64919994415dff1c71a89ed72a0e87dd6a73484a Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Sun, 27 Mar 2022 08:10:03 -0500 Subject: [PATCH 13/18] Add news bullet --- NEWS.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/NEWS.md b/NEWS.md index 8d461e04..7ece26ff 100644 --- a/NEWS.md +++ b/NEWS.md @@ -2,6 +2,9 @@ ## Mar 2022 +* Now require explicit `S4_register()` in order to use register R7 method + for S4 method. `S4_register()` creates full S4 class spec (#182, #214). + * Exported `class_factor`, `class_Date`, `class_POSIXct`, and `class_data.frame`. From 8934a65e516385919c3caab7546a76a1a9b7a5cf Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Sun, 27 Mar 2022 08:10:53 -0500 Subject: [PATCH 14/18] Apply suggestions from code review Co-authored-by: Davis Vaughan --- R/S4.R | 4 ++-- tests/testthat/test-S4.R | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/R/S4.R b/R/S4.R index 208010d4..d4465b21 100644 --- a/R/S4.R +++ b/R/S4.R @@ -1,7 +1,7 @@ #' Register an R7 class with S4 #' #' @description -#' If you want to use and R7 class with S4 (e.g. to use [method<-] to register an +#' If you want to use an R7 class with S4 (e.g. to use [method<-] to register an #' method for an S4 generic with an R7 class) you need to call `S4_register()` #' once. This generates a full S4 class specification that: #' @@ -27,7 +27,7 @@ S4_register <- function(class, env = parent.frame()) { name <- class@name contains <- double_to_numeric(setdiff(class_dispatch(class), "ANY")[-1]) - # S4 classes inherits slots from parent but R7 classes flatten + # S4 classes inherit slots from parent but R7 classes flatten props <- class@properties if (is_class(class@parent) && class@parent@name != "R7_object") { parent_props <- class@parent@properties diff --git a/tests/testthat/test-S4.R b/tests/testthat/test-S4.R index ae5522b4..a2190ad0 100644 --- a/tests/testthat/test-S4.R +++ b/tests/testthat/test-S4.R @@ -164,7 +164,7 @@ describe("S4 registration", { expect_type(slot(obj, "x"), "double") }) - it("checks it's inputs", { + it("checks its inputs", { expect_snapshot(S4_register("x"), error = TRUE) }) }) From 757ef2f0bc4030f095ee0a759036091506e55dd3 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Sun, 27 Mar 2022 08:12:23 -0500 Subject: [PATCH 15/18] Update snapshot + docs --- man/S4_register.Rd | 2 +- tests/testthat/_snaps/S4.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/man/S4_register.Rd b/man/S4_register.Rd index efb74665..78746162 100644 --- a/man/S4_register.Rd +++ b/man/S4_register.Rd @@ -12,7 +12,7 @@ S4_register(class, env = parent.frame()) \item{env}{Expert use only. Environment where S4 class will be registered.} } \description{ -If you want to use and R7 class with S4 (e.g. to use \link{method<-} to register an +If you want to use an R7 class with S4 (e.g. to use \link{method<-} to register an method for an S4 generic with an R7 class) you need to call \code{S4_register()} once. This generates a full S4 class specification that: \itemize{ diff --git a/tests/testthat/_snaps/S4.md b/tests/testthat/_snaps/S4.md index 2ddbbb12..a83767b0 100644 --- a/tests/testthat/_snaps/S4.md +++ b/tests/testthat/_snaps/S4.md @@ -5,7 +5,7 @@ Error Unsupported S4 object: must be a class generator or a class definition, not a . -# S4 registration: checks it's inputs +# S4 registration: checks its inputs Code S4_register("x") From f8557a86f7d5058094c41f8441d099c3211baa0a Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Sun, 27 Mar 2022 08:20:16 -0500 Subject: [PATCH 16/18] Correct class cleanup --- tests/testthat/test-S4.R | 47 ++++++++++++++++++++++------------------ 1 file changed, 26 insertions(+), 21 deletions(-) diff --git a/tests/testthat/test-S4.R b/tests/testthat/test-S4.R index a2190ad0..010ef07c 100644 --- a/tests/testthat/test-S4.R +++ b/tests/testthat/test-S4.R @@ -111,55 +111,60 @@ describe("S4_class_dispatch", { describe("S4 registration", { it("can register simple class hierarchy", { - foo <- new_class("foo") - foo2 <- new_class("foo2", foo) + on.exit(S4_remove_classes(c("foo1", "foo2"), where = environment())) - S4_register(foo) + foo1 <- new_class("foo1") + foo2 <- new_class("foo2", foo1) + + S4_register(foo1) S4_register(foo2) - expect_s4_class(getClass("foo"), "classRepresentation") + expect_s4_class(getClass("foo1"), "classRepresentation") expect_s4_class(getClass("foo2"), "classRepresentation") }) it("ties S4 validation to R7 validation", { - on.exit(S4_remove_classes("Foo")) + on.exit(S4_remove_classes(c("foo1", "Foo2"), where = environment())) - foo3 <- new_class("foo3", + foo1 <- new_class("foo1", parent = class_integer, validator = function(self) { if (R7_data(self) < 0) "Must be positive" } ) # Create invalid object - R7_obj <- foo3(1L) + R7_obj <- foo1(1L) R7_obj[[1]] <- -1L - S4_register(foo3) - Foo <- setClass("Foo", slots = list(x = "foo3")) - S4_obj <- Foo(x = R7_obj) + S4_register(foo1) + Foo2 <- setClass("Foo2", slots = list(x = "foo1")) + S4_obj <- Foo2(x = R7_obj) expect_error(validObject(S4_obj, complete = TRUE), "Must be positive") }) it("can register slots", { - foo4 <- new_class("foo4", properties = list(x = class_integer)) - foo5 <- new_class("foo5", foo4, properties = list(y = class_character)) - - S4_register(foo4) - S4_register(foo5) - expect_equal(getClass("foo4")@slots$x, structure("integer", package = "methods")) - expect_equal(getClass("foo5")@slots$x, structure("integer", package = "methods")) - expect_equal(getClass("foo5")@slots$y, structure("character", package = "methods")) + on.exit(S4_remove_classes(c("foo1", "foo2"), where = environment())) + + foo1 <- new_class("foo1", properties = list(x = class_integer)) + foo2 <- new_class("foo2", foo1, properties = list(y = class_character)) + + S4_register(foo1) + S4_register(foo2) + expect_equal(getClass("foo1")@slots$x, structure("integer", package = "methods")) + expect_equal(getClass("foo2")@slots$x, structure("integer", package = "methods")) + expect_equal(getClass("foo2")@slots$y, structure("character", package = "methods")) }) it("translates double to numeric", { - foo6 <- new_class("foo6", + on.exit(S4_remove_classes("foo1", where = environment())) + foo1 <- new_class("foo1", parent = class_double, properties = list(x = class_double) ) - S4_register(foo6) + S4_register(foo1) - obj <- new("foo6") + obj <- new("foo1") expect_type(obj, "double") expect_type(slot(obj, "x"), "double") }) From 1f0ddbb2f27bc8b36df7a49c45f46535bcd733db Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Sun, 27 Mar 2022 08:25:12 -0500 Subject: [PATCH 17/18] Make environment specification more consistent --- R/S4.R | 2 +- tests/testthat/test-S4.R | 26 +++++++++++++------------- tests/testthat/test-class-spec.R | 2 +- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/R/S4.R b/R/S4.R index d4465b21..470a7c88 100644 --- a/R/S4.R +++ b/R/S4.R @@ -152,7 +152,7 @@ S4_class_name <- function(x) { } } -S4_remove_classes <- function(classes, where = globalenv()) { +S4_remove_classes <- function(classes, where = parent.frame()) { for (class in classes) { methods::removeClass(class, topenv(where)) } diff --git a/tests/testthat/test-S4.R b/tests/testthat/test-S4.R index 010ef07c..2f0e7552 100644 --- a/tests/testthat/test-S4.R +++ b/tests/testthat/test-S4.R @@ -1,5 +1,5 @@ test_that("can work with classGenerators", { - on.exit(S4_remove_classes("Foo")) + on.exit(S4_remove_classes("Foo", where = globalenv())) Foo <- setClass("Foo", where = globalenv()) expect_equal(S4_to_R7_class(Foo), getClass("Foo")) }) @@ -10,7 +10,7 @@ test_that("converts S4 base classes to R7 base classes", { }) test_that("converts S4 unions to R7 unions", { - on.exit(S4_remove_classes(c("Foo1", "Foo2", "Foo3", "Union1", "Union2"))) + on.exit(S4_remove_classes(c("Foo1", "Foo2", "Foo3", "Union1", "Union2"), where = globalenv())) setClass("Foo1", slots = "x", where = globalenv()) setClass("Foo2", slots = "x", where = globalenv()) @@ -40,13 +40,13 @@ test_that("errors on non-S4 classes", { describe("S4_class_dispatch", { it("returns name of base class", { - on.exit(S4_remove_classes("Foo1")) + on.exit(S4_remove_classes("Foo1", where = globalenv())) setClass("Foo1", slots = list("x" = "numeric"), where = globalenv()) expect_equal(S4_class_dispatch("Foo1"), "S4/Foo1") }) it("respects single inheritance hierarchy", { - on.exit(S4_remove_classes(c("Foo1", "Foo2","Foo3"))) + on.exit(S4_remove_classes(c("Foo1", "Foo2","Foo3"), where = globalenv())) setClass("Foo1", slots = list("x" = "numeric"), where = globalenv()) setClass("Foo2", contains = "Foo1", where = globalenv()) @@ -55,7 +55,7 @@ describe("S4_class_dispatch", { }) it("performs breadth first search for multiple dispatch", { - on.exit(S4_remove_classes(c("Foo1a", "Foo1b","Foo2a", "Foo2b", "Foo3"))) + on.exit(S4_remove_classes(c("Foo1a", "Foo1b","Foo2a", "Foo2b", "Foo3"), where = globalenv())) setClass("Foo1a", slots = list("x" = "numeric"), where = globalenv()) setClass("Foo1b", contains = "Foo1a", where = globalenv()) setClass("Foo2a", slots = list("x" = "numeric"), where = globalenv()) @@ -68,13 +68,13 @@ describe("S4_class_dispatch", { }) it("handles extensions of base classes", { - on.exit(S4_remove_classes("Foo1")) + on.exit(S4_remove_classes("Foo1", where = globalenv())) setClass("Foo1", contains = "character", where = globalenv()) expect_equal(S4_class_dispatch("Foo1"), c("S4/Foo1", "character")) }) it("handles extensions of S3 classes", { - on.exit(S4_remove_classes(c("Soo1", "Foo2", "Foo3"))) + on.exit(S4_remove_classes(c("Soo1", "Foo2", "Foo3"), where = globalenv())) setOldClass(c("Soo1", "Soo"), where = globalenv()) setClass("Foo2", contains = "Soo1", where = globalenv()) @@ -83,7 +83,7 @@ describe("S4_class_dispatch", { }) it("ignores unions", { - on.exit(S4_remove_classes(c("Foo1", "Foo2", "Foo3"))) + on.exit(S4_remove_classes(c("Foo1", "Foo2", "Foo3"), where = globalenv())) setClass("Foo1", slots = list("x" = "numeric"), where = globalenv()) setClass("Foo2", slots = list("x" = "numeric"), where = globalenv()) @@ -94,7 +94,7 @@ describe("S4_class_dispatch", { }) it("captures explicit package name", { - on.exit(S4_remove_classes("Foo1")) + on.exit(S4_remove_classes("Foo1", where = globalenv())) setClass("Foo1", package = "pkg", where = globalenv()) expect_equal(S4_class_dispatch("Foo1"), "S4/pkg::Foo1") }) @@ -111,7 +111,7 @@ describe("S4_class_dispatch", { describe("S4 registration", { it("can register simple class hierarchy", { - on.exit(S4_remove_classes(c("foo1", "foo2"), where = environment())) + on.exit(S4_remove_classes(c("foo1", "foo2"))) foo1 <- new_class("foo1") foo2 <- new_class("foo2", foo1) @@ -124,7 +124,7 @@ describe("S4 registration", { }) it("ties S4 validation to R7 validation", { - on.exit(S4_remove_classes(c("foo1", "Foo2"), where = environment())) + on.exit(S4_remove_classes(c("foo1", "Foo2"))) foo1 <- new_class("foo1", parent = class_integer, @@ -144,7 +144,7 @@ describe("S4 registration", { }) it("can register slots", { - on.exit(S4_remove_classes(c("foo1", "foo2"), where = environment())) + on.exit(S4_remove_classes(c("foo1", "foo2"))) foo1 <- new_class("foo1", properties = list(x = class_integer)) foo2 <- new_class("foo2", foo1, properties = list(y = class_character)) @@ -157,7 +157,7 @@ describe("S4 registration", { }) it("translates double to numeric", { - on.exit(S4_remove_classes("foo1", where = environment())) + on.exit(S4_remove_classes("foo1")) foo1 <- new_class("foo1", parent = class_double, properties = list(x = class_double) diff --git a/tests/testthat/test-class-spec.R b/tests/testthat/test-class-spec.R index 1206c3c4..17b6c223 100644 --- a/tests/testthat/test-class-spec.R +++ b/tests/testthat/test-class-spec.R @@ -162,7 +162,7 @@ test_that("can work with R7 classes that extend S3 classes", { # S4 ---------------------------------------------------------------------- test_that("can work with S4 classes", { - on.exit(S4_remove_classes(c("Foo1", "Foo2", "Foo3", "Foo4"))) + on.exit(S4_remove_classes(c("Foo1", "Foo2", "Foo3", "Foo4"), where = globalenv())) methods::setClass("Foo1", contains = "character", where = globalenv()) methods::setClass("Foo2", contains = "Foo1", where = globalenv()) From b516f2a129e5f23eae1af5661a56fcefc96354ab Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Mon, 18 Apr 2022 11:54:33 -0500 Subject: [PATCH 18/18] Add minutes --- vignettes/minutes/2022-04-18.Rmd | 33 ++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) create mode 100644 vignettes/minutes/2022-04-18.Rmd diff --git a/vignettes/minutes/2022-04-18.Rmd b/vignettes/minutes/2022-04-18.Rmd new file mode 100644 index 00000000..88fd11bc --- /dev/null +++ b/vignettes/minutes/2022-04-18.Rmd @@ -0,0 +1,33 @@ +--- +title: "Minutes 2022-04-18" +--- + +## Changes + +- All base wrappers use common naming scheme, e.g. `class_integer`, `class_numeric`, `class_missing`. + Exported wrappers for key S3 classes: `class_factor`, `class_Date`, `class_POSIXct`, and `class_data.frame`. + +- `convert()` allows you to convert an object into another class. + \ + `super()` replaces `next_method()`. + +- Require explicit `S4_register()` in order to use register a method for R7 class on a S4 generic. + +- Can now register methods for double-dispatch base Ops (currently only works if both classes are R7, or the first argument is R7 and the second doesn't have a method for the Ops generic). + +## Discussion + +- Lightweight syntax for unions: --- no strong feelings against. + +- Next steps + + - Should we aim for a CRAN release of R7? + Allow us to get more feedback before it moves into base R and if tidyverse is to use R7, will also need some way to access in older versions of R. + + - Serialization: + + - Base R extension points: + + - Will look into creating a patch to implement minimal set of changes. + + - Will need to tweak package to use if in R devel, otherwise register some shims to make it work in current R.