Implement convert() and super() #181

hadley · 2022-02-21T19:16:06Z

Fixes #110. Fixes #136

hadley · 2022-02-21T22:29:07Z

@lawremi points out that the main feature of up_cast() is not performance, but that it only affects the dispatch of a single method.

hadley · 2022-02-21T23:32:51Z

https://github.com/RConsortium/OOP-WG/pull/181/files#diff-a68deddd00e9d282550d4445b10c54406e9a17fc9d44e0968ebd44bcc7248080R48-R62 illustrates the difference between cast() and cast_next().

Conflicts: tests/testthat/test-class-spec.R

hadley · 2022-02-22T16:29:38Z

Another possible name for cast_next() would be super()

Conflicts: NEWS.md

hadley · 2022-02-22T17:41:50Z

Renamed to super(); I think that makes it more clear that these are related, but distinct, ideas.

R/cast.R

DavisVaughan · 2022-02-22T19:25:43Z

R/cast.R

+    if (is_base_class(to)) {
+      R7_data(from)


This removes both the class and the properties belonging to from, while the documentation implies that only the class will be removed:

#' `cast()` provides an automatic fallback if `from` inherits from `to`. You #' can override this if you need some special behavior other than simply #' stripping class.

And the other code paths only remove the class

I'm not sure if that was intended, or if the documentation just needs to be more precise.

foo <- new_class("foo", parent = "character", properties = list(a = "integer")) bar <- new_class("bar", parent = foo, properties = list(b = "integer")) x <- bar() x #> <bar> chr(0) #> @ a: int(0) #> @ b: int(0) # lost everything cast(x, "character") #> character(0) # retains `b` attributes(cast(x, foo)) #> $object_class #> <R7_class> #> @ name : foo #> @ parent: <character> #> @ properties: #> $ a: <integer> #> #> $class #> [1] "foo" "character" "R7_object" #> #> $a #> integer(0) #> #> $b #> integer(0)

It's not clear to me what a cast should do here — should it strip properties or keep them? I think you see this inconsistency in the various paths. I'll standardise to make the minimal changes to the class.

R/cast.R

R/class-spec.R

R/super.R

_pkgdown.yml

src/method-dispatch.c

tests/testthat/test-cast.R

DavisVaughan · 2022-02-22T20:27:27Z

tests/testthat/test-super.R

+    method(bar, foo1) <- function(x) 1
+    method(bar, foo3) <- function(x) 3
+
+    expect_error(bar(super(foo3())), "Can't find method")


I feel like this is inconsistent

foo1 <- new_class("foo1") foo2 <- new_class("foo2", foo1) foo3 <- new_class("foo3", foo2) bar <- new_generic("bar", "x") method(bar, foo1) <- function(x) 1 method(bar, foo3) <- function(x) 3 # this doesnt work bar(super(foo3())) #> Error: Can't find method for generic `bar()` with dispatch classes: #> - x: foo2 # but this does bar(foo2()) #> [1] 1

I feel like this is why I previously had mentioned dispatch = class_register(to) in super() might should be dispatch = class_dispatch(to), so match the fact that the C code of method_call_ () typically calls obj_dispatch()

Hmmmm, I'm not sure. It feels weird to do super(foo3, foo2) and then have it dispatch to foo1. I think I'd rather eliminate the dispatch hierarchy here in favour of forcing the user to be explicit.

Do you think we could talk about this one in the next OOP working group meeting?

To me it feels like super(foo3, foo2) says "I return an object of class foo2", meaning that bar(foo2()) is identical to bar(super(foo3(), foo2)) in my head

I am interpreting your argument as: If the user wanted the foo1 behavior, then they should have done super(foo3, foo1), which also feels reasonable (and more explicit). But it still might be nice to discuss this point.

BTW this is consistent with convert() if you do convert(x, to = foo2), you don't expect to get an instance of foo1 back.

But you do get something back that you can call bar() on, and that will dispatch to the foo1 method?

x <- foo3() bar(x) #> [1] 3 bar(convert(x, to = foo2)) #> [1] 1 bar(super(x, to = foo2)) #> Error: Can't find method for generic `bar()` with dispatch classes: #> - x: foo2

Right. It's not directly analogous, but I think it's in the same spirit.

Ok, let's change this so that super(..., to = foo2) behaves exactly like a foo2

ltierney · 2022-02-22T20:36:48Z

Unfortunately I haven't been able to put the time into keeping up with things, so maybe this is all irrelevent -- feel free to ignore if it is. I'm worried about the idea of using casting for delegating method handling up the disptch chain. In other seetings where I've seen this tried it looks OK at first but runs into trouble with slightly more complex settings. Two simple scenarios: 1) A base class A, B a subclass of A, C a subclass of B. f has methods f.A, f.B, and f.C with f.C <- function(x) next_method(x) ## delegate to f.B f.B <- function(x) next_method(x) ## delegate to f.A So f(<a C>) should be handled by f.A. 2) g is another generic with methods g.A and g.B. The method f.A ia defined as f.A <- function(x) g(x) So in f(<a B>) the call in f.A() to g() should be handled by g.B(), not g.A(). Are these going to be handled properly with these casting approaches?

…

-- Luke Tierney Ralph E. Wareham Professor of Mathematical Sciences University of Iowa Phone: 319-335-3386 Department of Statistics and Fax: 319-335-3017 Actuarial Science 241 Schaeffer Hall email: ***@***.*** Iowa City, IA 52242 WWW: http://www.stat.uiowa.edu

Co-authored-by: Davis Vaughan <davis@rstudio.com>

Add test for self

hadley · 2022-02-23T14:21:29Z

@ltierney no problems; the development has been fairly hectic and I've been down a few dead ends. I'd no longer characterise super() as a casting based — it creates a transient wrapper that only affects dispatch for the next generic. I think this resolves your concerns:

library(R7)
A <- new_class("A")
B <- new_class("B", A)
C <- new_class("C", B)

f <- new_generic("f", "x")
# You (now) have to supply the class you want to dispatch
# to, so there is no longer any ambiguity:
method(f, C) <- function(x) c("f-C", f(super(x, B)))
method(f, B) <- function(x) c("f-B", f(super(x, A)))
method(f, A) <- function(x) "f-A"

f(C())
#> [1] "f-C" "f-B" "f-A"

method(f, A) <- function(x) c("f-A", g(x))

# super() only affects dispatch for the next generic, so
# future generics used by the method dispatch on the original class
g <- new_generic("g", "x")
method(g, C) <- function(x) c("g-C", g(super(x, B)))
method(g, B) <- function(x) c("g-B", g(super(x, A)))
method(g, A) <- function(x) "g-A"

f(B())
#> [1] "f-B" "f-A" "g-B" "g-A"

^{Created on 2022-02-23 by the reprex package (v2.0.1)}

Conflicts: NEWS.md R/class.R

hadley · 2022-02-23T22:51:17Z

We should also consider if cast() is the right name. convert() might be more likely to evoke the right sense.

And probably worth naming the second argument as a role model: convert(x, to = y).

ltierney · 2022-02-24T02:20:25Z

Thanks -- that makes sense. So from a user perspective in a call f(super(x, B)) super() is just (an optional) part of the syntax of calling a generic, not an actual function that could be used outside a call to a generic. Whether it creates an ephemeral wrapper or communicates in some other way to the dispatch process is part of the implementation, not something user visible. If I'm understanding this correctly ...

…

On Wed, 23 Feb 2022, Hadley Wickham wrote: @ltierney no problems; the development has been fairly hectic and I've been down a few dead ends. I'd no longer characterise super() as a casting based — it creates a transient wrapper that only affects dispatch for the next generic. I think this resolves your concerns: library(R7) A <- new_class("A") B <- new_class("B", A) C <- new_class("C", B) f <- new_generic("f", "x") # You (now) have to supply the class you want to dispatch # to, so there is no longer any ambiguity: method(f, C) <- function(x) c("f-C", f(super(x, B))) method(f, B) <- function(x) c("f-B", f(super(x, A))) method(f, A) <- function(x) "f-A" f(C()) #> [1] "f-C" "f-B" "f-A" method(f, A) <- function(x) c("f-A", g(x)) # super() only affects dispatch for the next generic, so # future generics used by the method dispatch on the original class g <- new_generic("g", "x") method(g, C) <- function(x) c("g-C", g(super(x, B))) method(g, B) <- function(x) c("g-B", g(super(x, A))) method(g, A) <- function(x) "g-A" f(B()) #> [1] "f-B" "f-A" "g-B" "g-A" Created on 2022-02-23 by the reprex package (v2.0.1) — Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android. You are receiving this because you werementioned.[AA55UVHV2FQQ6XVK7ARV5X3U4TUPJA5CNFSM5O7M7H7KYY3PNVWWK3TUL52HS4DF VREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOH2B6HJY.gif] Message ID: ***@***.***>

-- Luke Tierney Ralph E. Wareham Professor of Mathematical Sciences University of Iowa Phone: 319-335-3386 Department of Statistics and Fax: 319-335-3017 Actuarial Science 241 Schaeffer Hall email: ***@***.*** Iowa City, IA 52242 WWW: http://www.stat.uiowa.edu

hadley · 2022-02-24T13:19:51Z

@ltierney Right. Currently super() does create a transient object, but that's just an implementation detail. I'll think about what we can do to make that more obvious: it at least needs a print method, and maybe @ should give an informative error?

Conflicts: NAMESPACE

lawremi · 2022-03-08T03:44:44Z

I like that super() is an actual function, rather than special syntax that requires special documentation.

hadley · 2022-03-14T16:50:17Z

R/super.R

+#' # convert() affects every generic:
+#' bar2(convert(obj, to = foo1))
+#' # super() only affects the _next_ generic:
+#' bar2(super(obj, foo1))


Name to argument

Conflicts: NEWS.md R/method-introspect.R

hadley added 5 commits February 21, 2022 11:50

First stab at cast()

b3134c4

Test cast methods

a087d76

Use better test for S3 inheritance

f1835ec

Test custom dispatch

acb8674

Add missing function to index

b7f667c

Implement cast_next() to only affect one dispatch

88fa1b1

hadley added 4 commits February 21, 2022 17:33

Merge commit '4675283b713e63aabfd7b935673d629bf40ba336'

0d4cfa9

Conflicts: tests/testthat/test-class-spec.R

Test input validation

e376bfc

Add cast_next to reference index

61518fc

Add example of cast() vs cast_next()

d16bbcd

hadley changed the title ~~Implement cast()~~ Implement cast() and up_cast() Feb 22, 2022

hadley changed the title ~~Implement cast() and up_cast()~~ Implement cast() and cast_next() Feb 22, 2022

hadley added 5 commits February 22, 2022 10:29

Update generic

4b13e4a

Update vignette

5560773

Merge commit '4d5a8509e0879727518993c22d8e8abd2c152145'

c216ce2

Conflicts: NEWS.md

Rename cast_next() to super()

8cd0188

Split apart cast() and super()

aaaedd2

hadley requested a review from DavisVaughan February 22, 2022 17:41

hadley changed the title ~~Implement cast() and cast_next()~~ Implement cast() and super() Feb 22, 2022

Fix reference index again

13a6597

DavisVaughan reviewed Feb 22, 2022

View reviewed changes

hadley and others added 4 commits February 23, 2022 07:27

Apply suggestions from code review

3258c54

Co-authored-by: Davis Vaughan <davis@rstudio.com>

Re-document & test

ee52e79

Make fallback casts more consistent

77f928d

Require class in super()

5a0e56f

hadley added 2 commits February 23, 2022 08:10

Rearrange fallback tests

d097f6f

Add test for self

Re-document

a7aa309

hadley added 2 commits February 23, 2022 08:25

Merge commit '8ba54c71081877db2e2649a30a2a1b587b2b6bd1'

c716937

Conflicts: NEWS.md R/class.R

Update news bullets

62fb2dc

hadley added 6 commits February 24, 2022 08:59

Merge commit 'aca72447509489064938fde2f23e34aa3867d8d4'

22af6d9

Conflicts: NAMESPACE

Strip superclass properties

97aafba

Rename cast to convert

809f20c

Always spell out to argument name

412efe1

Tweak dispatch and document

0f223df

Add some basic convert examples

8e67900

hadley changed the title ~~Implement cast() and super()~~ Implement convert() and super() Feb 24, 2022

Add basic print + str methods for super()

5e1e3f0

hadley commented Mar 14, 2022

View reviewed changes

hadley added 3 commits March 14, 2022 16:33

Merge commit '568b7f48609c8816e4c506eb1206c8583dfbfcbd'

024be53

Conflicts: NEWS.md R/method-introspect.R

Use full class hierarchy

2fef870

Use named to argument in tests/examples

61c11ce

hadley merged commit ad6516c into main Mar 15, 2022

hadley deleted the cast branch March 15, 2022 14:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement convert() and super() #181

Implement convert() and super() #181

hadley commented Feb 21, 2022

hadley commented Feb 21, 2022

hadley commented Feb 21, 2022

hadley commented Feb 22, 2022

hadley commented Feb 22, 2022

DavisVaughan Feb 22, 2022

DavisVaughan Feb 22, 2022

hadley Feb 23, 2022

DavisVaughan Feb 22, 2022

DavisVaughan Feb 22, 2022

hadley Feb 23, 2022

DavisVaughan Feb 23, 2022

hadley Feb 24, 2022

DavisVaughan Feb 24, 2022

hadley Feb 24, 2022

hadley Mar 14, 2022

ltierney commented Feb 22, 2022 via email

hadley commented Feb 23, 2022

hadley commented Feb 23, 2022 •

edited

Loading

ltierney commented Feb 24, 2022 via email

hadley commented Feb 24, 2022

lawremi commented Mar 8, 2022

hadley Mar 14, 2022

Implement convert() and super() #181

Implement convert() and super() #181

Conversation

hadley commented Feb 21, 2022

hadley commented Feb 21, 2022

hadley commented Feb 21, 2022

hadley commented Feb 22, 2022

hadley commented Feb 22, 2022

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

Choose a reason for hiding this comment

ltierney commented Feb 22, 2022 via email

hadley commented Feb 23, 2022

hadley commented Feb 23, 2022 • edited Loading

ltierney commented Feb 24, 2022 via email

hadley commented Feb 24, 2022

lawremi commented Mar 8, 2022

Choose a reason for hiding this comment

hadley commented Feb 23, 2022 •

edited

Loading