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

feat: provide a stack trace for fatal errors in chunk #2369

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

Conversation

eternal-flame-AD
Copy link

This is the idea which will provide a backtrace of the error instead of just the message: r-lib/evaluate#226 (comment).

Before:

Error in select(., nothing) : Can't select columns that don't exist.Column `nothing` doesn't exist.

Quitting from lines 3-11 [unnamed-chunk-1] (bad.Rmd)

After:

Error in `eng_r()`:
! Error in chunk unnamed-chunk-1
Caused by error in `select()`:
! Can't select columns that don't exist.Column `nothing` doesn't exist.
Backtrace:
 1. global myerror()
 4. dplyr:::select.data.frame(., nothing)

Quitting from lines 3-11 [unnamed-chunk-1] (bad.Rmd)

@CLAassistant
Copy link

CLAassistant commented Oct 10, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Owner

@yihui yihui left a comment

Choose a reason for hiding this comment

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

To be honest, I don't fully understand the condition handling stuff in R, but I wonder if this will be a better place to make the change:

knitr/R/output.R

Lines 312 to 315 in 0f59bba

withCallingHandlers(
if (tangle) process_tangle(group) else process_group(group),
error = function(e) if (xfun::pkg_available('rlang', '1.0.0')) rlang::entrace(e)
),

Thank you!

@yihui yihui requested a review from cderv October 10, 2024 14:52
@eternal-flame-AD
Copy link
Author

I will think about this more later today, but my initial thoughts for this part is:

Rlang documentation says

entrace() is meant to be used as a global handler.

It seems to be a UX improvement tool to make stop() emit a trace rather than a wrapper that magically adds stack trace. I tried a toy example:

myerr <- \() { entrace(cnd = errorCondition("Test")) } # no stack trace when called
trace <- \() myerr() # stack trace only contains trace(), not myerr()

xfun::handle_error seems to be undocumented API and, is this meant to catch errors that are even supposed to crash R so it is written as an exit handler instead of just a catch?

@yihui
Copy link
Owner

yihui commented Oct 10, 2024

xfun:::handle_error() doesn't try to catch errors at all (it doesn't use tryCatch() or withCallingHandlers()). It is intended to run a "handler" function when errors occur in the expression. You can basically treat xfun:::handle_error(expr) as expr.

@eternal-flame-AD
Copy link
Author

My original thought about writing it there was I think an R backtrace is only useful for R engine. Seems only R engine output is improved (presumably because the call to evaluate() here erased the call stack before your chunk-level handler took it.

We probably don't really have a case of where we intentionally want to suppress traces for one type of engine. It is fine and probably clearer to lift it up.

Found another issue when testing this that I can reproduce on master ... I will look at it this weekend to see what condition triggered this and if it is my environment is bad or another bug

```{python}
print("Hello Python!")
```

```
  Error in loadable(pkg) : node stack overflow
Error during wrapup: node stack overflow
Error: no more error handlers available (recursive errors?); invoking 'abort' restart

Quitting from lines 90-91 [hello] (error.Rmd)
```
```

Signed-off-by: eternal-flame-AD <[email protected]>
@eternal-flame-AD
Copy link
Author

Just a bump in case this got lost :)

@cderv
Copy link
Collaborator

cderv commented Oct 23, 2024

No it does not get lost. I have a current workload on Quarto work because we're are working on v1.6 release shortly. So my focus is not right now in R work and I prefer to be focused on topic when I worked on them - sorry to keep you waiting. I'll come back to this very quickly !

Thank you for your patience

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.

4 participants