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

service/dap: validate the client configurations in initialize request #2435

Merged
merged 7 commits into from
May 6, 2021

Conversation

suzmue
Copy link
Contributor

@suzmue suzmue commented Apr 13, 2021

The client can specify certain configurations in the initialize request.
For example, pathFormat determines the pathFormat. We do not currently
support configuring pathFormat, linesStartAt1, or columnsStartAt1, so
we report an error if the client attempts to set these to an
unsupported value.

The client can specify certain configurations in the initialize request.
For example, pathFormat determines the pathFormat. We do not currently
support configuring pathFormat, linesStartAt1, or columnsStartAt1, so
we report an error if the client attempts to set these to an
unsupported value.
@suzmue
Copy link
Contributor Author

suzmue commented Apr 13, 2021

cc @hyangah @polinasok

service/dap/server.go Outdated Show resolved Hide resolved
service/dap/server_test.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@polinasok polinasok left a comment

Choose a reason for hiding this comment

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

I have conflicting thoughts about this. What prompted this change? Unfortunately, the DAP spec doesn't make it clear if we should be responding with an error or just ignoring the arguments we do not support: https://microsoft.github.io/debug-adapter-protocol/specification. The editor itself, for example, just ignores quietly some of the things that the adapter supports, but the editor does not. I do see a mention here https://microsoft.github.io/debug-adapter-protocol/overview that "A debug adapter is expected to return error messages that honor this locale."

service/dap/server_test.go Outdated Show resolved Hide resolved
@suzmue
Copy link
Contributor Author

suzmue commented Apr 20, 2021

What prompted this change?

This change was prompted by one of our tests in the vscode-go extension, which tests that we expect an error response when provided with an invalid 'pathFormat'. The vscode-debugadapter checks that pathFormat = 'path' and returns an errror if it is something else.

I think this is a bit different from just ignoring something that we do not support, because if someone sets "pathFormat" to "url", they are saying that they plan to send urls instead of filepaths. Or if they set LinesStartAt1=false, then every line number they send to us and we send to them will be off by 1. This would result in a very broken experience for the user, that may not be obvious at first (for example when it comes to line numbers).

service/dap/server_test.go Outdated Show resolved Hide resolved
@suzmue
Copy link
Contributor Author

suzmue commented May 5, 2021

Just rebased this change.

@aarzilli @derekparker I don't think there is anything left to do on this could you please review? Thanks!

Copy link
Member

@aarzilli aarzilli left a comment

Choose a reason for hiding this comment

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

LGTM

@aarzilli aarzilli merged commit 49555a9 into go-delve:master May 6, 2021
@suzmue suzmue deleted the pathformat branch May 6, 2021 17:26
polinasok pushed a commit to polinasok/delve that referenced this pull request May 7, 2021
…go-delve#2435)

The client can specify certain configurations in the initialize request.
For example, pathFormat determines the pathFormat. We do not currently
support configuring pathFormat, linesStartAt1, or columnsStartAt1, so
we report an error if the client attempts to set these to an
unsupported value.
derekparker pushed a commit that referenced this pull request May 17, 2021
* service/dap: support pause request

* service/dap: validate the client configurations in initialize request (#2435)

The client can specify certain configurations in the initialize request.
For example, pathFormat determines the pathFormat. We do not currently
support configuring pathFormat, linesStartAt1, or columnsStartAt1, so
we report an error if the client attempts to set these to an
unsupported value.

* TeamCity: fix Windows builds (#2467)

Bintray is shutting down and the URL we used to install mingw is no
longer available. Use chocolatey instead.

* proc/native: low level support for watchpoints in linux/amd64 (#2301)

Adds the low-level support for watchpoints (aka data breakpoints) to
the native linux/amd64 backend.

Does not add user interface or functioning support for watchpoints
on stack variables.

Updates #279

* simplify pause test

Co-authored-by: Polina Sokolova <polinasok@users.noreply.github.com>
Co-authored-by: Suzy Mueller <suzmue@golang.org>
Co-authored-by: Alessandro Arzilli <alessandro.arzilli@gmail.com>
suzmue added a commit to suzmue/delve that referenced this pull request Jun 4, 2021
…go-delve#2435)

The client can specify certain configurations in the initialize request.
For example, pathFormat determines the pathFormat. We do not currently
support configuring pathFormat, linesStartAt1, or columnsStartAt1, so
we report an error if the client attempts to set these to an
unsupported value.
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.

None yet

4 participants