Merge pull request #286 from r-lib/feature/env-unlock

Remove depency on `env_unlock()`

DESCRIPTION

@@ -23,6 +23,7 @@ Imports:
     desc,
     fs,
     glue,
+    lifecycle,
     methods,
     pkgbuild,
     processx,

NEWS.md

@@ -1,5 +1,10 @@
 # pkgload (development version)
 
+* The `reset` argment of `load_all()` is no longer supported because preserving
+  the namespace requires unlocking its environment, which is no longer possible
+  in recent versions of R. It should no longer be necessary as the performance
+  issues caused by resetting the namespace were resolved a while ago.
+
 * New experimental feature for generating a `compile_commands.json` file after
   each `load_all()`. This file is used by LSP servers such as clangd to provide
   intellisense features in your native files. To enable it, add this directive

R/load.R

@@ -65,11 +65,9 @@
 #'    be useful for checking for missing exports.
 #'
 #' @param path Path to a package, or within a package.
-#' @param reset clear package environment and reset file cache before loading
-#'   any pieces of the package. This largely equivalent to running
-#'   [unload()], however the old namespaces are not completely removed and no
-#'   `.onUnload()` hooks are called. Use `reset = FALSE` may be faster for
-#'   large code bases, but is a significantly less accurate approximation.
+#' @param reset `r lifecycle::badge("deprecated")` This is no longer supported
+#'   because preserving the namespace requires unlocking its environment, which
+#'   is no longer possible in recent versions of R.
 #' @param compile If `TRUE` always recompiles the package; if `NA`
 #'   recompiles if needed (as determined by [pkgbuild::needs_compile()]);
 #'   if `FALSE`, never recompiles.
@@ -106,9 +104,6 @@
 #' # Running again loads changed files
 #' load_all("./")
 #'
-#' # With reset=TRUE, unload and reload the package for a clean start
-#' load_all("./", TRUE)
-#'
 #' # With export_all=FALSE, only objects listed as exports in NAMESPACE
 #' # are exported
 #' load_all("./", export_all = FALSE)
@@ -125,6 +120,13 @@ load_all <- function(path = ".",
                      quiet = NULL,
                      recompile = FALSE,
                      warn_conflicts = TRUE) {
+  if (!isTRUE(reset)) {
+    lifecycle::deprecate_warn(
+      when = "1.3.5", 
+      what = "load_all(reset)",
+      details = "`reset = FALSE` is no longer supported."
+    )
+  }
 
   path <- pkg_path(path)
   package <- pkg_name(path)
@@ -163,31 +165,24 @@ load_all <- function(path = ".",
   }
 
   old_methods <- list()
+  clear_cache()
 
-  if (reset) {
-    clear_cache()
-
-    # Remove package from known namespaces. We don't unload it to allow
-    # safe usage of dangling references.
-    if (is_loaded(package)) {
-      patch_colon(package)
+  # Remove package from known namespaces. We don't unload it to allow
+  # safe usage of dangling references.
+  if (is_loaded(package)) {
+    patch_colon(package)
 
-      methods_env <- ns_s3_methods(package)
-      unregister(package)
+    methods_env <- ns_s3_methods(package)
+    unregister(package)
 
-      # Save foreign methods after unregistering the package's own
-      # methods. We'll restore the foreign methods but let the package
-      # register its own methods again.
-      old_methods <- as.list(methods_env)
-      old_methods <- Filter(function(x) is_foreign_method(x, package), old_methods)
-    }
+    # Save foreign methods after unregistering the package's own
+    # methods. We'll restore the foreign methods but let the package
+    # register its own methods again.
+    old_methods <- as.list(methods_env)
+    old_methods <- Filter(function(x) is_foreign_method(x, package), old_methods)
   }
 
-  if (is_loaded(package)) {
-    rlang::env_unlock(ns_env(package))
-  } else {
-    create_ns_env(path)
-  }
+  create_ns_env(path)
 
   out <- list(env = ns_env(package))
 
@@ -388,11 +383,24 @@ is_loading <- function(pkg = NULL) {
   }
 }
 
-# Ensure that calls to `::` resolve to the original unregistered
-# namespace
+# Ensure that calls to `::` resolve to the original detached namespace. It is
+# uncommon to refer to functions in your own package with `::` but it sometimes
+# is necessary, e.g. in standalone files.
+#
+# To enable this, assign `::` in your namespace, e.g.
+#
+# ```
+# on_load(`::` <- base::`::`)
+# ```
 patch_colon <- function(package) {
   ns <- asNamespace(package)
-  rlang::env_unlock(ns)
+
+  if (!env_has(ns, "::")) {
+    return()
+  }
+
+  rlang::env_binding_unlock(ns, "::")
+  on.exit(rlang::env_binding_lock(ns, "::"))
 
   ns[["::"]] <- function(lhs, rhs) {
     lhs <- as.character(substitute(lhs))
@@ -407,6 +415,4 @@ patch_colon <- function(package) {
       eval(bquote(base::`::`(.(lhs), .(rhs))), baseenv())
     }
   }
-
-  lockEnvironment(ns)
 }

revdep/failures.md

@@ -5,7 +5,7 @@
 * Version: NA
 * GitHub: NA
 * Source code: https://github.com/cran/zellkonverter
-* Number of recursive dependencies: 157
+* Number of recursive dependencies: 153
 
 Run `revdepcheck::cloud_details(, "zellkonverter")` for more info
 

tests/testthat/test-load-hooks.R

@@ -25,10 +25,6 @@ test_that("hooks called in correct order", {
     c("pkg_load", "user_load", "pkg_attach", "user_attach")
   )
 
-  reset_events()
-  load_all("testHooks", reset = FALSE)
-  expect_equal(globalenv()$hooks$events, character())
-
   reset_events()
   unload("testHooks")
   expect_equal(globalenv()$hooks$events,
@@ -80,13 +76,6 @@ test_that("onLoad and onAttach", {
   expect_equal(the$b, 2)
   expect_equal(the$c, 1)
 
-  # ===================================================================
-  # Loading again without reset won't change a, b, and c in the
-  # namespace env, and also shouldn't trigger onload or onattach. But
-  # the existing namespace values will be copied over to the package
-  # environment
-  load_all("testLoadHooks", reset = FALSE)
-
   # Shouldn't form new environments
   expect_identical(nsenv, ns_env("testLoadHooks"))
   expect_identical(pkgenv, pkg_env("testLoadHooks"))
@@ -96,10 +85,10 @@ test_that("onLoad and onAttach", {
   expect_equal(the$c, 1)
 
   # ===================================================================
-  # With reset=TRUE, there should be new package and namespace
+  # When loading again there should be new package and namespace
   # environments, and the values should be the same as the first
   # load_all.
-  load_all("testLoadHooks", reset = TRUE)
+  load_all("testLoadHooks")
   nsenv2 <- ns_env("testLoadHooks")
   pkgenv2 <- pkg_env("testLoadHooks")
 

tests/testthat/test-s4-unload.R

@@ -67,7 +67,7 @@ test_that("loading and reloading s4 classes", {
   })
 
   # Loading again shouldn't result in any errors or warnings
-  expect_no_warning(load_all("testS4union", reset = FALSE) )
+  expect_no_warning(load_all("testS4union") )
 
   unload("testS4union")
   unloadNamespace("stats4")   # This was imported by testS4union