From 7b2b8be00486edbf9b721459a59c3d1cbf6dadd3 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Mon, 8 Jul 2024 10:17:23 +0200 Subject: [PATCH 1/5] Ensure on.exit() runs at end of chunk Fixes #201 --- NEWS.md | 1 + R/conditions.R | 2 +- R/evaluate.R | 65 +++++++++++++++++++------------ tests/testthat/_snaps/evaluate.md | 15 +++++++ tests/testthat/test-evaluate.R | 8 ++++ 5 files changed, 65 insertions(+), 26 deletions(-) diff --git a/NEWS.md b/NEWS.md index 2a52938d..71d992a3 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,6 @@ # evaluate (development version) +* `on.exit()` will now run at the end of the evaluate code, rather than immediately. This makes the scope of knitr chunks resemble the scope of R functions (#201). * `parse_all()` adds a `\n` to the end of every line, even the last one if it didn't have one in the input. * Setting `ACTIONS_STEP_DEBUG=1` (as in a failing GHA workflow) will automatically set `log_echo` and `log_warning` to `TRUE` (#175). * New `local_reproducible_output()` helper that sets various options and env vars to help ensure consistency of output across environments. diff --git a/R/conditions.R b/R/conditions.R index 37a7d97d..18e5a64e 100644 --- a/R/conditions.R +++ b/R/conditions.R @@ -51,7 +51,7 @@ with_handlers <- function(code, handlers) { } sanitize_call <- function(cnd) { - if (identical(cnd$call, quote(eval(expr, envir)))) { + if (identical(cnd$call, quote(withVisible(do)))) { cnd$call <- NULL } cnd diff --git a/R/evaluate.R b/R/evaluate.R index f541e8a5..819c0014 100644 --- a/R/evaluate.R +++ b/R/evaluate.R @@ -134,34 +134,49 @@ evaluate <- function(input, # The user's condition handlers have priority over ours handlers <- c(user_handlers, evaluate_handlers) - for (tle in tles) { - watcher$push_source(tle$src, tle$exprs) - if (debug || log_echo) { - cat_line(tle$src, file = stderr()) - } + cb <- function() { + do <- NULL # silence R CMD check note - continue <- withRestarts( - with_handlers( - { - for (expr in tle$exprs) { - ev <- withVisible(eval(expr, envir)) - watcher$capture_plot_and_output() - watcher$print_value(ev$value, ev$visible) - } - TRUE - }, - handlers - ), - eval_continue = function() TRUE, - eval_stop = function() FALSE, - eval_error = function(cnd) stop(cnd) - ) - watcher$check_devices() - - if (!continue) { - break + for (tle in tles) { + watcher$push_source(tle$src, tle$exprs) + if (debug || log_echo) { + cat_line(tle$src, file = stderr()) + } + + continue <- withRestarts( + with_handlers( + { + for (expr in tle$exprs) { + # Using `delayedAssign()` as an interface to the C-level function + # `Rf_eval()`. Unlike the R-level `eval()`, this doesn't create + # an unwinding scope. + eval(bquote(delayedAssign("do", .(expr), eval.env = envir))) + + ev <- withVisible(do) + watcher$capture_plot_and_output() + watcher$print_value(ev$value, ev$visible) + } + TRUE + }, + handlers + ), + eval_continue = function() TRUE, + eval_stop = function() FALSE, + eval_error = function(cnd) stop(cnd) + ) + watcher$check_devices() + + if (!continue) { + break + } } } + + # Here we use `eval()` to create an unwinding scope for `envir`. + # We call ourselves back immediately once the scope is created. + eval(as.call(list(cb)), envir) + watcher$capture_output() + # Always capture last plot, even if incomplete watcher$capture_plot(TRUE) diff --git a/tests/testthat/_snaps/evaluate.md b/tests/testthat/_snaps/evaluate.md index ce5316ea..9c438877 100644 --- a/tests/testthat/_snaps/evaluate.md +++ b/tests/testthat/_snaps/evaluate.md @@ -1,3 +1,18 @@ +# on.exit is evaluated at end of code + + Code + ev + Output + + Source code: + on.exit(print('bye')) + Source code: + print('hi') + Text output: + [1] "hi" + Text output: + [1] "bye" + # check_stop_on_error converts integer to enum Code diff --git a/tests/testthat/test-evaluate.R b/tests/testthat/test-evaluate.R index 8a559e99..f8de0974 100644 --- a/tests/testthat/test-evaluate.R +++ b/tests/testthat/test-evaluate.R @@ -97,6 +97,14 @@ test_that("multiple lines of comments do not lose the terminating \\n", { expect_equal(ev[[1]]$src, "# foo\n") }) +test_that("on.exit is evaluated at end of code", { + ev <- evaluate::evaluate(c( + "on.exit(print('bye'))", + "print('hi')" + )) + expect_snapshot(ev) +}) + test_that("check_stop_on_error converts integer to enum", { expect_equal(check_stop_on_error(0), "continue") expect_equal(check_stop_on_error(1), "stop") From 9034b84cd0af4ca8f209e52f5924e0a0f1c5479f Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 9 Jul 2024 16:33:47 +0200 Subject: [PATCH 2/5] Add a test for early return --- tests/testthat/test-evaluate.R | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/testthat/test-evaluate.R b/tests/testthat/test-evaluate.R index f8de0974..aa2e079f 100644 --- a/tests/testthat/test-evaluate.R +++ b/tests/testthat/test-evaluate.R @@ -105,6 +105,16 @@ test_that("on.exit is evaluated at end of code", { expect_snapshot(ev) }) +test_that("return causes an early return", { + ev <- evaluate::evaluate(c( + "1 + 1", + "return()", + "2 + 2" + )) + expect_output_types(ev, c("source", "text", "source")) +}) + + test_that("check_stop_on_error converts integer to enum", { expect_equal(check_stop_on_error(0), "continue") expect_equal(check_stop_on_error(1), "stop") From 4483f42e4ff0c77480da2891e32baaaf02911f06 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Wed, 10 Jul 2024 16:06:49 +0200 Subject: [PATCH 3/5] Strip call for top-level errors --- R/conditions.R | 4 ++++ tests/testthat/test-conditions.R | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/R/conditions.R b/R/conditions.R index 18e5a64e..dee08f0e 100644 --- a/R/conditions.R +++ b/R/conditions.R @@ -54,5 +54,9 @@ sanitize_call <- function(cnd) { if (identical(cnd$call, quote(withVisible(do)))) { cnd$call <- NULL } + if (identical(cnd$call, quote(eval(as.call(list(cb)), envir)))) { + cnd$call <- NULL + } + cnd } diff --git a/tests/testthat/test-conditions.R b/tests/testthat/test-conditions.R index f760d835..abaf6d21 100644 --- a/tests/testthat/test-conditions.R +++ b/tests/testthat/test-conditions.R @@ -16,6 +16,10 @@ test_that("all condition handlers first capture output", { test_that("conditions get calls stripped", { expect_equal(evaluate("warning('x')")[[2]]$call, NULL) expect_equal(evaluate("stop('x')")[[2]]$call, NULL) + + # including errors emitted by C + expect_equal(evaluate("mpg")[[2]]$call, NULL) + expect_equal(evaluate("3()")[[2]]$call, NULL) }) test_that("envvar overrides keep_* arguments", { From 12673a312559460cd166bf8664975a218944c44b Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Thu, 11 Jul 2024 07:46:48 +0200 Subject: [PATCH 4/5] Tweak news --- NEWS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/NEWS.md b/NEWS.md index 71d992a3..dcc02ec8 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,6 +1,6 @@ # evaluate (development version) -* `on.exit()` will now run at the end of the evaluate code, rather than immediately. This makes the scope of knitr chunks resemble the scope of R functions (#201). +* evaluation "chunks" now provide a function-like scope. This means that `on.exit()` will now run at the end of the evaluate code, rather than immediately and `return()` will cause the evaluation to finish (#201). * `parse_all()` adds a `\n` to the end of every line, even the last one if it didn't have one in the input. * Setting `ACTIONS_STEP_DEBUG=1` (as in a failing GHA workflow) will automatically set `log_echo` and `log_warning` to `TRUE` (#175). * New `local_reproducible_output()` helper that sets various options and env vars to help ensure consistency of output across environments. From 68e23001d66c81d0b61ce6250ae3136913e4c9d1 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Thu, 11 Jul 2024 07:52:33 +0200 Subject: [PATCH 5/5] Drop snapshot test in favour of `expect_output_types()` --- tests/testthat/_snaps/evaluate.md | 15 --------------- tests/testthat/test-evaluate.R | 2 +- 2 files changed, 1 insertion(+), 16 deletions(-) diff --git a/tests/testthat/_snaps/evaluate.md b/tests/testthat/_snaps/evaluate.md index 9c438877..ce5316ea 100644 --- a/tests/testthat/_snaps/evaluate.md +++ b/tests/testthat/_snaps/evaluate.md @@ -1,18 +1,3 @@ -# on.exit is evaluated at end of code - - Code - ev - Output - - Source code: - on.exit(print('bye')) - Source code: - print('hi') - Text output: - [1] "hi" - Text output: - [1] "bye" - # check_stop_on_error converts integer to enum Code diff --git a/tests/testthat/test-evaluate.R b/tests/testthat/test-evaluate.R index aa2e079f..6bfbbe50 100644 --- a/tests/testthat/test-evaluate.R +++ b/tests/testthat/test-evaluate.R @@ -102,7 +102,7 @@ test_that("on.exit is evaluated at end of code", { "on.exit(print('bye'))", "print('hi')" )) - expect_snapshot(ev) + expect_output_types(ev, c("source", "source", "text", "text")) }) test_that("return causes an early return", {