Polish S4-backed S7 edge cases#720
Draft
t-kalinowski wants to merge 132 commits into
Draft
Conversation
…cing to an S3 class, and use methods::extends() to capture the entire class vector, not just the first class
…@() behavior, which needs to change in base R.
…S4_register() can now communicate any S7 class structure through the S4class= argument.
…t initialize() except uses the S7 property setting path.
… parent object construction in constructor.
…mplex hierarchies
…be convenient for passing to methods package functions
…d have it return the class name.
[P2] Preserve S4 data-part attributes — /Users/tomasz/github/RConsortium/S7/R/S4.R:842-844: When `object` is an S4 data-part object, collecting only formal slots drops attributes that S4 stores on the object rather than in the `.Data` slot. For example, a child of an S4 `contains = "numeric"` class constructed from `.Data = c(a = 1)` loses the vector names, and the same happens when initializing from an S4 source object. [P2] Seed abstract S4 data-part descendants with .Data — /Users/tomasz/github/RConsortium/S7/R/constructor.R:33-37: For a concrete S7 class whose abstract S7 parent extends a virtual S4 data-part class, this direct-parent check is false, so the generated constructor seeds `new_object()` with `S7_object()` and stores `.Data` as an ignored attribute. Such constructors fail validation for inputs like `.Data = 1` because `prop(x, ".Data")` reads the underlying data object instead of the supplied value. [P2] Preserve S4 package identity during validity coercion — /Users/tomasz/github/RConsortium/S7/R/class-spec.R:270-275: When an S7 class extends an S4 class from another package and another S4 class with the same name is visible, validating by coercing the old-class S7 object with `methods::as(object, class@className)` can resolve through the wrong class definition. In that package-collision case, constructing the S7 child can fail slot assignment or run the wrong validity method even though the package-qualified S4 parent works on its own. [P3] Keep data attributes that share S7 property names — /Users/tomasz/github/RConsortium/S7/R/S4.R:873-881: Including logical property names here strips data-part attributes even when the property is stored under a renamed slot. For an S4-backed numeric class with an S7 property named `names`, `S7_data()` and `S7_data<-` drop the vector names; S4-only slot collisions are already covered by `slotNames()` and S7 storage collisions by `prop_storage_names()`.
[P2] Preserve package identity in generated union names — /Users/tomasz/github/RConsortium/S7/R/S4.R:203-205: When a union contains an S4 class from a package, `S4_class()` returns `x@className` with the package only as an attribute, and `paste0()` drops that attribute. This makes `pkgA::Foo | class_character` and `pkgB::Foo | class_character` both register as `Foo_OR_character`; registering the second union redefines the first, so a name returned by `S4_register()` can start matching the wrong package's class. [P2] Use identity shims for package-local S4 union members — /Users/tomasz/github/RConsortium/S7/R/S4.R:221-223: When the S4 member is defined in the same namespace as `S4_env`, `S4_class_needs_identity()` is false and this returns the bare class name. `setClassUnion()` then creates an unconditional extension for that name, and `methods::is()` also accepts same-named classes from `.GlobalEnv`; a package union like `Foo | character` will match a user's global `Foo` object.
[P2] Reject S4 parent objects in custom constructors — /Users/tomasz/github/RConsortium/S7/R/class.R:327-328: When an S7 class has an S4 parent and a custom constructor passes an actual S4 parent instance to `new_object()` (for example `new_object(methods::new("Parent", ...))`), this branch returns without normalizing or rejecting it; `new_object()` then installs S7 attributes on the S4 object and the constructor never returns. The generated constructor avoids S4 `_parent` values, but custom constructors are public API, so this path should fail fast or convert the S4 parent before validation.
[P3] Add subclasses to global variables — /Users/tomasz/github/RConsortium/S7/R/S4.R:310-310: Running `devtools::check()` now reports `S4_prune_union_subclass: no visible binding for global variable 'subclasses'` because this `@subclasses` subassignment is parsed by R's code checker as a global variable reference. Add `"subclasses"` to `globalVariables()` or avoid the direct slot subassignment so the patch does not introduce a new check NOTE.
[P2] Preserve S7_object inheritance for data-part parents — /Users/tomasz/github/RConsortium/S7/R/S4.R:627-632. When an S7 class has a base/S3 data-part parent such as `class_integer` or `class_factor`, the S4 compatibility class is registered as containing only that data-part class plus `VIRTUAL`; `setOldClass(c(Foo, integer, S7_object), S4Class = Foo)` does not make the S4 class extend `S7_object` in that case. As a result, `setClass("Child", contains = S4_contains(Foo)); new("Child", ...)` fails in `S4_initialize()` because the child prototype is not recognized as an S7 object, so S4 subclasses cannot be constructed for these advertised S7 classes.
[P2] Register S7 union members before naming the union — /Users/tomasz/github/RConsortium/S7/R/S4.R:86-87: When the union contains an S7/S3 member that has not already been registered, this calls `S4_union_name()` and resolves each member through `S4_class()` before the later `register = TRUE` path can run. As a result, `S4_register(Foo | class_character)` fails with “please call S4_register(Foo)” instead of registering the union, so the new public union registration API only works after an undocumented extra registration step. [P2] Preserve package identity when skipping S4 validity — /Users/tomasz/github/RConsortium/S7/R/valid.R:170-173: When validating an S4 subclass that combines an S7-over-S4 ancestor from one package with another S4 superclass of the same bare name from another package, this skip list drops the package from `className`. `S4_validate_old_class()` then skips the unrelated superclass as if it had already been validated, so `validate(obj)` can return successfully even though `methods::validObject(obj, test = TRUE)` reports that superclass's validity error.
[P2] Allow subclasses in package-qualified S4 unions — /Users/tomasz/github/RConsortium/S7/R/S4.R:325-325: When a registered S7 union contains a package-qualified S4 class and a class in another package extends that S4 class, `methods::is(subclass_obj, union_name)` returns `FALSE`. The conditional extension compares the concrete object's package from `class(object)` with the union member's package, excluding valid subclasses defined in other packages. [P2] Isolate global S4 union members from name collisions — /Users/tomasz/github/RConsortium/S7/R/S4.R:300-304: When the union member is a `.GlobalEnv` S4 class and a package also defines an unrelated class with the same bare name, this returns `FALSE` and the union is registered against the bare class name. Instances of the package class then satisfy the global union, so S4 slots or method signatures using `S4_register(globalFoo | ...)` can accept the wrong class.
Finding 1: [P2] Reject invalid seeds for S4-backed custom constructors — /Users/tomasz/github/RConsortium/S7/R/class.R:327-334: When an S7 class has an S4 parent without a `.Data` slot, this branch accepts any non-S4 `_parent`, so a custom constructor can call `new_object(1, x = ...)` and create an object whose underlying data is `double` rather than the required `S7_object`; both `validate()` and `methods::validObject()` then pass because validation stops at the S4 parent. This breaks the core S7 object invariant for S4-backed classes. Finding 2: [P2] Check S4 slot overrides through S7 ancestors — /Users/tomasz/github/RConsortium/S7/R/class.R:501-503: This guard only rejects custom getters/setters when the immediate parent is S4. If `P` extends an S4 class with slot `x`, then `C <- new_class(parent = P, properties = list(x = new_property(getter = ...)))` is accepted; the registered S4 class still inherits the original `x` slot, so `prop(obj, "x")` and `methods::slot(obj, "x")` can disagree and S4 validity checks the stale slot value.
Reject S4 data parents that inherit a vector data part without exposing an explicit `.Data` slot.
Finding addressed:
[P2] Reject S4 data parents without explicit slots — /Users/tomasz/github/RConsortium/S7/R/constructor.R:166-172: When `parent` is an S4 data class whose representation is implicit rather than present in `parent@slots` (for example `methods::getClass("matrix")`), `parent_nms` is empty and this fallback seeds the S7 instance with `S7_object()` instead of a matrix. `Child := new_class(parent = methods::getClass("matrix"), package = NULL); methods::validObject(Child(), test = TRUE)` then reports an invalid `.Data` slot, so `new_class()` can create objects that do not satisfy their declared S4 parent.
[P2] Avoid calling S4 initialize while preparing validity objects — /Users/tomasz/github/RConsortium/S7/R/class-spec.R:291-291: When a concrete S4 parent has an `initialize` method that requires arguments and a validity method that reads the object, constructing an S7 child fails during validation because `S4_as_validity_class()` calls `methods::new(class)` with no arguments before copying the slots over. The parent's `initialize` method errors even though the S7 constructor already supplied the required slot values, breaking S4 initialization and validity interop for non-virtual parents.
[P2] Preserve union membership for same-package subclasses — /Users/tomasz/github/RConsortium/S7/R/S4.R:295. When `S4_register()` is called from a package namespace for a union that includes one of that package's S4 classes, this pruning removes the real class edge from the union. Same-package S4 subclasses then no longer satisfy the union, so S4 slots or methods declared with the registered union reject valid subclasses.
[P2] Don't treat ordinary .Data properties as data parts — /Users/tomasz/github/RConsortium/S7/R/S4.R:631. `slot_properties` includes every stored property, including `.Data`, even when the S7 class has no base/S4 data-part parent. For `new_class("Foo", properties = list(.Data = class_numeric), package = NULL)`, `S4_register(Foo)` creates a special S4 `.Data` data-part slot, and `methods::validObject(Foo(.Data = 1), test = TRUE)` reports the data part has class "object" rather than "numeric"; either reject this case or only reify `.Data` when the class actually has a data part.
[P1] Resolve the native symbol at call time — /Users/tomasz/github/RConsortium/S7/R/class-spec.R:300-302: When the package is installed, the captured `NativeSymbolInfo` is serialized into the lazy-load database with a null pointer, causing `S4_new_object()` to fail with `NULL value passed as symbol address` whenever S7 validates an S7 object against an S4 parent. Look up `methods`' native routine inside the function instead of capturing the pointer. [P1] Make S4 identity tests self-contained — /Users/tomasz/github/RConsortium/S7/R/S4.R:333-338: For unions containing an S4 class from another package, `methods::setIs()` propagates this test onto the transitive union extension with `namespace:methods` as its environment, so unqualified S7-internal helper calls are not found in an installed package. `methods::is(pkg_object, union_name)` after `S4_register(pkg_s4 | class_character)` then errors during validity checks, even though it works under `load_all()`.
[P2] Add .S3Class for S3 data-parent registrations — /Users/tomasz/github/RConsortium/S7/R/S4.R:707-707: When an S7 class wraps a custom S3 class whose class vector includes a base data class, `base_parent(class)` is an `S7_S3_class` and `parent_slot_names` is empty, so this returns `FALSE`. `S4_register()` then creates the S4 class without `.S3Class`; after `setOldClass()` adds `.Data`, `S4_set_S7_object_extension()` fails with “missing slot ... (.S3Class)”, breaking the documented registration path for S7 classes with S3 parents. [P2] Reject duplicate S7 storage names after merging properties — /Users/tomasz/github/RConsortium/S7/R/class.R:159-159: When an S4 parent has a slot whose name is already an S7 storage name, such as `_names`, and the S7 child adds the corresponding public property `names`, this merge leaves both properties in `all_props` even though `prop_storage_rename()` maps both to `_names`. The generated constructor accepts both arguments but `prop(obj, "names")` reads the parent `_names` slot, so the child `names` value is ignored or overwritten; reject duplicate storage names before storing `all_props`.
[P2] Avoid the unregistered methods C entry point — /Users/tomasz/github/RConsortium/S7/R/class-spec.R:300-302: `.Call(C_new_object, class)` triggers an R CMD check foreign-function registration NOTE because `C_new_object` is looked up from the `methods` namespace rather than being a registered symbol in this package. This also relies on an unexported methods implementation detail; use a supported construction path or avoid the internal C call.
[P2] Preserve public S7 property names from S4 parents — /Users/tomasz/github/RConsortium/S7/R/S4.R:1161-1165: When an S7 class extends an S4 class that itself extends an S7 class with a special-name property like `names`, the inherited S4 slot is the storage name (`_names`). Importing slot names verbatim makes the new S7 child expose `_names` instead of the public inherited `names` property, so `Grand(names = ...)` is rejected and `prop(obj, "names")` is missing even though the object still inherits from the original S7 class.
[P2] Re-register stale S7 property classes — /Users/tomasz/github/RConsortium/S7/R/S4.R:511-514: When a stored property uses an S7 class whose S4 definition already exists but no longer matches the current class object, such as after redefining that property class during development, the `isClass()` guard skips `S4_register()`. The following `S4_class()` then rejects the stale definition, so `S4_register(Holder)` fails unless users manually re-register the nested class, defeating the automatic property-class registration added here.
Finding 1: [P2] Preserve S3 data attributes for S4 data parts — /Users/tomasz/github/RConsortium/S7/R/S4.R:1100-1103: Because this treats every S4 slot as protected metadata, S4 subclasses of S7 classes with S3 data parents lose the attributes stored in those slots. For example, after `S4_register(new_class(parent = class_factor))`, `S7_data()` on an S4 subclass returns raw integer codes and `S7_data<-` drops new `levels`, rather than preserving a factor; data frames similarly lose `names`/`row.names`. Finding 2: [P2] Register existing package subclasses with S4 unions — /Users/tomasz/github/RConsortium/S7/R/S4.R:297-298: When a non-global S4 subclass already exists before `S4_register(parent | ...)` is called, this only connects the package-qualified member to the shim after the union has been built, so the existing subclass is not added to the union and `methods::is(child, union_name)`/union methods fail. This is a common package load order when classes are defined before unions, so existing subclasses need to be propagated to the union too.
[P2] Preserve S7 validators through S4 hops: In /Users/tomasz/github/RConsortium/S7/R/class-spec.R:253-254, when an S7 class is extended by an S4 class and then used as the parent of another S7 class, validation of the original S7 superclass can be skipped. For example, with P -> Mid via S4_contains(P) and G := new_class(parent = getClass("Mid")), S7_inherits(G(), P) is TRUE, but mutating G after construction can bypass P's property/class validators because validation of Mid skips the reified P class.
[P2] Require a reified S7 class for S4_contains(): In /Users/tomasz/github/RConsortium/S7/R/S4.R:78-80, old-style setOldClass(c("Foo", "S7_object")) registrations are accepted even though they only have .S3Class and lack the new _S7_class slot or S7 property slots. An S4 subclass created from that return value cannot initialize inherited S7 properties, e.g. methods::new("Child", x = 1) fails because slot x was never exposed; S4_contains() should force or reject old registrations rather than treating them as suitable contains targets.
[P2] Preserve S4 subclass prototypes over deferred defaults: In /Users/tomasz/github/RConsortium/S7/R/S4.R:1061-1064, for S4 subclasses of registered S7 classes, using the concrete class prototype as the sentinel makes an explicit subclass prototype look like an unset inherited value. If the S7 property has a deferred default such as quote({ 1 }) and the S4 subclass sets prototype = list(x = 5), methods::new("Child") overwrites x with 1 instead of preserving the subclass prototype 5.
[P2] Preserve .S3Class slots from S4 parents — /Users/tomasz/github/RConsortium/S7/R/S4.R:1223-1225. When an S4 parent has a real `.S3Class` slot, e.g. `setClass("P", contains = "factor")`, dropping it leaves direct S7 children missing part of the parent representation. In that case `S7_inherits(child, getClass("P"))` is `FALSE` and `convert(child, to = getClass("P"))` fails, even though the class was declared with that S4 parent. Preserve `.S3Class` for non-S7-reified parents or reject these parents up front.
[P2] Preserve S4 data-part attributes for direct S7 children — /Users/tomasz/github/RConsortium/S7/R/data.R:31-33: When an S7 class directly extends an S4 class with an S3 data part, such as `setClass("P", contains = "factor")`, instances are intentionally not `isS4()`, so they skip this branch and fall through to `zap_attr()`. Because slots like `levels` and `.S3Class` were imported as S7 properties, `S7_data(Child(.Data = 1:2, levels = c("a", "b"), .S3Class = "factor"))` returns bare integers instead of a factor. Handle S4 data-part ancestry here or reject these parents when defining the class.
[P2] Check concrete S4 slot types for S7-backed slots — /Users/tomasz/github/RConsortium/S7/R/valid.R:166. When an S4 subclass redeclares an inherited S7 property slot with a narrower S4 type, construction only runs S4 validity methods and skips formal slot-class checks. For example, a parent S7 property `x = class_numeric` with a child S4 slot `x = "integer"` lets `methods::new("Child", x = 1)` return an object that `methods::validObject()` immediately rejects, so construction can produce invalid S4 objects.
[P2] Preserve S7_class when upcasting — /Users/tomasz/github/RConsortium/S7/R/convert.R:201-201: When the target S7 parent has a user property named `S7_class`, the hard-coded removal drops that property even though it is included in `to_props`. Upcasting `Child(S7_class = 1, ...)` to a parent that defines `S7_class` returns an object where `@S7_class` is `NULL` and validation fails; the legacy class attribute should only be removed when it is not a target property.
[P2] Unwrap S4 values before replacing S4 data parts — /Users/tomasz/github/RConsortium/S7/R/S4.R:1128. When the replacement `.Data` is itself an S4 data-part object and the target is a direct S7 child of an S4 data-part class (`isS4(object)` is false), this branch strips and reattributes the whole S4 wrapper instead of its `.Data` slot. Calls such as `initialize(child, .Data = methods::new("Parent", .Data = 2))` can hang or produce a corrupt object; unwrap incoming S4 data-part values with `S4_data_part(value)` before copying attributes.
[P2] Reject or handle multiple S7 superclasses in S4 contains — /Users/tomasz/github/RConsortium/S7/R/class-spec.R:372. When an S4 class contains more than one `S4_contains()` superclass, this validation path tries to validate each reified S7 superclass through `prop()`, but `prop()` resolves properties through the object's single `_S7_class` slot. For `setClass("Both", contains = c(S4_contains(Foo), S4_contains(Bar)))`, properties or validators from the secondary S7 class can fail with `Property not found` or be inconsistent with `S7_class()`, so this should either be rejected up front or validated without relying on a single `_S7_class`.
Collaborator
|
Thanks for doing this @t-kalinowski. It's very helpful. What this shows is that fully supporting every edge case would likely double the size and complexity of S7, which probably wouldn't be worth it. The goal is to provide a migration path from S4 to S7, rather than to achieve 100% compatibility between two different ecosystems, one of which we hope to eventually deprecate. I agree with you that we should take an incremental approach here. Since nothing here seems to invalidate the approach taken in #659, let's merge that, and then we can address these issues in some prioritized way. |
lawremi
force-pushed
the
issue-456-s4-extends-s7
branch
from
8da17d8 to
616de46
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
I am far from an S4 expert, and this branch is heavily LLM-assisted. It addresses correctness issues flagged by automated/agent-assisted code review of the original branch for #659.
One failure mode of that review process is that it can pull in extra complexity: fixes for edge cases that are technically defensible, but may not be worth supporting in practice. Without an explicit decision in the code or tests about what S7 should and should not support, those fixes can look like scope creep. I am too far removed from the S4 ecosystem to make those judgment calls confidently.
@lawremi, could you look over this branch, especially the tests, and help decide which cases matter and which ones we should drop?
The branch currently contains changes for cases where:
.S3Classmetadata when used as S7 parents.It also adjusts S4 registration and validation helpers around metadata refresh and validity proxy reuse, and adds regression tests for construction, inheritance, conversion, validation, data-part handling, method registration, and installed-package compatibility.
One thing worth noting is that even though there are already a lot of changes on this branch that build on top of the already large #659, automated code review is still flagging issues and has not yet come back clean. It might be better to split #659 (and some portions of this PR we deem are worth keeping) into a series of smaller, more incremental stacked PRs. That would make each step easier to reason about and review, both for humans and agents.