internal/lsp: remove the last diagnostics pop-up warning

The "you are neither in a module nor your GOPATH" warning doesn't fit
our current notification style, so adjust it to appear as a progress
report and clean up the associated tests.

Change-Id: I32b96f17f3b9715403e465e3e0f9705f5d859147
Reviewed-on: https://go-review.googlesource.com/c/tools/+/275537
Trust: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>

Author: stamblerre

gopls/internal/regtest/diagnostics_test.go

@@ -538,43 +538,26 @@ func _() {
 
 // Expect a module/GOPATH error if there is an error in the file at startup.
 // Tests golang/go#37279.
-func TestShowMessage_Issue37279(t *testing.T) {
+func TestShowCriticalError_Issue37279(t *testing.T) {
 	const noModule = `
 -- a.go --
 package foo
 
-func f() {
-	fmt.Printl()
-}
-`
-	runner.Run(t, noModule, func(t *testing.T, env *Env) {
-		env.OpenFile("a.go")
-		env.Await(env.DiagnosticAtRegexp("a.go", "fmt.Printl"), ShownMessage(""))
-	})
-}
-
-// Expect no module/GOPATH error if there is no error in the file.
-// Tests golang/go#37279.
-func TestNoShowMessage_Issue37279(t *testing.T) {
-	const noModule = `
--- a.go --
-package foo
+import "mod.com/hello"
 
 func f() {
+	hello.Goodbye()
 }
 `
 	runner.Run(t, noModule, func(t *testing.T, env *Env) {
 		env.OpenFile("a.go")
 		env.Await(
-			OnceMet(
-				CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidOpen), 1),
-				NoDiagnostics("a.go"),
-			),
-			NoShowMessage(),
+			OutstandingWork(lsp.WorkspaceLoadFailure, "outside of a module"),
+		)
+		env.RegexpReplace("a.go", `import "mod.com/hello"`, "")
+		env.Await(
+			NoOutstandingWork(),
 		)
-		// introduce an error, expect no Show Message
-		env.RegexpReplace("a.go", "func", "fun")
-		env.Await(env.DiagnosticAtRegexp("a.go", "fun"), NoShowMessage())
 	})
 }
 
@@ -1526,14 +1509,14 @@ package main
 	run(t, pkg, func(t *testing.T, env *Env) {
 		env.OpenFile("go.mod")
 		env.Await(
-			OutstandingWork("Error loading workspace", "unknown directive"),
+			OutstandingWork(lsp.WorkspaceLoadFailure, "unknown directive"),
 		)
 		env.EditBuffer("go.mod", fake.NewEdit(0, 0, 3, 0, `module mod.com
 
 go 1.hello
 `))
 		env.Await(
-			OutstandingWork("Error loading workspace", "invalid go version"),
+			OutstandingWork(lsp.WorkspaceLoadFailure, "invalid go version"),
 		)
 		env.RegexpReplace("go.mod", "go 1.hello", "go 1.12")
 		env.Await(
@@ -1681,7 +1664,7 @@ package b
 				env.Await(
 					env.DiagnosticAtRegexp("a/a.go", "package a"),
 					env.DiagnosticAtRegexp("b/go.mod", "module b.com"),
-					OutstandingWork("Error loading workspace", "gopls requires a module at the root of your workspace."),
+					OutstandingWork(lsp.WorkspaceLoadFailure, "gopls requires one module per workspace folder."),
 				)
 			})
 		})

internal/lsp/cache/load.go

@@ -234,7 +234,7 @@ func (s *snapshot) WorkspaceLayoutError(ctx context.Context) *source.CriticalErr
 	}
 	s.mu.Unlock()
 
-	msg := `gopls requires a module at the root of your workspace.
+	msg := `gopls requires one module per workspace folder.
 You can work with multiple modules by opening each one as a workspace folder.
 Improvements to this workflow will be coming soon (https://github.com/golang/go/issues/32394),
 and you can learn more here: https://github.com/golang/go/issues/36899.`

internal/lsp/cache/pkg.go

@@ -117,9 +117,20 @@ func (p *pkg) GetImport(pkgPath string) (source.Package, error) {
 }
 
 func (p *pkg) MissingDependencies() []string {
+	// We don't invalidate metadata for import deletions, so check the package
+	// imports via the *types.Package. Only use metadata if p.types is nil.
+	if p.types == nil {
+		var md []string
+		for i := range p.m.missingDeps {
+			md = append(md, string(i))
+		}
+		return md
+	}
 	var md []string
-	for i := range p.m.missingDeps {
-		md = append(md, string(i))
+	for _, pkg := range p.types.Imports() {
+		if _, ok := p.m.missingDeps[packagePath(pkg.Path())]; ok {
+			md = append(md, pkg.Path())
+		}
 	}
 	return md
 }

internal/lsp/cache/view.go

@@ -528,11 +528,8 @@ func (s *snapshot) initialize(ctx context.Context, firstAttempt bool) {
 
 		// If we have multiple modules, we need to load them by paths.
 		var scopes []interface{}
-		var modErrors source.ErrorList
+		var modErrors []*source.Error
 		addError := func(uri span.URI, err error) {
-			if modErrors == nil {
-				modErrors = source.ErrorList{}
-			}
 			modErrors = append(modErrors, &source.Error{
 				URI:      uri,
 				Category: "compiler",

internal/lsp/diagnostics.go

@@ -68,14 +68,7 @@ func hashDiagnostics(diags ...*source.Diagnostic) string {
 func (s *Server) diagnoseDetached(snapshot source.Snapshot) {
 	ctx := snapshot.BackgroundContext()
 	ctx = xcontext.Detach(ctx)
-	showWarning := s.diagnose(ctx, snapshot, false)
-	if showWarning {
-		// If a view has been created or the configuration changed, warn the user.
-		s.client.ShowMessage(ctx, &protocol.ShowMessageParams{
-			Type:    protocol.Warning,
-			Message: `You are neither in a module nor in your GOPATH. 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.`,
-		})
-	}
+	s.diagnose(ctx, snapshot, false)
 	s.publishDiagnostics(ctx, true, snapshot)
 }
 
@@ -138,22 +131,22 @@ func (s *Server) diagnoseChangedFiles(ctx context.Context, snapshot source.Snaps
 		go func(pkg source.Package) {
 			defer wg.Done()
 
-			_ = s.diagnosePkg(ctx, snapshot, pkg, false)
+			s.diagnosePkg(ctx, snapshot, pkg, false)
 		}(pkg)
 	}
 	wg.Wait()
 }
 
 // diagnose is a helper function for running diagnostics with a given context.
 // Do not call it directly. forceAnalysis is only true for testing purposes.
-func (s *Server) diagnose(ctx context.Context, snapshot source.Snapshot, forceAnalysis bool) bool {
+func (s *Server) diagnose(ctx context.Context, snapshot source.Snapshot, forceAnalysis bool) {
 	ctx, done := event.Start(ctx, "Server.diagnose")
 	defer done()
 
 	// Wait for a free diagnostics slot.
 	select {
 	case <-ctx.Done():
-		return false
+		return
 	case s.diagnosticsSema <- struct{}{}:
 	}
 	defer func() {
@@ -163,7 +156,7 @@ func (s *Server) diagnose(ctx context.Context, snapshot source.Snapshot, forceAn
 	// First, diagnose the go.mod file.
 	modReports, modErr := mod.Diagnostics(ctx, snapshot)
 	if ctx.Err() != nil {
-		return false
+		return
 	}
 	if modErr != nil {
 		event.Error(ctx, "warning: diagnose go.mod", modErr, tag.Directory.Of(snapshot.View().Folder().Filename()), tag.Snapshot.Of(snapshot.ID()))
@@ -179,7 +172,14 @@ func (s *Server) diagnose(ctx context.Context, snapshot source.Snapshot, forceAn
 	// Diagnose all of the packages in the workspace.
 	wsPkgs, err := snapshot.WorkspacePackages(ctx)
 	if s.shouldIgnoreError(ctx, snapshot, err) {
-		return false
+		return
+	}
+	// Even if packages didn't fail to load, we still may want to show
+	// additional warnings.
+	if err == nil {
+		if msg := shouldShowAdHocPackagesWarning(snapshot, wsPkgs); msg != "" {
+			err = fmt.Errorf(msg)
+		}
 	}
 
 	// Even if workspace packages were returned, there still may be an error
@@ -198,16 +198,15 @@ func (s *Server) diagnose(ctx context.Context, snapshot source.Snapshot, forceAn
 	// error progress reports will be closed.
 	s.showCriticalErrorStatus(ctx, snapshot, err)
 
-	if err != nil {
-		event.Error(ctx, "errors diagnosing workspace", err, tag.Snapshot.Of(snapshot.ID()), tag.Directory.Of(snapshot.View().Folder()))
-		return false
+	// If there are no workspace packages, there is nothing to diagnose and
+	// there are no orphaned files.
+	if len(wsPkgs) == 0 {
+		return
 	}
 
 	var (
-		wg              sync.WaitGroup
-		shouldShowMsgMu sync.Mutex
-		shouldShowMsg   bool
-		seen            = map[span.URI]struct{}{}
+		wg   sync.WaitGroup
+		seen = map[span.URI]struct{}{}
 	)
 	for _, pkg := range wsPkgs {
 		wg.Add(1)
@@ -219,33 +218,26 @@ func (s *Server) diagnose(ctx context.Context, snapshot source.Snapshot, forceAn
 		go func(pkg source.Package) {
 			defer wg.Done()
 
-			show := s.diagnosePkg(ctx, snapshot, pkg, forceAnalysis)
-			if show {
-				shouldShowMsgMu.Lock()
-				shouldShowMsg = true
-				shouldShowMsgMu.Unlock()
-			}
+			s.diagnosePkg(ctx, snapshot, pkg, forceAnalysis)
 		}(pkg)
 	}
 	wg.Wait()
+
 	// Confirm that every opened file belongs to a package (if any exist in
 	// the workspace). Otherwise, add a diagnostic to the file.
-	if len(wsPkgs) > 0 {
-		for _, o := range s.session.Overlays() {
-			if _, ok := seen[o.URI()]; ok {
-				continue
-			}
-			diagnostic := s.checkForOrphanedFile(ctx, snapshot, o)
-			if diagnostic == nil {
-				continue
-			}
-			s.storeDiagnostics(snapshot, o.URI(), orphanedSource, []*source.Diagnostic{diagnostic})
+	for _, o := range s.session.Overlays() {
+		if _, ok := seen[o.URI()]; ok {
+			continue
 		}
+		diagnostic := s.checkForOrphanedFile(ctx, snapshot, o)
+		if diagnostic == nil {
+			continue
+		}
+		s.storeDiagnostics(snapshot, o.URI(), orphanedSource, []*source.Diagnostic{diagnostic})
 	}
-	return shouldShowMsg
 }
 
-func (s *Server) diagnosePkg(ctx context.Context, snapshot source.Snapshot, pkg source.Package, alwaysAnalyze bool) bool {
+func (s *Server) diagnosePkg(ctx context.Context, snapshot source.Snapshot, pkg source.Package, alwaysAnalyze bool) {
 	includeAnalysis := alwaysAnalyze // only run analyses for packages with open files
 	var gcDetailsDir span.URI        // find the package's optimization details, if available
 	for _, pgf := range pkg.CompiledGoFiles() {
@@ -271,7 +263,7 @@ func (s *Server) diagnosePkg(ctx context.Context, snapshot source.Snapshot, pkg
 		reports, err := source.Analyze(ctx, snapshot, pkg, typeCheckResults)
 		if err != nil {
 			event.Error(ctx, "warning: diagnose package", err, tag.Snapshot.Of(snapshot.ID()), tag.Package.Of(pkg.ID()))
-			return false
+			return
 		}
 		for uri, diags := range reports {
 			s.storeDiagnostics(snapshot, uri, analysisSource, diags)
@@ -294,7 +286,6 @@ func (s *Server) diagnosePkg(ctx context.Context, snapshot source.Snapshot, pkg
 			s.storeDiagnostics(snapshot, id.URI, gcDetailsSource, diags)
 		}
 	}
-	return shouldWarn(snapshot, pkg)
 }
 
 // storeDiagnostics stores results from a single diagnostic source. If merge is
@@ -329,21 +320,6 @@ func (s *Server) storeDiagnostics(snapshot source.Snapshot, uri span.URI, dsourc
 	s.diagnostics[uri].reports[dsource] = report
 }
 
-// shouldWarn reports whether we should warn the user about their build
-// configuration.
-func shouldWarn(snapshot source.Snapshot, pkg source.Package) bool {
-	if snapshot.ValidBuildConfiguration() {
-		return false
-	}
-	if len(pkg.MissingDependencies()) > 0 {
-		return true
-	}
-	if len(pkg.CompiledGoFiles()) == 1 && hasUndeclaredErrors(pkg) {
-		return true // The user likely opened a single file.
-	}
-	return false
-}
-
 // clearDiagnosticSource clears all diagnostics for a given source type. It is
 // necessary for cases where diagnostics have been invalidated by something
 // other than a snapshot change, for example when gc_details is toggled.
@@ -355,22 +331,24 @@ func (s *Server) clearDiagnosticSource(dsource diagnosticSource) {
 	}
 }
 
-// hasUndeclaredErrors returns true if a package has a type error
-// about an undeclared symbol.
-//
-// TODO(findleyr): switch to using error codes in 1.16
-func hasUndeclaredErrors(pkg source.Package) bool {
-	for _, err := range pkg.GetErrors() {
-		if err.Kind != source.TypeError {
-			continue
-		}
-		if strings.Contains(err.Message, "undeclared name:") {
-			return true
+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 len(pkg.MissingDependencies()) > 0 {
+			return adHocPackagesWarning
 		}
 	}
-	return false
+	return ""
 }
 
+const WorkspaceLoadFailure = "Error loading workspace"
+
 // showCriticalErrorStatus shows the error as a progress report.
 // If the error is nil, it clears any existing error progress report.
 func (s *Server) showCriticalErrorStatus(ctx context.Context, snapshot source.Snapshot, err error) {
@@ -381,21 +359,18 @@ func (s *Server) showCriticalErrorStatus(ctx context.Context, snapshot source.Sn
 	// status bar.
 	var errMsg string
 	if err != nil {
-		// Some error messages can also be displayed as diagnostics. But don't
-		// show source.ErrorLists as critical errors--only CriticalErrors
-		// should be shown.
+		event.Error(ctx, "errors loading workspace", err, tag.Snapshot.Of(snapshot.ID()), tag.Directory.Of(snapshot.View().Folder()))
+
+		// Some error messages can also be displayed as diagnostics.
 		if criticalErr := (*source.CriticalError)(nil); errors.As(err, &criticalErr) {
 			s.storeErrorDiagnostics(ctx, snapshot, typeCheckSource, criticalErr.ErrorList)
-		} else if srcErrList := (source.ErrorList)(nil); errors.As(err, &srcErrList) {
-			s.storeErrorDiagnostics(ctx, snapshot, typeCheckSource, srcErrList)
-			return
 		}
 		errMsg = strings.Replace(err.Error(), "\n", " ", -1)
 	}
 
 	if s.criticalErrorStatus == nil {
 		if errMsg != "" {
-			s.criticalErrorStatus = s.progress.start(ctx, "Error loading workspace", errMsg, nil, nil)
+			s.criticalErrorStatus = s.progress.start(ctx, WorkspaceLoadFailure, errMsg, nil, nil)
 		}
 		return
 	}

internal/lsp/mod/diagnostics.go

@@ -69,8 +69,6 @@ func ErrorsForMod(ctx context.Context, snapshot source.Snapshot, fh source.FileH
 		// Some error messages can also be displayed as diagnostics.
 		if criticalErr := (*source.CriticalError)(nil); errors.As(err, &criticalErr) {
 			return criticalErr.ErrorList, nil
-		} else if srcErrList := (source.ErrorList)(nil); errors.As(err, &srcErrList) {
-			return srcErrList, nil
 		}
 		return nil, err
 	}

internal/lsp/source/view.go

@@ -556,7 +556,7 @@ type Package interface {
 
 type CriticalError struct {
 	MainError error
-	ErrorList
+	ErrorList []*Error
 }
 
 func (err *CriticalError) Error() string {
@@ -566,16 +566,6 @@ func (err *CriticalError) Error() string {
 	return err.MainError.Error()
 }
 
-type ErrorList []*Error
-
-func (err ErrorList) Error() string {
-	var list []string
-	for _, e := range err {
-		list = append(list, e.Error())
-	}
-	return strings.Join(list, "\n\t")
-}
-
 // An Error corresponds to an LSP Diagnostic.
 // https://microsoft.github.io/language-server-protocol/specification#diagnostic
 type Error struct {