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

Improve traceback #218

Closed
wants to merge 1 commit into from
Closed

Improve traceback #218

wants to merge 1 commit into from

Conversation

hadley
Copy link
Member

@hadley hadley commented Aug 21, 2024

Fixes #214

@hadley hadley requested a review from lionel- August 21, 2024 16:46
14. | | | \-base::eval(call)
15. | | \-base::withCallingHandlers(...)
16. | \-base::withVisible(do) at evaluate/R/evaluate.R:153:15
17. \-evaluate (local) f() at evaluate/R/evaluate.R:153:15
Copy link
Member Author

Choose a reason for hiding this comment

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

At least we now get the actual error traceback. Is there some obvious way to trim off some of the earlier frames?

Copy link
Member

Choose a reason for hiding this comment

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

We can no longer use frame environments as boundaries (or top_env) because we no longer evaluate in an eval() stack frame. We might have to introduce a new way of setting a boundary in the call stack...

Copy link
Member Author

Choose a reason for hiding this comment

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

Should we create an evaluate_ wrapper that we can call from evaluate() so we still get one regular frame on the call stack?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm that doesn't make any difference.

Copy link
Member

Choose a reason for hiding this comment

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

Did you set the rlang_trace_top_env option (from memory) in that wrapper?

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 tried this in both evaluate() and evaluate_():

old <- options(rlang_trace_top_env = environment())
defer(options(old))

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 also wonder if this is the root cause of #219

14. | | | \-base::eval(call)
15. | | \-base::withCallingHandlers(...)
16. | \-base::withVisible(do) at evaluate/R/evaluate.R:153:15
17. \-evaluate (local) f() at evaluate/R/evaluate.R:153:15
Copy link
Member

Choose a reason for hiding this comment

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

We can no longer use frame environments as boundaries (or top_env) because we no longer evaluate in an eval() stack frame. We might have to introduce a new way of setting a boundary in the call stack...

@lionel-
Copy link
Member

lionel- commented Aug 28, 2024

@hadley Locally with this fix I don't see any changes to the backtraces of unexpected errors.

However with this change I do see the same backtrace as you do:

@@ -31,10 +31,11 @@ condition_handlers <- function(watcher, on_error, on_warning, on_message) {
       cnd <- sanitize_call(cnd)
       watcher$push(cnd)
 
-      switch(on_error,
+      switch(
+        on_error,
         continue = invokeRestart("eval_continue"),
         stop = invokeRestart("eval_stop"),
-        error = invokeRestart("eval_error", cnd)
+        error = NULL  # Decline to handle
       )
     }
   )

@lionel-
Copy link
Member

lionel- commented Aug 28, 2024

Worth noting that having the evaluate machinery as part of the backtrace is consistent with the current CRAN behaviour.

Copy link
Member

@lionel- lionel- left a comment

Choose a reason for hiding this comment

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

@hadley hadley closed this Sep 5, 2024
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.

Dev evaluate gives bad traceback
2 participants