Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error in NextMethod after load_all #1636

Closed
Enchufa2 opened this issue Jan 27, 2022 · 13 comments · Fixed by #1637
Closed

Error in NextMethod after load_all #1636

Enchufa2 opened this issue Jan 27, 2022 · 13 comments · Fixed by #1637

Comments

@Enchufa2
Copy link

Enchufa2 commented Jan 27, 2022

Not sure what's going on here, and I have been unable to produce a MVE. This only happened to me with the units package. This is a (not minimal) reproducible example:

$ git clone https://github.com/r-quantities/units.git && cd units
$ R
> iris.u <- iris
> iris.u[1:4] <- lapply(iris.u[1:4], function(x) units::set_units(x, cm))
> plot(Sepal.Length ~ Sepal.Width, iris.u) # plot with units, all ok
> pkgload::load_all()
> plot(Sepal.Length ~ Sepal.Width, iris.u)
Error in NextMethod("plot", xlab = xlab, ylab = ylab) : 
  no method to invoke

Somehow load_all messes up S3 dispatch. Given that this is called by devtools::test in RStudio, I'm basically unable to test formula-based plots there.

> sessionInfo()
R version 4.1.2 (2021-11-01)
Platform: x86_64-redhat-linux-gnu (64-bit)
Running under: Fedora Linux 35 (Thirty Five)

Matrix products: default
BLAS/LAPACK: /usr/lib64/libflexiblas.so.3.1

locale:
 [1] LC_CTYPE=es_ES.UTF-8       LC_NUMERIC=C              
 [3] LC_TIME=en_US.UTF-8        LC_COLLATE=es_ES.UTF-8    
 [5] LC_MONETARY=es_ES.UTF-8    LC_MESSAGES=es_ES.UTF-8   
 [7] LC_PAPER=es_ES.UTF-8       LC_NAME=C                 
 [9] LC_ADDRESS=C               LC_TELEPHONE=C            
[11] LC_MEASUREMENT=es_ES.UTF-8 LC_IDENTIFICATION=C       

attached base packages:
[1] stats     graphics  grDevices datasets  utils     methods   base     

other attached packages:
[1] units_0.8-0    testthat_3.1.2

loaded via a namespace (and not attached):
 [1] Rcpp_1.0.8        prettyunits_1.1.1 ps_1.6.0          crayon_1.4.2     
 [5] withr_2.4.3       rprojroot_2.0.2   brio_1.1.3        R6_2.5.1         
 [9] magrittr_2.0.1    rlang_0.4.12      cli_3.1.1         callr_3.7.0      
[13] desc_1.4.0        tools_4.1.2       glue_1.5.1        processx_3.5.2   
[17] pkgload_1.2.4     compiler_4.1.2    pkgbuild_1.3.0    CoprManager_0.3.9
@lionel-
Copy link
Member

lionel- commented May 30, 2022

I can't reproduce this. Could you try with dev pkgload please?

@Enchufa2
Copy link
Author

Same issue with the dev version. Here is a more reproducible environment:

Set up a container and start R:

$ podman run --rm -it rocker/r-rspm:20.04 bash
$ apt-get update
$ apt-get install -y git libudunits2-dev
$ git clone https://github.com/r-quantities/units.git && cd units
$ R

Then I get:

> install.packages(c("Rcpp", "remotes", "pkgbuild"))
> remotes::install_github("r-lib/pkgload")
> remotes::install_local(".")
> iris.u <- iris
> iris.u[1:4] <- lapply(iris.u[1:4], function(x) units::set_units(x, cm))
> plot(Sepal.Length ~ Sepal.Width, iris.u) # plot with units, all ok
> pkgload::load_all()
> plot(Sepal.Length ~ Sepal.Width, iris.u)
Error in NextMethod("plot", xlab = xlab, ylab = ylab) : 
  no method to invoke

@Enchufa2
Copy link
Author

(Of course, you can use docker instead of podman)

@lionel-
Copy link
Member

lionel- commented May 31, 2022

Actually I can now reproduce locally.

This is because of the way graphics:::plot.formula() dispatches, by manually retrieving and invoking methods. This is inherently buggy because:

  • It reaches into the search path, which R no longer dispatches to.
  • It ignores registered functions.
  • By bypassing the generics, it doesn't set up the state necessary for NextMethod() to operate correctly (IIUC).

You can reproduce without pkgload in this way:

iris.u <- iris
iris.u[1:4] <- lapply(iris.u[1:4], function(x) units::set_units(x, cm))

# Assign the method on the search path
plot.units <- units:::plot.units

plot(Sepal.Length ~ Sepal.Width, iris.u)

And you can fix the error with pkgload like this:

pkgload::load_all(export_all = FALSE)

@Enchufa2
Copy link
Author

This issue happens when clicking "check" in RStudio, which calls devtools::test, which uses pkgload::load_all at some point. So the question is:

  • Does package check work with export_all = FALSE? I suppose that then all the non-exported functions should be namespaced in the tests.
  • And if so, how can I set that option if I'm not calling it directly?

@lionel-
Copy link
Member

lionel- commented May 31, 2022

I'm not an RStudio user but I believe the Check button runs devtools::check(), not devtools::test(). Only the latter calls load_all(). Is it maybe the "test" button that you've clicked? If test() doesn't work for some reason, then you're probably looking for a devtools feature request to run in a mode that sources files in the tests folder. This would be the most similar to what R CMD check does. I remember we discussed such a feature a few months ago, but I don't think it has been implemented.

oh hmm another path that might explain load_all() being called with check() is through devtools::document(), which calls roxygen2::roxygenise(), which calls load_all() by default. You can configure this behaviour in your description file, see https://www.tidyverse.org/blog/2019/11/roxygen2-7-0-0/#code-loading.

I can't compile your package under RStudio right now (trouble installing libudunits for x86 and RStudio doesn't run with arm64 R yet it seems). But both devtools::check() and devtools::test() seem to succeed outside of RStudio. Maybe the problem occurs in non-fresh sessions after you've interacted with R?
Edit: Though both check() and test() should run in a fresh session if I'm not mistaken, and in that case it's surprising I can't reproduce.

@hadley
Copy link
Member

hadley commented May 31, 2022

This is the "base mode" for test that @lionel- is thinking of: r-lib/devtools#2361

@Enchufa2
Copy link
Author

Enchufa2 commented Jun 1, 2022

I'm not an RStudio user but I believe the Check button runs devtools::check(), not devtools::test().

Yes, I meant "test", not "check".

Maybe the problem occurs in non-fresh sessions after you've interacted with R?

No, I can always reproduce the issue in a fresh R session outside RStudio (as the container workflow above demonstrates).

The main problem of this issue is that I can't add formula-based plots to the package tests, because then those tests fail under devtools::test() and all the vdiff files are removed, which is pretty annoying. The referenced "base mode" would be great, but right now, as a workaround I would prefer to be able to set export_all = FALSE, because anyway I tend to call non-exported functions as pkg:::fun() in the tests, and my understanding is that this is the only drawback.

But maybe this is not the place for this? Or maybe load_all could honor some global option.

lionel- referenced this issue in r-lib/pkgload Jun 1, 2022
@lionel-
Copy link
Member

lionel- commented Jun 1, 2022

as the container workflow above demonstrates

As discussed above I can reproduce your workflow. However it doesn't use devtools::test(), which I'm able to run without failure on your package.

I would prefer to be able to set export_all = FALSE

This would require changes in testthat to an already complex evaluation machinery. So before we think about this I would like to understand precisely how the problem manifests to make sure we aren't missing anything. Could you please provide a reprex with devtools::test() rather than pkgload::load_all(), which is mostly an internal function?

@Enchufa2
Copy link
Author

Enchufa2 commented Jun 1, 2022

As discussed above I can reproduce your workflow. However it doesn't use devtools::test(), which I'm able to run without failure on your package.

devtools::test() runs without issue now because I removed all the tests involving plots with formulae.

Could you please provide a reprex with devtools::test() rather than pkgload::load_all(), which is mostly an internal function?

Sure, that's the easiest thing:

$ podman run --rm -it rocker/r-rspm:20.04 bash
$ apt-get update
$ apt-get install -y git libudunits2-dev
$ git clone https://github.com/r-quantities/units.git && cd units
$ Rscript -e 'install.packages(c("Rcpp", "devtools"))'
$ R

then

> devtools::test()
> iris[1:4] <- lapply(iris[1:4], function(x) units::set_units(x, cm))
> plot(Sepal.Length ~ Sepal.Width, iris)
Error in NextMethod("plot", xlab = xlab, ylab = ylab) : 
  no method to invoke

So, to be clear, devtools::test() succeeds because there are no tests plotting formulae. The fact that the subsequent plot fails shows that I cannot add such tests.

@lionel-
Copy link
Member

lionel- commented Jun 1, 2022

Thank you, I can reproduce by adding a new vdiffr test for the formula plot.

Actually, in this code path (

source = pkgload::load_all(test_dir, helpers = FALSE, quiet = TRUE)
), I think we don't need to pass export_all = TRUE to load_all(). We evaluate in the namespace env which contains all the internal functions anyway. The export_all only concerns the package env, the one attached to the search path, and which we don't use much in tests.

The only downside is that after running devtools::test(), users would have to run load_all() again to get internal functions on the search path. This is already necessary to get testthat helpers on the search path as they are currently not loaded by the load_all() call (this started happening a while ago, probably in #1084, and has affected my workflow since then). Worth noting that this downside does not concern RStudio users since they run tests in a fresh session.

I think it would make sense to pass helpers = TRUE by default and make both export_all and helpers configurable as you suggested.

@lionel- lionel- transferred this issue from r-lib/pkgload Jun 1, 2022
@hadley
Copy link
Member

hadley commented Jun 1, 2022

@lionel- the reason we don't use export_all = FALSE there is to avoid messing with the developer experience; I'd be very hesitant to change that in order to fix a special case that affects relatively few people.

lionel- added a commit that referenced this issue Jun 1, 2022
And load helpers to the search path by default to match previous behaviour.

Closes #1636
@lionel-
Copy link
Member

lionel- commented Jun 1, 2022

@hadley I did not want to change the default for the reason that you mention. However after examining the issue I thought it makes sense to allow export_all = FALSE on a case by case basis.

lionel- added a commit that referenced this issue Jun 2, 2022
And load helpers to the search path by default to match previous behaviour.

Closes #1636
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants