From 24439e3c78b400661460bbf8b5bce90ea2d40673 Mon Sep 17 00:00:00 2001 From: Heschi Kreinick Date: Sun, 28 Feb 2021 00:30:59 -0500 Subject: [PATCH] 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 Run-TryBot: Heschi Kreinick gopls-CI: kokoro TryBot-Result: Go Bot Reviewed-by: Robert Findley --- internal/lsp/cache/pkg.go | 11 ++++++-- internal/lsp/code_action.go | 2 +- internal/lsp/diagnostics.go | 16 +++++++---- internal/lsp/mod/diagnostics.go | 6 +---- internal/lsp/source/diagnostics.go | 41 +++++------------------------ internal/lsp/source/rename_check.go | 2 +- internal/lsp/source/view.go | 2 +- 7 files changed, 30 insertions(+), 50 deletions(-) diff --git a/internal/lsp/cache/pkg.go b/internal/lsp/cache/pkg.go index 5badb33e8e..1ac6c831eb 100644 --- a/internal/lsp/cache/pkg.go +++ b/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 { diff --git a/internal/lsp/code_action.go b/internal/lsp/code_action.go index 6c41dc3539..952922bf98 100644 --- a/internal/lsp/code_action.go +++ b/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 } diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go index 8b9305881f..f91039813a 100644 --- a/internal/lsp/diagnostics.go +++ b/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]) } } diff --git a/internal/lsp/mod/diagnostics.go b/internal/lsp/mod/diagnostics.go index e515adae10..ca636f4a19 100644 --- a/internal/lsp/mod/diagnostics.go +++ b/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()]...) } } diff --git a/internal/lsp/source/diagnostics.go b/internal/lsp/source/diagnostics.go index f9b82de845..3e90d64ad7 100644 --- a/internal/lsp/source/diagnostics.go +++ b/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. diff --git a/internal/lsp/source/rename_check.go b/internal/lsp/source/rename_check.go index 0f1bf9244b..a46254c3cd 100644 --- a/internal/lsp/source/rename_check.go +++ b/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()) diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index 2e06670342..a0e5252468 100644 --- a/internal/lsp/source/view.go +++ b/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