internal/lsp: allow diagnostics to be computed per-file or per-package

This change splits source.Diagnostics into source.FileDiagnostics and
source.PackageDiagnostics. There isn't much difference in functionality,
but this just simplifies the process of getting diagnostics if you only
have a PackageHandle.

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

internal/lsp/diagnostics.go

@@ -37,30 +37,20 @@ func (s *Server) diagnoseSnapshot(ctx context.Context, snapshot source.Snapshot)
 		return
 	}
 	for _, id := range wsPackages {
-		ph, err := snapshot.PackageHandle(ctx, id)
-		if err != nil {
-			log.Error(ctx, "diagnoseSnapshot: no PackageHandle for workspace package", err, telemetry.Package.Of(id))
-			continue
-		}
-		if len(ph.CompiledGoFiles()) == 0 {
-			continue
-		}
-		// Find a file on which to call diagnostics.
-		uri := ph.CompiledGoFiles()[0].File().Identity().URI
-		fh, err := snapshot.GetFile(ctx, uri)
-		if err != nil {
-			continue
-		}
-		// Run diagnostics on the workspace package.
-		go func(snapshot source.Snapshot, fh source.FileHandle) {
-			reports, _, err := source.Diagnostics(ctx, snapshot, fh, false, snapshot.View().Options().DisabledAnalyses)
+		go func(id string) {
+			ph, err := snapshot.PackageHandle(ctx, id)
 			if err != nil {
-				log.Error(ctx, "no diagnostics", err, telemetry.URI.Of(fh.Identity().URI))
+				log.Error(ctx, "diagnoseSnapshot: no PackageHandle for workspace package", err, telemetry.Package.Of(id))
+				return
+			}
+			reports, _, err := source.PackageDiagnostics(ctx, snapshot, ph, false, snapshot.View().Options().DisabledAnalyses)
+			if err != nil {
+				log.Error(ctx, "diagnoseSnapshot: no diagnostics", err, telemetry.Package.Of(ph.ID()))
 				return
 			}
 			// Don't publish empty diagnostics.
 			s.publishReports(ctx, reports, false)
-		}(snapshot, fh)
+		}(id)
 	}
 	// Run diagnostics on the go.mod file.
 	s.diagnoseModfile(snapshot)
@@ -73,7 +63,7 @@ func (s *Server) diagnoseFile(snapshot source.Snapshot, fh source.FileHandle) {
 
 	ctx = telemetry.File.With(ctx, fh.Identity().URI)
 
-	reports, warningMsg, err := source.Diagnostics(ctx, snapshot, fh, true, snapshot.View().Options().DisabledAnalyses)
+	reports, warningMsg, err := source.FileDiagnostics(ctx, snapshot, fh, true, snapshot.View().Options().DisabledAnalyses)
 	// Check the warning message first.
 	if warningMsg != "" {
 		s.client.ShowMessage(ctx, &protocol.ShowMessageParams{

internal/lsp/lsp_test.go

@@ -96,7 +96,7 @@ func (r *runner) Diagnostics(t *testing.T, uri span.URI, want []source.Diagnosti
 		t.Fatal(err)
 	}
 	identity := fh.Identity()
-	results, _, err := source.Diagnostics(r.ctx, v.Snapshot(), fh, true, nil)
+	results, _, err := source.FileDiagnostics(r.ctx, v.Snapshot(), fh, true, nil)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -350,7 +350,7 @@ func (r *runner) SuggestedFix(t *testing.T, spn span.Span) {
 		t.Fatal(err)
 	}
 	fileID := fh.Identity()
-	diagnostics, _, err := source.Diagnostics(r.ctx, snapshot, fh, true, nil)
+	diagnostics, _, err := source.FileDiagnostics(r.ctx, snapshot, fh, true, nil)
 	if err != nil {
 		t.Fatal(err)
 	}

internal/lsp/source/diagnostics.go

@@ -40,10 +40,7 @@ type RelatedInformation struct {
 	Message string
 }
 
-func Diagnostics(ctx context.Context, snapshot Snapshot, fh FileHandle, withAnalysis bool, disabledAnalyses map[string]struct{}) (map[FileIdentity][]Diagnostic, string, error) {
-	ctx, done := trace.StartSpan(ctx, "source.Diagnostics", telemetry.File.Of(fh.Identity().URI))
-	defer done()
-
+func FileDiagnostics(ctx context.Context, snapshot Snapshot, fh FileHandle, withAnalysis bool, disabledAnalyses map[string]struct{}) (map[FileIdentity][]Diagnostic, string, error) {
 	if fh.Identity().Kind != Go {
 		return nil, "", errors.Errorf("unexpected file type: %q", fh.Identity().URI.Filename)
 	}
@@ -63,14 +60,24 @@ func Diagnostics(ctx context.Context, snapshot Snapshot, fh FileHandle, withAnal
 			log.Error(ctx, "error checking common errors", err, telemetry.File.Of(fh.Identity().URI))
 		}
 	}
+	reports, msg, err := PackageDiagnostics(ctx, snapshot, ph, withAnalysis, disabledAnalyses)
+	if warningMsg == "" {
+		warningMsg = msg
+	}
+	return reports, warningMsg, err
+}
+
+func PackageDiagnostics(ctx context.Context, snapshot Snapshot, ph PackageHandle, withAnalysis bool, disabledAnalyses map[string]struct{}) (map[FileIdentity][]Diagnostic, string, error) {
 	pkg, err := ph.Check(ctx)
 	if err != nil {
 		return nil, "", err
 	}
+	var warningMsg string
 	// If we have a package with a single file and errors about "undeclared" symbols,
 	// 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 warningMsg == "" && len(pkg.CompiledGoFiles()) == 1 && hasUndeclaredErrors(pkg) {
+	if len(pkg.CompiledGoFiles()) == 1 && hasUndeclaredErrors(pkg) {
+		fh := pkg.CompiledGoFiles()[0].File()
 		if warningMsg, err = checkCommonErrors(ctx, snapshot.View(), fh.Identity().URI); err != nil {
 			log.Error(ctx, "error checking common errors", err, telemetry.File.Of(fh.Identity().URI))
 		}
@@ -86,9 +93,13 @@ func Diagnostics(ctx context.Context, snapshot Snapshot, fh FileHandle, withAnal
 		if e.Kind != ListError {
 			continue
 		}
-		// If no file is associated with the error, default to the current file.
+		// If no file is associated with the error, pick an open file from the package.
 		if e.File.URI.Filename() == "" {
-			e.File = fh.Identity()
+			for _, ph := range pkg.CompiledGoFiles() {
+				if snapshot.View().Session().IsOpen(ph.File().Identity().URI) {
+					e.File = ph.File().Identity()
+				}
+			}
 		}
 		clearReports(snapshot, reports, e.File)
 	}
@@ -98,9 +109,9 @@ func Diagnostics(ctx context.Context, snapshot Snapshot, fh FileHandle, withAnal
 		if err := analyses(ctx, snapshot, ph, disabledAnalyses, reports); err != nil {
 			// Exit early if the context has been canceled.
 			if err == context.Canceled {
-				return nil, "", err
+				return nil, warningMsg, err
 			}
-			log.Error(ctx, "failed to run analyses", err, telemetry.File.Of(fh.Identity().URI))
+			log.Error(ctx, "failed to run analyses", err, telemetry.Package.Of(ph.ID()))
 		}
 	}
 	// Updates to the diagnostics for this package may need to be propagated.

internal/lsp/source/source_test.go

@@ -90,7 +90,7 @@ func (r *runner) Diagnostics(t *testing.T, uri span.URI, want []source.Diagnosti
 		t.Fatal(err)
 	}
 	fileID := fh.Identity()
-	results, _, err := source.Diagnostics(r.ctx, snapshot, fh, true, nil)
+	results, _, err := source.FileDiagnostics(r.ctx, snapshot, fh, true, nil)
 	if err != nil {
 		t.Fatal(err)
 	}