[go: up one dir, main page]

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

Mutating & erring S3-dispatch attempts #432

Open
mmuurr opened this issue Jan 9, 2024 · 0 comments
Open

Mutating & erring S3-dispatch attempts #432

mmuurr opened this issue Jan 9, 2024 · 0 comments

Comments

@mmuurr
Copy link
mmuurr commented Jan 9, 2024

structure() modifies the object (unexpectedly)

This is somewhat related to #62, but only that it was discovered via the conversation and thoughts around that issue.
I think independent of that issue there's a small change that could prevent a hard-to-track-down problem (for some).
The issue is if toJSON() is ever called -- even when it shouldn't be -- on a reference-semantics object, like an R6 object, simply calling toJSON() will 'damage' the original object.
Here's a trivial example:

x <- R6::R6Class("Klass")$new()  ## do-nothing Klass

class(x)
# [1] "Klass" "R6"

x
# <Klass>
#   Public:
#     clone: function (deep = FALSE)

jsonlite::toJSON(x)  ## won't work, because no methods for Klass nor R6
# Error: No method asJSON S3 class: R6

class(x)  ## but now `x` has been stripped of Klass! this is (probably) unexpected
# [1] "R6"

x  ## indeed, `x` has been modified _in-place_
# <R6>
#   Public:
#     clone: function (deep = FALSE)

The culprit is this line, where structure() modifies reference-semantics objects in-place:

return(asJSON(structure(x, class = class(x)[-1]), force = force, ...))

I'm not aware of an obvious way to deal with this directly via the object x, and attempting to clone the object (to then recurse on the cloned version) seems both inefficient and way of out scope for {jsonlite}.

BUT, since at this point in the dispatch code we'd be dealing with S3 objects (well, not S4 objects), and the existing code effectively replicates the NextMethod() procedure, we can conceivably look for the first non-NULL getMethod() result, like so:

...
} else if (length(class(x)) > 1) {
  for (cls in class(x)[-1]) {  ## this should always fail for the first el of class(), else we'd already have dispatched to its method
    f <- getMethod("asJSON", cls, optional = TRUE)
    if (!is.null(f)) f(x, ...)  ## note that the `force` arg is no longer needed since we found our method
  }
} else if ...

toJSON() errs via unclass(), even when force = TRUE is used.

A second unexpected error can happen when unclass(x) fails, even with force = TRUE, here:

jsonlite/R/asJSON.ANY.R

Lines 14 to 16 in 0134cba

} else if (isTRUE(force) && existsMethod("asJSON", class(unclass(x))[1])) {
# As a last resort we can force encoding using the unclassed object
return(asJSON(unclass(x), force = force, ...))

thing <- R6::R6Class("Thing")$new()
jsonlite::toJSON(thing, force = TRUE)

# Error in unclass(x) : cannot unclass an environment

This one is easy-enough to handle, with some try() or tryCatch() logic, I think.

PR

I've opened a PR that helps to deal with these cases; all tests pass and I think there's only minimal additional overhead (versus the structure() & unclass() recursive approach). I haven't added tests yet for preventing either the mutation of x or erring (when unclass(x) fails), but if this PR has any potential future of being merged, I'm obviously happy to write those test cases, too.

Side-note, is it possible these two lines are out-of-order?

jsonlite/R/asJSON.ANY.R

Lines 18 to 19 in 0134cba

return(asJSON(NULL))
warning("No method asJSON S3 class: ", class(x))

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

1 participant