Skip to content

Added delve 'call' support #101

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

Added delve 'call' support #101

wants to merge 4 commits into from

Conversation

ekulabuhov
Copy link
Contributor

@ekulabuhov ekulabuhov commented May 27, 2020 •
edited by hyangah
Loading

Updates #100

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added the cla: no Used by googlebot to label PRs as having an invalid CLA. The text of this label should not change. label May 27, 2020
@ekulabuhov ekulabuhov changed the title 2655 Added delve 'call' support Added delve 'call' support May 27, 2020
@hyangah
Copy link
Contributor

hyangah commented Jun 26, 2020

@ekulabuhov - Thank you for the PR! I am so excited about this feature.

Can you please sign the CLA so we can start the review process?
Here is the instruction for contribution (how to write the PR/commit message, etc). https://github.com/golang/vscode-go/blob/v0.15.0-rc.3/docs/contributing.md

@ekulabuhov
Copy link
Contributor Author

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change. and removed cla: no Used by googlebot to label PRs as having an invalid CLA. The text of this label should not change. labels Jun 27, 2020
@gopherbot
Copy link
Collaborator

This PR (HEAD: 27d736c) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/vscode-go/+/240200 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Collaborator

Message from Gobot Gobot:

Patch Set 1:

Congratulations on opening your first change. Thank you for your contribution!

Next steps:
A maintainer will review your change and provide feedback. See
https://golang.org/doc/contribute.html#review for more info and tips to get your
patch through code review.

Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.

Please don’t reply on this GitHub thread. Visit golang.org/cl/240200.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Collaborator

This PR (HEAD: 49b2904) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/vscode-go/+/240200 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Collaborator

This PR (HEAD: 261bea2) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/vscode-go/+/240200 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Collaborator

This PR (HEAD: c321337) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/vscode-go/+/240200 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Collaborator

Message from Hyang-Ah Hana Kim:

Patch Set 4:

(2 comments)

Eugene, thanks for the contribution!

Eli, can you please take a look?

Please don’t reply on this GitHub thread. Visit golang.org/cl/240200.
After addressing review feedback, remember to publish your drafts!

eliben
eliben requested changes Jun 30, 2020
View reviewed changes
Copy link
Member

@eliben eliben left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! A request for documentation first.

Also, a question: did you try this with both API versions for Delve?

src/debugAdapter/goDebug.ts
// #2326: Set the fully qualified name for variable mapping
variable.fullyQualifiedName = variable.name;
response.body = this.convertDebugVariableToProtocolVariable(variable);
const re = new RegExp(/\w+(?=\(.*\))/, 'g');
Copy link
Member

Choose a reason for hiding this comment

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

Please add a detailed comment here (or above the evaluateRequest method) explaining this contract: what exactly we're looking for in the eval request to interpret as a 'call', and how it's treated. A reader writing a test for this should be able to read the comment and understand how to craft test DAP messages that would trigger this behavior, what its limitations and corner cases are, etc.

@mojtabacazi
Copy link

@eliben @ekulabuhov Can we move this forward?

@hyangah
Copy link
Contributor

hyangah commented Aug 19, 2020

@ekulabuhov I created a cl based on your change.
https://go-review.googlesource.com/c/vscode-go/+/249377
I wanted to add you to the cl for review but gerrit doesn't seem to be able to find your account.

FYI Sorry that it would've been ideal if I could push the commit to this branch and the gerritbot could handle this without me creating a separate cl. I filed a feature request here for gerritbot improvement. golang/go#40906

gopherbot pushed a commit that referenced this pull request Sep 3, 2020
@hyangah @ekulabuhov
src/debugAdapter: add delve 'call' command support
5a7752c 
Delve supports function calls. Even though it is still
experimental and can be applied only to a limited set
of functions, this is a useful feature, many vscode-go
users long for.

Unlike other javascript/typescript debuggers, delve
treats function calls specially and requires different
call paths than usual expression evaluation. That is
because Go is a compiled, runtime-managed GC language,
calling a function safely from debugger is complex.
DAP and VS Code UI does not distinguish function calls
and other expression evaluation either, so we have to
implement this in the same `evaluateRequest` context.

We use a heuristic to guess which route (call or
expression evaluation) we need to take based on
evaluateRequest's request.

The set of expressions delve supports includes some of
the builtin function calls like `len`, `cap`. We also
expect some grammar changes due to the ongoing effort for
Go2 experiment. In order to cope with this uncertainty,
this change requires users to specify their intention
using the 'call' keyword. Any expression that starts with
'call' and contains function-call-like strings, will cause
the evaluateRequest to take the path for the `call` command.

And, DAP and VSCode assumes all expressions return only
one result, but in Go, function call can return multiple
values. In this CL, we work around this sementic difference
by wrapping all the return values in one wrapper result
that has all return values as children. Users can expand
each value.

While we are here, this CL also adds a logic to handle the
interface type value. Previously, it was captured by the
default case that does not show the underlying type and value.
Interface types behave differently depending on the underlying
data type and value (e.g. nil error https://golang.org/doc/faq#nil_error)
so showing the underlying type/value upfront helps debugging
much easier. In particular, the 'error' type is commonly used
as the last return value of a function call and the program
control flow changes depending on whether the error is nil.
So, here we surface the underlying type and, if the underlying
type is invalid and the addr is 0 (nil error), present the
result as 'nil <error>'. This allows users to recognize the 'nil'
error value without extra value expansion.

Any error with an underlying type will be presented as
`<error(someUnderlyingType)>` and require the value expansion.

I see we can further improve this display value handling and
try to follow delve's data presentation as much as possible but
that is a bigger change and is beyond the scope of this CL.

This is based on github.com//pull/101

Fixes #100

Co-authored-by: Eugene Kulabuhov <[email protected]>
Change-Id: I4e1e876ab1582b17fa2c7bf0b911f31ec348b484
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/249377
Run-TryBot: Hyang-Ah Hana Kim <[email protected]>
TryBot-Result: kokoro <[email protected]>
Reviewed-by: Polina Sokolova <[email protected]>
@hyangah
Copy link
Contributor

hyangah commented Sep 3, 2020

I just submitted https://go-review.googlesource.com/c/vscode-go/+/249377 that started based on @ekulabuhov's PR.
The feature will be available in the next release. Thanks!

@hyangah hyangah closed this Sep 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eliben eliben eliben requested changes

Assignees
No one assigned
Labels
cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ekulabuhov @googlebot @hyangah @gopherbot @mojtabacazi @eliben