devtools::document() errors before NAMESPACE can be updated

[daynefiler]

When running devtools::document() using roxygen2 to manage the namespace, the function errors before the NAMESPACE file can be updated/written.

I have run into this in two situations:

(1) adding a new import; e.g. trying to add the S4Vectors::setValidity2 function using #' @importFrom S4Vectors setvalidity2:

devtools::document()
# Updating badInstall documentation
# Loading badInstall
# Error in setValidity2(Class = "TestClass", method = .TestClass.validity) 
# (from methods-TestClass.R#12) : 
#   could not find function "setValidity2"

(2) renaming a generic method:

devtools::document()
# Updating badInstall documentation
# Loading badInstall
# Error in add_classes_to_exports(ns = nsenv, package = package, exports = exports,  : 
#   in ‘badInstall’ methods for export not found: myOldGeneric
#   In addition: Warning message:
#   In setup_ns_exports(path, export_all, export_imports) :
#     Objects listed as exports, but not present in namespace: myOldGeneric

I am using RStudio: v1.3.959

sessionInfo()
# R version 4.0.0 (2020-04-24)
# Platform: x86_64-apple-darwin17.0 (64-bit)
# Running under: macOS Catalina 10.15.4
# 
# Matrix products: default
# BLAS:   /System/Library/Frameworks/Accelerate.framework/Versions/A/Frameworks/vecLib.framework/Versions/A/libBLAS.dylib
# LAPACK: /Library/Frameworks/R.framework/Versions/4.0/Resources/lib/libRlapack.dylib
# 
# locale:
# [1] en_US.UTF-8/en_US.UTF-8/en_US.UTF-8/C/en_US.UTF-8/en_US.UTF-8
# 
# attached base packages:
# [1] stats     graphics  grDevices utils     datasets  methods   base     
# 
# loaded via a namespace (and not attached):
#  [1] Rcpp_1.0.4.6        compiler_4.0.0      prettyunits_1.1.1  
#  [4] remotes_2.1.1       tools_4.0.0         testthat_2.3.2     
#  [7] digest_0.6.25       pkgbuild_1.0.8      pkgload_1.0.2      
# [10] memoise_1.1.0       rlang_0.4.6         cli_2.0.2          
# [13] rstudioapi_0.11     parallel_4.0.0      xfun_0.14          
# [16] withr_2.2.0         stringr_1.4.0       roxygen2_7.1.0     
# [19] knitr_1.28          xml2_1.3.2          desc_1.2.0         
# [22] S4Vectors_0.26.1    fs_1.4.1            devtools_2.3.0     
# [25] stats4_4.0.0        rprojroot_1.3-2     glue_1.4.1         
# [28] R6_2.4.1            processx_3.4.2      fansi_0.4.1        
# [31] sessioninfo_1.1.1   callr_3.4.3         purrr_0.3.4        
# [34] magrittr_1.5        backports_1.1.7     ps_1.3.3           
# [37] ellipsis_0.3.1      BiocGenerics_0.34.0 usethis_1.6.1      
# [40] assertthat_0.2.1    stringi_1.4.6       crayon_1.3.4  

[daynefiler]

If it's helpful, I threw up this repository: https://github.com/daynefiler/badInstall.

[daynefiler]

@jimhester any insight here?

[hadley]

This is a roxygen2 issue so I've moved it there.

[adolgert]

A workaround is to delete the NAMESPACE file. It will be rebuilt without an error.

[hadley]

Yeah, we don't have a good automated solution for this — as @adolgert suggests, your best bet is just to delete the NAMESPACE and start from scratch. I'll leave this open since there might be something we can do in pkgload, I just need to think about it a bit more.

[hadley]

It looks like this might be a regression introduced by https://github.com/r-lib/roxygen2/commit/97c6c2d7eede09aa09bd72860304a483d7269005 — we really need to do the namespace ore-processing before any code is run, but now we need to load the package in order to evaluate inline code.

[MichaelChirico]

Related issue (please LMK if this is more appropriate as an independent issue), continued from https://github.com/r-lib/pkgload/issues/239.

I would expect roxygenize() to work even in the absence of a NAMESPACE file as long as all the @import/@importFrom entries are already in roxygen2 mark-up. Is that not possible due to some circular dependency issue?

[hadley]

Yeah, the problem is that roxygen2 is a mostly dynamic doc system, so it executes the code as a part of the tag generation. That means you need imports defined, which gets us into this circular dependency problem.

It's possible to break the cycle in the same way as we did for collate (see update_collate()), but it requires quite a bit of work, since we need to handle @import and @importFrom manually (and presumably a few others like @useDynLib).

[MichaelChirico]

Maybe there could be a new roclet initiate_namespace? That only handles the @import/@importFrom part of the current namespace roclet? Not sure if that actually gets around the problem in any meaningful way.

[hadley]

Yeah, the problem is that it can't be a roclet, because all the roclet machinery assumes that you have not just the sources, but the package namespace.

[MichaelChirico]

FWIW, I wrote a janky script to initialize the NAMESPACE before load_all() is attempted. Sharing in case it's useful to anyone else in this thread:

r_files <- list.files(
  file.path(package_dir, "R"),
  # exclude e.g. sysdata.rda
  pattern = "\\.[rR]$",
  full.names = TRUE
)
namespace_imports <- unlist(lapply(r_files, function(r_file) {
  # NB: roxygen2::parse_package() will attempt to compile src code
  file_roxy <- roxygen2:::tokenize_file(r_file)
  unlist(lapply(file_roxy, function(roxy) {
    unlist(lapply(roxy$tag, function(entry) {
      # For entries '@import pkg', NAMESPACE entry is import(pkg)
      # For entries '@importFrom pkg ...', it's importFrom(pkg, ...)
      #   ditto for @importClassesFrom and @importMethodsFrom.
      switch(entry$tag,
        import = ,
        importFrom = ,
        importClassesFrom = ,
        importMethodsFrom = sprintf(
          "%s(%s)",
          entry$tag, toString(roxygen2:::auto_quote(entry$val))
        )
      )
    }))
  }))
}))
if (!is.null(namespace_imports)) {
  # must fake the roxygen2 header in order for the next pass to
  #   process @exports correctly.
  # NB: unique() doesn't necessarily eliminate all duplicate exports here.
  writeLines(
    c(roxygen2:::made_by("#"), "", sort(unique(namespace_imports))),
    file.path(package_dir, "NAMESPACE")
  )
}

[MichaelChirico]

Also FWIW, I think the current documentation is a bit ?misleading?

The NAMESPACE is generated in two passes: the first generates only import directives (because this can be computed without evaluating package code), and the second generates everything (after the package has been loaded).

That describes exactly what we wanted out of this roclet. But now it seems it's not possible to fully rely on that behavior because load_all() is attempted before the roclet. Does that paragraph need some fine-tuning/caveats?

[hadley]

Oh hmmm, maybe I forgot that I already implemented this, and then broke it with another change?

[MichaelChirico]

For us it is the pkgload update that breaks our flow, rather than anything in roxygen2 per se.

We also found it's necessary in some cases working with S4 methods (haven't nailed down an MRE on what exactly triggers it).

[hadley]

Ok yeah, the problem is that the package is being loaded before the roclet_preprocess.roclet_namespace() is called. But it looks like it's not going to be trivial simply load the package later because the markdown parsing (in particular ```R support) requires the package to be loaded.

Fixing this is going to require some work. I think we could either try to put off the markdown parsing until after block_set_env or we could rewrite roclet_preprocess.roclet_namespace() to work standalone like update_collate().

[MichaelChirico]

FWIW the current code works in the vast majority of cases to initialize the NAMESPACE from scratch.

Of ~500 packages we only see 4 packages and two classifications of error:

• S4 methods table issues [applies to pkgload 1.2.4+]
• ~sysdata.Rda data sets [applies to pkgload 1.3.2+]~

We patched the above snippet as an exported function and that fixed the S4 methods case, leaving only 1 package where the sysdata.Rda bug persists (~I'm not sure how to fix that yet~ See my next comment below).

So, not sure it's worth fixing TBH, as long as workarounds are documented, and perhaps we could export the init_namespace() helper?

[hadley]

@MichaelChirico it bugs me enough that I'd prefer to fix it properly 😄

[MichaelChirico]

(I'm not sure how to fix that yet).

One small update, I finally figured out what's up with the sysdata.Rda case. TL;DR is the package was bad and the new behavior to error is good. I also am not sure there's anything roxygen2 can do to easily improve the error messaging here.

But basically, sysdata.Rda objects are hidden -- not exported! (https://cran.r-project.org/doc/manuals/R-exts.html#Package-subdirectories) So documenting them doesn't make sense. However we had a package where the author'd tried to document a hidden data set by mistake.

Here's an MRE*:

# CREATE MINIMAL PACKAGE
setwd(tempdir())
dir.create("rdapkg")
setwd("rdapkg")
dir.create("R")

c(
  Package = "rdapkg",
  Description = "rdapkg.",
  Title = "rdapkg",
  Version = "0.0.1",
  RoxygenNote = "7.2.3",
  Encoding = "UTF-8"
) |>
  write.dcf("DESCRIPTION")

hidden = mtcars
save(hidden, file = "R/sysdata.Rda")

c(
  "foo <- function() 1 + 1",
  "",
  "#' A hidden data set",
  '"hidden"'
) |>
  writeLines("R/foo.R")

## NOW TRY TO roxygenize()!
roxygen2::roxygenize()
# ℹ Loading rdapkg
# Writing NAMESPACE
# Error: 'hidden' is not an exported object from 'namespace:rdapkg'

* Haven't tested whether it's properly minimal in that it used to work on old {pkgload}