internal/lsp: move the missing imports handling into the metadata

This change uses the missing imports detection to return diagnostics and
warning messages to the user.

Updates golang/go#34484

Change-Id: If7bb7e702b8bdbd7e1ad5e26f93acc50d16209b0
Reviewed-on: https://go-review.googlesource.com/c/tools/+/196985
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>

internal/lsp/cache/check.go

@@ -158,6 +158,14 @@ func (cph *checkPackageHandle) ID() string {
 	return string(cph.m.id)
 }
 
+func (cph *checkPackageHandle) MissingDependencies() []string {
+	var md []string
+	for i := range cph.m.missingDeps {
+		md = append(md, string(i))
+	}
+	return md
+}
+
 func (cph *checkPackageHandle) Cached(ctx context.Context) (source.Package, error) {
 	return cph.cached(ctx)
 }

internal/lsp/cache/gofile.go

@@ -23,10 +23,6 @@ type goFile struct {
 	// which can be modified during type-checking.
 	mu sync.Mutex
 
-	// missingImports is the set of unresolved imports for this package.
-	// It contains any packages with `go list` errors.
-	missingImports map[packagePath]struct{}
-
 	imports []*ast.ImportSpec
 }
 
@@ -35,21 +31,47 @@ type packageKey struct {
 	mode source.ParseMode
 }
 
-func (f *goFile) CheckPackageHandles(ctx context.Context) (cphs []source.CheckPackageHandle, err error) {
+func (f *goFile) CheckPackageHandles(ctx context.Context) (results []source.CheckPackageHandle, err error) {
 	ctx = telemetry.File.With(ctx, f.URI())
 	fh := f.Handle(ctx)
 
-	cphs = f.isDirty(ctx, fh)
-	if len(cphs) == 0 {
-		cphs, err = f.view.loadParseTypecheck(ctx, f, fh)
+	var dirty bool
+	meta, pkgs := f.view.getSnapshot(f.URI())
+	if len(meta) == 0 {
+		dirty = true
+	}
+	for _, m := range meta {
+		if len(m.missingDeps) != 0 {
+			dirty = true
+		}
+	}
+	for _, cph := range pkgs {
+		// If we're explicitly checking if a file needs to be type-checked,
+		// we need it to be fully parsed.
+		if cph.mode() != source.ParseFull {
+			continue
+		}
+		// Check if there is a fully-parsed package to which this file belongs.
+		for _, file := range cph.Files() {
+			if file.File().Identity() == fh.Identity() {
+				results = append(results, cph)
+			}
+		}
+	}
+	if dirty || len(results) == 0 {
+		cphs, err := f.view.loadParseTypecheck(ctx, f, fh)
 		if err != nil {
 			return nil, err
 		}
+		if cphs == nil {
+			return results, nil
+		}
+		results = cphs
 	}
-	if len(cphs) == 0 {
+	if len(results) == 0 {
 		return nil, errors.Errorf("no CheckPackageHandles for %s", f.URI())
 	}
-	return cphs, nil
+	return results, nil
 }
 
 func (v *view) GetActiveReverseDeps(ctx context.Context, uri span.URI) (results []source.CheckPackageHandle) {
@@ -78,27 +100,3 @@ func (v *view) GetActiveReverseDeps(ctx context.Context, uri span.URI) (results
 	}
 	return results
 }
-
-// isDirty is true if the file needs to be type-checked.
-// It assumes that the file's view's mutex is held by the caller.
-func (f *goFile) isDirty(ctx context.Context, fh source.FileHandle) []source.CheckPackageHandle {
-	meta, cphs := f.view.getSnapshot(f.URI())
-	if len(meta) == 0 {
-		return nil
-	}
-	var results []source.CheckPackageHandle
-	for key, cph := range cphs {
-		// If we're explicitly checking if a file needs to be type-checked,
-		// we need it to be fully parsed.
-		if key.mode != source.ParseFull {
-			continue
-		}
-		// Check if there is a fully-parsed package to which this file belongs.
-		for _, file := range cph.Files() {
-			if file.File().Identity() == fh.Identity() {
-				results = append(results, cph)
-			}
-		}
-	}
-	return results
-}

internal/lsp/cache/load.go

@@ -27,6 +27,11 @@ func (v *view) loadParseTypecheck(ctx context.Context, f *goFile, fh source.File
 	if err != nil {
 		return nil, err
 	}
+	// If load has explicitly returns nil metadata and no error,
+	// it means that we should not re-typecheck the packages.
+	if meta == nil {
+		return nil, nil
+	}
 	var (
 		cphs    []*checkPackageHandle
 		results []source.CheckPackageHandle
@@ -129,7 +134,15 @@ func (v *view) checkMetadata(ctx context.Context, f *goFile, fh source.FileHandl
 		// Return this error as a diagnostic to the user.
 		return nil, err
 	}
-	return v.updateMetadata(ctx, f.URI(), pkgs)
+	m, prevMissingImports, err := v.updateMetadata(ctx, f.URI(), pkgs)
+	if err != nil {
+		return nil, err
+	}
+	meta, err := validateMetadata(ctx, m, prevMissingImports)
+	if err != nil {
+		return nil, err
+	}
+	return meta, nil
 }
 
 // shouldRunGopackages reparses a file's package and import declarations to
@@ -161,3 +174,36 @@ func (v *view) shouldRunGopackages(ctx context.Context, f *goFile, file *ast.Fil
 	}
 	return false
 }
+
+func validateMetadata(ctx context.Context, metadata []*metadata, prevMissingImports map[packageID]map[packagePath]struct{}) ([]*metadata, error) {
+	// If we saw incorrect metadata for this package previously, don't both rechecking it.
+	for _, m := range metadata {
+		if len(m.missingDeps) > 0 {
+			prev, ok := prevMissingImports[m.id]
+			// There are missing imports that we previously hadn't seen before.
+			if !ok {
+				return metadata, nil
+			}
+			// The set of missing imports has changed.
+			if !sameSet(prev, m.missingDeps) {
+				return metadata, nil
+			}
+		} else {
+			// There are no missing imports.
+			return metadata, nil
+		}
+	}
+	return nil, nil
+}
+
+func sameSet(x, y map[packagePath]struct{}) bool {
+	if len(x) != len(y) {
+		return false
+	}
+	for k := range x {
+		if _, ok := y[k]; !ok {
+			return false
+		}
+	}
+	return true
+}

internal/lsp/cache/pkg.go

@@ -24,9 +24,8 @@ type pkg struct {
 	view *view
 
 	// ID and package path have their own types to avoid being used interchangeably.
-	id      packageID
-	pkgPath packagePath
-
+	id         packageID
+	pkgPath    packagePath
 	files      []source.ParseGoHandle
 	errors     []packages.Error
 	imports    map[packagePath]*pkg

internal/lsp/cache/snapshot.go

@@ -20,17 +20,18 @@ type snapshot struct {
 }
 
 type metadata struct {
-	id         packageID
-	pkgPath    packagePath
-	name       string
-	files      []span.URI
-	typesSizes types.Sizes
-	parents    map[packageID]bool
-	children   map[packageID]*metadata
-	errors     []packages.Error
+	id          packageID
+	pkgPath     packagePath
+	name        string
+	files       []span.URI
+	typesSizes  types.Sizes
+	parents     map[packageID]bool
+	children    map[packageID]*metadata
+	errors      []packages.Error
+	missingDeps map[packagePath]struct{}
 }
 
-func (v *view) getSnapshot(uri span.URI) ([]*metadata, map[packageKey]*checkPackageHandle) {
+func (v *view) getSnapshot(uri span.URI) ([]*metadata, []*checkPackageHandle) {
 	v.snapshotMu.Lock()
 	defer v.snapshotMu.Unlock()
 
@@ -38,7 +39,11 @@ func (v *view) getSnapshot(uri span.URI) ([]*metadata, map[packageKey]*checkPack
 	for _, id := range v.snapshot.ids[uri] {
 		m = append(m, v.snapshot.metadata[id])
 	}
-	return m, v.snapshot.packages[uri]
+	var cphs []*checkPackageHandle
+	for _, cph := range v.snapshot.packages[uri] {
+		cphs = append(cphs, cph)
+	}
+	return m, cphs
 }
 
 func (v *view) getMetadata(uri span.URI) []*metadata {
@@ -59,11 +64,17 @@ func (v *view) getPackages(uri span.URI) map[packageKey]*checkPackageHandle {
 	return v.snapshot.packages[uri]
 }
 
-func (v *view) updateMetadata(ctx context.Context, uri span.URI, pkgs []*packages.Package) ([]*metadata, error) {
+func (v *view) updateMetadata(ctx context.Context, uri span.URI, pkgs []*packages.Package) ([]*metadata, map[packageID]map[packagePath]struct{}, error) {
 	v.snapshotMu.Lock()
 	defer v.snapshotMu.Unlock()
 
 	// Clear metadata since we are re-running go/packages.
+	prevMissingImports := make(map[packageID]map[packagePath]struct{})
+	for _, id := range v.snapshot.ids[uri] {
+		if m, ok := v.snapshot.metadata[id]; ok && len(m.missingDeps) > 0 {
+			prevMissingImports[id] = m.missingDeps
+		}
+	}
 	without := make(map[span.URI]struct{})
 	for _, id := range v.snapshot.ids[uri] {
 		v.remove(id, without, map[packageID]struct{}{})
@@ -80,11 +91,14 @@ func (v *view) updateMetadata(ctx context.Context, uri span.URI, pkgs []*package
 			pkg:     pkg,
 			parent:  nil,
 		}); err != nil {
-			return nil, err
+			return nil, nil, err
 		}
 		results = append(results, v.snapshot.metadata[packageID(pkg.ID)])
 	}
-	return results, nil
+	if len(results) == 0 {
+		return nil, nil, errors.Errorf("no metadata for %s", uri)
+	}
+	return results, prevMissingImports, nil
 }
 
 type importGraph struct {
@@ -134,6 +148,10 @@ func (v *view) updateImportGraph(ctx context.Context, g *importGraph) error {
 		}
 		// Don't remember any imports with significant errors.
 		if importPkgPath != "unsafe" && len(importPkg.CompiledGoFiles) == 0 {
+			if m.missingDeps == nil {
+				m.missingDeps = make(map[packagePath]struct{})
+			}
+			m.missingDeps[importPkgPath] = struct{}{}
 			continue
 		}
 		if _, ok := m.children[packageID(importPkg.ID)]; !ok {

internal/lsp/source/diagnostics.go

@@ -47,6 +47,16 @@ func Diagnostics(ctx context.Context, view View, f GoFile, disabledAnalyses map[
 		return nil, "", err
 	}
 	cph := WidestCheckPackageHandle(cphs)
+
+	// If we are missing dependencies, it may because the user's workspace is
+	// not correctly configured. Report errors, if possible.
+	var warningMsg string
+	if len(cph.MissingDependencies()) > 0 {
+		warningMsg, err = checkCommonErrors(ctx, view, f.URI())
+		if err != nil {
+			log.Error(ctx, "error checking common errors", err, telemetry.File.Of(f.URI))
+		}
+	}
 	pkg, err := cph.Check(ctx)
 	if err != nil {
 		log.Error(ctx, "no package for file", err)
@@ -59,15 +69,6 @@ func Diagnostics(ctx context.Context, view View, f GoFile, disabledAnalyses map[
 		clearReports(view, reports, fh.File().Identity().URI)
 	}
 
-	// If we have `go list` errors, we may want to offer a warning message to the user.
-	var warningMsg string
-	if hasListErrors(pkg.GetErrors()) {
-		warningMsg, err = checkCommonErrors(ctx, view, f.URI())
-		if err != nil {
-			log.Error(ctx, "error checking common errors", err, telemetry.File.Of(f.URI))
-		}
-	}
-
 	// Prepare any additional reports for the errors in this package.
 	for _, err := range pkg.GetErrors() {
 		if err.Kind != packages.ListError {

internal/lsp/source/errors.go

@@ -12,7 +12,20 @@ import (
 	"golang.org/x/tools/internal/span"
 )
 
+const (
+	// TODO(rstambler): We should really be able to point to a link on the website.
+	modulesWiki = "https://github.com/golang/go/wiki/Modules"
+)
+
 func checkCommonErrors(ctx context.Context, view View, uri span.URI) (string, error) {
+	// Unfortunately, we probably can't have go/packages expose a function like this.
+	// Since we only really understand the `go` command, check the user's GOPACKAGESDRIVER
+	// and, if they are using `go list`, consider the possible error cases.
+	gopackagesdriver := os.Getenv("GOPACKAGESDRIVER")
+	if gopackagesdriver != "" && gopackagesdriver != "off" {
+		return "", nil
+	}
+
 	// Some cases we should be able to detect:
 	//
 	//  1. The user is in GOPATH mode and is working outside their GOPATH
@@ -49,7 +62,7 @@ func checkCommonErrors(ctx context.Context, view View, uri span.URI) (string, er
 			msg = "You are in module mode, but you are not inside of a module. Please create a module."
 		}
 	} else {
-		msg = "You are neither in a module nor in your GOPATH. Please see X for information on how to set up your Go project."
+		msg = fmt.Sprintf("You are neither in a module nor in your GOPATH. Please see %s for information on how to set up your Go project.", modulesWiki)
 	}
 	return msg, nil
 }

internal/lsp/source/view.go

@@ -122,6 +122,8 @@ type CheckPackageHandle interface {
 
 	// Cached returns the Package for the CheckPackageHandle if it has already been stored.
 	Cached(ctx context.Context) (Package, error)
+
+	MissingDependencies() []string
 }
 
 // Cache abstracts the core logic of dealing with the environment from the