Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't use a restart to handle stop_on_error = 2L #232

Merged
merged 16 commits into from
Jan 8, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions DESCRIPTION
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,14 @@ BugReports: https://github.com/r-lib/evaluate/issues
Depends:
R (>= 3.6.0)
Suggests:
callr,
covr,
ggplot2 (>= 3.3.6),
lattice,
methods,
pkgload,
rlang,
knitr,
testthat (>= 3.0.0),
withr
Config/Needs/website: tidyverse/tidytemplate
Expand Down
3 changes: 2 additions & 1 deletion R/conditions.R
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ condition_handlers <- function(watcher, on_error, on_warning, on_message) {
switch(on_error,
continue = invokeRestart("eval_continue"),
stop = invokeRestart("eval_stop"),
error = invokeRestart("eval_error", cnd)
# No need to invoke a restart as we want the error to be thrown in this case.
error = NULL
)
}
)
Expand Down
8 changes: 6 additions & 2 deletions R/evaluate.R
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,11 @@ evaluate <- function(input,
}
local_inject_funs(envir)

if (is.null(getOption("rlang_trace_top_env"))) {
# If not already set, indicate the top environment to trim traceback
options(rlang_trace_top_env = envir)
}
cderv marked this conversation as resolved.
Show resolved Hide resolved

# Handlers for warnings, errors and messages
user_handlers <- output_handler$calling_handlers
evaluate_handlers <- condition_handlers(
Expand Down Expand Up @@ -151,8 +156,7 @@ evaluate <- function(input,
handlers
),
eval_continue = function() TRUE,
eval_stop = function() FALSE,
eval_error = function(cnd) {signalCondition(cnd); stop(cnd)}
eval_stop = function() FALSE
)
watcher$check_devices()

Expand Down
4 changes: 2 additions & 2 deletions tests/testthat/_snaps/conditions.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@
Warning in `f()`:
Hi!

# all three starts of stop_on_error work as expected
# all three values of stop_on_error work as expected

Code
evaluate("stop(\"1\")\n2", stop_on_error = 2L)
ev <- evaluate("stop(\"1\")\n2", stop_on_error = 2L)
Condition
Error:
! 1
Expand Down
10 changes: 10 additions & 0 deletions tests/testthat/_snaps/conditions/abort-error.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
Error in `h()`:
! !
Backtrace:
x
1. \-global f()
2. \-global g()
3. \-global h()
4. \-rlang::abort("!")
Execution halted
Ran 8/8 deferred expressions
22 changes: 22 additions & 0 deletions tests/testthat/_snaps/conditions/rmd-abort-error.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
---
title: document with error
---


``` r
f <- function() g()
g <- function() h()
h <- function() rlang::abort("!")
f()
```

```
## Error in `h()`:
## ! !
## Backtrace:
## x
## 1. \-evaluate (local) f()
## 2. \-evaluate (local) g()
## 3. \-evaluate (local) h()
## 4. \-rlang::abort("!")
```
12 changes: 12 additions & 0 deletions tests/testthat/_snaps/conditions/rmd-abort-error.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@


processing file: ressources/with-abort-error.Rmd
Error in `h()`:
! !
Backtrace:
1. global f()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do we need to do to display a tree here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised to see the simplified backtrace which is no longer the default in rlang. Do we have a print(err, simplify = "branch") parameter somewhere?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@lionel- lionel- Jan 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cderv Could you test with r-lib/rlang#1769 please? I'm hoping it helps in non-interactive sessions without adverse effects in interactive ones (calling knit() at the console).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the two snapshot, they are "fixed"

diff --git a/tests/testthat/_snaps/conditions/rmd-abort-error.txt b/tests/testthat/_snaps/conditions/rmd-abort-error.txt
index 114acb6..6b87b65 100644
--- a/tests/testthat/_snaps/conditions/rmd-abort-error.txt
+++ b/tests/testthat/_snaps/conditions/rmd-abort-error.txt
@@ -4,9 +4,11 @@ processing file: ressources/with-abort-error.Rmd
 Error in `h()`:
 ! !
 Backtrace:
- 1. global f()
- 2. global g()
- 3. global h()
+
+ 1. └─global f()
+ 2.   └─global g()
+ 3.     └─global h()
+ 4.       └─rlang::abort("!")
 
 Quitting from lines 6-10 [unnamed-chunk-1] (ressources/with-abort-error.Rmd)
 Execution halted
diff --git a/tests/testthat/_snaps/conditions/rmd-stop-error-auto-entrace.txt b/tests/testthat/_snaps/conditions/rmd-stop-error-auto-entrace.txt
index c2ea7b0..a1867ee 100644
--- a/tests/testthat/_snaps/conditions/rmd-stop-error-auto-entrace.txt
+++ b/tests/testthat/_snaps/conditions/rmd-stop-error-auto-entrace.txt
@@ -4,9 +4,10 @@ processing file: ressources/with-stop-error-auto-entrace.Rmd
 Error in `h()`:
 ! !
 Backtrace:
- 1. global f()
- 2. global g()
- 3. global h()
+
+ 1. └─global f()
+ 2.   └─global g()
+ 3.     └─global h()

 Quitting from lines 6-10 [unnamed-chunk-1] (ressources/with-stop-error-auto-entrace.Rmd)
 Execution halted

I then tested manually and I think I covered most of cases. It does not seem to have adverse affect on interactive session. It looks good to merge.

Maybe I'll update the snapshot once next rlang is release so that it does not prevent evaluate release.

2. global g()
3. global h()

Quitting from lines 6-10 [unnamed-chunk-1] (ressources/with-abort-error.Rmd)
Execution halted
12 changes: 12 additions & 0 deletions tests/testthat/_snaps/conditions/rmd-stop-error-auto-entrace.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@


processing file: ressources/with-stop-error-auto-entrace.Rmd
Error in `h()`:
! !
Backtrace:
1. global f()
2. global g()
3. global h()

Quitting from lines 6-10 [unnamed-chunk-1] (ressources/with-stop-error-auto-entrace.Rmd)
Execution halted
27 changes: 27 additions & 0 deletions tests/testthat/_snaps/conditions/rmd-stop-error-entrace-sewed.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
---
title: document with error
---


``` r
rlang::global_entrace()
options(rlang_backtrace_on_error_report = "full")
```


``` r
f <- function() g()
g <- function() h()
h <- function() stop("!")
f()
```

```
## Error in `h()`:
## ! !
## Backtrace:
## x
## 1. \-evaluate (local) f()
## 2. \-evaluate (local) g()
## 3. \-evaluate (local) h()
```
15 changes: 15 additions & 0 deletions tests/testthat/_snaps/conditions/rmd-stop-error.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
---
title: document with error
---


``` r
f <- function() g()
g <- function() h()
h <- function() stop("!")
f()
```

```
## Error in h(): !
```
4 changes: 4 additions & 0 deletions tests/testthat/_snaps/conditions/stop-error-no-trace.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Error in h() : !
Calls: <Anonymous> ... withCallingHandlers -> withVisible -> eval -> eval -> f -> g -> h
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the error you see when a vignette has an error, so that might suggest if we can entrace in a vignette we can get better errors?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. I'll see if I can adapt knitr for this.

Execution halted
Ran 8/8 deferred expressions
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
Error in `h()`:
! !
Backtrace:
x
1. \-global f()
2. \-global g()
3. \-global h()
Execution halted
Ran 8/8 deferred expressions
24 changes: 24 additions & 0 deletions tests/testthat/_snaps/conditions/stop-error-trace-trim.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
Error in `h()`:
! !
Backtrace:
x
1. +-evaluate::evaluate(...)
2. | +-base::withRestarts(...)
3. | | \-base (local) withRestartList(expr, restarts)
4. | | +-base (local) withOneRestart(withRestartList(expr, restarts[-nr]), restarts[[nr]])
5. | | | \-base (local) doWithOneRestart(return(expr), restart)
6. | | \-base (local) withRestartList(expr, restarts[-nr])
7. | | \-base (local) withOneRestart(expr, restarts[[1L]])
8. | | \-base (local) doWithOneRestart(return(expr), restart)
9. | +-evaluate:::with_handlers(...)
10. | | +-base::eval(call)
11. | | | \-base::eval(call)
12. | | \-base::withCallingHandlers(...)
13. | +-base::withVisible(eval(expr, envir))
14. | \-base::eval(expr, envir)
15. | \-base::eval(expr, envir)
16. \-global f()
17. \-global g()
18. \-global h()
Execution halted
Ran 8/8 deferred expressions
9 changes: 9 additions & 0 deletions tests/testthat/_snaps/conditions/stop-error-trace-wch.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
Error in `h()`:
! !
Backtrace:
x
1. \-global f()
2. \-global g()
3. \-global h()
Execution halted
Ran 8/8 deferred expressions
17 changes: 17 additions & 0 deletions tests/testthat/helper.R
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,20 @@ expect_output_types <- function(x, types) {
output_types <- vapply(x, output_type, character(1))
expect_equal(output_types, types)
}

quick_install <- function(package, lib, quiet = TRUE) {
opts <- c(
"--data-compress=none",
"--no-byte-compile",
"--no-data",
"--no-demo",
"--no-docs",
"--no-help",
"--no-html",
"--no-libs",
"--use-vanilla",
sprintf("--library=%s", lib),
package
)
invisible(callr::rcmd("INSTALL", opts, show = !quiet, fail_on_status = TRUE))
}
7 changes: 7 additions & 0 deletions tests/testthat/ressources/with-abort-error.R
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
testthat::local_reproducible_output()
evaluate::evaluate(function() {
f <- function() g()
g <- function() h()
h <- function() rlang::abort("!")
f()
}, stop_on_error = 2L)
10 changes: 10 additions & 0 deletions tests/testthat/ressources/with-abort-error.Rmd
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
---
title: document with error
---

```{r}
f <- function() g()
g <- function() h()
h <- function() rlang::abort("!")
f()
```
10 changes: 10 additions & 0 deletions tests/testthat/ressources/with-stop-error-auto-entrace.Rmd
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
---
title: document with error
---

```{r}
f <- function() g()
g <- function() h()
h <- function() stop("!")
f()
```
7 changes: 7 additions & 0 deletions tests/testthat/ressources/with-stop-error-no-trace.R
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
testthat::local_reproducible_output()
evaluate::evaluate(function() {
f <- function() g()
g <- function() h()
h <- function() stop("!")
f()
}, stop_on_error = 2L)
15 changes: 15 additions & 0 deletions tests/testthat/ressources/with-stop-error-sewed.Rmd
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
---
title: document with error
---

```{r}
rlang::global_entrace()
options(rlang_backtrace_on_error_report = "full")
```

```{r}
f <- function() g()
g <- function() h()
h <- function() stop("!")
f()
```
12 changes: 12 additions & 0 deletions tests/testthat/ressources/with-stop-error-trace-trim.R
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
testthat::local_reproducible_output()
handlers <- evaluate::new_output_handler(
calling_handlers = list(error = function(cnd) rlang::entrace(cnd))
)
library(evaluate)
options(rlang_trace_top_env = rlang::pkg_env("evaluate"))
evaluate(function() {
f <- function() g()
g <- function() h()
h <- function() stop("!")
f()
}, stop_on_error = 2L, output_handler = handlers)
10 changes: 10 additions & 0 deletions tests/testthat/ressources/with-stop-error-trace.R
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
testthat::local_reproducible_output()
handlers <- evaluate::new_output_handler(
calling_handlers = list(error = function(cnd) rlang::entrace(cnd))
)
evaluate::evaluate(function() {
f <- function() g()
g <- function() h()
h <- function() stop("!")
f()
}, stop_on_error = 2L, output_handler = handlers)
10 changes: 10 additions & 0 deletions tests/testthat/ressources/with-stop-error-wch.R
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
testthat::local_reproducible_output()
withCallingHandlers(
error = function(cnd) rlang::entrace(cnd),
evaluate::evaluate(function() {
f <- function() g()
g <- function() h()
h <- function() stop("!")
f()
}, stop_on_error = 2L)
)
Loading
Loading