Consistent ... handling (#655)

* Allow `...` to be a single unnamed list in `new_object()`, `set_props()` and `convert()`. Fixes #497.
* Use `_` prefix in `new_object()` and `set_props()` to avoid accidental matches to existing properties. Fixes #423.

Author: hadley

.claude/skills/property-dots/SKILL.md

@@ -0,0 +1,89 @@
+---
+name: property-dots
+description: >
+  Guide for writing functions that accept property name-value pairs via 
+  `...`. Use when adding a function whose `...` collects property values.
+---
+
+# Accept properties via `...`
+
+Use this skill when writing a new function that collects property name-value pairs
+through `...`, e.g. ``new_object(`_parent`, ...)`` or ``set_props(`_object`, ...)``.
+
+Two problems show up whenever `...` carries property values:
+
+1. A fixed argument can shadow a property of the same name. If `set_props()`
+   had a formal called `object`, then `set_props(x, object = 1)` would bind
+   `object` to the formal instead of treating it as the property `object`.
+2. Callers sometimes want to supply property values programmatically, as a
+   single list, rather than as individual `name = value` pairs.
+
+`collect_dots()` and the underscore naming convention solve these.
+
+For example, take (a simple version of) `update_props()`
+
+```r
+update_props <- function(`_object`, ...) {
+  props(`_object`) <- collect_dots(...)
+  `_object`
+}
+
+# Both work
+update_props(x, object = 1, width = 10)
+update_props(x, list(object = 1, width = 10))
+```
+
+## The underscore convention
+
+Property names starting with `_` are reserved for internal use. This lets you name a fixed argument with a leading `_` so it can never collide with a real property passed through `...`.
+
+- ``new_object(`_parent`, ...)`` — `_parent` is the parent object.
+- ``set_props(`_object`, ...)`` — `_object` is the object to modify.
+
+`_parent` and `_object` are not syntactically valid names, so they **must be
+backtick-quoted** everywhere they appear:
+
+```r
+set_props <- function(`_object`, ..., .check = TRUE) {
+  props(`_object`, check = .check) <- collect_dots(...)
+  `_object`
+}
+```
+
+Because these arguments are always passed positionally (and generated
+constructors pass the parent positionally), the unusual name never burdens
+callers — it only stops the name from competing with `...`.
+
+Apply the convention when:
+
+- The function takes properties via `...`, AND
+- It also needs a fixed argument that a user might plausibly use as a property
+  name (`object`, `parent`, `value`, ...).
+
+
+## Collecting the dots with `collect_dots()`
+
+`collect_dots(...)` returns a named list of the `...` values. As a convenience,
+if `...` is a single unnamed list, its elements are used instead, which makes
+it easy to build the values programmatically. It errors if any value is
+unnamed.
+
+```r
+collect_dots(x = 1, y = 2)      # list(x = 1, y = 2)
+collect_dots(list(x = 1, y = 2)) # list(x = 1, y = 2) -- spliced
+collect_dots(1)                  # error: All arguments to `...` must be named.
+collect_dots(list(1))            # error: All elements of `..1` must be named.
+```
+Use `collect_dots()` instead of `list(...)`.
+
+## Documenting it
+
+In the `@param` for `...`, mention the single-list shortcut:
+
+```r
+#' @param ... Name-value pairs of properties to set. As a convenience, you can
+#'   supply a single unnamed list instead of individual name-value pairs, which
+#'   makes it easy to set properties programmatically.
+```
+
+roxygen handles `@param _object` and a backtick-quoted formal fine.

NEWS.md

@@ -8,13 +8,15 @@
 * `convert()` now errors when upcasting to an abstract class, rather than producing an instance of that abstract class (#680, #686).
 * `convert()` no longer automatically converts between sibling classes (classes that merely share a common ancestor); the default downcast now applies only when `to` is genuinely a descendant of `from`'s class (#509).
 * `convert()` now falls back to the corresponding `as.*()` function (e.g. `as.character()`) when converting to a base type like `class_character` and no method or inheritance-based default applies, so `convert(1, class_character)` works out of the box (#472).
+* `convert()` accepts a single unnamed list of property overrides when downcasting, as a shortcut for individual name-value pairs (#497).
 * `convert()` no longer errors when `from` is a base or S3 object and `to` is an S7 class that inherits from `from`'s class. The base/S3 value is now passed as `.data` to the `to` constructor (#537).
 * `method<-` now works for double-dispatch operators (e.g. `+`, `==`, `%*%`) with plain S3 or S4 classes, even when neither operand is an S7 object (#544).
 * `method<-` no longer embeds a copy of a generic owned by another package in your package namespace. Instead it returns a sentinel value that the new `S7_on_build()` removes from the namespace at build time; call `S7_on_build()` at the top level of `zzz.R` (see `vignette("packages")`) (#364).
 * `method<-` now accepts `NULL` to unregister an existing method, e.g. `method(foo, class_character) <- NULL` (#613).
 * `convert()` is now idempotent when `from` is already an instance of `to`, returning it unchanged. When `from` inherits from `to` but is more specific, dispatch is now restricted to classes more specific than `to`, so an inherited downcasting method can no longer be selected in place of an upcast (#429).
 * `method<-` now gives a clear error when assigning a primitive function (e.g. `log`) as a method (#608).
 * `method<-` and `method()` now accept a length-1 list as `signature` for single-dispatch generics, matching the list-of-classes form required for multi-dispatch (#555).
+* `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).
 * `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).
@@ -36,6 +38,7 @@
 * `S7_data<-()` now preserves attributes (like `names` or `dim`) from the replacement data instead of carrying over the originals, so resizing the underlying data works correctly (#478).
 * `S7_error_method_not_found` now has a correct class vector without a duplicate `"error"` entry (@jjjermiah, #604).
 * `S7_inherits()` and `check_is_S7()` now accept any class specification (S7 class, S7 union, S3 class, S4 class, or base type wrapper like `class_integer`), not just S7 classes (#556).
+* `set_props()` now names its first argument `_object` to minimise the chances 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 set properties programmatically (#497).
 * `S7_on_load()` is the new name for `methods_register()`, giving it a nicer symmetry with `S7_on_build()`; `methods_register()` remains available for backward compatibility (#615).
 * `str()` on S7 objects that inherit from data.frame (or other S3 classes whose underlying data has a `dim` attribute incompatible with the bare base type) no longer errors (#494).
 * `super()` now works with S3 and S4 objects, not just S7 objects (#500).

R/class.R

@@ -284,7 +284,7 @@ check_parent <- function(parent, class, call = sys.call(-1L)) {
   parent_class <- class@parent
   if (is.null(parent_class)) {
     stop2(
-      "`.parent` must not be supplied when class has no parent.",
+      "`_parent` must not be supplied when class has no parent.",
       call = call
     )
   }
@@ -298,7 +298,7 @@ check_parent <- function(parent, class, call = sys.call(-1L)) {
     return()
   }
   msg <- sprintf(
-    "`.parent` must be an instance of %s, not %s.",
+    "`_parent` must be an instance of %s, not %s.",
     class_desc(parent_class),
     obj_desc(parent)
   )
@@ -307,11 +307,15 @@ check_parent <- function(parent, class, call = sys.call(-1L)) {
 
 # Object ------------------------------------------------------------------
 
-#' @param .parent,... Parent object and named properties used to construct the
+#' @param _parent,... Parent object and named properties used to construct the
 #'   object.
+#'
+#'   As a convenience, if `...` is a single unnamed list, then the elements of
+#'   that list are used as the properties. This makes it easy to
+#'   programmatically construct an object from a list of property values.
 #' @rdname new_class
 #' @export
-new_object <- function(.parent, ...) {
+new_object <- function(`_parent`, ...) {
   class <- sys.function(-1)
   if (!inherits(class, "S7_class")) {
     stop2("`new_object()` must be called from within a constructor.")
@@ -324,48 +328,45 @@ new_object <- function(.parent, ...) {
     stop2(msg)
   }
 
-  if (!missing(.parent)) {
-    check_parent(.parent, class)
+  if (!missing(`_parent`)) {
+    check_parent(`_parent`, class)
   }
 
-  args <- list(...)
-  if ("" %in% names2(args)) {
-    stop2("All arguments to `...` must be named.")
-  }
+  args <- collect_dots(...)
 
   has_setter <- vlapply(class@properties[names(args)], prop_has_setter)
   self_attrs <- args[!has_setter]
   names(self_attrs) <- prop_storage_rename(names(self_attrs))
 
-  # We must awkwardly operate on `.parent` rather than binding to a local
+  # We must awkwardly operate on `_parent` rather than binding to a local
   # variable; since otherwise the extra binding causes ALTREP-wrapped values to
   # be materialised when byte-compiled (#607).
   attrs <- c(
     list(class = class_dispatch(class), S7_class = class),
     self_attrs,
-    attributes(.parent)
+    attributes(`_parent`)
   )
   attrs <- attrs[!duplicated(names(attrs))]
-  attributes(.parent) <- attrs
+  attributes(`_parent`) <- attrs
 
   # invoke custom property setters
   prop_setter_vals <- args[has_setter]
   for (name in names(prop_setter_vals)) {
-    prop(.parent, name, check = FALSE) <- prop_setter_vals[[name]]
+    prop(`_parent`, name, check = FALSE) <- prop_setter_vals[[name]]
   }
 
   # Don't need to validate if parent class already validated,
   # i.e. it's a non-abstract S7 class
   parent_validated <- inherits(class@parent, "S7_object") &&
     !class@parent@abstract
   validate_from(
-    .parent,
+    `_parent`,
     parent = if (parent_validated) class@parent,
     # Attribute validation failures to the constructor call, not new_object()
     call = sys.call(-1L)
   )
 
-  .parent
+  `_parent`
 }
 
 #' @export

R/convert.R

@@ -49,7 +49,9 @@
 #' @param to An S7 class specification, passed to [as_class()].
 #' @param ... Other arguments passed to custom `convert()` methods. For
 #'   downcasting, these can be used to override existing properties or set new
-#'   ones.
+#'   ones. As a convenience, you can supply a single unnamed list instead of
+#'   individual name-value pairs, which makes it easy to override properties
+#'   programmatically.
 #' @return Either `from` coerced to class `to`, or an error if the coercion
 #'   is not possible.
 #' @export
@@ -122,7 +124,8 @@ convert <- function(from, to, ...) {
   } else if (class_inherits(from, to)) {
     convert_up(from, to)
   } else if (is_down_cast(from, to)) {
-    convert_down(from, to, ...)
+    dots <- collect_dots(...)
+    convert_down(from, to, dots)
   } else if (is_base_class(to)) {
     base_coerce(from, to, ...)
   } else {
@@ -195,12 +198,13 @@ is_down_cast <- function(x, class) {
   class_dispatch_extends(obj_dispatch(x), class_dispatch(class))
 }
 
-convert_down <- function(from, to, ...) {
+convert_down <- function(from, to, user_args = list()) {
   from_class <- S7_class(from)
 
   if (!is_class(from_class)) {
     # `from` is a base or S3 object; pass it as `.data` to the constructor
-    return(to(.data = from, ...))
+    user_args$.data <- from
+    return(do.call(to, user_args))
   }
 
   # Use `from` as a prototype/seed when constructing `to`: copy over property
@@ -217,7 +221,6 @@ convert_down <- function(from, to, ...) {
   }
 
   # Drop properties overridden by user-supplied arguments
-  user_args <- list(...)
   from_prop_names <- setdiff(from_prop_names, names(user_args))
 
   from_prop_values <- props(from, from_prop_names)

R/property.R

@@ -497,11 +497,14 @@ props <- function(object, names = prop_names(object)) {
 }
 
 #' @export
-#' @param ... Name-value pairs given property to modify and new value.
+#' @param _object The object to modify.
+#' @param ... Name-value pairs given property to modify and new value. As a
+#'   convenience, you can supply a single unnamed list instead of individual
+#'   name-value pairs, which makes it easy to set properties programmatically.
 #' @rdname props
-set_props <- function(object, ..., .check = TRUE) {
-  props(object, check = .check) <- list(...)
-  object
+set_props <- function(`_object`, ..., .check = TRUE) {
+  props(`_object`, check = .check) <- collect_dots(...)
+  `_object`
 }
 
 as_properties <- function(x, call = sys.call(-1L)) {

R/utils.R

@@ -52,6 +52,27 @@ names2 <- function(x) {
   }
 }
 
+# Collect `...` into a named list. As a convenience, a single unnamed list is
+# spliced in so its elements become the values, making it easy to supply
+# values programmatically. All values must be named.
+collect_dots <- function(..., call = sys.call(-1)) {
+  args <- list(...)
+
+  is_single_list <- length(args) == 1L &&
+    !nzchar(names2(args)) &&
+    is.list(args[[1L]])
+
+  if (is_single_list) {
+    args <- args[[1L]]
+    if ("" %in% names2(args)) {
+      stop2("All elements of `..1` must be named.", call = call)
+    }
+  } else if ("" %in% names2(args)) {
+    stop2("All arguments to `...` must be named.", call = call)
+  }
+  args
+}
+
 is_prefix <- function(x, y) {
   length(x) <= length(y) && identical(unclass(x), unclass(y)[seq_along(x)])
 }

man/convert.Rd

@@ -160,26 +160,26 @@
       Error in `new_object()`:
       ! `new_object()` must be called from within a constructor.
 
-# new_object() / errors if `.parent` doesn't inherit from the parent class (#409)
+# new_object() / errors if `_parent` doesn't inherit from the parent class (#409)
 
     Code
       Foo()
     Condition
       Error in `new_object()`:
-      ! `.parent` must be an instance of <Bar>, not S3<S7_base_class>.
+      ! `_parent` must be an instance of <Bar>, not S3<S7_base_class>.
     Code
       Baz()
     Condition
       Error in `new_object()`:
-      ! `.parent` must be an instance of <integer>, not <character>.
+      ! `_parent` must be an instance of <integer>, not <character>.
 
-# new_object() / errors if `.parent` is supplied but class has no parent
+# new_object() / errors if `_parent` is supplied but class has no parent
 
     Code
       NoParent()
     Condition
       Error in `new_object()`:
-      ! `.parent` must not be supplied when class has no parent.
+      ! `_parent` must not be supplied when class has no parent.
 
 # new_object() / validates object