gopls/internal/lsp/cache: add assertions
This change documents and asserts a few invariants that should
be maintained by the code. The crash in golang/go#60551 shows
that they are not, but I still can't see the mistake in my proof.
Updates golang/go#60551
Change-Id: I833f7575f1d7372837ab5d7ba5988c94650ce07f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/500055
Auto-Submit: Alan Donovan <adonovan@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@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 4679041..04e2651 100644
--- a/gopls/internal/lsp/cache/analysis.go
+++ b/gopls/internal/lsp/cache/analysis.go
@@ -23,7 +23,6 @@
"sort"
"strings"
"sync"
- "time"
"golang.org/x/sync/errgroup"
"golang.org/x/tools/go/analysis"
@@ -32,6 +31,7 @@
"golang.org/x/tools/gopls/internal/lsp/protocol"
"golang.org/x/tools/gopls/internal/lsp/source"
"golang.org/x/tools/internal/event"
+ "golang.org/x/tools/internal/event/tag"
"golang.org/x/tools/internal/facts"
"golang.org/x/tools/internal/gcimporter"
"golang.org/x/tools/internal/memoize"
@@ -164,8 +164,6 @@
// Destroy()
// - share cache.{goVersionRx,parseGoImpl}
-var born = time.Now()
-
// Analyze applies a set of analyzers to the package denoted by id,
// and returns their diagnostics for that package.
//
@@ -174,9 +172,8 @@
// Precondition: all analyzers within the process have distinct names.
// (The names are relied on by the serialization logic.)
func (s *snapshot) Analyze(ctx context.Context, id PackageID, analyzers []*source.Analyzer) ([]*source.Diagnostic, error) {
- if false { // debugging
- log.Println("Analyze@", time.Since(born)) // called after the 7s IWL in k8s
- }
+ ctx, done := event.Start(ctx, "snapshot.Analyze", tag.Package.Of(string(id)))
+ defer done()
// Filter and sort enabled root analyzers.
// A disabled analyzer may still be run if required by another.
@@ -199,19 +196,13 @@
}
}
- if false { // debugging
- // TODO(adonovan): use proper tracing.
- t0 := time.Now()
- defer func() {
- log.Printf("%v for analyze(%s, %s)", time.Since(t0), id, enabled)
- }()
- }
-
// Run the analysis.
res, err := s.analyze(ctx, id, enabled)
if err != nil {
return nil, err
}
+ // Inv: res is the successful result of analyzeImpl(analyzers, id),
+ // which augments the successful result of actuallyAnalyze.
// Report diagnostics only from enabled actions that succeeded.
// Errors from creating or analyzing packages are ignored.
@@ -225,7 +216,11 @@
// results, we should propagate the per-action errors.
var results []*source.Diagnostic
for _, a := range enabled {
- summary := res.Actions[a.Name]
+ summary, ok := res.Actions[a.Name]
+ if summary == nil {
+ panic(fmt.Sprintf("analyzeSummary.Actions[%q] = (nil, %t); got %v (#60551)",
+ a.Name, ok, res.Actions))
+ }
if summary.Err != "" {
continue // action failed
}
@@ -309,7 +304,7 @@
// analyze is a memoization of analyzeImpl.
func (s *snapshot) analyze(ctx context.Context, id PackageID, analyzers []*analysis.Analyzer) (*analyzeSummary, error) {
- // Use the sorted list of names of analyzers in the key.
+ // Use the caller-sorted list of names of analyzers in the key.
//
// TODO(adonovan): opt: account for analysis results at a
// finer grain to avoid duplicate work when a
@@ -568,6 +563,9 @@
// actuallyAnalyze implements the cache-miss case.
// This function does not access the snapshot.
+//
+// Postcondition: on success, the analyzeSummary.Actions
+// key set is {a.Name for a in analyzers}.
func actuallyAnalyze(ctx context.Context, analyzers []*analysis.Analyzer, m *source.Metadata, vdeps map[PackageID]*analyzeSummary, compiledGoFiles []source.FileHandle) (*analyzeSummary, error) {
// Create a local FileSet for processing this package only.
fset := token.NewFileSet()
@@ -637,6 +635,7 @@
// Execute the graph in parallel.
execActions(roots)
+ // Inv: each root's summary is set (whether success or error).
// Don't return (or cache) the result in case of cancellation.
if err := ctx.Err(); err != nil {
@@ -645,8 +644,11 @@
// Return summaries only for the requested actions.
summaries := make(map[string]*actionSummary)
- for _, act := range roots {
- summaries[act.a.Name] = act.summary
+ for _, root := range roots {
+ if root.summary == nil {
+ panic("root has nil action.summary (#60551)")
+ }
+ summaries[root.a.Name] = root.summary
}
return &analyzeSummary{
@@ -897,6 +899,7 @@
}
// execActions executes a set of action graph nodes in parallel.
+// Postcondition: each action.summary is set, even in case of error.
func execActions(actions []*action) {
var wg sync.WaitGroup
for _, act := range actions {
@@ -917,6 +920,9 @@
}
}
})
+ if act.summary == nil {
+ panic("nil action.summary (#60551)")
+ }
}()
}
wg.Wait()
