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

Allow read-only properties to be set in the constructor #404

Closed
guslipkin opened this issue Jun 10, 2024 · 11 comments
Closed

Allow read-only properties to be set in the constructor #404

guslipkin opened this issue Jun 10, 2024 · 11 comments
Milestone

Comments

@guslipkin
Copy link

guslipkin commented Jun 10, 2024

Currently the documentation suggests that read-only properties (those with only a getter and no setter) are for computed values such as now or calculating using other properties of the object. However, sometimes you want to create an object with a value that doesn't change, isn't allowed to change, and isn't calculated from other values.

Take this person class, for example. We want to allow the birthdate to be set once and only once on construction, but the person's age will be calculated using the current date.

person <- S7::new_class(
  name = 'person',
  properties = list(
    'name' = S7::class_character,
    'birthdate' = S7::new_property(
      class = S7::class_Date,
      getter = \(self) { self@birthdate }
    ),
    'age' = S7::new_property(
      class = S7::class_any,
      getter = \(self) { Sys.Date() - self@birthdate }
    )
  ),
  constructor = \(name, birthdate) {
    S7::new_object(
      S7::S7_object(),
      'name' = name,
      'birthdate' = birthdate
    )
  }
)

person('Unixtime', as.Date('1970-01-01'))
@lawremi
Copy link
Collaborator

lawremi commented Aug 21, 2024

I think you could do this by having a setter that throws an error if the Date is already set (not NULL). There's also been the suggestion of a property with a dynamic default, see #376.

@guslipkin
Copy link
Author

That's a good idea, but I can't quite get it to work because there are some barriers in the way, including the issue I mentioned in #405 where you can't use default values and a constructor in tandem.

  • If I create a setter that simply assigns @birthdate without any checks, it cannot be NULL because the birthdate property has a class S7::class_Date. I would need to create a union with S7::class_any because NULL does not have a class.
  • If I do the above, but set default = NULL then call person('Unixtime'), I get an error that there is no default value because the default argument defaults to NULL, so that requires a switch to NA
  • However, I've so far been unable to get any combination of default value and setter to work so you need to set the default in the constructor which is less safe in my opinion than using the default argument.

Our setup now looks like this:

person <- S7::new_class(
  name = 'person',
  properties = list(
    'name' = S7::class_character,
    'birthdate' = S7::new_property(
      class = S7::class_Date,
      getter = \(self) { self@birthdate },
      setter = \(self, value) {
        if (is.na(self@birthdate)) self@birthdate <- value else warning('Cannot set read-only value')
        return(self)
      },
    ),
    'age' = S7::new_property(
      class = S7::class_any,
      getter = \(self) { Sys.Date() - self@birthdate }
    )
  ),
  constructor = \(name, birthdate = as.Date(NA)) {
    S7::new_object(
      S7::S7_object(),
      'name' = name,
      'birthdate' = birthdate
    )
  }
)

person('Unixtime')

If you run this, you'll get a C stack error. I'm not really sure why this is.

@lawremi
Copy link
Collaborator

lawremi commented Aug 26, 2024

NULL is a class in S7, so to create a union with NULL it would just be NULL | class_Date. Make sure NULL is first in the union, so that it is used as the property default.

But the value of @birthdate will be NULL the first time the setter runs anyway, since the value has never been set. new_object() sets the default values on the properties, so the setter is run even when setting the default.

However, I was surprised to learn that property access does not behave as I would expect. The getter is run if and only if the underlying attribute value is NULL. This is what causes the infinite recursion: the getter tries to access the property, which has been initialized to NULL, so it runs the getter again, etc. See issue #403 (the one just before this one, ironically).

Anyway, this is an approach you can take:

Person <- S7::new_class(
  name = 'Person',
  properties = list(
    'name' = S7::class_character,
    'birthdate' = S7::new_property(
      class = S7::class_Date,
      setter = \(self, value) {
          if (is.null(self@birthdate))
              self@birthdate <- value
          else warning('Cannot set read-only value')
        return(self)
      },
    ),
    'age' = S7::new_property(
      class = S7::class_any,
      getter = \(self) { Sys.Date() - self@birthdate }
    )
  ),
  constructor = \(name, birthdate = as.Date(NA)) {
    S7::new_object(
      S7::S7_object(),
      'name' = name,
      'birthdate' = birthdate
    )
  }
)
Person("foo")

No getter is needed on @birthdate. Also, S7 class names should generally be capitalized. In this case, it's important, since there is already a "person" S3 class in base R.

@t-kalinowski
Copy link
Member

t-kalinowski commented Sep 5, 2024

Having 'set-once' properties seems like a relatively common use case, and this requires a lot of boilerplate code. I wonder if there's a way we could support this without requiring users to manually specify the constructor.

Two approaches come to mind:

  1. Adjust the signature of new_property() to take an additional argument:
new_property(..., getter = `@`, settable_once = TRUE)
  1. Accept a sentinel value for setter:
new_property(..., getter = `@`, setter = "on_init_only")

Neither strikes me as particularly elegant. Any other ideas?

@t-kalinowski
Copy link
Member

t-kalinowski commented Sep 5, 2024

What if we used new_property(setter = FALSE) for read-only, non-dynamic properties, which can only be set once during object construction?

After reviewing the "Classes and Objects" vignette, I find it helpful to frame properties in terms of dynamic vs. non-dynamic, and settable vs. read-only:

  • Dynamic properties: A property is considered dynamic if it has a getter(). These properties are excluded from the default constructor, regardless of whether they are read-only.
  • Non-dynamic properties: These are always included in the constructor. In this context, "read-only" for non-dynamic properties means "settable only during construction."

Based on this framework:

  • To make a dynamic property read-only, omit the setter (as we currently do).
  • To make a non-dynamic property read-only, we would use setter = FALSE. This includes the property in the constructor signature but prevents modification afterward. (Though users could still manually modify it via attr(self, "property") <- value, this would serve as a fallback.)

Here's a summary of the property types we'd support:

Settable Read-only
Dynamic (excluded from constructor) new_property(getter = <function>, setter = <function>) new_property(getter = <function>)
Non-dynamic (included in constructor) new_property(default = <optional-value>) new_property(setter = FALSE, default = <optional-value>)

@mmaechler
Copy link
Collaborator

I've thought much less than you about this.. still it makes much sense to think of such a 2 x 2 table of possibilities
(I did like the original 2 x 2 table for documentation you had here).

Needing clarification: in your list, you use
new_property(setter = FALSE, default = <value>) ... and indeed the default argument should be compulsory in this non-dynamic read-only case. OTOH, I don't understand why / in which case one should additionally have to say getter = NULL. Shouldn't that be implied by setter = FALSE, default = <value> ?

@t-kalinowski
Copy link
Member

Thanks, @mmaechler. I've edited my previous comment to restore the table and slightly edited the code examples to omit unnecessary args to new_property() (getter = NULL).

In this proposal, supplying default to non-dynamic properties would be optional in both the settable and read-only cases. new_property(setter = FALSE) would result in a non-dynamic property that can only be set once during object construction and appears in the constructor signature with a default value of NULL.

This opens the question of how a user could specify a property that does not have a default value in the constructor (asked in #376). Also, there's the related question of how to specify a default that is a promise that gets evaluated at construction time, like Sys.time().

These might be addressed by supplying a language object to default:

new_property(setter = FALSE, default = quote(expr =))
new_property(setter = FALSE, default = quote(Sys.time()))

@mmaechler
Copy link
Collaborator

...

Also, there's the related question of how to specify a default that is a promise that gets evaluated at construction time, like Sys.time().

These might be addressed by supplying a language object to default:

new_property(setter = FALSE, default = quote(expr =))
new_property(setter = FALSE, default = quote(Sys.time()))

Yes, that would be a nice and "R-like" (though advanced R) way, indeed!

@lawremi
Copy link
Collaborator

lawremi commented Sep 11, 2024

  1. For dynamic default values, I'd prefer to use a function, as it is more familiar to people. The only downside of a dynamic default is that if someone actually wants a default value for a function property, they will need to define a function that returns the function (same applies for using a language object).
  2. What is the rationale for excluding a property with a setter from the default constructor? If there is a setter, it seems likely that the property is required, i.e., not setting it during construction would result in an invalid object.

@t-kalinowski
Copy link
Member

t-kalinowski commented Sep 16, 2024

Meeting notes, in agreement this is a good idea:

# Current behavior, not included in constructor because getter is a function
new_property(getter = <function>, setter = <function>)

## proposal for new behavior:
# if there is a setter(), just always include it in the constructor
new_property(getter = <function>, setter = <function>)

@t-kalinowski
Copy link
Member

Closing, as we’ve reached a workable state for the next release.

In S7, you can create a read-only property by throwing an error from a custom setter. For example:

setter = function(self, value) {
  if (is.null(self@birthdate)) {
    self@birthdate <- value
    self
  } else {
    stop('@birthdate can only be set once')
  }
}

See #449 for a summary of the discussion on this and related issues.

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

No branches or pull requests

4 participants