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

gopls/vulncheck: use result from gopls.vulncheck, rather than gopls.fetch_vulncheck_result #3572

Open
findleyr opened this issue Oct 21, 2024 · 5 comments
Assignees
Milestone

Comments

@findleyr
Copy link
Contributor

findleyr commented Oct 21, 2024

In https://go.dev/cl/621055, I added support async request handling to gopls, using it to make long running command handlers run asynchronous to other LSP requests. This means that we no longer need our hacky gopls.fetch_vulncheck_result command to collect vulncheck results: the client can simply await the command result.

I didn't remove the gopls.fetch_vulncheck_result command. However, there IS an observable change in behavior for vulncheck integration in gopls: previously the gopls.run_vulncheck command would return quickly. Now it does not.

Therefore, we need to:

  1. Check that this blocking of the workspace/executeCommand request doesn't cause problems for VS Code.
  2. Eventually stop using gopls.fetch_vulncheck_result in VS Code.
@findleyr findleyr added this to the v0.44.0 milestone Oct 21, 2024
@findleyr
Copy link
Contributor Author

Decisions, from our meeting:

  1. We'll revert gopls.run_vulncheck to its old behavior, since the new behavior breaks special handling to stream vulncheck results to a terminal.
  2. We'll add a new gopls.vulncheck command that implements the new behavior. So the old behavior uses two commands (gopls.run_vulncheck and gopls.fetch_vulncheck_result), and the new behavior has just one (gopls.vulncheck).
  3. We'll generalize the special handling that displays vulncheck results in a terminal output. Specifically, we'll document special metadata that gopls may embed in a header of the workDoneProgressBegin message.

In pseudocode:

workDoneProgressBegin{
  title: "⚡ govulncheck -C path/to/dir",
  message: "style: log\n\nStarting govulncheck...",
}

In this case, the header has a single piece of metadata: style: log, which signals to clients that the preferred output style for this progress notification is a log (=append) style. Of course, most clients will not have any special handling, but the default UX will be somewhat reasonable: the progress notification will show the most recent log message. Previously, we had discussed style: terminal, but @adonovan points out that this may make it unclear whether the output should support ANSI escape codes.

CC @hyangah @h9jiang @adonovan

@adonovan
Copy link
Member

FWIW this is the upstream issue for the LSP protocol change that would make this unnecessary:

(Which will happen first: this proposal getting approved, or the sun exhausting its nonmetallic fuel? Only time will tell!)

@hyangah
Copy link
Contributor

hyangah commented Nov 12, 2024

I don't know what is the current status of gopls.
Our CI started to be failing with vulncheck unit tests and I guess it's related to gopls v0.17.0-pre.1 release.
https://go-review.git.corp.google.com/c/vscode-go/+/627276

@findleyr
Copy link
Contributor Author

Shoot, I forgot to fix this before the -pre.1 release. I'll fix now, and we can cut -pre.2.

@gopherbot
Copy link
Collaborator

Change https://go.dev/cl/627556 mentions this issue: gopls/internal/server: revert the gopls.run_govulncheck command

gopherbot pushed a commit to golang/tools that referenced this issue Nov 13, 2024
As described in golang/vscode-go#3572 this CL reverts the behavior of
the gopls.run_govulncheck command to be asynchronous. Instead, we
introduce a new gopls.vulncheck command to run synchronously. We also
introduce a new "vulncheck" codelens setting to control the availability
of codelenses for this new command.

For expedience, the command handler is simply copied rather than
refactored, and minimal tests are added/modified to test the new
command. Hopefully we can migrate everything to the new command soon,
and delete the old command.

For golang/vscode-go#3572

Change-Id: Ib3cffd5fd038813680087fa1916127663f377581
Reviewed-on: https://go-review.googlesource.com/c/tools/+/627556
Reviewed-by: Alan Donovan <[email protected]>
Auto-Submit: Robert Findley <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
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

No branches or pull requests

5 participants