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

Parent validators are called multiple times during object construction #248

Closed
KatherineCox opened this issue Aug 7, 2022 · 7 comments · Fixed by #291
Closed

Parent validators are called multiple times during object construction #248

KatherineCox opened this issue Aug 7, 2022 · 7 comments · Fixed by #291
Labels
bug an unexpected problem or unintended behavior

Comments

@KatherineCox
Copy link

Naively this feels like it should be true, particularly if the parent class comes next in line when looking up methods. I.e. if I have a parent class pet, and child classes dog and cat, a valid dog object should also be a valid pet object.

On the other hand I imagine it could get slow/complicated to be running through a whole chain of ancestor validators. And possibly a child class might not want to conform to the parent validator if it's too strict, and belongs to somebody else so can't be adjusted. So is this the sort of thing that should be handled by trying to behave politely instead by enforcing it?

If that's the case, If I'm creating a child class, is there a way I could choose to run the parent class's validator on my objects if I'm trying to play nicely?

I don't have much of an OOP background, so I don't know how this is handled in S4 or in other languages.

@hadley
Copy link
Member

hadley commented Aug 7, 2022

I’m pretty sure child classes already have to be valid instances of the parent. Do you have some reason to suspect otherwise or do we need to document this better?

@KatherineCox
Copy link
Author

Ah, you're right, it does run the ancestor validator(s) on their descendents.

library(R7)

# copied from R7 vignette
range <- new_class("range",
  properties = list(
    start = class_double,
    end = class_double
  ),
  validator = function(self) {
    if (length(self@start) != 1) {
      "@start must be length 1"
    } else if (length(self@end) != 1) {
      "@end must be length 1"
    } else if (self@end < self@start) {
      sprintf(
        "@end (%i) must be greater than or equal to @start (%i)",
        self@end,
        self@start
      )
    }
  }
)

# new child class
positive_range <- new_class("positive_range", parent = range,
  validator = function(self) {
    if (self@start < 0) {
      "@start must be greater than or equal to zero"
    }
  }
)

# caught by parent range validator
positive_range(1, -1)
#> Error: <range> object is invalid:
#> - @end (-1) must be greater than or equal to @start (1)

# caught by child positive_range validator
positive_range(-1, 1)
#> Error: <positive_range> object is invalid:
#> - @start must be greater than or equal to zero

From the documentation (which I know is WIP), my impression was that validation

  1. Magically validates property types
  2. Runs the validator function defined in new_class

It was not clear to me that it also runs the parent validator function. It would be helpful to

  1. make that explicit
  2. clarify what order they run in - e.g. can I rely on the parent range validator to be sure start and end are length 1, before running a check in the child positive_range validator that relies on them being length 1, or should I check again in the child?

@KatherineCox
Copy link
Author

While poking around at this, I did notice that it seems to run the ancestor validators multiple times:

library(R7)
parent <- new_class("parent",
  validator = function(self){ print("running parent validator"); NULL })
child <- new_class("child", parent = parent,
  validator = function(self){ print("running child validator"); NULL })
grandchild <- new_class("grandchild", parent = child,
  validator = function(self){ print("running grandchild validator"); NULL })

parent()
#> [1] "running parent validator"
#> <parent>

child()
#> [1] "running parent validator"
#> [1] "running child validator"
#> [1] "running parent validator"
#> <child>

grandchild()
#> [1] "running parent validator"
#> [1] "running child validator"
#> [1] "running parent validator"
#> [1] "running grandchild validator"
#> [1] "running child validator"
#> [1] "running parent validator"
#> <grandchild>

Is this expected behavior?

@hadley
Copy link
Member

hadley commented Aug 8, 2022

Oh good catch, that's definitely not desired behavior!

@hadley
Copy link
Member

hadley commented Aug 8, 2022

Oh this is because the way that the grandchild constructor works is to create a parent object, then use that to construct a child object, then use that to construct a grandchild object. So maybe we should hold off on constructor validation until the final object is created? i.e. maybe we just need a valid_eventually() in the constructor somewhere?

@hadley
Copy link
Member

hadley commented Aug 8, 2022

Note to self: need to fix #249 before this to make debugging easier.

@hadley hadley added the bug an unexpected problem or unintended behavior label Apr 10, 2023
@hadley
Copy link
Member

hadley commented Apr 14, 2023

Updated/somewhat more minimal reprex:

library(S7)
parent <- new_class("parent", validator = function(self) cat("parent\n"))
child <- new_class("child", parent, validator = function(self) cat("child\n"))
grandchild <- new_class("grandchild", child, validator = function(self) cat("grandchild\n"))

parent()
#> parent
#> <parent>

child()
#> parent
#> child
#> parent
#> <child>

grandchild()
#> parent
#> child
#> parent
#> grandchild
#> child
#> parent
#> <grandchild>

Created on 2023-04-14 with reprex v2.0.2

hadley added a commit that referenced this issue Apr 14, 2023
@hadley hadley changed the title Should instances of a child classes also be valid instances of the parent class? Parent validators are called multiple times during object construction Apr 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants