From 8adb69c12754f27f73b9165abe4a2f7c3a395241 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Sat, 20 Jun 2026 07:03:38 -0500 Subject: [PATCH 1/2] Ensure that dynamic properties also narrow the parent In practice, I don't expect this to have much impact since I think dynamic properties are mostly going to be `class_any`, but I don't see any resason not to check any the type information that the user did provide. Fixes #708 --- NEWS.md | 2 +- R/class.R | 6 ------ R/property.R | 2 -- tests/testthat/_snaps/class.md | 10 ++++++++++ tests/testthat/test-class.R | 19 +++++++++++++++---- 5 files changed, 26 insertions(+), 13 deletions(-) diff --git a/NEWS.md b/NEWS.md index 94f68d5e..e6a62f5f 100644 --- a/NEWS.md +++ b/NEWS.md @@ -19,7 +19,7 @@ * `new_object()` now names its first argument `_parent` to minimise the chance of a clash with a property (#423). It also accepts a single unnamed named list as a shortcut for splicing property values, making it easier to programmatically construct an object from a list of properties (#497). * `method<-` can now register methods on S3 and S4 generics with base types (e.g. `class_character`), S3 classes (`new_S3_class()`, `class_factor`, etc.), S7 unions (expanded to one registration per class), `class_any` (registered as the `default` method), and `NULL` (registered as the `NULL` method) (#455). * `method<-` no longer emits an "Overwriting method" message when re-registering an identical method, eliminating spurious messages from `devtools::load_all()` (#474). -* `new_class()` now errors if a child class overrides a parent property with a type that doesn't extend the parent's type, since such a class could never be instantiated. Narrowing the type is still allowed, as are dynamic (getter) properties (#352). +* `new_class()` now errors if a child class overrides a parent property with a type that doesn't extend the parent's type, since such a class could never be instantiated (#352, #708). * `new_class()` now allows properties named `names`, `dim`, `dimnames`, `class`, `comment`, `tsp`, and `row.names`. But property names beginning with `_` are now reserved for internal use (#579). * `new_class()` experimentally allows `class_environment` as a parent again, so you can build S7 objects that share R's reference semantics for environments. This support is provisional: because environments are mutated in place, some operations behave differently than for value-typed S7 objects, and the API may change. `S7_data()` and `S7_data<-()` error on environment-based objects, since they would otherwise destroy the object's S7 attributes in place (#590). * `new_class()`'s default constructor now respects properties overridden in a subclass: the subclass's default is used (#467) and its setter is run during construction (#585). Values for overridden properties are passed to both the parent constructor and the new object, so a subclass can override a parent property whose default is mandatory. diff --git a/R/class.R b/R/class.R index 549f0e25..c3e47e58 100644 --- a/R/class.R +++ b/R/class.R @@ -454,12 +454,6 @@ check_prop_overrides <- function( for (prop in overridden) { child_prop <- child_props[[prop]] - # Dynamic properties are computed, not stored, so they're never validated - # against the parent's type - if (prop_is_dynamic(child_prop)) { - next - } - child_class <- child_prop$class parent_class <- parent_props[[prop]]$class diff --git a/R/property.R b/R/property.R index dc3d62ba..fe6db113 100644 --- a/R/property.R +++ b/R/property.R @@ -598,5 +598,3 @@ prop_is_read_only <- function(prop) { } prop_has_setter <- function(prop) is.function(prop$setter) - -prop_is_dynamic <- function(prop) is.function(prop$getter) diff --git a/tests/testthat/_snaps/class.md b/tests/testthat/_snaps/class.md index 941c207a..68408d7f 100644 --- a/tests/testthat/_snaps/class.md +++ b/tests/testthat/_snaps/class.md @@ -149,6 +149,16 @@ - @x is . - @x is . +# dynamic child properties must also narrow the parent's type (#708) + + Code + new_class("foo2", foo1, properties = list(x = widen)) + Condition + Error in `new_class()`: + ! @x must narrow @x. + - @x is . + - @x is . + # abstract classes can't be instantiated Code diff --git a/tests/testthat/test-class.R b/tests/testthat/test-class.R index e31814cd..1aee63f7 100644 --- a/tests/testthat/test-class.R +++ b/tests/testthat/test-class.R @@ -193,10 +193,21 @@ test_that("inheritance doesn't let child properties widen or change the parent's }) }) -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") - expect_no_error(new_class("foo2", foo1, properties = list(x = readonly))) +test_that("dynamic child properties must also narrow the parent's type (#708)", { + foo1 <- new_class( + "foo1", + properties = list(x = class_integer), + package = NULL + ) + + widen <- new_property(class_character, getter = function(self) "x") + expect_snapshot( + error = TRUE, + new_class("foo2", foo1, properties = list(x = widen)) + ) + + narrow <- new_property(class_integer, getter = function(self) 1L) + expect_no_error(new_class("foo3", foo1, properties = list(x = narrow))) }) test_that("abstract classes can't be instantiated", { From c90715a2b4ae2e491a42cdeb100c374586e269f0 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Mon, 22 Jun 2026 18:37:19 -0500 Subject: [PATCH 2/2] Use the correct check --- R/class.R | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/R/class.R b/R/class.R index c3e47e58..8b7eeaa6 100644 --- a/R/class.R +++ b/R/class.R @@ -457,6 +457,12 @@ check_prop_overrides <- function( child_class <- child_prop$class parent_class <- parent_props[[prop]]$class + # Read-only properties are computed, not stored, so when the user hasn't + # declared a type there's nothing to narrow. + if (prop_is_read_only(child_prop) && is_class_any(child_class)) { + next + } + if (!class_extends(child_class, parent_class)) { child_desc <- paste0("<", name, ">") parent_desc <- class_desc(parent)