Using @export on a S3 method registers it and does not export it

courtiol commented 2 years ago

Adding #'@export to a function such as predict.myclass() adds S3method(predict, myclass) to the NAMESPACE file of the package {foo}. However it does not add export(predict.myclass).

The result is that foo::predict.myclass does not exist (only foo:::predict.myclass).

That can create problem.

For example, if a package {bar} makes no call to foo::fn but only to the generic stats::predict(), then the dispatch won't work. This is because the foo namespace will not be present in the search path (even if package is listed in Imports).

We can go around this problem by e.g. using requireNamespace("foo") inside bar, but this is not elegant.

Also it is sometimes nice to refer, in internal code at least, directly to the function used so as not to obfuscate where functions come from. (Another good reason for this is that CMD won't fault if foo is not declared in Imports if stats::predict() is used).

Perhaps you had a good reason not to create export(predict.myclass) on top of S3method(predict, myclass), but I think that #'@export suggests to the package developer that the function will be exported while in fact what happens now is that the function is registered but not exported.

If that does not cause problems I don't anticipate, I would thus recommend that #'@export should generate the export statement in the NAMESPACE file (in addition to the registration).

gaborcsardi commented 2 years ago

The current practice is that the S3 methods themselves are not exported. Most of the time there is no need to export the methods. The generics will still find them at dispatch time, as long as they are registered, and this is exactly what roxygen2 does.

Practically all packages follow this pattern, including base R itself. E.g: https://github.com/wch/r-source/blob/71e6c5e488522a9051b4710bd74940a890905311/src/library/tools/NAMESPACE#L66

If you still want to export an S3 method, in addition to registering it, you can do it like this:

#' @export format.foo
#' @export
format.foo <- function(x, ...) {
  ...
}

But considering the current practice, this is not something roxygen2 should do by default.

courtiol commented 2 years ago

Thanks @gaborcsardi,

It is not a problem for base because its namespace will always be attached. The problem occurs whenever a package namespace is not attached, which happens in the admittedly rare case where the only function used from a package is a S3 method (I could produce a reprex if needed).

I cannot judge of the current practice since at least in the case of predict(), it is used in stats packages not relying on Roxygen which have a call to exportPattern("^[[:alpha:]]+") and hence do export their methods (e.g. https://github.com/cran/spaMM/blob/0b72e9f0dd460fa4d35c24e874f3e3bb3527fa3a/NAMESPACE#L1).

I am OK with your solution. Let's see if that happens to more people...

gaborcsardi commented 2 years ago

It is not a problem for base because its namespace will always be attached.

that links was to the tools package

The problem occurs whenever a package namespace is not attached

For S3 method search it does not matter if a package is attached or not. E.g.

> debug(tools:::format.undoc)
> search()
[1] ".GlobalEnv"        "package:stats"     "package:graphics"
[4] "package:grDevices" "package:utils"     "package:datasets"
[7] "package:methods"   "Autoloads"         "package:base"
> x <- structure(list("foo"), class = "undoc")
> format(x)
debugging in: format.undoc(x)
debug: {
...

Let's see if that happens to more people...

I am fairly sure that there is no issue to solve here. But if you create a reprex or show your package, then we can be completely sure.

courtiol commented 2 years ago

Thanks @gaborcsardi,

here is what I think constitutes a reprex using the following packages:

bar_0.0.0.9000.tar.gz foo_0.0.0.9000.tar.gz

remotes::install_local("./bar_0.0.0.9000.tar.gz")
#> * checking for file ‘/tmp/RtmpaVTyxD/remotes9e0f564d6ed8c/bar/DESCRIPTION’ ... OK
#> * preparing ‘bar’:
#> * checking DESCRIPTION meta-information ... OK
#> * checking for LF line-endings in source and make files and shell scripts
#> * checking for empty or unneeded directories
#> * building ‘bar_0.0.0.9000.tar.gz’
#> Installing package into '/home/courtiol/R/x86_64-pc-linux-gnu-library/4.1'
#> (as 'lib' is unspecified)
devtools::check_built("./foo_0.0.0.9000.tar.gz")
#> ── Checking ───────────────────────────────────────────────────────────── foo ──
#> Setting env vars:
#> • _R_CHECK_CRAN_INCOMING_USE_ASPELL_: TRUE
#> • _R_CHECK_CRAN_INCOMING_REMOTE_    : FALSE
#> • _R_CHECK_CRAN_INCOMING_           : FALSE
#> • _R_CHECK_FORCE_SUGGESTS_          : FALSE
#> ── R CMD check ─────────────────────────────────────────────────────────────────
#> * using log directory ‘/tmp/RtmpaVTyxD/foo.Rcheck’
#> * using R version 4.1.3 (2022-03-10)
#> * using platform: x86_64-pc-linux-gnu (64-bit)
#> * using session charset: UTF-8
#> * using options ‘--no-manual --as-cran’
#> * checking for file ‘foo/DESCRIPTION’ ... OK
#> * this is package ‘foo’ version ‘0.0.0.9000’
#> * package encoding: UTF-8
#> * checking package namespace information ... OK
#> * checking package dependencies ... OK
#> * checking if this is a source package ... OK
#> * checking if there is a namespace ... OK
#> * checking for executable files ... OK
#> * checking for hidden files and directories ... OK
#> * checking for portable file names ... OK
#> * checking for sufficient/correct file permissions ... OK
#> * checking serialization versions ... OK
#> * checking whether package ‘foo’ can be installed ... OK
#> * checking installed package size ... OK
#> * checking package directory ... OK
#> * checking for future file timestamps ... OK
#> * checking DESCRIPTION meta-information ... OK
#> * checking top-level files ... OK
#> * checking for left-over files ... OK
#> * checking index information ... OK
#> * checking package subdirectories ... OK
#> * checking R files for non-ASCII characters ... OK
#> * checking R files for syntax errors ... OK
#> * checking whether the package can be loaded ... OK
#> * checking whether the package can be loaded with stated dependencies ... OK
#> * checking whether the package can be unloaded cleanly ... OK
#> * checking whether the namespace can be loaded with stated dependencies ... OK
#> * checking whether the namespace can be unloaded cleanly ... OK
#> * checking loading without being on the library search path ... OK
#> * checking dependencies in R code ... OK
#> * checking S3 generic/method consistency ... OK
#> * checking replacement functions ... OK
#> * checking foreign function calls ... OK
#> * checking R code for possible problems ... OK
#> * checking Rd files ... OK
#> * checking Rd metadata ... OK
#> * checking Rd line widths ... OK
#> * checking Rd cross-references ... OK
#> * checking for missing documentation entries ... OK
#> * checking for code/documentation mismatches ... OK
#> * checking Rd \usage sections ... OK
#> * checking Rd contents ... OK
#> * checking for unstated dependencies in examples ... OK
#> * checking R/sysdata.rda ... OK
#> * checking examples ... ERROR
#> Running examples in ‘foo-Ex.R’ failed
#> The error most likely occurred in:
#> > base::assign(".ptime", proc.time(), pos = "CheckExEnv")
#> > ### Name: test
#> > ### Title: Test
#> > ### Aliases: test
#> > 
#> > ### ** Examples
#> > 
#> > test()
#> $x
#> [1] 1
#> attr(,"class")
#> [1] "bar"
#> attr(,"row.names")
#> [1] 1
#> Error in UseMethod("predict") : 
#>   no applicable method for 'predict' applied to an object of class "bar"
#> Calls: test -> <Anonymous>
#> Execution halted
#> * checking for non-standard things in the check directory ... OK
#> * checking for detritus in the temp directory ... OK
#> * DONE
#> 
#> Status: 1 ERROR
#> See
#>   ‘/tmp/RtmpaVTyxD/foo.Rcheck/00check.log’
#> for details.
#> 
#> ── R CMD check results ───────────────────────────────────── foo 0.0.0.9000 ────
#> Duration: 8.9s
#> 
#> > checking examples ... ERROR
#>   Running examples in ‘foo-Ex.R’ failed
#>   The error most likely occurred in:
#>   
#>   > base::assign(".ptime", proc.time(), pos = "CheckExEnv")
#>   > ### Name: test
#>   > ### Title: Test
#>   > ### Aliases: test
#>   > 
#>   > ### ** Examples
#>   > 
#>   > test()
#>   $x
#>   [1] 1
#>   
#>   attr(,"class")
#>   [1] "bar"
#>   attr(,"row.names")
#>   [1] 1
#>   Error in UseMethod("predict") : 
#>     no applicable method for 'predict' applied to an object of class "bar"
#>   Calls: test -> <Anonymous>
#>   Execution halted
#> 
#> 1 error x | 0 warnings ✓ | 0 notes ✓
#> Error: R CMD check found ERRORs

^{Created on 2022-04-09 by the reprex package (v2.0.1)}

Setup

{bar} contains a single function -- predict.bar() -- defined as follows:

#' Predict for class bar
#'
#' @param object input
#' @param ... other
#' @export
#'
predict.bar <- function(object, ...) {
  print("predict for class bar")
}

The NAMESPACE file of {bar} contains the registration of the method:

# Generated by roxygen2: do not edit by hand

S3method(predict,bar)

{foo} contains an internal object of class bar --d-- stored in R/sysdata.rda (following https://r-pkgs.org/data.html#data-sysdata #14.2). The check results above do show that d is found.

{foo} also contain a single function -- test() -- defined as follows:

#' Test
#'
#' @export
#' @examples
#' test()
test <- function() {
  print(d)
  stats::predict(d)
}

The DESCRIPTION file from {foo} does declare the Import of {bar} :

Package: foo
Title: What the Package Does (One Line, Title Case)
Version: 0.0.0.9000
Authors@R: 
    person("First", "Last", , "first.last@example.com", role = c("aut", "cre"),
           comment = c(ORCID = "YOUR-ORCID-ID"))
Description: What the package does (one paragraph).
License: GPL (>= 3)
Encoding: UTF-8
Roxygen: list(markdown = TRUE)
RoxygenNote: 7.1.2
Depends: 
    R (>= 2.10)
LazyData: true
Imports: 
    bar,
    stats

So the mystery to solve here is why we have a 'no applicable method for 'predict' applied to an object of class "bar"'.

So either I am missing something big and obvious (very possible) or something is wrong.

My hypothesis was that it is because predict.bar() is not exported sensu stricto by Roxygen. But I played with exports and it does not seem to solve the issue.

Now I think that the issue is that {bar} is not automatically added to the search path because there is not a single call to bar::, which I guess has nothing to do with Roxygen (although if predict.bar() was exported for real, a call to bar::predict.bar() would solve the issue.)

Of course it is easy to go around the issue by calling requireNamespace("bar") in test(), but it still feels wrong (to me at least).

Perhaps I should try to lobby CRAN so that all packages added to Imports are added to the search path?

Or did I really miss something big and obvious?

Thanks for having a look, Alex

f-rousset commented 2 years ago

I reproduce the problem if I use remotes::install_local("./bar_0.0.0.9000.tar.gz") and devtools::check_built("./foo_0.0.0.9000.tar.gz") as shown above, but not by remotes::install_local("./bar_0.0.0.9000.tar.gz") and devtools::check(document = FALSE) (i.e. by Rstudio's shortcut for checking packages...)

courtiol commented 2 years ago

I reproduce the problem if I use remotes::install_local("./bar_0.0.0.9000.tar.gz") and devtools::check_built("./foo_0.0.0.9000.tar.gz") as shown above, but not by remotes::install_local("./bar_0.0.0.9000.tar.gz") and devtools::check(document = FALSE) (i.e. by Rstudio's shortcut for checking packages...)

In project foo, devtools::check() gives me the same error as devtools::check_built(). @f-rousset, did you test in bar rather than in foo?

courtiol commented 2 years ago

The same problem is also detected outside R (with an additional informative NOTE supporting my last remark):

R CMD INSTALL bar_0.0.0.9000.tar.gz  
R CMD check foo_0.0.0.9000.tar.gz

<...>
* checking dependencies in R code ... NOTE
Namespace in Imports field not imported from: ‘bar’
  All declared Imports should be used.
<...>
Error in UseMethod("predict") : 
  no applicable method for 'predict' applied to an object of class "bar"
Calls: test -> <Anonymous>
Execution halted
<...>

f-rousset commented 2 years ago

Sorry, I mixed foo and bar. So starting again: I now precisely have first the NOTE that you show: * checking dependencies in R code ... NOTE Namespace in Imports field not imported from: ‘bar’ All declared Imports should be used. But precisely shouldn't this be corrected first? E.g. (referring to another discussion) if rangeris in the Imports then one should importFrom it something (Here I would expect one function able to create an object of class "bar"; there is no need for exportPattern("^[[:alpha:]]+") which was mentioned earlier in this thread). I tested this by adding a function bar in package bar , exporting it by export("bar") in its NAMESPACE, and adding importFrom("bar","bar") in the foo NAMESPACE (All manual edits, not using roxygen; and there is no need to actually use bar::bar in the foo code AFAICS). The Error in UseMethod("predict") then disappears when checking foo.

courtiol commented 2 years ago

To sum up:

Behaviour

registering a S3 method is sufficient (i.e. no true export needed) as long as the namespace for the method is included in the search path.
for a namespace to be automatically included in the search path, at least one function of the same package as that of the method must be truly exported (registration not sufficient per se)

Possible issues

R does not add packages in the search path based on the DESCRIPTION file alone
rcmdcheck::rcmdcheck() (called by devtools in example above) does not capture the informative note generated by R CMD check
Roxygen label @export is misleading in this context since it does not truly export S3 methods

Possible workarounds

use any other function from {bar} in {foo}
calling requireNamespace("bar") to add {bar} in the search path directly
double export (@export predict.bar + @export in predict.bar() as indicated by @gaborcsardi) and then direct use of bar::predict.bar() instead of generic stats::predict().

Open questions

which of the "possible issues" are to be considered as genuine issues and reported?
should we revise the current practice which consists in not truly exporting any S3 methods?

gaborcsardi commented 2 years ago

This has nothing to do with the search path. R fails to find the method if the package that adds the method is not loaded:

❯ R -q -e 'foo::test()'
> foo::test()
$x
[1] 1

attr(,"class")
[1] "bar"
attr(,"row.names")
[1] 1
Error in UseMethod("predict") :
  no applicable method for 'predict' applied to an object of class "bar"
Calls: <Anonymous> -> <Anonymous>
Execution halted

If you load bar, then the method is registered at load time:

❯ R -q -e 'loadNamespace("bar"); foo::test()'
> loadNamespace("bar"); foo::test()
<environment: namespace:bar>
$x
[1] 1

attr(,"class")
[1] "bar"
attr(,"row.names")
[1] 1
[1] "predict for class bar"

So if you don't import anything from bar, you need to make sure that you load it, if you want to use methods that bar defines for your objects.

courtiol commented 2 years ago

I thought loading = adding namespace to search path, and it does not solve the open questions, but OK.

gaborcsardi commented 2 years ago

I thought loading = adding namespace to search path

No, that is not true. You can load a package without adding it to the search path. E.g. if package A imports package B (via NAMESPACE), then library(A) will load and attach A, and load, but not attach B. So A will be on the search path, B will not.

loadNamespace() loads a package, so its S3 methods are registered, but it does not attach it, so it won't be on the search path.

it does not solve the open questions which of the "possible issues" are to be considered as genuine issues and reported?

There are no issues as far as I can tell. If you want to use a class & method from a package (bar in your example), then you need to make sure that you load that package. You can load it by importing something from it, via NAMESPACE, which is what commonly happens. In the rare case when you are not importing anything from it, you need to load it manually with loadNamespace().

should we revise the current practice which consists in not truly exporting any S3 methods?

I don't think so. roxygen2 works correctly, and according to the current practices.

courtiol commented 2 years ago

Thanks a lot @gaborcsardi !

r-lib / roxygen2