fix: harden kaleido context cleanup

Author: cpsievert

R/kaleido.R

@@ -217,10 +217,10 @@ legacyKaleidoScope <- function(kaleido) {
 }
 
 .py_run_string_with_context <- function(code, context = list(), convert = TRUE) {
-  py <- reticulate::py
   context_names <- names(context)
   old_values <- vector("list", length(context))
   had_value <- logical(length(context))
+  was_set <- logical(length(context))
 
   if (length(context) > 0) {
     if (is.null(context_names) || any(context_names == "")) {
@@ -229,26 +229,28 @@ legacyKaleidoScope <- function(kaleido) {
     if (any(!grepl("^[A-Za-z_][A-Za-z0-9_]*$", context_names))) {
       rlang::abort("`context` names must be valid Python identifiers.")
     }
-    
-    for (i in seq_along(context)) {
-      name <- context_names[[i]]
-      had_value[[i]] <- reticulate::py_has_attr(py, name)
-      if (had_value[[i]]) {
-        old_values[[i]] <- py[[name]]
-      }
-      py[[name]] <- context[[i]]
-    }
-    
+
+    py <- reticulate::py
     on.exit({
-      for (i in rev(seq_along(context))) {
+      for (i in rev(which(was_set))) {
         name <- context_names[[i]]
         if (had_value[[i]]) {
-          py[[name]] <- old_values[[i]]
+          reticulate::py_set_attr(py, name, old_values[[i]])
         } else {
-          reticulate::py_run_string(paste("del", name))
+          reticulate::py_del_attr(py, name)
         }
       }
     }, add = TRUE)
+    
+    for (i in seq_along(context)) {
+      name <- context_names[[i]]
+      had_value[[i]] <- reticulate::py_has_attr(py, name)
+      if (had_value[[i]]) {
+        old_values[[i]] <- reticulate::py_get_attr(py, name)
+      }
+      reticulate::py_set_attr(py, name, context[[i]])
+      was_set[[i]] <- TRUE
+    }
   }
 
   reticulate::py_run_string(code, convert = convert)

tests/testthat/test-kaleido.R

@@ -1,6 +1,14 @@
 test_that("newKaleidoScope does not inline Windows temp paths into Python code", {
   skip_if_not_installed("reticulate")
   skip_if_not(suppressWarnings(reticulate::py_available(TRUE)))
+  withr::defer({
+    py <- reticulate::py
+    for (name in c("fig", "tmp_json_path")) {
+      if (reticulate::py_has_attr(py, name)) {
+        reticulate::py_del_attr(py, name)
+      }
+    }
+  })
 
   win_path <- "C:\\users\\name\\AppData\\Local\\Temp\\Rtmp\\file.json"
   py_calls <- character()
@@ -52,6 +60,8 @@ test_that("newKaleidoScope does not inline Windows temp paths into Python code",
 })
 
 test_that("py_run_string_with_context rejects invalid Python identifiers", {
+  skip_if_not_installed("reticulate")
+
   expect_error(
     plotly:::.py_run_string_with_context(
       "value = 1",
@@ -60,3 +70,38 @@ test_that("py_run_string_with_context rejects invalid Python identifiers", {
     "`context` names must be valid Python identifiers\\."
   )
 })
+
+test_that("py_run_string_with_context cleans up partial assignments", {
+  skip_if_not_installed("reticulate")
+  skip_if_not(suppressWarnings(reticulate::py_available(TRUE)))
+
+  deleted <- character()
+
+  testthat::local_mocked_bindings(
+    py_has_attr = function(x, name) FALSE,
+    py_get_attr = function(x, name, silent = FALSE) stop("unexpected py_get_attr call"),
+    py_set_attr = function(x, name, value) {
+      if (identical(name, "second")) {
+        stop("boom")
+      }
+      invisible(NULL)
+    },
+    py_del_attr = function(x, name) {
+      deleted <<- c(deleted, name)
+      invisible(NULL)
+    },
+    py_run_string = function(code, local = FALSE, convert = TRUE) {
+      stop("unexpected py_run_string call")
+    },
+    .package = "reticulate"
+  )
+
+  expect_error(
+    plotly:::.py_run_string_with_context(
+      "value = 1",
+      context = list(first = 1, second = 2)
+    ),
+    "boom"
+  )
+  expect_identical(deleted, "first")
+})