gopls/internal/server: revert the gopls.run_govulncheck command

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]>

gopls/doc/codelenses.md

@@ -97,16 +97,15 @@ Default: off
 
 File type: Go
 
-## `run_govulncheck`: Run govulncheck
+## `run_govulncheck`: Run govulncheck (legacy)
 
 
-This codelens source annotates the `module` directive in a
-go.mod file with a command to run Govulncheck.
+This codelens source annotates the `module` directive in a go.mod file
+with a command to run Govulncheck asynchronously.
 
-[Govulncheck](https://go.dev/blog/vuln) is a static
-analysis tool that computes the set of functions reachable
-within your application, including dependencies;
-queries a database of known security vulnerabilities; and
+[Govulncheck](https://go.dev/blog/vuln) is a static analysis tool that
+computes the set of functions reachable within your application, including
+dependencies; queries a database of known security vulnerabilities; and
 reports any potential problems it finds.
 
 
@@ -157,4 +156,20 @@ Default: on
 
 File type: go.mod
 
+## `vulncheck`: Run govulncheck
+
+
+This codelens source annotates the `module` directive in a go.mod file
+with a command to run govulncheck synchronously.
+
+[Govulncheck](https://go.dev/blog/vuln) is a static analysis tool that
+computes the set of functions reachable within your application, including
+dependencies; queries a database of known security vulnerabilities; and
+reports any potential problems it finds.
+
+
+Default: off
+
+File type: go.mod
+
 <!-- END Lenses: DO NOT MANUALLY EDIT THIS SECTION -->

gopls/internal/doc/api.json

@@ -811,7 +811,7 @@
 						},
 						{
 							"Name": "\"run_govulncheck\"",
-							"Doc": "`\"run_govulncheck\"`: Run govulncheck\n\nThis codelens source annotates the `module` directive in a\ngo.mod file with a command to run Govulncheck.\n\n[Govulncheck](https://go.dev/blog/vuln) is a static\nanalysis tool that computes the set of functions reachable\nwithin your application, including dependencies;\nqueries a database of known security vulnerabilities; and\nreports any potential problems it finds.\n",
+							"Doc": "`\"run_govulncheck\"`: Run govulncheck (legacy)\n\nThis codelens source annotates the `module` directive in a go.mod file\nwith a command to run Govulncheck asynchronously.\n\n[Govulncheck](https://go.dev/blog/vuln) is a static analysis tool that\ncomputes the set of functions reachable within your application, including\ndependencies; queries a database of known security vulnerabilities; and\nreports any potential problems it finds.\n",
 							"Default": "false"
 						},
 						{
@@ -833,6 +833,11 @@
 							"Name": "\"vendor\"",
 							"Doc": "`\"vendor\"`: Update vendor directory\n\nThis codelens source annotates the `module` directive in a\ngo.mod file with a command to run [`go mod\nvendor`](https://go.dev/ref/mod#go-mod-vendor), which\ncreates or updates the directory named `vendor` in the\nmodule root so that it contains an up-to-date copy of all\nnecessary package dependencies.\n",
 							"Default": "true"
+						},
+						{
+							"Name": "\"vulncheck\"",
+							"Doc": "`\"vulncheck\"`: Run govulncheck\n\nThis codelens source annotates the `module` directive in a go.mod file\nwith a command to run govulncheck synchronously.\n\n[Govulncheck](https://go.dev/blog/vuln) is a static analysis tool that\ncomputes the set of functions reachable within your application, including\ndependencies; queries a database of known security vulnerabilities; and\nreports any potential problems it finds.\n",
+							"Default": "false"
 						}
 					]
 				},
@@ -953,8 +958,8 @@
 		{
 			"FileType": "go.mod",
 			"Lens": "run_govulncheck",
-			"Title": "Run govulncheck",
-			"Doc": "\nThis codelens source annotates the `module` directive in a\ngo.mod file with a command to run Govulncheck.\n\n[Govulncheck](https://go.dev/blog/vuln) is a static\nanalysis tool that computes the set of functions reachable\nwithin your application, including dependencies;\nqueries a database of known security vulnerabilities; and\nreports any potential problems it finds.\n",
+			"Title": "Run govulncheck (legacy)",
+			"Doc": "\nThis codelens source annotates the `module` directive in a go.mod file\nwith a command to run Govulncheck asynchronously.\n\n[Govulncheck](https://go.dev/blog/vuln) is a static analysis tool that\ncomputes the set of functions reachable within your application, including\ndependencies; queries a database of known security vulnerabilities; and\nreports any potential problems it finds.\n",
 			"Default": false
 		},
 		{
@@ -977,6 +982,13 @@
 			"Title": "Update vendor directory",
 			"Doc": "\nThis codelens source annotates the `module` directive in a\ngo.mod file with a command to run [`go mod\nvendor`](https://go.dev/ref/mod#go-mod-vendor), which\ncreates or updates the directory named `vendor` in the\nmodule root so that it contains an up-to-date copy of all\nnecessary package dependencies.\n",
 			"Default": true
+		},
+		{
+			"FileType": "go.mod",
+			"Lens": "vulncheck",
+			"Title": "Run govulncheck",
+			"Doc": "\nThis codelens source annotates the `module` directive in a go.mod file\nwith a command to run govulncheck synchronously.\n\n[Govulncheck](https://go.dev/blog/vuln) is a static analysis tool that\ncomputes the set of functions reachable within your application, including\ndependencies; queries a database of known security vulnerabilities; and\nreports any potential problems it finds.\n",
+			"Default": false
 		}
 	],
 	"Analyzers": [

gopls/internal/mod/code_lens.go

@@ -21,10 +21,11 @@ import (
 // CodeLensSources returns the sources of code lenses for go.mod files.
 func CodeLensSources() map[settings.CodeLensSource]cache.CodeLensSourceFunc {
 	return map[settings.CodeLensSource]cache.CodeLensSourceFunc{
-		settings.CodeLensUpgradeDependency: upgradeLenses,   // commands: CheckUpgrades, UpgradeDependency
-		settings.CodeLensTidy:              tidyLens,        // commands: Tidy
-		settings.CodeLensVendor:            vendorLens,      // commands: Vendor
-		settings.CodeLensRunGovulncheck:    vulncheckLenses, // commands: RunGovulncheck
+		settings.CodeLensUpgradeDependency: upgradeLenses,        // commands: CheckUpgrades, UpgradeDependency
+		settings.CodeLensTidy:              tidyLens,             // commands: Tidy
+		settings.CodeLensVendor:            vendorLens,           // commands: Vendor
+		settings.CodeLensVulncheck:         vulncheckLenses,      // commands: Vulncheck
+		settings.CodeLensRunGovulncheck:    runGovulncheckLenses, // commands: RunGovulncheck
 	}
 }
 
@@ -162,6 +163,29 @@ func vulncheckLenses(ctx context.Context, snapshot *cache.Snapshot, fh file.Hand
 		return nil, err
 	}
 
+	vulncheck := command.NewVulncheckCommand("Run govulncheck", command.VulncheckArgs{
+		URI:     uri,
+		Pattern: "./...",
+	})
+	return []protocol.CodeLens{
+		{Range: rng, Command: vulncheck},
+	}, nil
+}
+
+func runGovulncheckLenses(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle) ([]protocol.CodeLens, error) {
+	pm, err := snapshot.ParseMod(ctx, fh)
+	if err != nil || pm.File == nil {
+		return nil, err
+	}
+	// Place the codelenses near the module statement.
+	// A module may not have the require block,
+	// but vulnerabilities can exist in standard libraries.
+	uri := fh.URI()
+	rng, err := moduleStmtRange(fh, pm)
+	if err != nil {
+		return nil, err
+	}
+
 	vulncheck := command.NewRunGovulncheckCommand("Run govulncheck", command.VulncheckArgs{
 		URI:     uri,
 		Pattern: "./...",

gopls/internal/protocol/command/interface.go

@@ -186,16 +186,30 @@ type Interface interface {
 	// runner.
 	StopProfile(context.Context, StopProfileArgs) (StopProfileResult, error)
 
-	// RunGovulncheck: Run vulncheck
+	// GoVulncheck: run vulncheck synchronously.
 	//
 	// Run vulnerability check (`govulncheck`).
 	//
-	// This command is asynchronous; clients must wait for the 'end' progress notification.
+	// This command is synchronous, and returns the govulncheck result.
+	Vulncheck(context.Context, VulncheckArgs) (VulncheckResult, error)
+
+	// RunGovulncheck: Run vulncheck asynchronously.
+	//
+	// Run vulnerability check (`govulncheck`).
+	//
+	// This command is asynchronous; clients must wait for the 'end' progress
+	// notification and then retrieve results using gopls.fetch_vulncheck_result.
+	//
+	// Deprecated: clients should call gopls.vulncheck instead, which returns the
+	// actual vulncheck result.
 	RunGovulncheck(context.Context, VulncheckArgs) (RunVulncheckResult, error)
 
 	// FetchVulncheckResult: Get known vulncheck result
 	//
 	// Fetch the result of latest vulnerability check (`govulncheck`).
+	//
+	// Deprecated: clients should call gopls.vulncheck instead, which returns the
+	// actual vulncheck result.
 	FetchVulncheckResult(context.Context, URIArg) (map[protocol.DocumentURI]*vulncheck.Result, error)
 
 	// MemStats: Fetch memory statistics
@@ -506,13 +520,12 @@ type VulncheckArgs struct {
 type RunVulncheckResult struct {
 	// Token holds the progress token for LSP workDone reporting of the vulncheck
 	// invocation.
-	//
-	// Deprecated: previously, this was used as a signal to retrieve the result
-	// using gopls.fetch_vulncheck_result. Clients should ignore this field:
-	// gopls.vulncheck now runs synchronously, and returns a result in the Result
-	// field.
 	Token protocol.ProgressToken
+}
 
+// GovulncheckResult holds the result of synchronously running the vulncheck
+// command.
+type VulncheckResult struct {
 	// Result holds the result of running vulncheck.
 	Result *vulncheck.Result
 }

gopls/internal/server/command.go

@@ -11,6 +11,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"log"
 	"os"
 	"path/filepath"
 	"regexp"
@@ -392,6 +393,22 @@ func (c *commandHandler) run(ctx context.Context, cfg commandConfig, run command
 		return err
 	}
 
+	// For legacy reasons, gopls.run_govulncheck must run asynchronously.
+	// TODO(golang/vscode-go#3572): remove this (along with the
+	// gopls.run_govulncheck command entirely) once VS Code only uses the new
+	// gopls.vulncheck command.
+	if c.params.Command == "gopls.run_govulncheck" {
+		if cfg.progress == "" {
+			log.Fatalf("asynchronous command gopls.run_govulncheck does not enable progress reporting")
+		}
+		go func() {
+			if err := runcmd(); err != nil {
+				showMessage(ctx, c.s.client, protocol.Error, err.Error())
+			}
+		}()
+		return nil
+	}
+
 	return runcmd()
 }
 
@@ -1210,21 +1227,91 @@ func (c *commandHandler) FetchVulncheckResult(ctx context.Context, arg command.U
 
 const GoVulncheckCommandTitle = "govulncheck"
 
+func (c *commandHandler) Vulncheck(ctx context.Context, args command.VulncheckArgs) (command.VulncheckResult, error) {
+	if args.URI == "" {
+		return command.VulncheckResult{}, errors.New("VulncheckArgs is missing URI field")
+	}
+
+	var commandResult command.VulncheckResult
+	err := c.run(ctx, commandConfig{
+		progress:    GoVulncheckCommandTitle,
+		requireSave: true, // govulncheck cannot honor overlays
+		forURI:      args.URI,
+	}, func(ctx context.Context, deps commandDeps) error {
+		jsonrpc2.Async(ctx) // run this in parallel with other requests: vulncheck can be slow.
+
+		workDoneWriter := progress.NewWorkDoneWriter(ctx, deps.work)
+		dir := filepath.Dir(args.URI.Path())
+		pattern := args.Pattern
+
+		result, err := scan.RunGovulncheck(ctx, pattern, deps.snapshot, dir, workDoneWriter)
+		if err != nil {
+			return err
+		}
+		commandResult.Result = result
+
+		snapshot, release, err := c.s.session.InvalidateView(ctx, deps.snapshot.View(), cache.StateChange{
+			Vulns: map[protocol.DocumentURI]*vulncheck.Result{args.URI: result},
+		})
+		if err != nil {
+			return err
+		}
+		defer release()
+
+		// Diagnosing with the background context ensures new snapshots are fully
+		// diagnosed.
+		c.s.diagnoseSnapshot(snapshot.BackgroundContext(), snapshot, nil, 0)
+
+		affecting := make(map[string]bool, len(result.Entries))
+		for _, finding := range result.Findings {
+			if len(finding.Trace) > 1 { // at least 2 frames if callstack exists (vulnerability, entry)
+				affecting[finding.OSV] = true
+			}
+		}
+		if len(affecting) == 0 {
+			showMessage(ctx, c.s.client, protocol.Info, "No vulnerabilities found")
+			return nil
+		}
+		affectingOSVs := make([]string, 0, len(affecting))
+		for id := range affecting {
+			affectingOSVs = append(affectingOSVs, id)
+		}
+		sort.Strings(affectingOSVs)
+
+		showMessage(ctx, c.s.client, protocol.Warning, fmt.Sprintf("Found %v", strings.Join(affectingOSVs, ", ")))
+
+		return nil
+	})
+	if err != nil {
+		return command.VulncheckResult{}, err
+	}
+	return commandResult, nil
+}
+
+// RunGovulncheck is like Vulncheck (in fact, a copy), but is tweaked slightly
+// to run asynchronously rather than return a result.
+//
+// This logic was copied, rather than factored out, as this implementation is
+// slated for deletion.
+//
+// TODO(golang/vscode-go#3572)
 func (c *commandHandler) RunGovulncheck(ctx context.Context, args command.VulncheckArgs) (command.RunVulncheckResult, error) {
 	if args.URI == "" {
 		return command.RunVulncheckResult{}, errors.New("VulncheckArgs is missing URI field")
 	}
 
-	var commandResult command.RunVulncheckResult
+	// Return the workdone token so that clients can identify when this
+	// vulncheck invocation is complete.
+	//
+	// Since the run function executes asynchronously, we use a channel to
+	// synchronize the start of the run and return the token.
+	tokenChan := make(chan protocol.ProgressToken, 1)
 	err := c.run(ctx, commandConfig{
 		progress:    GoVulncheckCommandTitle,
 		requireSave: true, // govulncheck cannot honor overlays
 		forURI:      args.URI,
 	}, func(ctx context.Context, deps commandDeps) error {
-		// For compatibility with the legacy asynchronous API, return the workdone
-		// token that clients used to use to identify when this vulncheck
-		// invocation is complete.
-		commandResult.Token = deps.work.Token()
+		tokenChan <- deps.work.Token()
 
 		jsonrpc2.Async(ctx) // run this in parallel with other requests: vulncheck can be slow.
 
@@ -1236,7 +1323,6 @@ func (c *commandHandler) RunGovulncheck(ctx context.Context, args command.Vulnch
 		if err != nil {
 			return err
 		}
-		commandResult.Result = result
 
 		snapshot, release, err := c.s.session.InvalidateView(ctx, deps.snapshot.View(), cache.StateChange{
 			Vulns: map[protocol.DocumentURI]*vulncheck.Result{args.URI: result},
@@ -1273,7 +1359,12 @@ func (c *commandHandler) RunGovulncheck(ctx context.Context, args command.Vulnch
 	if err != nil {
 		return command.RunVulncheckResult{}, err
 	}
-	return commandResult, nil
+	select {
+	case <-ctx.Done():
+		return command.RunVulncheckResult{}, ctx.Err()
+	case token := <-tokenChan:
+		return command.RunVulncheckResult{Token: token}, nil
+	}
 }
 
 // MemStats implements the MemStats command. It returns an error as a