Should registered S3 methods be removed when module is unloaded?

[jtlandis]

I noticed that registering external S3 methods in a module will keep that method available, even if the module that registered it is removed with box::unload()

take the example in mod/a.r:

#' @export
foo <- function() structure(list(), class = "foo")

#' @export
bar <- function(x) UseMethod("bar", x)

local_var <- 1L

bar.foo <- function(x) cat("method `foo`, with local var ", local_var,"\n")

and another mod mod/b.r

box::use(./a[bar])

local_var <- 2L

box::register_S3_method("bar", "default", function(x) cat("some method called, with local var ", local_var,"\n"))

Then if you were to use these modules

box::use(mod/a)

a$bar(a$foo())
#method `foo`, with local var  1

a$bar(NULL)
#Error in UseMethod("bar", x) : 
#   no applicable method for 'bar' applied to an object of class "NULL"

box::use(mod/b)
a$bar(NULL)
#some method called, with local var  2 

box::unload(b)
a$bar(NULL)
#some method called, with local var 2

With this you can see that the b module is still in the environment, specifically bound to the .__S3MethodsTable__. of the a module, as as such it can still be changed if the user is determined enough.

b_env <- environment(environment(a$bar)$.__S3MethodsTable__.$bar.default)
unlockBinding("local_var", b_env)
b_env$local_var <- 5
lockBinding("local_var", b_env)
a$bar(NULL)
#some method called, with local var  5 

As the intention behind box is to have self contained modules, it may be surprising to users to find certain S3 methods are maintained (and by proxy, the module environment isn't fully unloaded) after a box::unload() is called.

So this leads me to my question of if external S3 methods should be removed from the .__S3MehtodsTable__. of where ever they are registered once the module itself is unloaded?

I think this requires some thought. I personally do not know all the hedge cases. I imagine there could be a case where there could be a mod/c.r that relies on the foo.default method to exist and would suddenly break if mod/b.r's method were to fully unregister.

[jtlandis]

I spent some more time looking through the documentation and as long as the respective module has an export it seems I can leverage the .on_unload hook and do the following in the mod/b.r file

unregister_S3_method <- function(name, class) {
  defenv <- environment(get(name, envir = environment()))
  tbl <- defenv[[".__S3MethodsTable__."]]
  func <- paste(name, class, sep = ".")
  unlockBinding(func, tbl)
  tbl[[func]] <- NULL
  invisible(TRUE)  
}

.on_unload <- function(ns) {
  unregister_S3_method("bar", "default")
  invisible(TRUE)
}