From c5bc13937c1fc821b019d3ca5b303a52e2c0f678 Mon Sep 17 00:00:00 2001
From: Konrad Rudolph <konrad.rudolph@roche.com>
Date: Mon, 11 Sep 2023 15:13:36 +0200
Subject: [PATCH 1/2] Make unloading work with active bindings

Active bindings in package namespaces should not be evaluated when
forcing the bindings during the unloading step, because their
side-effects may be undesirable.
---
 NEWS.md                                        | 3 +++
 R/namespace-env.R                              | 8 +++++++-
 tests/testthat/test-load.R                     | 6 ++++++
 tests/testthat/testActiveBindings/DESCRIPTION  | 6 ++++++
 tests/testthat/testActiveBindings/NAMESPACE    | 3 +++
 tests/testthat/testActiveBindings/R/bindings.r | 5 +++++
 6 files changed, 30 insertions(+), 1 deletion(-)
 create mode 100644 tests/testthat/testActiveBindings/DESCRIPTION
 create mode 100644 tests/testthat/testActiveBindings/NAMESPACE
 create mode 100644 tests/testthat/testActiveBindings/R/bindings.r

diff --git a/NEWS.md b/NEWS.md
index 6a5ba220..a0c9fb3d 100644
--- a/NEWS.md
+++ b/NEWS.md
@@ -1,5 +1,8 @@
 # pkgload (development version)
 
+* Fix handling of active bindings inside a package during unloading (#255, @klmr).
+
+
 # pkgload 1.3.3
 
 * pkgload now depends unconditionally on pkgbuild (#249).
diff --git a/R/namespace-env.R b/R/namespace-env.R
index 0cec5da0..c4d5fbc2 100644
--- a/R/namespace-env.R
+++ b/R/namespace-env.R
@@ -234,7 +234,13 @@ unregister_namespace <- function(name = NULL) {
   # unloaded, it might lead to decompress errors if unloaded or to
   # inconsistencies if reloaded (the bindings are resolved in the new
   # namespace).
-  eapply(ns_env(name), force, all.names = TRUE)
+  ns <- ns_env(name)
+  for (binding in ls(ns, all.names = TRUE)) {
+    # Do not evaluate active bindings, since these might have side-effects.
+    if (! bindingIsActive(binding, ns)) {
+      get(binding, ns, inherits = FALSE)
+    }
+  }
 
   # Remove the item from the registry
   env_unbind(ns_registry_env(), name)
diff --git a/tests/testthat/test-load.R b/tests/testthat/test-load.R
index b0e657db..eecbc84d 100644
--- a/tests/testthat/test-load.R
+++ b/tests/testthat/test-load.R
@@ -76,6 +76,12 @@ test_that("unloading or reloading forces bindings", {
   )
 })
 
+test_that("unloading or reloading does not call active bindings", {
+  on.exit(unload("testActiveBindings"))
+
+  expect_no_error(load_all(test_path("testActiveBindings")))
+})
+
 test_that("reloading a package unloads deleted S3 methods", {
   x <- structure(list(), class = "pkgload_foobar")
 
diff --git a/tests/testthat/testActiveBindings/DESCRIPTION b/tests/testthat/testActiveBindings/DESCRIPTION
new file mode 100644
index 00000000..4e064a97
--- /dev/null
+++ b/tests/testthat/testActiveBindings/DESCRIPTION
@@ -0,0 +1,6 @@
+Package: testActiveBindings
+Title: Test package with active bindings
+License: MIT
+Author: Konrad Rudolph <konrad.rudolph@gmail.com>
+Maintainer: Konrad Rudolph <konrad.rudolph@gmail.com>
+Version: 1.0
diff --git a/tests/testthat/testActiveBindings/NAMESPACE b/tests/testthat/testActiveBindings/NAMESPACE
new file mode 100644
index 00000000..449a7e53
--- /dev/null
+++ b/tests/testthat/testActiveBindings/NAMESPACE
@@ -0,0 +1,3 @@
+# Generated by roxygen2: do not edit by hand
+
+export(bar)
diff --git a/tests/testthat/testActiveBindings/R/bindings.r b/tests/testthat/testActiveBindings/R/bindings.r
new file mode 100644
index 00000000..d9eedeab
--- /dev/null
+++ b/tests/testthat/testActiveBindings/R/bindings.r
@@ -0,0 +1,5 @@
+
+makeActiveBinding("foo", function() rlang::abort("foo"), environment())
+
+#' @export
+makeActiveBinding("bar", function() rlang::abort("bar"), environment())

From 07c138a4591366e34329c6c32191de97308c5bc2 Mon Sep 17 00:00:00 2001
From: Lionel Henry <lionel.hry@gmail.com>
Date: Tue, 19 Sep 2023 10:05:38 +0200
Subject: [PATCH 2/2] Replace loop by rlang tools

---
 R/namespace-env.R | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/R/namespace-env.R b/R/namespace-env.R
index c4d5fbc2..1636a5ba 100644
--- a/R/namespace-env.R
+++ b/R/namespace-env.R
@@ -234,13 +234,12 @@ unregister_namespace <- function(name = NULL) {
   # unloaded, it might lead to decompress errors if unloaded or to
   # inconsistencies if reloaded (the bindings are resolved in the new
   # namespace).
+  #
+  # We take precautions not to trigger active bindings in case these
+  # have side effects such as throwing an error.
   ns <- ns_env(name)
-  for (binding in ls(ns, all.names = TRUE)) {
-    # Do not evaluate active bindings, since these might have side-effects.
-    if (! bindingIsActive(binding, ns)) {
-      get(binding, ns, inherits = FALSE)
-    }
-  }
+  active_bindings <- env_binding_are_active(ns)
+  env_get_list(ns, names(active_bindings)[!active_bindings])
 
   # Remove the item from the registry
   env_unbind(ns_registry_env(), name)