Special case imports code to run first (#1538)

This effectively makes the preprocess method irrelevant, but there's no much point in removing it since there are so few extension authors.

Fixes #1144

NAMESPACE

@@ -88,7 +88,6 @@ S3method(roclet_output,roclet_namespace)
 S3method(roclet_output,roclet_rd)
 S3method(roclet_output,roclet_vignette)
 S3method(roclet_preprocess,default)
-S3method(roclet_preprocess,roclet_namespace)
 S3method(roclet_process,roclet_namespace)
 S3method(roclet_process,roclet_rd)
 S3method(roclet_process,roclet_vignette)

NEWS.md

@@ -1,5 +1,12 @@
 # roxygen2 (development version)
 
+* The `NAMESPACE` roclet once again regenerates imports _before_ loading 
+  package code and parsing roxygen blocks. This has been the goal for a long 
+  time (#372), but we accidentally broke it when adding support for code 
+  execution in markdown blocks. This resolves a family of problems where you
+  somehow bork your `NAMESPACE` and can't easily get out of it because you
+  can't re-document the package because your code doesn't reload.
+
 * `escape_examples()` is now exported (#1450).
 
 * `@docType package` now works more like documenting `"_PACKAGE"`, 

R/namespace.R

@@ -30,22 +30,6 @@ namespace_roclet <- function() {
   roclet("namespace")
 }
 
-#' @export
-roclet_preprocess.roclet_namespace <- function(x, blocks, base_path) {
-  lines <- blocks_to_ns(blocks, emptyenv(), import_only = TRUE)
-  NAMESPACE <- file.path(base_path, "NAMESPACE")
-
-  if (length(lines) == 0 && !made_by_roxygen(NAMESPACE)) {
-    return(x)
-  }
-
-  lines <- c(lines, namespace_exports(NAMESPACE))
-  results <- c(made_by("#"), sort_c(unique(lines)))
-  write_if_different(NAMESPACE, results, check = TRUE)
-
-  invisible(x)
-}
-
 #' @export
 roclet_process.roclet_namespace <- function(x, blocks, env, base_path) {
   blocks_to_ns(blocks, env)
@@ -71,27 +55,87 @@ roclet_clean.roclet_namespace <- function(x, base_path) {
   }
 }
 
+# NAMESPACE updates -------------------------------------------------------
+
+import_directives <- c(
+  "import",
+  "importFrom",
+  "importClassesFrom",
+  "importMethodsFrom",
+  "useDynLib"
+)
+
+update_namespace_imports <- function(base_path) {
+  NAMESPACE <- file.path(base_path, "NAMESPACE")
+  if (!made_by_roxygen(NAMESPACE) || !file.exists(NAMESPACE)) {
+    return(invisible())
+  }
+
+  lines <- c(namespace_imports(base_path), namespace_exports(NAMESPACE))
+  results <- c(made_by("#"), sort_c(unique(lines)))
+  write_if_different(NAMESPACE, results, check = TRUE)
+
+  invisible()
+}
+
+# Here we hand roll parsing and tokenisation from roxygen2 primitives so
+# we can filter tags that we know don't require package code.
+namespace_imports <- function(base_path = ".") {
+  paths <- package_files(base_path)
+  parsed <- lapply(paths, parse, keep.source = TRUE)
+  srcrefs <- lapply(parsed, utils::getSrcref)
+  blocks <- unlist(lapply(srcrefs, namespace_imports_blocks), recursive = FALSE)
+
+  blocks_to_ns(blocks, emptyenv())
+}
+
+namespace_imports_blocks <- function(srcref) {
+  comment_refs <- comments(srcref)
+  tokens <- lapply(comment_refs, tokenise_ref)
+
+  import_tags <- c(import_directives, "rawNamespace")
+  tokens_filtered <- lapply(tokens, function(tokens) {
+    tokens[map_lgl(tokens, function(x) x$tag %in% import_tags)]
+  })
+
+  compact(lapply(tokens_filtered, function(tokens) {
+    block_create(
+      call = NULL,
+      srcref = srcref(srcfile("NAMESPACE"), rep(1, 4)),
+      tokens = tokens
+    )
+  }))
+}
+
+namespace_exports <- function(path) {
+  parsed <- as.list(parse(path, keep.source = TRUE))
+
+  is_import_directive <- function(x) is_call(x, import_directives)
+  export_lines <- attr(parsed, "srcref")[!map_lgl(parsed, is_import_directive)]
+  unlist(lapply(export_lines, as.character))
+}
+
 # NAMESPACE generation ----------------------------------------------------
 
-blocks_to_ns <- function(blocks, env, import_only = FALSE) {
-  lines <- map(blocks, block_to_ns, env = env, import_only = import_only)
+blocks_to_ns <- function(blocks, env) {
+  lines <- map(blocks, block_to_ns, env = env)
   lines <- unlist(lines) %||% character()
 
   sort_c(unique(lines))
 }
 
-block_to_ns <- function(block, env, import_only = FALSE) {
-  map(block$tags, roxy_tag_ns, block = block, env = env, import_only = import_only)
+block_to_ns <- function(block, env) {
+  map(block$tags, roxy_tag_ns, block = block, env = env)
 }
 
 # Namespace tag methods ---------------------------------------------------
 
-roxy_tag_ns <- function(x, block, env, import_only = FALSE) {
+roxy_tag_ns <- function(x, block, env) {
   UseMethod("roxy_tag_ns")
 }
 
 #' @export
-roxy_tag_ns.default <- function(x, block, env, import_only = FALSE) {
+roxy_tag_ns.default <- function(x, block, env) {
 
 }
 
@@ -100,11 +144,7 @@ roxy_tag_parse.roxy_tag_evalNamespace <- function(x) {
   tag_code(x)
 }
 #' @export
-roxy_tag_ns.roxy_tag_evalNamespace <- function(x, block, env, import_only = FALSE) {
-  if (import_only) {
-    return()
-  }
-
+roxy_tag_ns.roxy_tag_evalNamespace <- function(x, block, env) {
   roxy_tag_eval(x, env)
 }
 
@@ -113,11 +153,7 @@ roxy_tag_parse.roxy_tag_export <- function(x) {
   tag_words_line(x)
 }
 #' @export
-roxy_tag_ns.roxy_tag_export <- function(x, block, env, import_only = FALSE) {
-  if (import_only) {
-    return()
-  }
-
+roxy_tag_ns.roxy_tag_export <- function(x, block, env) {
   if (identical(x$val, "")) {
     # FIXME: check for empty exports (i.e. no name)
     default_export(block$object, block)
@@ -131,11 +167,7 @@ roxy_tag_parse.roxy_tag_exportClass <- function(x) {
   tag_words(x, 1)
 }
 #' @export
-roxy_tag_ns.roxy_tag_exportClass <- function(x, block, env, import_only = FALSE) {
-  if (import_only) {
-    return()
-  }
-
+roxy_tag_ns.roxy_tag_exportClass <- function(x, block, env) {
   export_class(x$val)
 }
 
@@ -144,10 +176,7 @@ roxy_tag_parse.roxy_tag_exportMethod <- function(x) {
   tag_words(x, min = 1)
 }
 #' @export
-roxy_tag_ns.roxy_tag_exportMethod <- function(x, block, env, import_only = FALSE) {
-  if (import_only) {
-    return()
-  }
+roxy_tag_ns.roxy_tag_exportMethod <- function(x, block, env) {
   export_s4_method(x$val)
 }
 
@@ -156,10 +185,7 @@ roxy_tag_parse.roxy_tag_exportPattern <- function(x) {
   tag_words(x, min = 1)
 }
 #' @export
-roxy_tag_ns.roxy_tag_exportPattern <- function(x, block, env, import_only = FALSE) {
-  if (import_only) {
-    return()
-  }
+roxy_tag_ns.roxy_tag_exportPattern <- function(x, block, env) {
   one_per_line("exportPattern", x$val)
 }
 
@@ -168,11 +194,7 @@ roxy_tag_parse.roxy_tag_exportS3Method <- function(x) {
   tag_words(x, min = 0, max = 2)
 }
 #' @export
-roxy_tag_ns.roxy_tag_exportS3Method <- function(x, block, env, import_only = FALSE) {
-  if (import_only) {
-    return()
-  }
-
+roxy_tag_ns.roxy_tag_exportS3Method <- function(x, block, env) {
   obj <- block$object
 
   if (identical(x$val, "")) {
@@ -219,7 +241,7 @@ roxy_tag_parse.roxy_tag_import <- function(x) {
   tag_words(x, min = 1)
 }
 #' @export
-roxy_tag_ns.roxy_tag_import <- function(x, block, env, import_only = FALSE) {
+roxy_tag_ns.roxy_tag_import <- function(x, block, env) {
   one_per_line_ignore_current("import", x$val)
 }
 
@@ -228,7 +250,7 @@ roxy_tag_parse.roxy_tag_importClassesFrom <- function(x) {
   tag_words(x, min = 2)
 }
 #' @export
-roxy_tag_ns.roxy_tag_importClassesFrom <- function(x, block, env, import_only = FALSE) {
+roxy_tag_ns.roxy_tag_importClassesFrom <- function(x, block, env) {
   repeat_first_ignore_current("importClassesFrom", x$val)
 }
 
@@ -237,7 +259,7 @@ roxy_tag_parse.roxy_tag_importFrom <- function(x) {
   tag_words(x, min = 2)
 }
 #' @export
-roxy_tag_ns.roxy_tag_importFrom <- function(x, block, env, import_only = FALSE) {
+roxy_tag_ns.roxy_tag_importFrom <- function(x, block, env) {
   pkg <- x$val[1L]
   if (requireNamespace(pkg, quietly = TRUE)) {
     importing <- x$val[-1L]
@@ -255,7 +277,7 @@ roxy_tag_parse.roxy_tag_importMethodsFrom <- function(x) {
   tag_words(x, min = 2)
 }
 #' @export
-roxy_tag_ns.roxy_tag_importMethodsFrom <- function(x, block, env, import_only = FALSE) {
+roxy_tag_ns.roxy_tag_importMethodsFrom <- function(x, block, env) {
   repeat_first_ignore_current("importMethodsFrom", x$val)
 }
 
@@ -264,7 +286,7 @@ roxy_tag_parse.roxy_tag_rawNamespace <- function(x) {
   tag_code(x)
 }
 #' @export
-roxy_tag_ns.roxy_tag_rawNamespace  <- function(x, block, env, import_only = FALSE) {
+roxy_tag_ns.roxy_tag_rawNamespace  <- function(x, block, env) {
   x$raw
 }
 
@@ -273,7 +295,7 @@ roxy_tag_parse.roxy_tag_useDynLib <- function(x) {
   tag_words(x, min = 1)
 }
 #' @export
-roxy_tag_ns.roxy_tag_useDynLib <- function(x, block, env, import_only = FALSE) {
+roxy_tag_ns.roxy_tag_useDynLib <- function(x, block, env) {
   if (length(x$val) == 1) {
     return(paste0("useDynLib(", auto_quote(x$val), ")"))
   }
@@ -354,14 +376,3 @@ repeat_first_ignore_current <- function(name, x) {
     repeat_first(name, x)
   }
 }
-
-namespace_exports <- function(path) {
-  if (!file.exists(path)) {
-    return(character())
-  }
-
-  parsed <- as.list(parse(path, keep.source = TRUE))
-  is_import <- function(x) is_call(x, c("import", "importFrom", "importClassesFrom", "importMethodsFrom", "useDynLib"))
-  export_lines <- attr(parsed, "srcref")[!map_lgl(parsed, is_import)]
-  unlist(lapply(export_lines, as.character))
-}

R/roxygenize.R

@@ -38,12 +38,15 @@ roxygenize <- function(package.dir = ".",
   lapply(packages, loadNamespace)
 
   roclets <- roclets %||% roxy_meta_get("roclets")
-  # Special case collate: it doesn't need to execute code, and must be run
-  # first to ensure that code can be executed
+
+  # To load code, we need a up-to-date Collate field and NAMESPACE
   if ("collate" %in% roclets) {
     update_collate(base_path)
     roclets <- setdiff(roclets, "collate")
   }
+  if ("namespace" %in% roclets) {
+    update_namespace_imports(base_path)
+  }
 
   if (length(roclets) == 0)
     return(invisible())

tests/testthat/_snaps/namespace.md

@@ -35,6 +35,13 @@
     Message
       x <text>:2: @importFrom must have at least 2 words, not 1.
 
+# can regenerate NAMESPACE even if its broken
+
+    Code
+      update_namespace_imports(path)
+    Message
+      Writing 'NAMESPACE'
+
 # rawNamespace must be valid code
 
     Code

tests/testthat/_snaps/roxygenize.md

@@ -0,0 +1,9 @@
+# can regenerate NAMESPACE even if its broken
+
+    Code
+      roxygenise(path)
+    Message
+      Writing 'NAMESPACE'
+      i Loading brokenNamespace
+      Writing 'NAMESPACE'
+

tests/testthat/broken-namespace/DESCRIPTION

@@ -0,0 +1,3 @@
+Package: brokenNamespace
+Version: 1.0.0
+Encoding: UTF-8

tests/testthat/broken-namespace/NAMESPACE

@@ -0,0 +1,3 @@
+# Generated by roxygen2: do not edit by hand
+
+importFrom(stats,foo)

tests/testthat/broken-namespace/R/x.R

@@ -0,0 +1,5 @@
+#' @importFrom stats median
+NULL
+
+#' @export
+f <- function() {}

tests/testthat/test-namespace.R

@@ -271,10 +271,22 @@ test_that("empty NAMESPACE generates zero-length vector", {
   expect_equal(results, character())
 })
 
+test_that("can regenerate NAMESPACE even if its broken", {
+  path <- local_package_copy(test_path("broken-namespace"))
+  expect_snapshot(update_namespace_imports(path))
+
+  expect_equal(
+    read_lines(file.path(path, "NAMESPACE")),
+    c(
+      "# Generated by roxygen2: do not edit by hand",
+      "",
+      "importFrom(stats,median)"
+    )
+  )
+})
 
 # Raw ---------------------------------------------------------------------
 
-
 test_that("rawNamespace must be valid code", {
   block <- "
     #' @rawNamespace a +

tests/testthat/test-roxygenize.R

@@ -0,0 +1,4 @@
+test_that("can regenerate NAMESPACE even if its broken", {
+  path <- local_package_copy(test_path("broken-namespace"))
+  expect_snapshot(roxygenise(path))
+})