VSCode: Function Names not Visible in the Call Stack View

[walles]

Originally reported as microsoft/vscode#178964 but redirected here.

1. What version of Delve are you using (dlv version)? 1.20.1
2. What version of Go are you using? (go version)? go1.20.2
3. What operating system and processor architecture are you using? macOS Big Sur 11.7.3, amd64
4. What did you do?
   1. Loaded the gopls project into VSCode
   2. Set a breakpoint
   3. Launched the TestLSP test
   4. Waited until the execution stopped at my breakpoint
5. What did you expect to see? A stack trace with function names in it, see video below
6. What did you see instead? A stack trace with all the function names out of sight, see video below

This video shows how wide I have to make the Call Stack View to see the relevant parts of the stack trace rows.

wide-or-unreadable-call-stack.mov

[walles]

Response from a VSCode maintainer:

Please file this on the repo for the Go extension. I think that the extension should either change what it is sending as the frame name (I'm not a Go expert so I'm not sure how to interpret what I see in your screenshot) or they can suggest a helpful way for us to truncate stack frame names in the UI that would preserve the most helpful info.

Not sure, but I assume this would refer to here...

[aarzilli]

cc @suzmue @hyangah

[suzmue]

We could reorganize the information to make sure that the critical information (including the function name), is going to appear in a reasonable distance from the start of the string.

We could do this by moving the qualified package name after the function name for long packages. I think for ease of reading, package names should be kept before the function name. A couple of examples for this idea are below:

Before:	net/http.Get
After:	http.Get (in net/http)

Before:	github.com/go-delve/delve/server/dap.TestEvaluateRequest
After:	dap.TestEvaluateRequest (in github.com/go-delve/delve/server/dap)

// Remove repetitive information when the package has only a single part.
Before:	runtime.gopark
After:	runtime.gopark

This would keep the names similarly long, but would prioritize having the function name appear before a potentially very long package path.

Before:

After:

The package path could also be dropped completely, though this does remove some information, and there is not a clear way in DAP to add this additional information to a stack frame (can't provide longer description to be displayed in hover), so I would be inclined to reorder for now.

Alternative:

Note: first stack frame should be dap.TestEvaluateRequest.func1

[hyangah]

Thanks @suzmue for exploring the options.

I noticed the DAP spec has StackFrameFormat which seems to allow clients to specify whether a module name should be included. Also StackFrame has a field called moduleId. I think Go's package name can map to DAP's module. Do you know how they influence VS Code's UI? (A quick code search in vscode repo doesn't show a reference to moduleId so it's possible that vscode doesn't utilize it yet).

VS Code shows a tooltip when hovering over a stack frame. I wonder if we can ask vscode team to present a long name in the tooltip while showing a short name in the stackframe ui.

[suzmue]

Related issue in vscode-go issue tracker golang/vscode-go#2707

[suzmue]

I noticed the DAP spec has StackFrameFormat which seems to allow clients to specify whether a module name should be included. Also StackFrame has a field called moduleId. I think Go's package name can map to DAP's module. Do you know how they influence VS Code's UI? (A quick code search in vscode repo doesn't show a reference to moduleId so it's possible that vscode doesn't utilize it yet).

This feature is not yet available in VS Code AFAICT (see microsoft/vscode#148125 and microsoft/vscode#28025 (comment)).

The workaround recommended for the variable use case is to add a context menu item that could trigger a custom request and then use an invalidate event to refresh the view (see microsoft/vscode#28025 (comment)).

[stefanhaller]

I experimented with the simple approach of reordering function name and package name to see how hard it would be, and it seems that this little patch does it:

diff --git a/service/dap/server.go b/service/dap/server.go
index 5bb38f1b..f129c6f3 100644
--- a/service/dap/server.go
+++ b/service/dap/server.go
@@ -1684,7 +1684,11 @@ func fnName(loc *proc.Location) string {
 	if loc.Fn == nil {
 		return "???"
 	}
-	return loc.Fn.Name
+	name := loc.Fn.Name
+	if idx := strings.LastIndex(name, "/"); idx != -1 {
+		return fmt.Sprintf("%s (in %s)", name[idx+1:], name[:idx])
+	}
+	return name
 }
 
 func fnPackageName(loc *proc.Location) string {

I wonder if I'm missing something (I'm still kind of new to go), but if this is how it would be done, then I'd appreciate some guidance what's missing to turn this into a PR. In particular, I have no idea what to do to server_test.go to test this.

However, I also have to say that I find the output still too noisy with the added package paths, and personally I would prefer to omit them, i.e.

diff --git a/service/dap/server.go b/service/dap/server.go
index 5bb38f1b..97c5abf6 100644
--- a/service/dap/server.go
+++ b/service/dap/server.go
@@ -1684,7 +1684,11 @@ func fnName(loc *proc.Location) string {
 	if loc.Fn == nil {
 		return "???"
 	}
-	return loc.Fn.Name
+	name := loc.Fn.Name
+	if idx := strings.LastIndex(name, "/"); idx != -1 {
+		return name[idx+1:]
+	}
+	return name
 }
 
 func fnPackageName(loc *proc.Location) string {

I wonder if this would need a config option.

Either way, this is such a dramatic improvement to usability that I'd like to make some progress on this.

[stefanhaller]

I went ahead and made a PR (#3497), including the option to strip the paths entirely. First-time contribution, please be gentle! 😉

[hyangah]

Thanks @stefanhaller

I hoped to surface the package path info using a different UI element (microsoft/vscode#193153) instead of introducing another setting or configuration param, but it sounds like we need to go the whole nine yards in both VS Code and Delve :-(

The more I think, the more I like the idea of hiding the package path from the stack frame.
If hover over the file location info, the tooltip shows the full path of the file name. Often times, users can infer the package paths from the file location info.

When VS Code and other DAP clients are ready, we can adopt their new feature to supply the package path.

Is there any case where showing the full package path is preferable?
As we've seen in the demo screenshots, using long strings as stackframe names is more likely to hide the file location info part. That is not ideal.

[stefanhaller]

I'd be more than happy to strip the package path unconditionally; I only provided the hidePackagePaths option in the PR because I was afraid it wouldn't have a chance of being accepted without.

Do you think we can do this already now, even when we can't provide the tooltip yet? I'd very much hope so, it's a dramatic usability improvement, as I said already. I have been using a locally patched dlv for a week or so now, the difference is night and day.

[hyangah]

I agree that shorter frame name is much better!
IMHO I think we should strip the package path from name right now.
There is an alternative way to infer the package info (file location) when a user really needs to know it.

If my proposal to extend the DAP microsoft/debug-adapter-protocol#433 is accepted, we can also add the full name as a longer description. Implementation-wise I think that's relatively easy.

[stefanhaller]

Great. Instead of changing the existing PR, I decided to open a new one: #3500. The implementation is much simpler, of course.