internal/lsp: fix flaking internal/lsp/cmd tests

Reloading metadata on demand fails for some our test packages,
because I don't understand how to construct arguments to commands.

Change-Id: Ib18454a09772e854a612528af898d06ce14133c2
Reviewed-on: https://go-review.googlesource.com/c/tools/+/214717
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>

internal/lsp/cache/check.go

@@ -298,7 +298,7 @@ func typeCheck(ctx context.Context, fset *token.FileSet, m *metadata, mode sourc
 	if pkg.pkgPath == "unsafe" {
 		pkg.types = types.Unsafe
 	} else if len(files) == 0 { // not the unsafe package, no parsed files
-		return nil, errors.Errorf("no parsed files for package %s, expected: %s, errors: %v", pkg.pkgPath, pkg.compiledGoFiles, actualErrors)
+		return nil, errors.Errorf("no parsed files for package %s, expected: %s, errors: %v, list errors: %v", pkg.pkgPath, pkg.compiledGoFiles, actualErrors, rawErrors)
 	} else {
 		pkg.types = types.NewPackage(string(m.pkgPath), m.name)
 	}

internal/lsp/cache/load.go

@@ -36,35 +36,33 @@ type metadata struct {
 }
 
 func (s *snapshot) load(ctx context.Context, scope interface{}) ([]*metadata, error) {
-	var query string
+	var query []string
 	switch scope := scope.(type) {
 	case []packagePath:
-		for i, p := range scope {
-			if i != 0 {
-				query += " "
-			}
-			query += string(p)
+		for _, p := range scope {
+			query = append(query, string(p))
 		}
 	case packagePath:
-		query = string(scope)
+		query = append(query, string(scope))
 	case fileURI:
-		query = fmt.Sprintf("file=%s", span.URI(scope).Filename())
+		query = append(query, fmt.Sprintf("file=%s", span.URI(scope).Filename()))
 	case directoryURI:
 		filename := span.URI(scope).Filename()
-		query = fmt.Sprintf("%s/...", filename)
+		q := fmt.Sprintf("%s/...", filename)
 		// Simplify the query if it will be run in the requested directory.
 		// This ensures compatibility with Go 1.12 that doesn't allow
 		// <directory>/... in GOPATH mode.
 		if s.view.folder.Filename() == filename {
-			query = "./..."
+			q = "./..."
 		}
+		query = append(query, q)
 	}
 
 	ctx, done := trace.StartSpan(ctx, "cache.view.load", telemetry.Query.Of(query))
 	defer done()
 
 	cfg := s.view.Config(ctx)
-	pkgs, err := packages.Load(cfg, query)
+	pkgs, err := packages.Load(cfg, query...)
 
 	// If the context was canceled, return early. Otherwise, we might be
 	// type-checking an incomplete result. Check the context directly,

internal/lsp/cache/snapshot.go

@@ -16,7 +16,6 @@ import (
 	"golang.org/x/tools/internal/lsp/source"
 	"golang.org/x/tools/internal/lsp/telemetry"
 	"golang.org/x/tools/internal/span"
-	"golang.org/x/tools/internal/telemetry/log"
 	errors "golang.org/x/xerrors"
 )
 
@@ -382,8 +381,7 @@ func (s *snapshot) KnownPackages(ctx context.Context) ([]source.PackageHandle, e
 		// so just build the package handle directly (without a reload).
 		ph, err := s.buildPackageHandle(ctx, pkgID, source.ParseExported)
 		if err != nil {
-			log.Error(ctx, "KnownPackages: failed to create PackageHandle", err, telemetry.Package.Of(pkgID))
-			continue
+			return nil, err
 		}
 		results = append(results, ph)
 	}

internal/lsp/source/diagnostics.go

@@ -56,8 +56,9 @@ func FileDiagnostics(ctx context.Context, snapshot Snapshot, fh FileHandle, with
 	// not correctly configured. Report errors, if possible.
 	var warningMsg string
 	if len(ph.MissingDependencies()) > 0 {
-		if warningMsg, err = checkCommonErrors(ctx, snapshot.View()); err != nil {
-			log.Error(ctx, "error checking common errors", err, telemetry.File.Of(fh.Identity().URI))
+		warningMsg, err = checkCommonErrors(ctx, snapshot.View())
+		if err != nil {
+			return nil, "", err
 		}
 	}
 	reports, msg, err := PackageDiagnostics(ctx, snapshot, ph, withAnalysis, disabledAnalyses)
@@ -77,9 +78,9 @@ func PackageDiagnostics(ctx context.Context, snapshot Snapshot, ph PackageHandle
 	// we may have an ad-hoc package with multiple files. Show a warning message.
 	// TODO(golang/go#36416): Remove this when golang.org/cl/202277 is merged.
 	if len(pkg.CompiledGoFiles()) == 1 && hasUndeclaredErrors(pkg) {
-		fh := pkg.CompiledGoFiles()[0].File()
-		if warningMsg, err = checkCommonErrors(ctx, snapshot.View()); err != nil {
-			log.Error(ctx, "error checking common errors", err, telemetry.File.Of(fh.Identity().URI))
+		warningMsg, err = checkCommonErrors(ctx, snapshot.View())
+		if err != nil {
+			return nil, "", err
 		}
 	}
 	// Prepare the reports we will send for the files in this package.

internal/lsp/source/errors.go

@@ -8,6 +8,8 @@ import (
 	"os/exec"
 	"path/filepath"
 	"strings"
+
+	errors "golang.org/x/xerrors"
 )
 
 const (
@@ -77,8 +79,8 @@ func checkCommonErrors(ctx context.Context, v View) (string, error) {
 	return msg, nil
 }
 
-// invokeGo returns the stdout of a go command invocation.
-// Borrowed from golang.org/x/tools/go/packages/golist.go.
+// InvokeGo returns the output of a go command invocation.
+// It does not try to recover from errors.
 func InvokeGo(ctx context.Context, dir string, env []string, args ...string) (*bytes.Buffer, error) {
 	stdout := new(bytes.Buffer)
 	stderr := new(bytes.Buffer)
@@ -99,12 +101,10 @@ func InvokeGo(ctx context.Context, dir string, env []string, args ...string) (*b
 		if ee, ok := err.(*exec.Error); ok && ee.Err == exec.ErrNotFound {
 			return nil, fmt.Errorf("'gopls requires 'go', but %s", exec.ErrNotFound)
 		}
-		if _, ok := err.(*exec.ExitError); !ok {
-			// Catastrophic error:
-			// - context cancellation
-			return nil, fmt.Errorf("couldn't exec 'go %v': %s %T", args, err, err)
+		if ctx.Err() != nil {
+			return nil, ctx.Err()
 		}
-		return stdout, fmt.Errorf("%s", stderr)
+		return stdout, errors.Errorf("err: %v: stderr: %s", err, stderr)
 	}
 	return stdout, nil
 }

internal/lsp/source/implementation.go

@@ -30,17 +30,14 @@ func Implementation(ctx context.Context, s Snapshot, f FileHandle, pp protocol.P
 		if impl.pkg == nil || len(impl.pkg.CompiledGoFiles()) == 0 {
 			continue
 		}
-
 		rng, err := objToMappedRange(s.View(), impl.pkg, impl.obj)
 		if err != nil {
 			return nil, err
 		}
-
 		pr, err := rng.Range()
 		if err != nil {
 			return nil, err
 		}
-
 		locations = append(locations, protocol.Location{
 			URI:   protocol.NewURI(rng.URI()),
 			Range: pr,