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't have property named after class in constructor #373

Closed
jonthegeek opened this issue Oct 2, 2023 · 7 comments
Closed

Can't have property named after class in constructor #373

jonthegeek opened this issue Oct 2, 2023 · 7 comments

Comments

@jonthegeek
Copy link
Contributor

This one was tricky to find, but this Stack Overflow post about the underlying error message helped me find it.

library(S7)

class_a <- new_class("class_a", properties = list(x = class_character))
class_b <- new_class(
  "class_b",
  properties = list(class_a = class_a),
  constructor = function(class_a = class_a()) {
    # Do things to normalize class_a or whatever.
    new_object(
      S7_object(),
      class_a = class_a
    )
  }
)
class_b()
#> Error in new_object(S7_object(), class_a = class_a): promise already under evaluation: recursive default argument reference or earlier problems?

Created on 2023-10-02 with reprex v2.0.2

The class used as a property specifically has to have the same name as the property to trigger the error. You also need to set class_a() (the constructor call) as the default for class_a (the property).

I think I can work around this (and it might be good to differentiate the instances of the classes (as properties) from the classes, but it led me into a pit of misunderstanding, since supplying class_missing as the default (like in the printed default constructor) seemed to fix things in every case except the thing in #372.

@jonthegeek
Copy link
Contributor Author

More info: it's specifically the name of the constructor function, not the name of the class:

library(S7)

class_a <- new_class("a", properties = list(x = class_character))
class_b <- new_class(
  "class_b",
  properties = list(class_a = class_a),
  constructor = function(class_a = class_a()) {
    # Do things to normalize class_a or whatever.
    new_object(
      S7_object(),
      class_a = class_a
    )
  }
)
class_b()
#> Error in new_object(S7_object(), class_a = class_a): promise already under evaluation: recursive default argument reference or earlier problems?

Created on 2023-10-05 with reprex v2.0.2

@hadley
Copy link
Member

hadley commented Oct 5, 2023

The root cause is almost certainly https://github.com/RConsortium/S7/blob/main/R/constructor.R#L41, where I'm trying to create a default constructor that looks like human written code. It's probably better to instead guarantee that there are no conflicts with user code by adding a non-syntactic prefix.

@jonthegeek
Copy link
Contributor Author

Does that get called if constructors are supplied? That code seems like a likely culprit, but this still produces the same error:

library(S7)

class_a <- new_class(
  "a",
  properties = list(x = class_character),
  constructor = function(x = character()) {
    new_object(
      S7_object(),
      x = x
    )
  }
)
class_b <- new_class(
  "class_b",
  properties = list(class_a = class_a),
  constructor = function(class_a = class_a()) {
    # Do things to normalize class_a or whatever.
    new_object(
      S7_object(),
      class_a = class_a
    )
  }
)
class_b()
#> Error in new_object(S7_object(), class_a = class_a): promise already under evaluation: recursive default argument reference or earlier problems?

Created on 2023-10-05 with reprex v2.0.2

@hadley
Copy link
Member

hadley commented Oct 5, 2023

Well now you've created the problem and are responsible for fixing it, e.g. with:

class_b <- new_class(
  "class_b",
  properties = list(class_a = class_a),
  constructor = function(class_a = NULL) {
    class_a <- class_a %||% class_a()
    # Do things to normalize class_a or whatever.
    new_object(
      S7_object(),
      class_a = class_a
    )
  }
)

But maybe that implies my diagnosis was incorrect and it's always been in your power to fix this?

@hadley
Copy link
Member

hadley commented Oct 5, 2023

(I made a note to mention this in the design book: tidyverse/design#172)

@jonthegeek
Copy link
Contributor Author

jonthegeek commented Oct 5, 2023

The example in the book is helpful! I couldn't wrap my head around what was happening and thought it was S7-specific-ish. With that example, I don't think it is!

Thanks for the clarification! I can deal with this from here. No bug on your end, I think?

@hadley
Copy link
Member

hadley commented Oct 5, 2023

Yeah, I think so.

@hadley hadley closed this as completed Oct 5, 2023
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

2 participants