Always return a bool from restarts

[hadley]

Fixes #225

I can't remember exactly why this comes up, but clearly in some circumstances rethrowing the error doesn't trigger immediate termination. This shouldn't have widespread effects.

I can't figure out how to test this, but I confirmed that it fixed the reported bug when rendering a qmd.

[eternal-flame-AD]

Should this guard be a stop(cnd)? Otherwise the caller will assume the execution can continue and there is nothing on their end they can see an error occurred even if they requested stop_on_error = 2. An alternative is clearly document that stop_on_error requires caller to set up a handler to catch this condition which {knitr} currently do not.

```{r should_stop, error=FALSE}
rlang::abort("Test")
```


```{r}
print("Hello")
```

test
rlang::abort("Test")
## Error:
## ! Test
print("Hello")
## [1] "Hello"

[hadley]

Oh hmmmm, good point.

I'll need to think about this more because I know that code originally used stop() instead of signalCondition() and that caused problems with us losing call stack info.

[hadley]

You pointed out this in the issue:

Calling handlers are established by withCallingHandlers. If a condition is signaled and the applicable handler is a calling handler, then the handler is called by signalCondition in the context where the condition was signaled but with the available handlers restricted to those below the handler called in the handler stack. If the handler returns, then the next handler is tried; once the last handler has been tried, signalCondition returns NULL.

But I don't understand why there is no handler. What happened to the global error handler?

[eternal-flame-AD]

I don't think there is a global handler for signalCondition, I learned about this too when debugging this issue, it's not intuitive or documented clearly...

r$> signalCondition(errorCondition("Test"))
NULL

I can try see what happen to the stack info later but from the documentation it seems like stop(cnd) is just equivalent to signalCondition and then if fell through, a hard error?

Usage:

     stop(..., call. = TRUE, domain = NULL)
     geterrmessage()

Arguments:

     ...: zero or more objects which can be coerced to character (and
          which are pasted together with no separator) or a single
          condition object.

   call.: logical, indicating if the call should become part of the
          error message.

  domain: see ‘gettext’.  If ‘NA’, messages will not be translated.

Details:

     The error action is controlled by error handlers established
     within the executing code and by the current default error handler
     set by ‘options(error=)’.  The error is first signaled as if using
     ‘signalCondition()’.  If there are no handlers or if all handlers
     return, then the error message is printed (if
     ‘options("show.error.messages")’ is true) and the default error
     handler is used.

[eternal-flame-AD]

if stop will really strip the call stack and there's nothing we could do maybe document that a user friendly caller should take care of this condition instead of letting it propagate to a stop(), at least this way a "wrong" call will just result in a bad traceback instead of logical problems.

[hadley]

Hmmm, this is brining back vague memories. If you look at the source code for stop() you see:

       .Internal(.signalCondition(cond, message, call))
       .Internal(.dfltStop(message, call))

So it both signals the condition and then does something else.

And rlang:::signal_abort() calls signalCondition() and then stop() as a fallback.

[eternal-flame-AD]

Maybe do this and let {rlang} save all the details?

https://rlang.r-lib.org/reference/topic-error-chaining.html#rethrowing-errors

[eternal-flame-AD]

If I change the code to:

eval_error = function(cnd) rlang::abort("Error occurred during evaluation", parent = cnd)

I get this which looks good?

Error in `fun()`:
! Error occurred during evaluation
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 [should_stop] (bad.Rmd)

[hadley]

We don't want to wrap the error though, we want to rethrow the original error. (And we can't take a dependency on rlang 😭)

Hmmmm, looking at rlang:cnd_signal() is informative — it (eventually) calls signalCondition() + stop() for errors. So that suggests your original idea is correct.

[eternal-flame-AD]

Yeah then probably signal then stop is the best option I could see then.. :(

The logic here gets weird too, from what I understand a handler must be noreturn otherwise it still will fall back through to your code so a caller that wants a nice error message should get your condition, wrap it themselves and call abort with it , right?

so probably the explanation for stop on error=2 need to be changed but probably luckily won't break existing code except maybe cosmetic things like debugging code that is wrong in the first place