-
Notifications
You must be signed in to change notification settings - Fork 82
Debugger Restart Frame #2582
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
Debugger Restart Frame #2582
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2582 +/- ##
=======================================
- Coverage 46% 46% -1%
- Complexity 6661 6665 +4
=======================================
Files 794 795 +1
Lines 65746 65800 +54
Branches 9852 9859 +7
=======================================
+ Hits 30634 30641 +7
- Misses 32746 32795 +49
+ Partials 2366 2364 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
DavyLandman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks mostly fine, I have one place where I want to ask @jurgenvinju to take a look once he's back.
| // Handle frame restart at this level | ||
| // Restore environment state and retry execution | ||
| // Continue the while loop to retry the call | ||
| if(ctx instanceof Evaluator) { | ||
| Evaluator evaluator = (Evaluator) ctx; | ||
| var stackTrace = evaluator.getCurrentStack(); | ||
| if(stackTrace.size() == 1){ // should not happen because this is the REPL frame | ||
| throw new ImplementationError("Cannot restart frame of the REPL"); | ||
| } | ||
| if (rfe.getTargetFrameId() == stackTrace.size() -1) { // the frame to restart | ||
| restartFrame = true; | ||
| continue; // restart the frame by continuing the while loop | ||
| } | ||
| else { // not the frame to restart, rethrow to let upper frames handle it | ||
| throw rfe; | ||
| } | ||
| } | ||
| throw new ImplementationError("Frame restart is only supported in Evaluator contexts"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is something for @jurgenvinju to review.
|
With the exception breakpoint PR merged, I adapted the code so that it's now possible to restart a frame when hitting a breakpoint exception : restartonexception.mp4 |
|
@jurgenvinju can you do a final review? |
| } | ||
|
|
||
| @Override | ||
| public Result<IValue> call(Type[] actualStaticTypes, IValue[] actuals, Map<String, IValue> keyArgValues) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this wrap of the whole function body in a while is a bit extreme, the args & types shouldn't change, so is there no way to make the while loop much closer to the actual body eval of this function?
|
@jurgenvinju suggested moving the while loop inside the runBody function. I thought this would work (and it would have reduced code inside the while loop) but in fact this is not the case.
Another side effect is that moving code inside runBody make it impossible to restart frame on exception breakpoint (this is not a main feature, but it's useful). |
|
@jbdoderlein thanks for explaining. Then let's go with your original solution. |
Add restart frame support to the DAP.
Demo :
restartframe.mp4