Roxygen2: assignment of an object of class “call” is not valid for @‘.Data’ in an object of class “MethodDefinition”; is(value, "function") is not TRUE

[kevinrue]

Hi all,

I have been stumbling into the same issue for a while now, when documenting packages that include S4 methods named like primitives of the base package. I have a kind of workaround (described further below), but I would like to learn if and what is the proper way of dealing with this kind of situation.

The issue is that Roxygen crashes with the following message. As a result, the NAMESPACE file is left with only import* directives, but none of the export* directives.

==> devtools::document(roclets=c('rd', 'collate', 'namespace'))

Updating unisets documentation
Writing NAMESPACE
Loading unisets
Error in (function (cl, name, valueClass)  : 
  assignment of an object of class “call” is not valid for @‘.Data’ in an object of class “MethodDefinition”; is(value, "function") is not TRUE
Calls: suppressPackageStartupMessages ... block_find_object -> object_from_call -> parser -> <Anonymous>
In addition: Warning message:
In asNamespace(ns, base.OK = FALSE) :
  operation not allowed on base namespace
Execution halted

Exited with status 1.

This crash happens when I document functions like subset or c like this:

#' @rdname BaseSets-methods
#' @aliases subset.BaseSets subset,BaseSets-method
#'
#' @param ... Additional arguments passed to and from other methods.
#'
#' @section Subsetting:
#'
#' `subset(object, subset, ..., drop=TRUE)` returns subsets of relations which meet conditions.
#' The `subset` argument should be a logical expression referring to any of `"element"`, `"set"`, and any available relation metadata indicating elements or rows to keep: missing values are taken as false.
#' The `drop` logical scalar controls whether elements and sets orphaned during the subsetting should be removed from the `elementData` and `setData` slots, respectively.

#'
#' @importFrom methods as
#' @importFrom BiocGenerics eval unique
#' @importFrom S4Vectors from to subset
#' @method subset BaseSets
#' @export
#'
#' @examples
#'
#' bs1 <- subset(bs, set == "set1" | element == "E")
#' bs1
setMethod("subset", "BaseSets", function(x, ...) {
    .local <- function(x, subset, select, drop=TRUE, ...) {
        # Match code layout of the FuzzySets method
        table <- as.data.frame(x)
        i <- eval(substitute(subset), table)
        out <- x[i, drop=drop]
        # For derived subclasses, coerce back to the original
        as(out, class(x))
    }
    .local(x, ...)
})

I've tried the following too:

> roxygen2::roxygenize()
Loading unisets
Error in (function (cl, name, valueClass)  : 
  assignment of an object of class “call” is not valid for @‘.Data’ in an object of class “MethodDefinition”; is(value, "function") is not TRUE
In addition: Warning message:
In asNamespace(ns, base.OK = FALSE) :
  operation not allowed on base namespace

My current workaround is to define both an S3 an S4 functions, and to document the S3 only, like this:

#' @rdname BaseSets-methods
#' @aliases subset.BaseSets subset,BaseSets-method
#'
#' @param ... Additional arguments passed to and from other methods.
#'
#' @section Subsetting:
#'
#' `subset(object, subset, ..., drop=TRUE)` returns subsets of relations which meet conditions.
#' The `subset` argument should be a logical expression referring to any of `"element"`, `"set"`, and any available relation metadata indicating elements or rows to keep: missing values are taken as false.
#' The `drop` logical scalar controls whether elements and sets orphaned during the subsetting should be removed from the `elementData` and `setData` slots, respectively.

#'
#' @importFrom methods as
#' @importFrom BiocGenerics eval unique
#' @importFrom S4Vectors from to subset
#' @method subset BaseSets
#' @export
#'
#' @examples
#'
#' bs1 <- subset(bs, set == "set1" | element == "E")
#' bs1
subset.BaseSets <- function(x, ...) subset(x, ...)

setMethod("subset", "BaseSets", function(x, ...) {
    .local <- function(x, subset, select, drop=TRUE, ...) {
        # Match code layout of the FuzzySets method
        table <- as.data.frame(x)
        i <- eval(substitute(subset), table)
        out <- x[i, drop=drop]
        # For derived subclasses, coerce back to the original
        as(out, class(x))
    }
    .local(x, ...)
})

Can anyone spot what I'm doing wrong?
The package source code is hosted here: https://github.com/kevinrue/unisets if anyone is willing to open a PR showing me the fix.

Thank you in advance!

[hadley]

If I convert this into the simplest possible reprex, it seems to work ok:

library(roxygen2)
roc_proc_text(namespace_roclet(), "
  setClass('Foo')

  #' @export
  setMethod('subset', 'Foo', function(x, ...) {
    10
  })
")
#> [1] "exportMethods(subset)"

Can you please have a go at making this more realistic to reveal your problem? This process will either reveal what you're doing wrong, or where the problem with roxygen2 lines.

[kevinrue]

Thanks @hadley for replying. Genuinely appreciated.
I'm sorry that I've left this aside for a while now. My code is still pretty much in the same state: https://github.com/kevinrue/unisets/blob/master/R/Sets-class.R#L325

To be honest, I'm still a little bit confused whether it's even necessary to write both S3 and S4 methods in this kind of scenario.
Also, I'm not sure what you mean by "realistic". All I wanted to do at the time is to implement a subset method following best - or at least good - practice.
Consider how subset is a base method, while I wrote an S4 class, I wasn't sure whether to implement an S3 or S4 method, or both.

Lastly I don't want to waste too much of anyone's time, as I will point out that there is effort to develop a similar package in a "tidy" style by the Bioconductor core team here: https://github.com/Kayla-Morrell/BiocSet

Again, this was all mostly to teach myself good practice in terms of package development. I'm mostly self taught and I didn't know where to turn to learn this particular S3/S4 point.

[hadley]

You should not need to write S3 and S4 methods. All I'm looking for is a reprex in the style above, the demonstrates the problem that you are seeing.

[kevinrue]

Here we go:

library(roxygen2)
roc_proc_text(namespace_roclet(), "
  setClass('Foo')

  #' @export
  setMethod('subset', 'Foo', function(x, ...) {
    .local <- function(x, subset, select, drop=TRUE, ...) {
      11
    }
    .local(x, ...)
  })
")
#> Error in (function (cl, name, valueClass) : assignment of an object of class "call" is not valid for @'.Data' in an object of class "MethodDefinition"; is(value, "function") is not TRUE

Seems to be due to function defined within other functions. Bad practice? I know I've picked that up from somewhere, I just can't find from where right now 😅

[kevinrue]

(sorry, I didn't mean to close the issue - accidental keyboard shortcut did that)

[hadley]

Ah, it looks like you’re copying something that S4 does automatically in some cases. I don’t think there’s any need for it here, so you should remove it, and I’ll also make a better error message here.

[kevinrue]

Thanks for the feedback.
It actually helped me figure out where I found this "coding style" in the first place.

E.g. For my own record in the S4Vectors package
https://github.com/Bioconductor/S4Vectors/blob/1a3b937784e7d26158b268a751474891a32baf57/R/Vector-class.R#L575

The source code below

subset_Vector <- function(x, subset, select, drop=FALSE, ...)
{
    ## Fix old objects on-the-fly (e.g. old GRanges, GRangesList, or
    ## GAlignments instances).
    x <- updateObject(x, check=FALSE)
    i <- evalqForSubset(subset, x, ...)
    x_mcols <- mcols(x, use.names=FALSE)
    if (!is.null(x_mcols)) {
        j <- evalqForSelect(select, x_mcols, ...)
        mcols(x) <- x_mcols[ , j, drop=FALSE]
    }
    x[i, drop=drop]
}
setMethod("subset", "Vector", subset_Vector)

is automatically turned by S4 into the following code:

> showMethods("subset", includeDefs = T, classes = "Vector")
Function: subset (package base)
x="Vector"
function (x, ...) 
{
    .local <- function (x, subset, select, drop = FALSE, ...) 
    {
        i <- evalqForSubset(subset, x, ...)
        x_mcols <- mcols(x, use.names = FALSE)
        if (!is.null(x_mcols)) {
            j <- evalqForSelect(select, x_mcols, ...)
            mcols(x) <- x_mcols[, j, drop = FALSE]
        }
        x[i, drop = drop]
    }
    .local(x, ...)
}

I clearly used the latter as template for writing my own source code.
I didn't realize that S4 was doing automatically this kind of conversion. Magic!

[hadley]

Looks like the code never did what it was supposed to (i.e. automatically extracting the S4 generated .local()) so thanks for reporting this!