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.

[hadley]

Maybe if ... is present it should always be the last argument? Or the first? Otherwise you end up with a mix of arguments that can/can't be supplied by position.

(I would also consider the idea that the first argument to every constructor should be ... just to force all arguments to be fully named)

[t-kalinowski]

the first argument to every constructor should be ...

I think this is going to be too annoying in practice. Many classes will only have a small handful of properties, and being unable to positionally match 1, 2, or 3 would be cumbersome.

Otherwise you end up with a mix of arguments that can/can't be supplied by position.

I think #446 might solve this by allowing the user to specify a property that is unset by default but still present as a named arg in the constructor formals that can match positionally.

[hadley]

In that case, I think you should make ... the last argument, if it's present.

[t-kalinowski]

I updated the code so ... is added at the end of the constructor. Note, deprecating this way now will potentially change the position of later arguments E.g.,

# before deprecating: 
function(firstName, lastName, country)

# after deprecating `firstName` and `lastName`:
function(name, country, ...)
# whereas with the previous approach, this was possible:
function(name, ..., country)

[hadley]

Maybe we should chat a bit more about this live, because I don't quite see the problem.

[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.

[hadley]

Looks great to me. Thanks for all your work on this!

[lawremi]

Shouldn't this firstName property be of class_character? Even though it is deprecated, we still want it to function properly. And I guess we would really want it to be a scalar. What happens if there is no reasonable default, as in this case? Could the default be made to be quote(first_name)? Then the setter avoids a warning if the value is the same as first_name.

[t-kalinowski]

Thanks! I updated the example to show class = class_character, default = quote(first_name). The check in the setter is now identical(value, self@first_name).

Another approach would be to make the class NULL | character, and keep the simple is.null() check in the setter.

[lawremi]

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