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

debug: surface StackFrame moduleId in the debug CALL STACK UI #193153

Closed
hyangah opened this issue Sep 14, 2023 · 5 comments
Closed

debug: surface StackFrame moduleId in the debug CALL STACK UI #193153

hyangah opened this issue Sep 14, 2023 · 5 comments
Assignees
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality
Milestone

Comments

@hyangah
Copy link

hyangah commented Sep 14, 2023

Go debug adapter (delve) has been using the fully qualified go function names as the stack frame names.
These names include the full package path (e.g. example.com/foo/bar) and the function name (e.g. pkg.ACoolFuncName), and many users requested to shorten the names presented in the CALL STACK view. For example, #178964, golang/vscode-go#2707.

One way to handle this is to shorten the name by dropping the full package path.
For example, our contributor is proposing to control this by adding a new launch config setting.
go-delve/delve#3497 I think it makes sense to show the function name,
but I wonder if we can still carry the package info ("module" in other languages).

StackFrame has the moduleId field.
The current VS Code doesn't seem to utilize this field at all.

How about utilizing the moduleId field and surfacing it in the UI?

I think the tooltip shown when hovering over the stackframe name is a good option.

Screenshot with a small change in callStackView.ts and a delve-side change:

Screenshot 2023-09-14 at 6 36 33 PM

One concern is it will be surprising for other language debug adapters that use the moduleId field already for other purposes.

cc @suzmue @stefanhaller @aarzilli

@roblourens roblourens added feature-request Request for new features or functionality debug Debug viewlet, configurations, breakpoints, adapter issues labels Sep 15, 2023
@roblourens roblourens modified the milestones: September 2023, Backlog Sep 15, 2023
@roblourens
Copy link
Member

Your proposal seems fine to me! I would take that PR

@roblourens
Copy link
Member

Wait a sec, moduleId is a number, so I think it won't be a string here and you need to resolve this to a Module with that id, right? main...hyangah:vscode:debugstackframe#diff-4ae236e16b22d9f993abf02e13ede86c77b7dd429159cb0be03c3c449335d54eR592

@hyangah
Copy link
Author

hyangah commented Sep 15, 2023

moduleId is number | string.

Even though the StackFrame spec is not explicitly stating the meaning of moduleId, I think you are right. moduleId seems to be Module's id and the client has to use ModulesRequest to retrieve the module information. And client has to use ModulesRequest only if the debug adapter declared supportsModulesRequest=true.

Unfortunately, VS Code does not seem to use the Modules Request feature yet. (Correct me if not true) Not many debug adapters implement that either.

llvm/llvm-project's lldb-vscode (module id is a uuid) and microsoft/debugpy dap (unique number) are one of the few that has supportsModulesRequest=true. For them, showing the moduleId value wouldn't be ideal.

Alternative proposals:

  • If the debug adapter does not support ModulesRequest but the moduleId is not empty, include the value in the tooltip. - exploiting unspecified part of DAP. When VS Code eventually implements and can issue ModulesRequest, it may choose to show the resolved module name. This way, we won't break the existing debug adapters, and we won't break the future VS Code that implements ModulesRequest.

  • Promote Support DAP's ValueFormat for formatting values in variables list #148125 and wait for VS Code to provide StackFrameFormat in StackTrace requests. (However, it's still unclear to me what DA is supposed to do when StackFrameFormat has module=true).

I also tried to format StackFrame's name in a way that can fit better in a narrow debug callstack panel, by placing the module path at the end. But that hides the source location info. That's not great.
Screenshot 2023-09-14 at 10 09 00 PM

@roblourens
Copy link
Member

Oh, I misread it. To me it's clear that this is meant to refer to the id of Module and I wouldn't try to hack around that to display the value directly. I would like to close some of the DAP implementation gaps in vscode, so #148125 and supporting ModuleRequest would be the best fix in the long run, but those won't be added soon. The DebugAdapterTracker possibility mentioned in that other issue would be a good one for you too. I'm going to close this in favor of the other issue.

@hyangah
Copy link
Author

hyangah commented Sep 15, 2023

Thanks. Agree that closing the gap is the best.
However I don't understand how the DebugAdapterTracker solution could be a workaround for this tbh (note that the workaround was suggested in the context of the value presentation, not the stack frame name presentation) - we want to present both function name and module name in a user friendly way. Concatenating all and supplying a long string as a stack frame doesn't produce good outcome. Can you please help us understand how the tracker can help control what to present on tooltip?

And, about the meaning of moduleId in the StackFrame, I couldn't find any part that limits the use of moduleId only when supportsModulesRequest is true yet. Can we clarify the spec by saying when moduleId is set but ModulesRequest is not supported, moduleId can be interpreted as the full module name?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality
Projects
None yet
Development

No branches or pull requests

2 participants