-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
service/dap: Add error handlers for unsupported and not-yet-supported requests #1918
Merged
Merged
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
f3ebcb0
Add no-op handlers for unsupported requests
polinasok 1211e48
Fix whitespace
polinasok 2fae6fd
Add couple of missing unsupported requests
polinasok 6e13cf5
More comments
polinasok ad90d2c
Sync with master
polinasok 3b4a198
Separate errors for unsupported and not-yet-implemented requests.
polinasok e0d2e0e
Sync with master
polinasok bf001e5
Fix go.mod/sum dependencies
polinasok File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I'm curious about the UX implications of this. Presumably controls to send this requests still exist in VSCode's UI, if the adapter does a no-op then the user can select them and receive no feedback? I presume an error response is percolated to the user, is it not? How is no feedback better than seeing an error?
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 PR only focuses on the requests that have corresponding capabilities flags set or unset/absent at the initialization (see 'supportsFoo' references in the comments for each handler).
From the spec, "the protocol is still at its first version because it was an explicit design goal to support new feature in a completely backward compatible way. Making this possible without version numbers requires that every new feature gets a corresponding flag that lets a development tool know whether a debug adapter supports the feature or not. The absence of the flag always means that the feature is not supported."
I think the idea is that based on these the editor will know when to disable the UI, avoid issuing unsupported requests, etc. That said I did find one example where this was not quite working as intended, but the vscode side was very responsive in fixing this when I flagged it (see microsoft/vscode#90001). But I figured it would be best to just maintain feature parity with the existing implementation and just do what they are doing - make these no-ops. For now, I left out the couple of requests that are also no-ops in vscode-go, but do not have corresponding capability flags to let the tool know explicitly not to rely on them. See also my questions at microsoft/debug-adapter-protocol#99
Also, when I asked my vscode contacts how to decide when a no-op vs an error response is more appropriate, they said "if it is an error that the user should see, then return an ErrorResponse, if it is not an error that the user should see, then don't return an ErrorResponse", which matches your assumption.
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.
It's a bit weird that we care whether or not they return an error if they are disabled by a feature flag, but alright. I think however we should at least put a TODO comment on the ones we want to implement in the future, as opposed to ones that just don't make sense for us (like RestartFrame)
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.
I agree that it is a bit odd. I am not completely happy with this change myself. I can take the handlers out and trust the development tools not to issue these requests and if they do, just fallback on the default unsupported errors that I had before, maybe differentiating between truly unsupported and not yet supported ones.
Which ones do you think deserve a TODO for future implementation? Here is the full list.
TerminateRequest
RestartRequest <== ?
SetFunctionBreakpointsRequest
StepBackRequest
RestartFrameRequest
SetExpressionRequest <==?
TerminateThreadsRequest
StepInTargetsRequest
GotoTargetsRequest
CompletionsRequest
ExceptionInfoRequest
LoadedSourcesRequest
SetDataBreakpointsRequest
ReadMemoryRequest
DisassembleRequest
CancelRequest
BreakpointLocationsRequest
ModulesRequest
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.
Based on a quick look at the spec I think these can be implemented:
TerminateRequest
RestartRequest
SetFunctionBreakpointsRequest
StepBackRequest
SetExpressionRequest
LoadedSourcesRequest
ReadMemoryRequest
DisassembleRequest
and these can not:
RestartFrameRequest
TerminateThreadsRequest
StepInTargetsRequest
GotoTargetsRequest
CompletionsRequest
SetDataBreakpointsRequest
BreakpointLocationsRequest
I'm unsure about:
ExceptionInfoRequest
CancelRequest
ModulesRequest
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.
I am updating the PR based on this discussion. Please stay tuned. In the meantime, you listed StepBack as a request to support. It comes in a pair with ReverseContinue. I see "rev" and "rewind", so this one can also be supported, right?
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.
Yes, that sounds like it could be implemented as well.