From 495cfa945983fc6f401a6e9f2849503dfba6b195 Mon Sep 17 00:00:00 2001
From: Hadley Wickham <h.wickham@gmail.com>
Date: Mon, 17 Jun 2024 20:40:18 +0100
Subject: [PATCH] Return output from tryCatch()

This ensures that the value is set everytime, not just when there's no error. Fixes #145
---
 DESCRIPTION                    |  2 +-
 R/eval.R                       | 15 ++++++++-------
 R/output.R                     |  3 +++
 man/new_output_handler.Rd      |  5 ++++-
 tests/testthat/test-evaluate.R |  5 +++++
 5 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/DESCRIPTION b/DESCRIPTION
index a85039c..365518c 100644
--- a/DESCRIPTION
+++ b/DESCRIPTION
@@ -37,4 +37,4 @@ Config/Needs/website: tidyverse/tidytemplate
 Config/testthat/edition: 3
 Encoding: UTF-8
 Roxygen: list(markdown = TRUE)
-RoxygenNote: 7.2.3
+RoxygenNote: 7.3.1
diff --git a/R/eval.R b/R/eval.R
index 8d2b84f..e0aa2a8 100644
--- a/R/eval.R
+++ b/R/eval.R
@@ -224,10 +224,12 @@ evaluate_call <- function(call,
     invokeRestart("muffleMessage")
   }
 
-  ev <- list(value = NULL, visible = FALSE)
-
   if (use_try) {
-    handle <- function(f) try(f, silent = TRUE)
+    handle <- function(code) {
+      tryCatch(code, error = function(err) {
+        list(value = NULL, visible = FALSE)
+      })
+    }
   } else {
     handle <- force
   }
@@ -252,8 +254,8 @@ evaluate_call <- function(call,
   multi_args <- length(formals(value_handler)) > 1
   for (expr in call) {
     srcindex <- length(output)
-    time <- timing_fn(handle(
-      ev <- withCallingHandlers(
+    time <- timing_fn(ev <- handle(
+      withCallingHandlers(
         withVisible(eval_with_user_handlers(expr, envir, enclos, user_handlers)),
         warning = wHandler,
         error = eHandler,
@@ -266,11 +268,10 @@ evaluate_call <- function(call,
 
     # If visible or the value handler has multi args, process and capture output
     if (ev$visible || multi_args) {
-      pv <- list(value = NULL, visible = FALSE)
       value_fun <- if (multi_args) value_handler else {
         function(x, visible) value_handler(x)
       }
-      handle(pv <- withCallingHandlers(withVisible(
+      pv <- handle(withCallingHandlers(withVisible(
         value_fun(ev$value, ev$visible)
       ), warning = wHandler, error = eHandler, message = mHandler))
       handle_output(TRUE)
diff --git a/R/output.R b/R/output.R
index c5e79cd..b10e723 100644
--- a/R/output.R
+++ b/R/output.R
@@ -81,6 +81,9 @@ render <- function(x) if (isS4(x)) methods::show(x) else print(x)
 #' @param value Function to handle the values returned from evaluation. If it
 #'   only has one argument, only visible values are handled; if it has more
 #'   arguments, the second argument indicates whether the value is visible.
+#' 
+#'   If the expression errored and `stop_on_error` is not `2`, value 
+#'   will be set to `NULL` and visible will  be set to `FALSE`.
 #' @param calling_handlers List of [calling handlers][withCallingHandlers].
 #'   These handlers have precedence over the exiting handler installed
 #'   by [evaluate()] when `stop_on_error` is set to 0.
diff --git a/man/new_output_handler.Rd b/man/new_output_handler.Rd
index 798a7a3..2e08417 100644
--- a/man/new_output_handler.Rd
+++ b/man/new_output_handler.Rd
@@ -39,7 +39,10 @@ drop the source from the output.}
 
 \item{value}{Function to handle the values returned from evaluation. If it
 only has one argument, only visible values are handled; if it has more
-arguments, the second argument indicates whether the value is visible.}
+arguments, the second argument indicates whether the value is visible.
+
+If the expression errored and \code{stop_on_error} is not \code{2}, value
+will be set to \code{NULL} and visible will  be set to \code{FALSE}.}
 
 \item{calling_handlers}{List of \link[=withCallingHandlers]{calling handlers}.
 These handlers have precedence over the exiting handler installed
diff --git a/tests/testthat/test-evaluate.R b/tests/testthat/test-evaluate.R
index e607d74..d2cbdeb 100644
--- a/tests/testthat/test-evaluate.R
+++ b/tests/testthat/test-evaluate.R
@@ -5,6 +5,11 @@ test_that("file with only comments runs", {
   expect_equal(classes(ev), c("source", "source"))
 })
 
+test_that("error after output works correctly", {
+  ev <- evaluate::evaluate("1;stop('x')")
+  expect_equal(classes(ev), c("source", "character", "simpleError"))
+})
+
 test_that("data sets loaded", {
   skip_if_not_installed("lattice")