Improve construction of subclasses#694
Conversation
* `constructor_args()` no longer drops child properties that shadow parent properties, so they become constructor args with the child's default. Fixes #467. * The generated body no longer passes declared properties to the parent constructor; they go to `new_object()` instead, which runs the child's setter and class check. Fixes #585. * Read-only properties are removed from the constructor. * Constructors for subclasses of abstract classes now include parent properties.
| } | ||
| } | ||
|
|
||
| # Combine properties from parent, overriding as needed |
There was a problem hiding this comment.
This manipulation now happens in new_constructor() so it has both the parent and overridden properties.
| new_constructor(foo1, list()) | ||
| Output | ||
| function () | ||
| function (x = numeric(0)) |
There was a problem hiding this comment.
This seems like a clear bug that we never noticed before.
| # Declared properties (including overrides of parent properties) are set by | ||
| # new_object() so that their defaults and setters are respected; | ||
| # only the remaining arguments are passed to the parent constructor. | ||
| parent_arg_nms <- setdiff(names(arg_info$parent), names2(properties)) |
There was a problem hiding this comment.
This implicitly requires that overridden properties are optional in the parent.
E.g., with this approach, this fails:
devtools::load_all()
#> ℹ Loading S7
foo <- new_class(
"foo",
properties = list(
x = new_property(class_numeric, default = quote(stop("need x")))
)
)
foo2 <- new_class(
"foo2",
parent = foo,
properties = list(
x = new_property(class_numeric, default = 2)
)
)
`attributes<-`(foo2, NULL)
#> function (x = 2)
#> S7::new_object(foo(), x = x)
#> <environment: 0x7698baa50>
foo2()
#> Error in `foo()`:
#> ! need x
foo2(x = 3)
#> Error in `foo()`:
#> ! need xIs this what we want?
There was a problem hiding this comment.
I agree that doesn't seem correct. OTOH I'm not sure if we fully support required properties — my mental mode of S7 was that you can always construct an object with no arguments, because that's such a useful way to generate a prototype. But it's possible that thinking is centred in vctrs and doesn't make sense in general.
If we do want to support required properties I don't see how to square that with respecting setters. For required properties to work you need to pass the property to the parent and to respect the child setter, you need to pass it to the child. Maybe we have to pass the property to both the child and the parent?
Maybe you could pass the property to both parent and child, but then you'd call the setter twice, which might be confusing (although it does mean that the child definitely can't weaken any checks that the parent does).
There was a problem hiding this comment.
I think the best approach is to pass it to both. This will have a performance impact unfortunately, but hopefully it's minor, and the benefits we get in correctness, enforcement, and simplicity are worth it.
t-kalinowski
left a comment
t-kalinowski
left a comment
There was a problem hiding this comment.
I pushed a few commits that try to make overridden parent properties behave more consistently in default constructors.
The implementation is a little hard to follow because it has to decide separately which arguments belong in the child constructor, which should be forwarded to the parent constructor, and which should be skipped. The tests are probably the clearest description of what I was aiming for here; each added test failed without the corresponding patch.
In short, the tests cover:
- subclass defaults replacing parent defaults
- mandatory parent properties being satisfiable from subclass defaults
- overridden properties still being forwarded through parent
... - subclass setters running during construction
- parent validators rerunning when inherited properties are reset
- read-only overrides being removed from the constructor without removing unrelated parent args
- incompatible dynamic settable overrides not being forwarded to the parent
Please take a close look. I’m not certain this is the simplest implementation, but I think the behavior in the tests is worth considering.
2 participants
constructor_args()no longer drops child properties that shadow parent properties, so they become constructor args with the child's default. Fixes Default value introduced in subclass does not propagate to constructor #467.new_object()instead, which runs the child's setter and class check. Fixes Child class property setters that mask parent class properties are unused at construction #585.