From d49f960b6a980c6c48994c7356e751978ce08fb6 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Wed, 31 Aug 2022 14:41:23 -0400 Subject: [PATCH] internal/lsp/cache: report analysis panics during testing Report a bug when an analysis dependency is found to be neither "vertical" (for facts) or "horizontal" (for results). This has been observed in practise: the lookup from a package ID to a package object is not consistent. The bug report is suppressed for command-line-arguments packages to see whether it occurs on ordinary packages too. Also, add disabled logic to disable recovery and dump all goroutines, for temporary use when debugging. Unrelated things found during debugging: - simplify package key used in analysis cache: the mode is always the same constant. - move TypeErrorAnalyzers logic closer to where needed. Updates golang/go#54655 Updates golang/go#54762 Change-Id: I0c2afd9ddd4fcc21748a03f8fa0769a2de09a56f Reviewed-on: https://go-review.googlesource.com/c/tools/+/426018 Reviewed-by: Robert Findley --- gopls/internal/lsp/cache/analysis.go | 74 +++++++++++++++++++--------- gopls/internal/lsp/cache/maps.go | 2 +- gopls/internal/lsp/cache/snapshot.go | 2 +- 3 files changed, 54 insertions(+), 24 deletions(-) diff --git a/gopls/internal/lsp/cache/analysis.go b/gopls/internal/lsp/cache/analysis.go index 0b5ce8995e..e7f2d41542 100644 --- a/gopls/internal/lsp/cache/analysis.go +++ b/gopls/internal/lsp/cache/analysis.go @@ -10,14 +10,16 @@ import ( "go/ast" "go/types" "reflect" + "runtime/debug" "sync" "golang.org/x/sync/errgroup" "golang.org/x/tools/go/analysis" + "golang.org/x/tools/gopls/internal/lsp/source" "golang.org/x/tools/internal/analysisinternal" + "golang.org/x/tools/internal/bug" "golang.org/x/tools/internal/event" "golang.org/x/tools/internal/event/tag" - "golang.org/x/tools/gopls/internal/lsp/source" "golang.org/x/tools/internal/memoize" "golang.org/x/tools/internal/span" ) @@ -60,7 +62,7 @@ func (s *snapshot) Analyze(ctx context.Context, id string, analyzers []*source.A } type actionKey struct { - pkg packageKey + pkgid PackageID analyzer *analysis.Analyzer } @@ -102,11 +104,7 @@ type packageFactKey struct { } func (s *snapshot) actionHandle(ctx context.Context, id PackageID, a *analysis.Analyzer) (*actionHandle, error) { - const mode = source.ParseFull - key := actionKey{ - pkg: packageKey{id: id, mode: mode}, - analyzer: a, - } + key := actionKey{id, a} s.mu.Lock() entry, hit := s.actions.Get(key) @@ -126,7 +124,7 @@ func (s *snapshot) actionHandle(ctx context.Context, id PackageID, a *analysis.A // use of concurrency would lead to an exponential amount of duplicated // work. We should instead use an atomically updated future cache // and a parallel graph traversal. - ph, err := s.buildPackageHandle(ctx, id, mode) + ph, err := s.buildPackageHandle(ctx, id, source.ParseFull) if err != nil { return nil, err } @@ -138,6 +136,8 @@ func (s *snapshot) actionHandle(ctx context.Context, id PackageID, a *analysis.A // Add a dependency on each required analyzer. var deps []*actionHandle for _, req := range a.Requires { + // TODO(adonovan): opt: there's no need to repeat the package-handle + // portion of the recursion here, since we have the pkg already. reqActionHandle, err := s.actionHandle(ctx, id, req) if err != nil { return nil, err @@ -227,7 +227,8 @@ func actionImpl(ctx context.Context, snapshot *snapshot, deps []*actionHandle, a // in-memory outputs of prerequisite analyzers // become inputs to this analysis pass. inputs[dep.analyzer] = data.result - } else if dep.analyzer == analyzer { // (always true) + + } else if dep.analyzer == analyzer { // Same analysis, different package (vertical edge): // serialized facts produced by prerequisite analysis // become available to this analysis pass. @@ -246,12 +247,34 @@ func actionImpl(ctx context.Context, snapshot *snapshot, deps []*actionHandle, a // to prevent side channels. packageFacts[key] = fact } + + } else { + // Edge is neither vertical nor horizontal. + // This should never happen, yet an assertion here was + // observed to fail due to an edge (bools, p) -> (inspector, p') + // where p and p' are distinct packages with the + // same ID ("command-line-arguments:file=.../main.go"). + // + // It is not yet clear whether the command-line-arguments + // package is significant, but it is clear that package + // loading (the mapping from ID to *pkg) is inconsistent + // within a single graph. + + // Use the bug package so that we detect whether our tests + // discover this problem in regular packages. + // For command-line-arguments we quietly abort the analysis + // for now since we already know there is a bug. + errorf := bug.Errorf // report this discovery + if source.IsCommandLineArguments(pkg.ID()) { + errorf = fmt.Errorf // suppress reporting + } + return errorf("internal error: unexpected analysis dependency %s@%s -> %s", analyzer.Name, pkg.ID(), dep) } return nil }) } if err := g.Wait(); err != nil { - return nil, err // e.g. cancelled + return nil, err // cancelled, or dependency failed } // Now run the (pkg, analyzer) analysis. @@ -338,13 +361,19 @@ func actionImpl(ctx context.Context, snapshot *snapshot, deps []*actionHandle, a var result interface{} var err error func() { - defer func() { - if r := recover(); r != nil { - // TODO(adonovan): use bug.Errorf here so that we - // detect crashes covered by our test suite. - err = fmt.Errorf("analysis %s for package %s panicked: %v", analyzer.Name, pkg.PkgPath(), r) - } - }() + // Set this flag temporarily when debugging crashes. + // See https://github.com/golang/go/issues/54762. + const norecover = false + if norecover { + debug.SetTraceback("all") // show all goroutines + } else { + defer func() { + if r := recover(); r != nil { + // Use bug.Errorf so that we detect panics during testing. + err = bug.Errorf("analysis %s for package %s panicked: %v", analyzer.Name, pkg.PkgPath(), r) + } + }() + } result, err = pass.Analyzer.Run(pass) }() if err != nil { @@ -414,13 +443,14 @@ func factType(fact analysis.Fact) reflect.Type { func (s *snapshot) DiagnosePackage(ctx context.Context, spkg source.Package) (map[span.URI][]*source.Diagnostic, error) { pkg := spkg.(*pkg) - // Apply type error analyzers. They augment type error diagnostics with their own fixes. - var analyzers []*source.Analyzer - for _, a := range s.View().Options().TypeErrorAnalyzers { - analyzers = append(analyzers, a) - } var errorAnalyzerDiag []*source.Diagnostic if pkg.HasTypeErrors() { + // Apply type error analyzers. + // They augment type error diagnostics with their own fixes. + var analyzers []*source.Analyzer + for _, a := range s.View().Options().TypeErrorAnalyzers { + analyzers = append(analyzers, a) + } var err error errorAnalyzerDiag, err = s.Analyze(ctx, pkg.ID(), analyzers) if err != nil { diff --git a/gopls/internal/lsp/cache/maps.go b/gopls/internal/lsp/cache/maps.go index 0493398798..d3fe4e45e5 100644 --- a/gopls/internal/lsp/cache/maps.go +++ b/gopls/internal/lsp/cache/maps.go @@ -212,5 +212,5 @@ func actionKeyLessInterface(a, b interface{}) bool { if x.analyzer.Name != y.analyzer.Name { return x.analyzer.Name < y.analyzer.Name } - return packageKeyLess(x.pkg, y.pkg) + return x.pkgid < y.pkgid } diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go index 4398142eb0..d1d2b1fb7b 100644 --- a/gopls/internal/lsp/cache/snapshot.go +++ b/gopls/internal/lsp/cache/snapshot.go @@ -1880,7 +1880,7 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC var actionsToDelete []actionKey s.actions.Range(func(k, _ interface{}) { key := k.(actionKey) - if _, ok := idsToInvalidate[key.pkg.id]; ok { + if _, ok := idsToInvalidate[key.pkgid]; ok { actionsToDelete = append(actionsToDelete, key) } })