gopls/internal/lsp/cache: fix nil panic in analysis toSourceDiagnostic
The set of enabled analyzers is expanded to include requirements,
but we should not use the expanded set in the final loop when
we map back to source.Analyzers. Nonetheless, it is surprising
that any analyzer only in the expandes set should report a
diagnostics, so I've added an assertion.
Fixes golang/go#60909
Change-Id: I3b6e21194f7d717628822b933e6f4a387f719a1c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/504818
Run-TryBot: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
diff --git a/gopls/internal/lsp/cache/analysis.go b/gopls/internal/lsp/cache/analysis.go
index d9a457f..ad55f2f 100644
--- a/gopls/internal/lsp/cache/analysis.go
+++ b/gopls/internal/lsp/cache/analysis.go
@@ -176,7 +176,7 @@
// Filter and sort enabled root analyzers.
// A disabled analyzer may still be run if required by another.
toSrc := make(map[*analysis.Analyzer]*source.Analyzer)
- var enabled []*analysis.Analyzer
+ var enabled []*analysis.Analyzer // enabled subset + transitive requirements
for _, a := range analyzers {
if a.IsEnabled(snapshot.view.Options()) {
toSrc[a.Analyzer] = a
@@ -190,16 +190,16 @@
// Register fact types of required analyzers.
enabled = requiredAnalyzers(enabled)
- var useFacts []*analysis.Analyzer
+ var facty []*analysis.Analyzer // facty subset of enabled + transitive requirements
for _, a := range enabled {
if len(a.FactTypes) > 0 {
- useFacts = append(useFacts, a)
+ facty = append(facty, a)
for _, f := range a.FactTypes {
gob.Register(f) // <2us
}
}
}
- useFacts = requiredAnalyzers(useFacts)
+ facty = requiredAnalyzers(facty)
// File set for this batch (entire graph) of analysis.
fset := token.NewFileSet()
@@ -208,7 +208,9 @@
// build the DAG of packages we're going to analyze.
//
// Root nodes will run the enabled set of analyzers,
- // whereas dependencies will run only the useFacts set.
+ // whereas dependencies will run only the facty set.
+ // Because (by construction) enabled is a superset of facty,
+ // we can analyze each node with exactly one set of analyzers.
nodes := make(map[PackageID]*analysisNode)
var leaves []*analysisNode // nodes with no unfinished successors
var makeNode func(from *analysisNode, id PackageID) (*analysisNode, error)
@@ -225,7 +227,7 @@
an = &analysisNode{
fset: fset,
m: m,
- analyzers: useFacts, // all nodes run at least the facty analyzers
+ analyzers: facty, // all nodes run at least the facty analyzers
allDeps: make(map[PackagePath]*analysisNode),
exportDeps: make(map[PackagePath]*analysisNode),
}
@@ -338,6 +340,23 @@
var results []*source.Diagnostic
for _, root := range roots {
for _, a := range enabled {
+ // Skip analyzers that were added only to
+ // fulfil requirements of the original set.
+ srcAnalyzer, ok := toSrc[a]
+ if !ok {
+ // Although this 'skip' operation is logically sound,
+ // it is nonetheless surprising that its absence should
+ // cause #60909 since none of the analyzers added for
+ // requirements (e.g. ctrlflow, inspect, buildssa)
+ // is capable of reporting diagnostics.
+ if summary := root.summary.Actions[a.Name]; summary != nil {
+ if n := len(summary.Diagnostics); n > 0 {
+ bug.Reportf("Internal error: got %d unexpected diagnostics from analyzer %s. This analyzer was added only to fulfil the requirements of the requested set of analyzers, and it is not expected that such analyzers report diagnostics. Please report this in issue #60909.", n, a)
+ }
+ }
+ continue
+ }
+
// Inv: root.summary is the successful result of run (via runCached).
summary, ok := root.summary.Actions[a.Name]
if summary == nil {
@@ -348,7 +367,7 @@
continue // action failed
}
for _, gobDiag := range summary.Diagnostics {
- results = append(results, toSourceDiagnostic(toSrc[a], &gobDiag))
+ results = append(results, toSourceDiagnostic(srcAnalyzer, &gobDiag))
}
}
}
