Include Settable Properties in Default Constructor

[t-kalinowski]

This patch implements the idea proposed in this comment and discussed in the last working group (WG) meeting: summarized here.

With this change, if a property has a setter function, the property will be included as an argument in the default class constructor, and the constructor will call the custom setter for that property.

[hadley]

Now that I see those failures, I'm pretty sure I tried doing this and that's exactly why I gave up 😆 Maybe we could somehow use the property default to determine if it gets turned into a constructor argument?

[t-kalinowski]

Tests pass, but one of the examples in new_property() fails. Specifically, the example where we suggest using custom getter and setter functions to issue a warning that a property is deprecated. Now the deprecated property setter gets called on object construction.

# These can be useful if you want to deprecate a property
person <- new_class("person", properties = list(
  first_name = class_character,
  firstName = new_property(
     getter = function(self) {
       warning("@firstName is deprecated; please use @first_name instead", call. = FALSE)
       self@first_name
     },
     setter = function(self, value) {
       warning("@firstName is deprecated; please use @first_name instead", call. = FALSE)
       self@first_name <- value
       self
     }
   )
))
hadley <- person(first_name = "Hadley")
#> Error: <person>@first_name must be <character>, not <NULL>
#> In addition: Warning message:
#> @firstName is deprecated; please use @first_name instead 

Some alternatives for how to omit a property with a custom setter from the constructor:

1. Require a custom constructor
2. Use quote(expr=) as a sentinel that now means "don't include in constructor," instead of "include as required arg in constructor"
3. Add an argument to new_property() like new_property(constructor_arg = TRUE | FALSE)

I think requiring option 1, a custom constructor here, is a reasonable solution.

[hadley]

The deprecated wrapper is such a nice idea and I'd really prefer not to have require a custom constructor for it. How do defaults work with settable properties currently?

[t-kalinowski]

We take whatever value is supplied to new_property(default = <value>) and use it as the default argument value when building the default constructor function. NULL is a special value that we may coerce to the correct type, but otherwise, we do no additional processing of the default value. This approach allows users to supply items like quoted language objects to create lazy or missing defaults.

[t-kalinowski]

If we add a required() function, we might also consider adding an omit() function. For example:

Foo <- new_class("Foo", properties = list(
  x = new_property(default = required()),
  y = new_property(default = omit())
))

This would result in:

formals(Foo) == alist(x =)

[t-kalinowski]

Currently, the example doesn’t quite work. Adding a getter to deprecate a property also removes that property from the constructor, potentially breaking existing user code unless a custom constructor is provided.

Some other approaches:

1. Infer Initialization Context

We could modify the setter to "do nothing" when it receives a NULL value:

firstName = new_property(
  class = NULL | class_character, 
  setter = function(self, value) {
    if (is.null(value)) return()
    warning("@firstName is deprecated; please use @first_name instead")
    self@first_name <- value
    self
  }
)

This suggests a potential need for a general mechanism to detect when the object is being initialized:

setter = function(self, value) {
  if (initializing(self)) {
    # Handle property during initialization
  } else {
    # Handle property after initialization
  }
}

2. Ignore Missing Values in new_object()

Another approach is to make new_object() ignore missing values like quote(expr=). This would allow deprecation strategies such as:

Person <- new_class("Person", properties = list(
  first_name = class_character,
  firstName = new_property(
     class_character,
     getter = <unchanged>,
     setter = <unchanged>,
     default = quote(expr =)
   )
))

With this approach:

• hadley <- Person(first_name = "Hadley") would work without error or warning.
• hadley <- Person(firstName = "Hadley") would trigger the deprecation warning.

The constructor signature would then be:

args(Person) == function(first_name = character(), firstName) {}

Here, the meaning of a missing argument would change from "this property value must be supplied to the constructor" to "if omitted, this property value is not set." Enforcing that a property value is "required" would then be the job of the validator, which seems reasonable to me.

[hadley]

Thanks for exploring this! Your final idea sounds reasonable to me; I presume you'd start with a PR to make this the default for regular properties first?

[lawremi]

Thanks tackling this. I agree that the second approach is best. One minor thing about the example is that first_name should be added after firstName in case the user was relying on positional matching.

[t-kalinowski]

How about this, starting from the current example in ?new_property:

Person <- new_class("Person", properties = list(
  <...unchanged...>
  default = quote(...)
))

Produces:

args(Person) == function(first_name = character(), ...) {}

The firstName$setter is not invoked by the constructor unless explicitly supplied as a named argument to the constructor call.

[t-kalinowski]

I've revised the PR to take a conservative approach, supporting the different use cases without introducing any new features that would warrant further discussion (e.g., the meaning of a missing or ... property default).

I updated the examples to show how to achieve all the different use cases in #449:

• The "deprecated property" is implemented with a setter that silently accepts NULL.
• The "required property" is implemented with a quoted error call.
• The "read-only (frozen) property" is implemented with a custom setter.

In the future, we may add features that would make some of these examples more ergonomic, but I don't think we'd introduce any changes that would break them as they are currently written.

This PR is ready for another review and merge.

[lawremi]

I like the simplicity of this approach. Please just see the comment I made on the example.