Optionally call winch::winch_add_trace_back() when collecting stack trace

[krlmlr]

This is for capturing a native stack trace when the error occurs. See the examples at https://github.com/r-prof/winch/pull/4/checks?check_run_id=961027969#step:10:28 and below. I hope to get better native stack traces with libunwind (r-prof/winch#2), I rather like how this seems to work already in all cases (stop(), Rf_error() and abort()).

Would you support this in rlang? I understand that we want to document and stabilize the structure of the "rlang_trace" before exposing it, and that we need tests. I don't mind marking this as "experimental".

[lionel-]

I would think the proper hook is globalCallingHandlers() (or options(error = ) on older Rs)?

[krlmlr]

I would have to rebuild a ton of rlang infrastructure to achieve compatibility with rlang::entrace(). I had my version of entrace() before, this PR is much easier.

Also I'm having a very hard time updating stack traces generated by rlang::abort() -- it's too late to do in the error handler, the stack traces are collected before that.

[lionel-]

it's too late to do in the error handler, the stack traces are collected before that.

It shouldn't be too late to use entrace() from a calling handler? What do you mean by too late?

[krlmlr]

rlang::abort() already calls trace_back() before the error handler kicks in?

rlang/R/cnd-abort.R

Line 136 in a144ac0

trace <- trace_back()

[krlmlr]

Results with libunwind are much better, see /vctrs.so in the stack trace below:

> vctrs::vec_as_location(quote, 2)
##[error]Error: Must subset elements with a valid subscript vector.
✖ Subscript has the wrong type `function`.
ℹ It must be logical, numeric, or character.
Backtrace:
    █
 1. └─vctrs::vec_as_location(quote, 2)
Backtrace:
    █
 1. └─vctrs::vec_as_location(quote, 2)
 2.   └─`/vctrs.so`::vctrs_as_location()
 3.     └─`/vctrs.so`::vec_as_location_opts()
 4.       ├─rlang::cnd_signal(...)
 5.       │ └─rlang:::signal_abort(cnd)
 6.       │   └─base::stop(fallback)
 7.       └─(function () ...

Test run: https://github.com/r-prof/winch/runs/965694700?check_suite_focus=true#step:11:171.

[lionel-]

rlang::abort() already calls trace_back() before the error handler kicks in?

Right but as a calling handler you can create another trace if you'd like. We may also move the trace capture at a later stage (after the signalCondition) in the future, I don't remember why we didn't end up doing that (it was the original plan).

[lionel-]

It would be good to add C stack support to the entracing in rlang, probably not with a hook though.

[hadley]

It would be good to come to some sort of conclusion here. I don't think we need a long-term solution immediately, but we do need something so that we all can start to try out the R/C tracebacks in our own work and see how they feel.

[lionel-]

Instead of a hook I think rlang should call winch if installed.

[hadley]

That's fine with me too

[krlmlr]

Done now.

[lionel-]

This should probably be opt-in during development? I.e. is_true(peek_option("rlang_trace_use_winch")).

Can you document what winch is doing? Is it making any assumptions about the trace internals? Note that these internals are not stable yet, we still have some work to do to match Gabor's traces.

[krlmlr]

I'm adding and modifying rows in the data frame, relying on the internal structure. Would it be better to provide a drop-in replacement for sys.calls() and sys.frames() instead?

[lionel-]

Why would this not inherit from rlang_trace?

[krlmlr]

Fixed now. Added versioned option to allow for incompatible changes.

[krlmlr]

winch 0.0.1 is on CRAN now.

[lionel-]

Can you take a quick look at these changes @krlmlr? I'm sending rlang to CRAN today

[lionel-]

Thanks a lot Kirill!

[lionel-]

rlang 0.4.8 is on CRAN