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

Auto-entrace errors #231

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Auto-entrace errors #231

wants to merge 2 commits into from

Conversation

hadley
Copy link
Member

@hadley hadley commented Nov 8, 2024

No description provided.

Copy link
Collaborator

@cderv cderv left a comment

Choose a reason for hiding this comment

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

I have looked into this traceback() change we identified. Here are some notes so that we can decide best way to adapt.

I found that new withRestarts() logic has a few implications

    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, envir)
          }
          TRUE
        },
        handlers
      ),
      eval_continue = function() TRUE,
      eval_stop = function() FALSE,
      eval_error = signal_error
    )

In there, handlers needs to invoke restarts, otherwise it will skip the logic here. This is what happen with rlang::global_entrace() which adds a user_handler doing rlang::entrace(). This leads to continue being NULL, and it throws an error.

By commenting # rlang::global_entrace(), , the result will depends on rlang::abort() or stop() being used due to signal_error calling signalConditon():

  • When using rlang::abort(), cnd is

     > str(cnd)
     List of 5
      $ message: chr "!"
      $ trace  :Classes 'rlang_trace', 'rlib_trace', 'tbl' and 'data.frame':	4 obs. of  6 variables:
       ..$ call       :List of 4
       .. ..$ : language f()
       .. ..$ : language g()
       .. ..$ : language h1()
       .. ..$ : language rlang::abort("!")
       ..$ parent     : int [1:4] 0 1 2 3
       ..$ visible    : logi [1:4] TRUE TRUE TRUE FALSE
       ..$ namespace  : chr [1:4] NA NA NA "rlang"
       ..$ scope      : chr [1:4] "global" "global" "global" "::"
       ..$ error_frame: logi [1:4] FALSE FALSE TRUE FALSE
       ..- attr(*, "version")= int 2
      $ parent : NULL
      $ rlang  :List of 1
       ..$ inherit: logi TRUE
      $ call   : language h1()
      - attr(*, "class")= chr [1:3] "rlang_error" "error" "condition"

    and signalCondition() will run and not stop

  • When using stop(), cnd is

     > str(cnd)
     List of 2
      $ message: chr "!"
      $ call   : language h2()
      - attr(*, "class")= chr [1:3] "simpleError" "error" "condition"

    in that case, it will stop there - it seems signalCondition(cnd) wil l call stop(). Not sure how to find which handler it is calling exactly.

This happens in RStudio IDE context. To note that in this context, the special IDE setting comes also into the mix with

> getOption("error")
(function () 
{
    .rs.recordTraceback(TRUE, 5, .rs.enqueueError)
})()

when signalCondition() is called and no more handlers. From ?conditions

globalCallingHandlers establishes calling handlers globally. These handlers are only called as a last resort, after the other handlers dynamically registered with withCallingHandlers have been invoked. They are called before the error global option (which is the legacy interface for global handling of errors).

I'll try to see difference in other context.

More generally, with this continue <- withRestarts(), I believe handlers needs to return TRUE or FALSE, or really stop() or abort() effectively.

I'll keep looking.

R/evaluate.R Outdated
signal_error <- function(cnd) {
signalCondition(cnd)
if (has_rlang) {
rlang::entrace(cnd)
Copy link
Collaborator

Choose a reason for hiding this comment

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

From my digging into the situation, I believe rlang::entrace() won't throw an error in some situation (when this is already a rlang_error)
From rlang::entrace()

  if (!missing(cnd) && inherits(cnd, "rlang_error")) {
    poke_last_error(cnd)
    return()
  }

And so with the example of using global_entrace(), this will error because signal_error() returns NULL

This document with this PR version of evaluate

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

```{r}
f <- function() g()
g <- function() h2()
h1 <- function() rlang::abort("!")
h2 <- function() stop("!")
f()
```

throws Error in `!continue`:

processing file: test.Rmd
  |....................................................................................................................................................| 100% [unnamed-chunk-3]Error in `!continue`:
! invalid argument type
Backtrace:
  1. xfun::in_dir("../rmarkdown", rmarkdown::render("test.Rmd"))
  2. rmarkdown::render("test.Rmd")
  3. knitr::knit(knit_input, knit_output, envir = envir, quiet = quiet)
  4. knitr:::process_file(text, output)
  7. knitr:::process_group(group)
  8. knitr:::call_block(x)
  9. knitr:::block_exec(params)
 10. knitr:::eng_r(options)
 13. knitr (local) evaluate(...)
 14. evaluate::evaluate(...)

Quitting from lines 17-22 [unnamed-chunk-3] (test.Rmd)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if this should directly by rlang::abort() instead of rlang::entrace() so that it really stops with traceback printed.

This is similar to what have been discussed in #226

Copy link
Member Author

Choose a reason for hiding this comment

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

You can't pass an existing condition object to abort() like you can with stop(). My read of the rlang docs does suggest that this is what entrace() does, so I think we'll need to get @lionel- to help.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've updated with something I tried locally that should at least avoid the !continue problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants