Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can properties not have a default value? #376

Open
mikmart opened this issue Nov 4, 2023 · 10 comments
Open

Can properties not have a default value? #376

mikmart opened this issue Nov 4, 2023 · 10 comments
Labels
feature a feature request or enhancement

Comments

@mikmart
Copy link

mikmart commented Nov 4, 2023

Related to #139.

Now that properties have a default value in the constructor by default, is there a simple way to not have a default value for a property in the constructor?

It could be nice to be able to do something like new_property(default = missing_arg()) or new_property(required = TRUE).

@hadley hadley added the feature a feature request or enhancement label Nov 23, 2023
@hadley
Copy link
Member

hadley commented Nov 23, 2023

Worth thinking about in the context of #356. If we extended new_property() to allow default to be a function that returned an expression, then I think you could do default = function() quote(expr = ). to make a required argument.

@hadley
Copy link
Member

hadley commented Nov 27, 2023

Not sure that's quite right — I think it's probably better for new_property() to use missing() to determine whether or not default is supplied, and use that to create a required field in the property object. Then required properties don't get a default in the constructor. But then what should the constructor use for the default value for properties with a default? Ideally it be the default value, implying that we also need #356 to fully solve this problem. Ideally, this would result in moving much of the property logic from new_object() into new_constructor().

Things to consider:

  • Need to update advice on creating a constructor — it's your responsibility to ensure that the argument defaults match the property defaults
  • Will this even work given the logic in new_object() around custom setters?
  • How does this influence the constructor for classes where the parent is in another package? (e.g. External S7 classes #341)

@hadley
Copy link
Member

hadley commented Dec 11, 2023

Would be nice to have to have a way to have a default value of "the current time".

@t-kalinowski
Copy link
Member

One solution could be to change the signature of the class constructor we build, so that we use the user provided default directly when building the function. This way, users can provide a language object to get the standard R arg promise behavior.

For example:

new_class("foo", properties = list(
  time = new_property(default = quote(Sys.time()))
))

could return a function with signature:

function(time = Sys.time())

while

new_class("foo", properties = list(
  time = new_property(default = Sys.time())
))

would return a function with signature:

function(time = structure(1702317376.61459, class = c("POSIXct", "POSIXt")))

@hadley
Copy link
Member

hadley commented Dec 11, 2023

@t-kalinowski yeah, I was thinking that too. And maybe we don't have to worry too much about backward compatibility because most simple values are the same when you quote them (e.g. identical(TRUE, quote(TRUE)))

@lawremi
Copy link
Collaborator

lawremi commented Aug 15, 2024

One to think about when it comes to defaults is the notion of a class prototype. S4 has this feature and it is quite powerful. By constructing a prototype, in the case of S7 from the property defaults, and then allowing the constructor to override those values, we would avoid some problems.

For example, new_object() sets properties on a partially initialized object, potentially resulting in calls to property setters failing. If we instead had already initialized the object from its prototype, these calls would behave as expected.

Construction would follow this algorithm:

  1. Form the prototype from the defaults
  2. Attach the class attribute
  3. Merge any seed object as described in Allow "seed" arguments to constructors #420
  4. Set any properties passed to the constructor
  5. Validate

This approach is still compatible with having the defaults in the formals of the constructor. That's great for making the constructor self-documenting. Step 4 would just set all of the properties again. We could avoid setting the ones that are missing, if we really wanted to prematurely optimize.

@lawremi
Copy link
Collaborator

lawremi commented Aug 15, 2024

To expand on prototype formation, there should be some effort towards a default default value, probably by calling the constructor of the property's class without arguments. If the no-arg constructor call fails, then we could just use the current default default, NULL.

The methods package uses methods:::tryNew() for this purpose. Note that it tries even harder, using the class's prototype without validation if no-arg construction fails. I'm not sure we should go that far, since if an object is invalid, it might as well be NULL.

Commenting on the original idea of a constructor requiring an argument, I would leave that to user-specified constructors. The default constructor should just rely on validation for whether the object was properly constructed.

@lawremi
Copy link
Collaborator

lawremi commented Aug 21, 2024

Another problem with setting the defaults, instead of forming a prototype from them, is in the example of new_property(), where there is a deprecated property firstName with a setter that throws a deprecation warning. Since all properties are set, regardless of whether they were specified in the call to the constructor, a warning is always thrown upon construction.

NOTE: this only happens on the prop_is_read_only branch, because in main the firstName property is dropped from the constructor due to #421.

If we do move forward with the constructor having property defaults as its argument defaults, then this problem would return, since we would be setting every property.

Two ways around this:

  1. Make construction-time property setting lazy, so that the setter is not called when the value is identical to the default value. This behavior would introduce some complexity though, because setters would only sometimes be called during construction.
  2. Expect the developer of the example person to specify a custom constructor which uses class_missing as the default for properties that should not be set unless specified by the user.

@lawremi
Copy link
Collaborator

lawremi commented Sep 2, 2024

Bug motivating inlining the defaults:

Foo <- new_class("Foo", properties = list(x = class_any))
Bar <- new_class("Bar", Foo, properties = list(x = class_list))
Bar()
# Error: <Bar> object properties are invalid:
# - @x must be <list>, not <NULL>

@t-kalinowski
Copy link
Member

In the development version of S7, it's currently possible to make a property "required" by setting the default value to an error call:

x = new_property(default = quote(stop("`x` is required")))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature a feature request or enhancement
Projects
None yet
Development

No branches or pull requests

4 participants