Consistent ... handling

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

@@ -0,0 +1,155 @@
+---
+name: property-dots
+description: Guide for writing S7 functions that accept property name-value pairs via `...`. Use when adding or editing a function whose `...` collects property values (like `new_object()`, `set_props()`, or `convert()`), so that the fixed arguments don't clash with property names.
+---
+
+# Accept properties via `...`
+
+Use this skill when writing a function that collects property name-value pairs
+through `...`, e.g. ``new_object(`_parent`, ...)``, ``set_props(`_object`, ...)``,
+or a downcasting `convert()` method.
+
+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.
+
+`splice_dots()` and the underscore naming convention solve these.
+
+## The underscore convention
+
+Property names starting with `_` are reserved for internal use (documented in
+`new_property()`). 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) <- splice_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`, ...).
+
+Do *not* rename existing public arguments where it would be a breaking change
+without good reason. For example `convert(from, to, ...)` keeps `from`/`to` as
+is; only the dots-handling was updated.
+
+## Collecting the dots with `splice_dots()`
+
+`splice_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
+splice_dots(x = 1, y = 2)      # list(x = 1, y = 2)
+splice_dots(list(x = 1, y = 2)) # list(x = 1, y = 2) -- spliced
+splice_dots(1)                  # error: All arguments to `...` must be named.
+splice_dots(list(1))            # error: All elements of `..1` must be named.
+```
+
+Replace any `args <- list(...)` that collects properties with
+`args <- splice_dots(...)`. Don't hand-roll the single-list check — that logic
+lives in `splice_dots()` (and `is_single_list()`) so all callers behave
+identically.
+
+## Getting the error call right
+
+`splice_dots()` reports its error against its caller via
+`error_call = sys.call(-1)`. This is correct for ordinary functions:
+
+```r
+new_object <- function(`_parent`, ...) {
+  ...
+  args <- splice_dots(...)   # errors blame `new_object()`
+  ...
+}
+```
+
+It is **not** reliable inside an S7 generic, because dispatch rewrites the call
+stack (you'll see a bogus call like `.set_ops_need_as_vector()`). For a
+generic, pass the call explicitly:
+
+```r
+convert <- function(from, to, ...) {
+  ...
+  } else if (is_down_cast(from, to)) {
+    convert_down(from, to, splice_dots(..., error_call = quote(convert())))
+  }
+  ...
+}
+```
+
+## Documenting it
+
+In the `@param` for the underscore argument, explain the unusual name and point
+to `new_property()`:
+
+```r
+#' @param _object The object to modify. It has an unusual name to avoid clashing
+#'   with property names supplied in `...`; see [new_property()] for details.
+```
+
+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; the usage
+line renders as ``set_props(`_object`, ...)``.
+
+## Worked example
+
+A function that returns a modified copy of an object, setting properties from
+`...`:
+
+```r
+#' @param _object The object to modify. It has an unusual name to avoid clashing
+#'   with property names supplied in `...`; see [new_property()] for details.
+#' @param ... Name-value pairs of properties to set. As a convenience, you can
+#'   supply a single unnamed list instead of individual name-value pairs.
+update_props <- function(`_object`, ...) {
+  props(`_object`) <- splice_dots(...)
+  `_object`
+}
+```
+
+Now both of these work, even for a property named `object`:
+
+```r
+update_props(x, object = 1, width = 10)
+update_props(x, list(object = 1, width = 10))
+```
+
+## Checklist
+
+- [ ] Name any clash-prone fixed argument with a leading `_`, backtick-quoted
+      everywhere it appears.
+- [ ] Collect property values with `splice_dots(...)`, not `list(...)`.
+- [ ] For an S7 generic, pass `error_call = quote(generic())` to `splice_dots()`.
+- [ ] Document the `_` argument (cross-reference `new_property()`) and the
+      single-list shortcut on `...`.
+- [ ] Add a test that a property whose name matches the old fixed argument can
+      now be set, and a test for the single-list shortcut.
+- [ ] Run `air format .` and re-document.

NEWS.md

@@ -5,9 +5,11 @@
 * Internal changes to support R-devel (4.6) (#592, #593, #598, #600).
 * Method dispatch on `class_missing` now correctly handles missing arguments forwarded through a wrapper functions (#595).
 * `S7_error_method_not_found` now has a correct class vector without a duplicate `"error"` entry (@jjjermiah, #604).
+* `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 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()` 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).
 * `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_object()` now gives an informative error when `.parent` is a class specification rather than an instance of the parent class (#409).
 * `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).
@@ -26,6 +28,8 @@
 * `S7_data()` now preserves the S3 class when the S7 class inherits from an S3 class, so e.g. `S7_data()` on a data.frame subclass now returns a data.frame (#380).
 * `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_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()` and `new_object()` now name their first arguments `_object` and `_parent`, so a property with any name (e.g. `object`) can be passed through `...` without clashing with the argument. Property names starting with `_` are now reserved for this sort of internal use (#423).
+* `set_props()` accepts a single unnamed named list as a shortcut for splicing property values, making it easier to set properties programmatically (#497).
 * `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).
 
 # S7 0.2.2

R/class.R

@@ -258,7 +258,7 @@ check_parent <- function(parent, class) {
   parent_class <- class@parent
   if (is.null(parent_class)) {
     stop(
-      "`.parent` must not be supplied when class has no parent.",
+      "`_parent` must not be supplied when class has no parent.",
       call. = FALSE
     )
   }
@@ -272,7 +272,7 @@ check_parent <- function(parent, class) {
     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)
   )
@@ -281,11 +281,18 @@ check_parent <- function(parent, class) {
 
 # Object ------------------------------------------------------------------
 
-#' @param .parent,... Parent object and named properties used to construct the
+#' @param _parent,... Parent object and named properties used to construct the
 #'   object.
+#'
+#'   `_parent` has an unusual name to avoid clashing with property names
+#'   supplied in `...`; see [new_property()] for details.
+#'
+#'   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")) {
     stop("`new_object()` must be called from within a constructor.")
@@ -298,41 +305,38 @@ new_object <- function(.parent, ...) {
     stop(msg)
   }
 
-  if (!missing(.parent)) {
-    check_parent(.parent, class)
+  if (!missing(`_parent`)) {
+    check_parent(`_parent`, class)
   }
 
-  args <- list(...)
-  if ("" %in% names2(args)) {
-    stop("All arguments to `...` must be named.")
-  }
+  args <- splice_dots(...)
 
   has_setter <- vlapply(class@properties[names(args)], prop_has_setter)
 
-  # 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),
     args[!has_setter],
-    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 = if (parent_validated) class@parent)
+  validate_from(`_parent`, parent = if (parent_validated) class@parent)
 
-  .parent
+  `_parent`
 }
 
 #' @export

R/convert.R

@@ -37,7 +37,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
@@ -86,7 +88,7 @@ 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, ...)
+    convert_down(from, to, splice_dots(..., error_call = quote(convert())))
   } else {
     msg <- paste_c(
       "Can't find method with dispatch classes:\n",
@@ -127,12 +129,13 @@ is_down_cast <- function(x, class) {
   inherits(x, setdiff(class_dispatch(class), "S7_object"))
 }
 
-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
@@ -149,7 +152,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

@@ -53,6 +53,8 @@
 #'   don't need to set this here, as it's more convenient to supply as
 #'   the element name when defining a list of properties. If both `name`
 #'   and a list-name are supplied, the list-name will be used.
+#'
+#'   Avoid names starting with `_`; they are reserved for internal use.
 #' @returns An S7 property, i.e. a list with class `S7_property`.
 #' @export
 #' @examples
@@ -459,11 +461,15 @@ 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. It has an unusual name to avoid clashing
+#'   with property names supplied in `...`; see [new_property()] for details.
+#' @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) <- splice_dots(...)
+  `_object`
 }
 
 as_properties <- function(x) {

R/utils.R

@@ -44,6 +44,28 @@ names2 <- function(x) {
   }
 }
 
+# Is `...` a single unnamed list argument? Used by functions that accept
+# either named arguments via `...` or a single list spliced in.
+is_single_list <- function(args) {
+  length(args) == 1L && !nzchar(names2(args)) && is.list(args[[1L]])
+}
+
+# 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.
+splice_dots <- function(..., error_call = sys.call(-1)) {
+  args <- list(...)
+  if (is_single_list(args)) {
+    args <- args[[1L]]
+    if ("" %in% names2(args)) {
+      stop(simpleError("All elements of `..1` must be named.", error_call))
+    }
+  } else if ("" %in% names2(args)) {
+    stop(simpleError("All arguments to `...` must be named.", error_call))
+  }
+  args
+}
+
 is_prefix <- function(x, y) {
   length(x) <= length(y) && identical(unclass(x), unclass(y)[seq_along(x)])
 }