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

DAP specification should clarify which requests are mandatory #99

Closed
polinasok opened this issue Mar 9, 2020 · 2 comments
Closed

DAP specification should clarify which requests are mandatory #99

polinasok opened this issue Mar 9, 2020 · 2 comments
Assignees
Labels
clarification Protocol clarification
Milestone

Comments

@polinasok
Copy link
Contributor

polinasok commented Mar 9, 2020

Up until recently setExceptionBreakpoints had to be supported regardless of the specifics of the language/debugger because even if exceptionBreakpointFilters were empty, this request was still issued on start-up. The documentation had conflicting info on the subject (although microsoft/vscode#90001 addressed this after I flagged the discrepancy, thanks!).

When investigating why my work-in-progress debug adaptor was getting stuck in the middle of a debug session, I have also realized that the threads request is also mandatory and must return at least one dummy thread even if thread information is unavailable/unsupported. See microsoft/vscode-go#2126 (comment). This last bit is referenced here and here, but not in the spec.

I think it is important to have a single comprehensive source of truth for the implementors of the new adaptors to reference on a request by request basis. The spec appears to be the most logical place one would check during incremental implementation work. Knowing in what order to implement the requests to have a minimal viable version up and running quickly would be of great help, especially if the new adaptor does not inherit from npm module "vscode-debugadapter" with its default no-op implementations of the requests.

Please update the spec to clarify which requests are mandatory and must be implemented with empty no-op or non-empty dummy responses. For the responses that are communicated as unsupported via initialize capabilities, is it still recommended to implement all of the corresponding requests like vscode-debugadapter does? What about any other unsupported requests? There appear to be some requests that do not have any corresponding capability flags (e.g. source) with either the exact name (like supportsRestartRequest for restartRequest) or mentioned in the spec comments (like supportsStepBack for reverseContinue). How would the client know not to issue them or enable them in the UI?

@weinand
Copy link
Contributor

weinand commented Mar 19, 2020

In order to be forward and backward compatible, the debug adapter protocol does not use version numbers.

After an initial release of the protocol all changes were made in a compatible way. So newly introduced requests are always optional and have a corresponding "supportsXxxx" capability. If there is no "supportsXxxx" capability, then the request is mandatory.

I will use this feature request to make sure that every protocol request clearly spells out whether it is mandatory or optional.

@weinand weinand self-assigned this Mar 19, 2020
@weinand weinand added the clarification Protocol clarification label Mar 19, 2020
@weinand weinand added this to the March 2020 milestone Mar 19, 2020
@polinasok
Copy link
Contributor Author

This is very helpful. Thank you so much for making the change!

Couple of additional suggestions:

(1) Add a summary explanation to the spec intro comment and/or to the overview page (which already has some info on capabilities, but could use additional details) covering the following points:

  • Requests from the initial release are mandatory
  • If not supported, mandatory requests must be implemented as ??? (no-op, dummy, error?)
  • Requests added after the initial release are optional and added with "supportsXxxx" capability
  • If not supported, optional requests can be skipped or should not be skipped, but instead implemented as ??? (no-op, error?)

(2) For each mandatory request, add an explicit "the foo request is mandatory and must be implemented with an empty/dummy/error implementation if the adaptor does not support it". This will make it explicit and hard to miss. I will be trivial to search for the word "mandatory" to identify such requests in the spec, so the implementors can always start with those.

Thank you!

@vscodebot vscodebot bot locked and limited conversation to collaborators May 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
clarification Protocol clarification
Projects
None yet
Development

No branches or pull requests

2 participants