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

[cderv]

After discussing with @lionel- , our conclusion was that invokeRestart("error") may not be necessary.
When stop_on_error = 2L, the code needs to stop on error, which will happen if the handler called in withCallingHandler() returns

So in this case, the handler could simply return NULL.

This solve the traceback problem encountered with latest evaluate and current rlang and knitr logic for entracing.

It also solve the traceback trimming problem that is created when invokeRestart() is used. With current invokeRestart("error", cnd), the sys.frames() that is handled by the traceback is not containing the same environments it seems.

Also calling signalCondition(cnd) in the restart handler was triggering some global handlers registered in Positron or in RStudio IDE - which led to differences in the context where rendering happened.

Which this change, rmarkdown::render() shows a rlang traceback with only the cell's function in all context: Positron, RStudio IDE and R console.

However, the snapshot test has changed, as output is now captured. I think it is ok though.

Regarding #231 and the idea of auto entracing, this was supposed to be done in knitr already. The change to withRestart() in recent evaluate demands an adaptation in knitr. I have a PR to come on knitr side to help with this.

[hadley]

Thanks so much for diving into this problem and figuring out a fix!

I think my only suggestion would be to make the snapshot a little clearer by doing:

ev <- evaluate("stop(\"1\")\n2", stop_on_error = 2L)

[hadley]

Do you have any thoughts on how we might test this to make sure we don't accidentally break RMarkdown tracebacks again in the future?

[cderv]

Do you have any thoughts on how we might test this to make sure we don't accidentally break RMarkdown tracebacks again in the future?

I tried a few things, but I am struggling to get a reproducible test for issue, let alone with evaluate only. It seems highly dependant on the context. So I am still unto this.

[hadley]

@cderv I think it would be taking taking a suggests dependency on knitr, rmarkdown etc if we could get a good reprex for this. Have you tried callr + rmarkdown::render()? Maybe we could do a snapshot test of the output?

[cderv]

Have you tried callr + rmarkdown::render()?

I was trying with evaluate() only by setting calling.handlers as would knitr or rlang do right now.

Maybe we could do a snapshot test of the output?

This is what I am trying, but with testthat using rlang and entracing by default error it seems, I can get something to reproduce the issue we have when using expect_snapshot() or expect_error() + printing.

I think the issue is that with current evaluate logic before this PR, stop() is used in the restart, and this hide the backtrace even if the condition is correctly a rlang_error with one. Anything that catch the condition and print it will no show the problem.

Even the trace cleaning problem is not an issue because it seems expect_error() does the right thing to clean trace.

If we can use callr + rmarkdown or knitr, I'll try that.

Initially, I tried to make a test in knitr instead directly (as this is knitr setting the logic of calling.handlers now) but it is not using testthat so I did not have snaphot available.

[cderv]

It seems something is different on CI than locally - looking into that.

[cderv]

New change reduced the difference, but it seems to be related to some reproducibility on character used

❯ difft.exe .\stop-error-trace-trimmed.txt .\stop-error-trace-trimmed.new.txt        
.\stop-error-trace-trimmed.new.txt --- Text
1 Error in `h()`:             1 Error in `h()`:
2 ! !                         2 ! !
3 Backtrace:                  3 Backtrace:
4     ▆                       4     x
5  1. └─global f()            5  1. \-global f()
6  2.   └─global g()          6  2.   \-global g()
7  3.     └─global h()        7  3.     \-global h()
8 Execution halted            8 Execution halted

I probably need to set some reproducibilty setting when passing to rscript execution 🤔

[cderv]

@hadley doing this in evaluate allows to get trimmed traceback in evaluate() itself. This is related to comment at #232 (comment)
See the new snapshot file without any evaluate internals.

For full context, this will be done at knitr level and so it will apply for evaluate() in knitr context.
However, I don't know of the impact on doing that directly in evaluate. evaluate() is used also in other tools (like testthat) and I wonder if this could have impact.

Maybe @lionel- you have some advice on setting this here ?

[hadley]

My gut feeling is that it's worth it, but I don't know if there will be other negative effects.