internal/lsp/source: eliminate GetTypeCheckDiagnostics

Most callers of source.Package.GetDiagnostics do it via
GetTypeCheckDiagnostics. Push its logic up or down as appropriate and
delete it.

Rather than requiring fully populated maps of diagnostics, which was
rather subtle, call storeDiagnostics for every Go file in the package.

Change-Id: If43b0cc922af1013e80f969362246538df14985b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/297878
Trust: Heschi Kreinick <heschi@google.com>
Run-TryBot: Heschi Kreinick <heschi@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>

internal/lsp/cache/pkg.go

@@ -86,8 +86,15 @@ func (p *pkg) GetSyntax() []*ast.File {
 	return syntax
 }
 
-func (p *pkg) GetDiagnostics() []*source.Diagnostic {
-	return p.diagnostics
+func (p *pkg) GetDiagnostics() map[span.URI][]*source.Diagnostic {
+	diags := map[span.URI][]*source.Diagnostic{}
+	for _, diag := range p.diagnostics {
+		// As of writing, source.Analyze modifies the diagnostics it receives.
+		// Shallow copy to avoid races.
+		clone := *diag
+		diags[diag.URI] = append(diags[diag.URI], &clone)
+	}
+	return diags
 }
 
 func (p *pkg) GetTypes() *types.Package {

internal/lsp/code_action.go

@@ -126,7 +126,7 @@ func (s *Server) codeAction(ctx context.Context, params *protocol.CodeActionPara
 			return nil, err
 		}
 		if (wanted[protocol.QuickFix] || wanted[protocol.SourceFixAll]) && len(diagnostics) > 0 {
-			pkgQuickFixes, err := quickFixesForDiagnostics(ctx, snapshot, diagnostics, pkg.GetDiagnostics())
+			pkgQuickFixes, err := quickFixesForDiagnostics(ctx, snapshot, diagnostics, pkg.GetDiagnostics()[fh.URI()])
 			if err != nil {
 				return nil, err
 			}

internal/lsp/diagnostics.go

@@ -245,14 +245,20 @@ func (s *Server) diagnose(ctx context.Context, snapshot source.Snapshot, forceAn
 func (s *Server) diagnosePkg(ctx context.Context, snapshot source.Snapshot, pkg source.Package, alwaysAnalyze bool) {
 	ctx, done := event.Start(ctx, "Server.diagnosePkg", tag.Snapshot.Of(snapshot.ID()), tag.Package.Of(pkg.ID()))
 	defer done()
+	enableDiagnostics := false
 	includeAnalysis := alwaysAnalyze // only run analyses for packages with open files
 	for _, pgf := range pkg.CompiledGoFiles() {
+		enableDiagnostics = enableDiagnostics || !snapshot.IgnoredFile(pgf.URI)
 		includeAnalysis = includeAnalysis || snapshot.IsOpen(pgf.URI)
 	}
+	// Don't show any diagnostics on ignored files.
+	if !enableDiagnostics {
+		return
+	}
 
-	typeCheckDiagnostics := source.GetTypeCheckDiagnostics(ctx, snapshot, pkg)
-	for uri, diags := range typeCheckDiagnostics {
-		s.storeDiagnostics(snapshot, uri, typeCheckSource, diags)
+	typeCheckDiagnostics := pkg.GetDiagnostics()
+	for _, cgf := range pkg.CompiledGoFiles() {
+		s.storeDiagnostics(snapshot, cgf.URI, typeCheckSource, typeCheckDiagnostics[cgf.URI])
 	}
 	if includeAnalysis && !pkg.HasListOrParseErrors() {
 		reports, err := source.Analyze(ctx, snapshot, pkg, typeCheckDiagnostics)
@@ -260,8 +266,8 @@ func (s *Server) diagnosePkg(ctx context.Context, snapshot source.Snapshot, pkg
 			event.Error(ctx, "warning: diagnose package", err, tag.Snapshot.Of(snapshot.ID()), tag.Package.Of(pkg.ID()))
 			return
 		}
-		for uri, diags := range reports {
-			s.storeDiagnostics(snapshot, uri, analysisSource, diags)
+		for _, cgf := range pkg.CompiledGoFiles() {
+			s.storeDiagnostics(snapshot, cgf.URI, analysisSource, reports[cgf.URI])
 		}
 	}
 

internal/lsp/mod/diagnostics.go

@@ -92,11 +92,7 @@ func DiagnosticsForMod(ctx context.Context, snapshot source.Snapshot, fh source.
 	}
 	if err == nil {
 		for _, pkg := range wspkgs {
-			for _, diag := range pkg.GetDiagnostics() {
-				if diag.URI == fh.URI() {
-					diagnostics = append(diagnostics, diag)
-				}
-			}
+			diagnostics = append(diagnostics, pkg.GetDiagnostics()[fh.URI()]...)
 		}
 	}
 

internal/lsp/source/diagnostics.go

@@ -24,25 +24,6 @@ type RelatedInformation struct {
 	Message string
 }
 
-func GetTypeCheckDiagnostics(ctx context.Context, snapshot Snapshot, pkg Package) map[span.URI][]*Diagnostic {
-	onlyIgnoredFiles := true
-	for _, pgf := range pkg.CompiledGoFiles() {
-		onlyIgnoredFiles = onlyIgnoredFiles && snapshot.IgnoredFile(pgf.URI)
-	}
-	if onlyIgnoredFiles {
-		return nil
-	}
-
-	diagSets := emptyDiagnostics(pkg)
-	for _, diag := range pkg.GetDiagnostics() {
-		diagSets[diag.URI] = append(diagSets[diag.URI], diag)
-	}
-	for uri, diags := range diagSets {
-		diagSets[uri] = cloneDiagnostics(diags)
-	}
-	return diagSets
-}
-
 func Analyze(ctx context.Context, snapshot Snapshot, pkg Package, typeCheckDiagnostics map[span.URI][]*Diagnostic) (map[span.URI][]*Diagnostic, error) {
 	// Exit early if the context has been canceled. This also protects us
 	// from a race on Options, see golang/go#36699.
@@ -57,7 +38,7 @@ func Analyze(ctx context.Context, snapshot Snapshot, pkg Package, typeCheckDiagn
 	}
 	analysisDiagnostics = cloneDiagnostics(analysisDiagnostics)
 
-	reports := emptyDiagnostics(pkg)
+	reports := map[span.URI][]*Diagnostic{}
 	// Report diagnostics and errors from root analyzers.
 	for _, diag := range analysisDiagnostics {
 		// If the diagnostic comes from a "convenience" analyzer, it is not
@@ -134,26 +115,16 @@ func FileDiagnostics(ctx context.Context, snapshot Snapshot, uri span.URI) (Vers
 	if err != nil {
 		return VersionedFileIdentity{}, nil, err
 	}
-	typeCheckDiagnostics := GetTypeCheckDiagnostics(ctx, snapshot, pkg)
-	diagnostics := typeCheckDiagnostics[fh.URI()]
+	diagnostics := pkg.GetDiagnostics()
+	fileDiags := diagnostics[fh.URI()]
 	if !pkg.HasListOrParseErrors() {
-		reports, err := Analyze(ctx, snapshot, pkg, typeCheckDiagnostics)
+		analysisDiags, err := Analyze(ctx, snapshot, pkg, diagnostics)
 		if err != nil {
 			return VersionedFileIdentity{}, nil, err
 		}
-		diagnostics = append(diagnostics, reports[fh.URI()]...)
+		fileDiags = append(fileDiags, analysisDiags[fh.URI()]...)
 	}
-	return fh.VersionedFileIdentity(), diagnostics, nil
-}
-
-func emptyDiagnostics(pkg Package) map[span.URI][]*Diagnostic {
-	diags := map[span.URI][]*Diagnostic{}
-	for _, pgf := range pkg.CompiledGoFiles() {
-		if _, ok := diags[pgf.URI]; !ok {
-			diags[pgf.URI] = nil
-		}
-	}
-	return diags
+	return fh.VersionedFileIdentity(), fileDiags, nil
 }
 
 // onlyDeletions returns true if all of the suggested fixes are deletions.

internal/lsp/source/rename_check.go

@@ -812,7 +812,7 @@ func (r *renamer) satisfy() map[satisfy.Constraint]bool {
 			// type-checker.
 			//
 			// Only proceed if all packages have no errors.
-			if diags := pkg.GetDiagnostics(); len(diags) > 0 {
+			if pkg.HasListOrParseErrors() || pkg.HasTypeErrors() {
 				r.errorf(token.NoPos, // we don't have a position for this error.
 					"renaming %q to %q not possible because %q has errors",
 					r.from, r.to, pkg.PkgPath())

internal/lsp/source/view.go

@@ -548,7 +548,7 @@ type Package interface {
 	CompiledGoFiles() []*ParsedGoFile
 	File(uri span.URI) (*ParsedGoFile, error)
 	GetSyntax() []*ast.File
-	GetDiagnostics() []*Diagnostic
+	GetDiagnostics() map[span.URI][]*Diagnostic
 	GetTypes() *types.Package
 	GetTypesInfo() *types.Info
 	GetTypesSizes() types.Sizes