Complete S3 method registration

[hadley]

Part of #455

[hadley]

@t-kalinowski I think this looks better now — it uses the technique that I remember, i.e. you need to register the generic in the global namespace. This is a bit ugly but I don't think there's much that S7 can do about it.

[t-kalinowski]

Unfortunatly, the snippet I gave earlier still fails:

s3_gen <- local({
  function(x) UseMethod("s3_gen")
})

local({
  method(s3_gen, class_character) <- function(x) "char"
  method(s3_gen, class_integer) <- function(x) "int"
})

s3_gen("foo")
#> Error in UseMethod("s3_gen") :
#>   no applicable method for 's3_gen' applied to an object of class "character"

I think this is a limitation of registerS3method(). It registers into environment(generic)$.__S3MethodsTable__., but UseMethod() consults the table associated with topenv(environment(generic)). For normal top-level/package generics these are the same, but for local/generated generics they differ.

If we manually assign into the table that UseMethod() actually consults, it works:

s3_gen <- local({
  function(x) UseMethod("s3_gen")
})

table_env <- topenv(environment(s3_gen))
table_env$.__S3MethodsTable__. <- new.env(hash = TRUE, parent = baseenv())

assign(
  "s3_gen.character",
  function(x) "char",
  envir = table_env$.__S3MethodsTable__.
)

s3_gen("foo")
#> [1] "char"

So I think the patch should keep using base registerS3method() generally, but branch for this special case:

register_S3_method <- function(generic, signature, method, envir = parent.frame()) {
  class <- s3_signature_class(signature[[1]])

  generic_fun <- get(generic$name, mode = "function", envir = envir)
  generic_env <- environment(generic_fun)
  dispatch_env <- topenv(generic_env)

  if (!identical(generic_env, dispatch_env)) {
    table <- dispatch_env[[".__S3MethodsTable__."]]
    if (is.null(table)) {
      table <- new.env(hash = TRUE, parent = baseenv())
      dispatch_env[[".__S3MethodsTable__."]] <- table
    }

    assign(paste(generic$name, class, sep = "."), method, envir = table)
    return(invisible(method))
  }

  registerS3method(generic$name, class, method, envir = envir)
  invisible(method)
}

That lets the ordinary path continue to delegate to base R, while avoiding registerS3method() for the case that it does not handle ideally.

[hadley]

Can you expand on your examples using local() are a use case that we should support? I don't think the goal here is to support every weird and wacky way that you might try to register S3 methods, but instead make this work for the typical use case (i.e. defining S3 methods in an package).

That said, your proposed fix isn't a lot of code, so maybe it's worth patching around the lack of comprehensive API in base R? It just makes me feel a little uncomfortable to be providing tools for registering S3 methods that don't exist elsewhere.

[t-kalinowski]

I'm thinking in general of the category of systems like https://github.com/klmr/box, https://github.com/rticulate/import, https://github.com/wahani/modules, (or my own https://t-kalinowski.github.io/envir/reference/import_from.html when used with an R script) where functions can be defined in one environment and then can be made available in another.

[hadley]

I'm a little sceptical that this is important but it's also not that much code.