Skip to content

debug: support function calls via delve 'call' #100

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

Closed
ekulabuhov opened this issue May 27, 2020 · 21 comments
Closed

debug: support function calls via delve 'call' #100

ekulabuhov opened this issue May 27, 2020 · 21 comments
Labels
Debug Issues related to the debugging functionality of the extension. FrozenDueToAge
Milestone

Comments

@ekulabuhov
Copy link
Contributor

Following up from microsoft/vscode-go#2655 as it was the most upvoted feature there and is still badly needed.

Description

Delve has added the call command to be able to make function calls while in debug.

Trying to execute any function in VSCode Debug Console results in:
Unable to eval expression: "function calls not allowed without using 'call'"

There was a patch attached in the linked issue that added basic support for this feature. I can add a PR rebased on the latest version of the code to see if it's possible to continue from there.

@stamblerre
Copy link
Contributor

/cc @polinasok @quoctruong

@stamblerre stamblerre added the Debug Issues related to the debugging functionality of the extension. label May 27, 2020
@hyangah
Copy link
Contributor

hyangah commented May 27, 2020

Thanks for moving the issue and creating a PR!

Because DAP doesn't have a separate method for Delve's call equivalent, the PR currently proposes to reuse the evaluateRequest and distinguish between the usual expression and the call request using a regular expression as a heuristic.

My only concern is the regexp matches the simple builtin functions and type assertion delve's expressions supports. As delve's expressions gets powerful, the set of expression can get wider and the current heuristic may be an issue. I didn't check the current state of Go2 yet, but the syntax is not fully finalized, so that's another thing we need to think about.

I wonder if we can ask users to be more explicit, e.g. adding a call suffix?

And, @polinasok - this feature needs to implemented in the delve dap. What do you think makes sense for call support?

@stamblerre stamblerre changed the title Function calls via delve 'call' are not supported debug: support function calls via delve 'call' Jun 3, 2020
@hyangah
Copy link
Contributor

hyangah commented Jun 4, 2020

microsoft/vscode-go#2992 is about supporting other delve commands beyond the call command. Since VS Code UI will trigger the same evaluateRequest, I think other commands need to be handled in similar ways.

@hyangah hyangah added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. NeedsFix The path to resolution is known, but the work has not been done. labels Jun 24, 2020
@hyangah
Copy link
Contributor

hyangah commented Jun 26, 2020

Talked with @eliben and delve devs, we will focus on the call command support initially - some other delve cli commands do not make sense within evaluateRequest, so it needs more thought.

@hyangah hyangah added this to the v0.16.0 milestone Jul 7, 2020
@hyangah hyangah modified the milestones: v0.16.0, v0.17.0 Jul 30, 2020
@mojtabacazi
Copy link

@haryps Any progress on this? Can we help?

@gopherbot
Copy link
Collaborator

Change https://golang.org/cl/249377 mentions this issue: src/debugAdapter: add delve 'call' command support

@polinasok
Copy link
Contributor

polinasok commented Aug 25, 2020

@hyangah Delve command-line client uses "print" (or "p") for expressions and "call" for function calls. Those same expressions are entered without "print" in the DEBUG CONSOLE in vscode (which I personally keep forgetting, so every time I use "p ...", I wonder why I get an error, haha). As for "call", +1000 that it makes perfect sense to require "call" from the debug console as well instead of trying to implement a Go-friendly parser that would tell apart user-defined function calls from built-in function calls from type conversions.

You mentioned that "some other delve cli commands do not make sense within evaluateRequest" - which ones are those? Do you mean the entire cli (with all of its requests for stacktrace, variables, etc) or the expressions that are supported as part of the "print" cli command? As far as I can tell most of those expressions work the way they would in dlv, at least in their simplest form. I tried some basic examples for each category listed here and only ran into an issue with string-to-[]byte conversion (try using []byte("ABC€") in dlv and from the debug console - you get nil <[]uint8>).

I think the part that frustrates the users here is that when the result is a nested variable, it is printed the same way it would be in the Variables pane with the little expander that you can click to view more and more layers. Each expansion issues a new request, which could take a minute, so for a while you wonder if it is even working as it pauses before eventually opening up. And if you have to rinse and repeat for more and more elements, that can get pretty tedious. At the same time "print same_expression" using dlv cli returns the result with all the fields and values inlined and flattened. I believe this is what microsoft/vscode-go#2992 was getting at:

  • {expression} -- displays the nested expandable view like in the Variables pane
  • print {expression} -- displays the inlined flattened string representation the same way dlv cli client would
    This is not something that can be easily supported in the current adapter, but we would have much more power here with my work on adding native DAP support to delve. In fact, I am currently working on the EvaluateRequest support and has been wondering myself if it would make more sense to match what the DEBUG console returns to what the dlv client returns and not even bother with all the nesting and expanding.

@hyangah
Copy link
Contributor

hyangah commented Aug 25, 2020

You mentioned that "some other delve cli commands do not make sense within evaluateRequest" - which ones are those? Do you mean the entire cli (with all of its requests for stacktrace, variables, etc) or the expressions that are supported as part of the "print" cli command?

The former. We wouldn't want to support arbitrary commands other than those selected ones (e.g. call) coming through DEBUG CONSOLE.

I think the part that frustrates the users here is that when the result is a nested variable, it is printed the same way it would be in the Variables pane with the little expander that you can click to view more and more layers. Each expansion issues a new request, which could take a minute, so for a while you wonder if it is even working as it pauses before eventually opening up. And if you have to rinse and repeat for more and more elements, that can get pretty tedious.

This is beyond the scope of this issue (even though microsoft/vscode-go#2992 is linked to this). The nested value handling issue exists in the existing expression. I don't see a way to implement this while working with DAP (the DEBUG CONSOLE speaks DAP). The approach discussed in go-delve/delve#383 (that also motivated dlv's multiclient support) look more feasible than working around DEBUG CONSOLE and DAP's limitation further.

@polinasok
Copy link
Contributor

Yes, agreed, we can't channel arbitrary delve commands from the debug console prompt. It is solely for evaluation of Go expressions and not for textual interfacing with the debugger like you would when running dlv from the command-line. I was confused about that in my early days, trying to issue dlv commands from there. But "call" definitely makes sense as an expression evaluation use case.

@hyangah
Copy link
Contributor

hyangah commented Aug 26, 2020

image

Here is the screenshot from cl/249377.

func countEnv() (int, error)

func currentTime() (time.Time, error)

func os.Environ() []string

@polinasok
Copy link
Contributor

polinasok commented Aug 27, 2020

It is a bit odd to see r0, r1, etc repeated first as a multi-line string and then as members of the virtual struct, indented. This does save some clicks, but can also get visually messy, especially with more return values. This is also inconsistent with how structs and arrays are shown in the variables pane. Other ideas for the top-level string:

  • "2 values returned" (to match "Values returned:" with dlv)
  • first returned value "+1 more"

I would love to hear the thoughts of others here.

@hyangah
Copy link
Contributor

hyangah commented Aug 27, 2020

As I responded in the cl, this is an artifact of working around DAP/VS Code's limitation. This made up variable even doesn't correspond to Go's internal data representation, so comparing and trying to match its behavior with struct or array doesn't make sense.

@hyangah
Copy link
Contributor

hyangah commented Aug 27, 2020

BTW, the previous version that shows results as a comma-separated list doesn't look too bad, even with complex structs. Returning more than a handful number of values should be avoided, so I am leaning towards this previous version.

image

func messyFunction() ([]S, S, *S, time.Time, error) 

@polinasok
Copy link
Contributor

Sure, it's not a real type. A better analogy would be scopes, where the top-level string is short (Locals, Globals, Exceptions). If having the details in the top-level string is required, then I agree that the previous version looked better because everything was on the same line. For simple return lists, you would get all the info you need from it. For more complex/nested, you would need to expand anyway.

@polinasok
Copy link
Contributor

It occurred to me to check how other languages do it. Turns out Python uses the comma-separate list:
image

@hyangah
Copy link
Contributor

hyangah commented Aug 28, 2020

@polinasok That makes sense - internally it's a tuple in python.

@matti
Copy link

matti commented Sep 4, 2020 via email

@hyangah
Copy link
Contributor

hyangah commented Sep 4, 2020

@matti what failure are you seeing?

This issue is included in the v0.17.0 milestone and v0.17.0 will be released when all the issues in the milestone are closed.

The feature is available in the nightly version of this extension. https://github.com/golang/vscode-go/blob/master/docs/nightly.md has the instruction.

@yahavi
Copy link

yahavi commented Sep 15, 2020

@hyangah,
I'm still having this issue in the nightly version, v2020.9.1414.
image

@hyangah
Copy link
Contributor

hyangah commented Sep 15, 2020

@yahavi thanks for trying the nightly. As the error message indicates, try call config.IsSet("hello").
Due to the concern mentioned in #100 (comment), we require an explicit call command to indicate the intention.

@gerrywastaken
Copy link

Answer (as it didn't stand out to me in the above comment after sifting through a sea of other comments)

In the debug console or the watch view you must write call in front of your function call: e.g.

call yourFunctionName(arg1, arg2)

@polinasok polinasok added FixedInBothDAs and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. NeedsFix The path to resolution is known, but the work has not been done. labels Jul 16, 2021
@golang golang locked and limited conversation to collaborators Jul 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Debug Issues related to the debugging functionality of the extension. FrozenDueToAge
Projects
None yet
Development

No branches or pull requests

9 participants