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

bugfix: debug-completion depends on order of stackTrace being requested #6037

Merged

Conversation

mekpavit
Copy link
Contributor

Some DAP client, when receiving Stoppage event, will get a list of all threads and iterate through that list to get each thread's stack-trace eagerly. One example of this is nvim-dap-ui.

Since metals maintains only the latest stack-trace response to use for frame-searching during debug-completion, debug-completion won't work if the frame-id in debug-completion request is not in the latest stack-trace response.

IMO, the behavior of debug-completion shouldn't depends on the order of stack-trace requests. And ideally, DAP server shouldn't even depends on the stack-trace being requested at all for debug-completion to work. However, since metals is currently just a proxy for scala-debug-adapter, I think the latter requirement can be relaxed.

So, this PR is trying to satisfy the first requirement where debug-completion will work regardless of the order of stack-trace requests. If DAP client, at least once, requests for the stack-trace, the debug-completion for any frame in that stack-trace will work.

@mekpavit mekpavit force-pushed the bugfix/debug-completion-multiple-threads branch from f6b8227 to d815927 Compare January 20, 2024 07:55
@mekpavit mekpavit changed the title bugfix: debug-completion depends on stackTrace being request bugfix: debug-completion depends on order of stackTrace being requested Jan 20, 2024
@mekpavit mekpavit force-pushed the bugfix/debug-completion-multiple-threads branch from 54dfa6e to 29e8498 Compare January 20, 2024 08:11
@mekpavit mekpavit force-pushed the bugfix/debug-completion-multiple-threads branch from 29e8498 to 3d47aad Compare January 20, 2024 09:27
Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM

@tgodzik tgodzik merged commit 860b11e into scalameta:main Jan 20, 2024
26 checks passed
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

2 participants