External S7 classes #341

hadley · 2023-09-09T17:24:06Z

Prior to this PR, if class A in package A extends class B in package B, class A will include a copy of the definition of class B at build-time. Methods are not also copied, so if class B is later updated and package B reinstalled, it's possible to end up with in an inconsistent state where you have the class definition from version 1 and the methods from version 2.

This PR adds a new "external class" object designed to ameliorate this problem by doing more work at run-time rather at build-time. For example, the constructor of class B will now have arguments ... and it will figure out which arguments go to which constructor at run-time (we prefer to do this at build time in order to generate a constructor with named arguments that can be auto-completed etc).

Fixes #317

#Conflicts: # R/zzz.R

hadley · 2023-09-11T18:21:08Z

To do:

Figure out how to test this
Avoid inlining the parent properties at build-time, possibly by setting to some sentinel value that indicates they're dynamic and then replacing class@properties with a call to a helper.

hadley · 2023-09-14T13:25:37Z

@DavisVaughan @t-kalinowski this will need discussion with the full committee before we merge it, but I'd love to get your initial thoughts on both the overall idea and the implementation.

#Conflicts: # R/zzz.R

DavisVaughan · 2023-10-16T13:10:56Z

R/class.R

+    if (!is.null(package) && is_class(parent)) {
+      if (!is.null(parent@package) && !identical(parent@package, package)) {


Does it need to be a nested if?

DavisVaughan · 2023-10-16T13:14:41Z

R/class.R

@@ -128,30 +136,44 @@ new_class <- function(
  }

  # Combine properties from parent, overriding as needed


This comment may be better placed inside inherit_properties() now

DavisVaughan · 2023-10-16T13:17:11Z

R/external-class.R

+
+  constructor <- constructor_fun()
+  if (!is_class(constructor)) {
+    stop("`constructor_fun()` must yield an S7 class")


This might be a fairly esoteric error if you don't actually provide constructor_fun and instead provided package and name incorrectly

DavisVaughan · 2023-10-16T13:17:49Z

R/external-class.R

+
+  constructor <- constructor_fun()
+  if (!is_class(constructor)) {
+    stop("`constructor_fun()` must yield an S7 class")


Suggested change

stop("`constructor_fun()` must yield an S7 class")

stop("`constructor_fun()` must return an S7 class")

DavisVaughan · 2023-10-16T13:18:13Z

R/external-class.R

+#' Classes from other packages
+#'
+#' @description
+#' You need an explicit external class when you want extend a class defined in


Suggested change

#' You need an explicit external class when you want extend a class defined in

#' You need an explicit external class when you want to extend a class defined in

DavisVaughan · 2023-10-16T13:19:42Z

R/external-class.R

+#' when the other package changes your package doesn't need to be rebuilt to
+#' get those changes.
+#'
+#' [new_class()] will automatically convert an S7 class to an external class


It automatically converts the parent right?

DavisVaughan · 2023-10-16T13:24:10Z

R/property.R

@@ -316,7 +317,7 @@ prop_names <- function(object) {
    c("name", "parent", "package", "properties", "abstract", "constructor", "validator")
  } else {
    class <- S7_class(object)
-    props <- attr(class, "properties", exact = TRUE)
+    props <- attr(class, "properties", exact = TRUE)()


Is it worth adding class_properties() or something similar that does this, and maybe adds a comment about the fact that properties are dynamic to support external classes? I think this is called in at least 3 places

DavisVaughan · 2023-10-16T13:26:13Z

tests/testthat/_snaps/class.md

-       @ properties :List of 2
-       .. $ x: <S7_property> 
-       ..  ..$ name     : chr "x"
-       ..  ..$ class    : <S7_base_class>: <integer>
-       ..  ..$ getter   : NULL
-       ..  ..$ setter   : NULL
-       ..  ..$ validator: NULL
-       ..  ..$ default  : NULL
-       .. $ y: <S7_property> 
-       ..  ..$ name     : chr "y"
-       ..  ..$ class    : <S7_base_class>: <integer>
-       ..  ..$ getter   : NULL
-       ..  ..$ setter   : NULL
-       ..  ..$ validator: NULL
-       ..  ..$ default  : NULL
+       @ properties : function ()  


I guess this reduction in info is expected because it is dynamic now?

DavisVaughan · 2023-10-16T13:28:22Z

tests/testthat/test-external-class.R

+      }
+    }
+  )
+  expect_error(foo2(x = -1), "invalid")


Worth a snapshot or no?

DavisVaughan · 2023-10-16T13:34:21Z

R/constructor.R

+  }
+}
+dynamic_args <- function(args, selected) {
+  missing <- setdiff(selected, names(args))


It took me a few minutes to figure this out, maybe add a comment like

# Pad `args` with currently missing arguments

t-kalinowski

The motivation for this PR very clear - the current situation is problematic and can easily lead to problems as packages get updated in a user library. The proposed implementation comes w/ some tradeoffs, (most of which can be mitigated w/ more code). It's worth spelling some of these out:

Needing to call class@properties() as a function is inconsistent w/ how the other class@ attributes are accessed. A new user might even call it surprising, (and also, syntactically noisy). Because this is a user-facing interface, I think we should explore alternative implementations that still lets us inject dynamic properties as needed, w/o converting this property to a closure.
The loss of args names in constructor functions in favor of ... is, I think, a big loss. It means users lose interactive autocomplete suggestions (w/o substantial work to compensate in the IDE) E.g., i imagine across tidyverse/tidymodels, many constructors will live in external packages.
Looking at the example in new_external_class() raises a few questions (mostly related to print()):

> foo <- new_class("foo", properties = list(x = class_integer))
> foo_ex <- new_external_class("S7", "foo", function() foo)
> foo2 <- new_class("foo", parent = foo_ex)
> foo2
<foo> class
@ parent : <S7::foo>
@ constructor: function(...) {...}
@ validator : <NULL>
@ properties :
 $ x: <integer>
> foo2()
<foo>
 @ x: int(0)

Should a properties origin be presented here. When I'm working w/ a subclassed class, I typically want to quickly know what is "new" or "different" from the base class, and having that information presented by default print() would go a long way.
The ... signature of the constructor is unfortunate. Would it make sense to include at least the "new" constructor args in the signature? For example, if foo2 had a property y, defined as new_class("foo", properties(list(y = class_character)), parent = foo_ex), then perhaps the constructor signature could be (..., y), with ... being forwarded to the parent constructor.
(side questions, not directly related to this PR: why are we presenting an empty class validator property?)

Some stray thoughts:

Would it make sense to convert @properties to a locked and sealed environment, rather than a list? Then the environment could be populated once via a call in .onLoad(), similar to the way we recommend registering S7 methods. (also, opens the door for active bindings)
Could other work be done in .onLoad()? Perhaps, building the default constructor with the proper signature? Or catch and warn about edge cases when pkgA must be reinstalled due to an update in pkgB?

lawremi · 2024-07-24T12:57:30Z

An alternative approach would be to record in the namespace every parent class passed to new_class() that is from another package. At namespace load, we simply check whether all of the cached classes are the same (by whatever definition we want) as those currently defined in their original package. If the check fails, we ask the user to reinstall the package. It's the safest approach, because it might be that the package needs to be updated anyway.

This is similar to how some C programs check for ABI breakage by comparing struct sizes with those compiled into a library.

First stab at a dynamic class

d4420ac

This comment was marked as outdated.

Sign in to view

hadley added 6 commits September 10, 2023 12:36

Use a dynamic constructor

99ff6f9

No longer need to cache constructor args

b64a2c3

Dynamic -> external

03618f8

Add basic docs & print method

45d51b8

Remove remnant of old approach

9417ed7

Check constructor_fun yields a constructor

be86116

hadley changed the title ~~First stab at a dynamic class~~ First stab at an external class Sep 10, 2023

hadley added 5 commits September 10, 2023 13:13

Add tests and fix broken code

ec4d621

Test property

2607bc1

Union short hand

3dea7d8

Automatically use external class when needed

9bcf578

Merge commit 'e37574c6355167c93a8035db37a20fdc4dc9796e'

4a33112

#Conflicts: # R/zzz.R

hadley changed the title ~~First stab at an external class~~ External S7 classes Sep 11, 2023

hadley added 4 commits September 13, 2023 07:35

Merged origin/main into dynamic-class

d186490

Ensure default values computed correctly

370c6da

Make properties dynamic

05246fa

Add tests

ab30c8e

hadley marked this pull request as ready for review September 14, 2023 13:24

hadley requested review from t-kalinowski and DavisVaughan and removed request for t-kalinowski September 14, 2023 13:25

Merge commit '507cc9d86985f920f9a8805cf0245fa1d5a5719f'

f8096e0

#Conflicts: # R/zzz.R

DavisVaughan approved these changes Oct 16, 2023

View reviewed changes

t-kalinowski reviewed Oct 16, 2023

View reviewed changes

hadley mentioned this pull request Nov 27, 2023

Can properties not have a default value? #376

Closed

t-kalinowski mentioned this pull request Feb 16, 2024

rewrite prop<-() in C #396

Merged

hadley mentioned this pull request Oct 28, 2024

Don't inline classes in constructor if possible #481

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

External S7 classes #341

External S7 classes #341

hadley commented Sep 9, 2023 •

edited

Loading

This comment was marked as outdated.

hadley commented Sep 11, 2023 •

edited

Loading

hadley commented Sep 14, 2023

DavisVaughan Oct 16, 2023

DavisVaughan Oct 16, 2023

DavisVaughan Oct 16, 2023

DavisVaughan Oct 16, 2023

DavisVaughan Oct 16, 2023

DavisVaughan Oct 16, 2023

DavisVaughan Oct 16, 2023

DavisVaughan Oct 16, 2023

DavisVaughan Oct 16, 2023

DavisVaughan Oct 16, 2023

t-kalinowski left a comment •

edited

Loading

lawremi commented Jul 24, 2024

		if (!is.null(package) && is_class(parent)) {
		if (!is.null(parent@package) && !identical(parent@package, package)) {

		@@ -128,30 +136,44 @@ new_class <- function(
		}

		# Combine properties from parent, overriding as needed

	stop("`constructor_fun()` must yield an S7 class")
	stop("`constructor_fun()` must return an S7 class")

	#' You need an explicit external class when you want extend a class defined in
	#' You need an explicit external class when you want to extend a class defined in

External S7 classes #341

Are you sure you want to change the base?

External S7 classes #341

Conversation

hadley commented Sep 9, 2023 • edited Loading

This comment was marked as outdated.

hadley commented Sep 11, 2023 • edited Loading

hadley commented Sep 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-kalinowski left a comment • edited Loading

Choose a reason for hiding this comment

lawremi commented Jul 24, 2024

hadley commented Sep 9, 2023 •

edited

Loading

hadley commented Sep 11, 2023 •

edited

Loading

t-kalinowski left a comment •

edited

Loading