klmr / box

Write reusable, composable and modular R code
https://klmr.me/box/
MIT License
862 stars 48 forks source link

Calling `methods::is` from inside a module fails for class names that are not registered #272

Closed amitramon closed 2 years ago

amitramon commented 2 years ago

Error description

Say I have the following function in a module (box):

test_is <- function(s) {
  methods::is(42L, s)
}

Assume the module is in a file "my_box" and I load it via use(my_box). If I call it on know classes everything is fine:

> my_box$test_is("integer")
[1] TRUE
> my_box$test_is("character")
[1] FALSE
> my_box$test_is("data.frame")
[1] FALSE

However, if I call the function with an argument that is not the name of a (registered) class, it fails:

> my_box$test_is("no.class")
Loading required package: my_box
Error in .requirePackage(package) : 
  unable to find required package ‘my_box’
In addition: Warning message:
In library(package, lib.loc = lib.loc, character.only = TRUE, logical.return = TRUE,  :
  there is no package called ‘my_box’

But if I call setOldClass for registering the made-up class name, all is well:

> setOldClass("registered.class")
> my_box$test_is("registered.class")
[1] FALSE

Calling is on some made-up class name directly in R works with no error

> methods::is(42L, "some.made.up.name" )
[1] FALSE

And this is what I would expect from box - this seems like a bug.

Just a side note, I have some old code that is using the old modules package. With that package, in an old branch before the fix of issue 147 it seems that is is working in all the above scenarios, but this could be just a coincidence.

R version

platform       x86_64-pc-linux-gnu         
arch           x86_64                      
os             linux-gnu                   
system         x86_64, linux-gnu           
status                                     
major          4                           
minor          0.5                         
year           2021                        
month          03                          
day            31                          
svn rev        80133                       
language       R                           
version.string R version 4.0.5 (2021-03-31)
nickname       Shake and Throw

‘box’ version

‘1.1.0’

klmr commented 2 years ago

Unfortunately ‘box’ currently doesn’t support S4 (see #95), and I there are currently no plans to add S4 support. Packages are hard-wired into S4, which is why adding S4 support for anything other than packages might actually be impossible.

The fact that setOldClass appears to work is a coincidence and I wouldn’t rely on it. And if using S3 instead of S4 is an option for you I recommend doing that directly (using inherits instead of methods::is, which makes setOldClass unnecessary).

in an old branch before the fix of #147 it seems that is is working in all the above scenarios

Yes, that’s probably correct: the fix for the above issue required giving ‘box’ modules a “package name”, otherwise S3 lookup would also be broken (because topenv() requires it). But even before that change S4 wasn’t really supported.

Incidentally, I am in principle interested in supporting S4, I’m just not sure that it’s possible; but on a more philosophical note I also personally don’t use S4 and I don’t recommend its use with modules anyway — I consider it inferior to both S3 and R6 in their respective domains. Supporting R7 will be higher priority, although this will likewise not be an easy task.

amitramon commented 2 years ago

Thanks for your elaborated response. It makes sense, and I believe I can get along with inhertits for what I need.