gopls/internal/lsp/cache: simplify ad-hoc package warning logic

The old logic used to inspect the syntax, on the assumption that
metadata was not reloaded after an import deletion. But the logic
of metadataChanges clearly shows that it is reloaded.
The new logic now inspects only the metadata.
There's already a test that the warning is properly dismissed.

A follow-up change will strength-reduce ActivePackages to
avoid type-checking.

Change-Id: I6b010b5b7b011544cbb69189145208101b94de65
Reviewed-on: https://go-review.googlesource.com/c/tools/+/456257
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>

Author: adonovan

gopls/internal/lsp/cache/snapshot.go

@@ -1440,50 +1440,30 @@ func (s *snapshot) GetCriticalError(ctx context.Context) *source.CriticalError {
 	return loadErr
 }
 
+// A portion of this text is expected by TestBrokenWorkspace_OutsideModule.
 const adHocPackagesWarning = `You are outside of a module and outside of $GOPATH/src.
 If you are using modules, please open your editor to a directory in your module.
 If you believe this warning is incorrect, please file an issue: https://github.com/golang/go/issues/new.`
 
 func shouldShowAdHocPackagesWarning(snapshot source.Snapshot, pkgs []source.Package) string {
-	if snapshot.ValidBuildConfiguration() {
-		return ""
-	}
-	for _, pkg := range pkgs {
-		if hasMissingDependencies(pkg) {
-			return adHocPackagesWarning
-		}
-	}
-	return ""
-}
+	if !snapshot.ValidBuildConfiguration() {
+		for _, pkg := range pkgs {
+			// TODO(adonovan): strength-reduce the parameter to []Metadata.
+			m := snapshot.Metadata(pkg.ID())
+			if m == nil {
+				continue
+			}
 
-func hasMissingDependencies(pkg source.Package) bool {
-	// We don't invalidate metadata for import deletions,
-	// so check the package imports via the syntax tree
-	// (not via types.Package.Imports, since it contains packages
-	// synthesized under the vendoring-hostile assumption that
-	// ImportPath equals PackagePath).
-	//
-	// rfindley says: it looks like this is intending to implement
-	// a heuristic "if go list couldn't resolve import paths to
-	// packages, then probably you're not in GOPATH or a module".
-	// This is used to determine if we need to show a warning diagnostic.
-	// It looks like this logic is implementing the heuristic that
-	// "even if the metadata has a MissingDep, if the types.Package
-	// doesn't need that dep anymore we shouldn't show the warning".
-	// But either we're outside of GOPATH/Module, or we're not...
-	//
-	// If we invalidate the metadata for import deletions (which
-	// should be fast) then we can simply return the blank entries
-	// in DepsByImpPath.
-	for _, f := range pkg.GetSyntax() {
-		for _, imp := range f.Imports {
-			importPath := source.UnquoteImportPath(imp)
-			if _, err := pkg.ResolveImportPath(importPath); err != nil {
-				return true
+			// A blank entry in DepsByImpPath
+			// indicates a missing dependency.
+			for _, importID := range m.DepsByImpPath {
+				if importID == "" {
+					return adHocPackagesWarning
+				}
 			}
 		}
 	}
-	return false
+	return ""
 }
 
 func containsCommandLineArguments(pkgs []source.Package) bool {

gopls/internal/regtest/diagnostics/diagnostics_test.go

@@ -543,6 +543,10 @@ func _() {
 // Expect a module/GOPATH error if there is an error in the file at startup.
 // Tests golang/go#37279.
 func TestBrokenWorkspace_OutsideModule(t *testing.T) {
+	// Versions of the go command before go1.16 do not support
+	// native overlays and so do not observe the deletion.
+	testenv.NeedsGo1Point(t, 16)
+
 	const noModule = `
 -- a.go --
 package foo
@@ -556,8 +560,10 @@ func f() {
 	Run(t, noModule, func(t *testing.T, env *Env) {
 		env.OpenFile("a.go")
 		env.Await(
+			// Expect the adHocPackagesWarning.
 			OutstandingWork(lsp.WorkspaceLoadFailure, "outside of a module"),
 		)
+		// Deleting the import dismisses the warning.
 		env.RegexpReplace("a.go", `import "mod.com/hello"`, "")
 		env.Await(
 			NoOutstandingWork(),