From f345449c09b356db134114ed7e407b4c1eedc55c Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Fri, 29 Mar 2024 17:29:20 -0400 Subject: [PATCH] gopls/internal/server: filter diagnostics to "best" views Filter diagnostics only to the "best" view for a file. This reduces the likelihood that we show spurious import diagnostics due to module graph pruning, as reported by golang/go#66425. Absent a reproducer this is hard to test, yet the change makes intuitive sense (arguably): it is confusing if diagnostics are inconsistent with other operations like jump-to-definition that find the "best" view. Fixes golang/go#66425 Change-Id: Iadb1a01518a30cc3dad2d412b1ded612ab35d6cc Reviewed-on: https://go-review.googlesource.com/c/tools/+/574718 Reviewed-by: Alan Donovan LUCI-TryBot-Result: Go LUCI --- gopls/internal/cache/session.go | 42 +++++++++++-------- .../internal/golang/completion/completion.go | 2 +- gopls/internal/server/diagnostics.go | 30 ++++++++++++- 3 files changed, 55 insertions(+), 19 deletions(-) diff --git a/gopls/internal/cache/session.go b/gopls/internal/cache/session.go index 3ad78336c5c..05ed0694148 100644 --- a/gopls/internal/cache/session.go +++ b/gopls/internal/cache/session.go @@ -533,27 +533,17 @@ checkFiles: // Views and viewDefinitions. type viewDefiner interface{ definition() *viewDefinition } -// bestView returns the best View or viewDefinition that contains the -// given file, or (nil, nil) if no matching view is found. -// -// bestView only returns an error in the event of context cancellation. -// -// Making this function generic is convenient so that we can avoid mapping view -// definitions back to views inside Session.DidModifyFiles, where performance -// matters. It is, however, not the cleanest application of generics. +// BestViews returns the most relevant subset of views for a given uri. // -// Note: keep this function in sync with defineView. -func bestView[V viewDefiner](ctx context.Context, fs file.Source, fh file.Handle, views []V) (V, error) { - var zero V - +// This may be used to filter diagnostics to the most relevant builds. +func BestViews[V viewDefiner](ctx context.Context, fs file.Source, uri protocol.DocumentURI, views []V) ([]V, error) { if len(views) == 0 { - return zero, nil // avoid the call to findRootPattern + return nil, nil // avoid the call to findRootPattern } - uri := fh.URI() dir := uri.Dir() modURI, err := findRootPattern(ctx, dir, "go.mod", fs) if err != nil { - return zero, err + return nil, err } // Prefer GoWork > GoMod > GOPATH > GoPackages > AdHoc. @@ -631,8 +621,26 @@ func bestView[V viewDefiner](ctx context.Context, fs file.Source, fh file.Handle bestViews = goPackagesViews case len(adHocViews) > 0: bestViews = adHocViews - default: - return zero, nil + } + + return bestViews, nil +} + +// bestView returns the best View or viewDefinition that contains the +// given file, or (nil, nil) if no matching view is found. +// +// bestView only returns an error in the event of context cancellation. +// +// Making this function generic is convenient so that we can avoid mapping view +// definitions back to views inside Session.DidModifyFiles, where performance +// matters. It is, however, not the cleanest application of generics. +// +// Note: keep this function in sync with defineView. +func bestView[V viewDefiner](ctx context.Context, fs file.Source, fh file.Handle, views []V) (V, error) { + var zero V + bestViews, err := BestViews(ctx, fs, fh.URI(), views) + if err != nil || len(bestViews) == 0 { + return zero, err } content, err := fh.Content() diff --git a/gopls/internal/golang/completion/completion.go b/gopls/internal/golang/completion/completion.go index b659529a08b..4da492762c8 100644 --- a/gopls/internal/golang/completion/completion.go +++ b/gopls/internal/golang/completion/completion.go @@ -497,7 +497,7 @@ func Completion(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle, p items, surrounding, innerErr := packageClauseCompletions(ctx, snapshot, fh, protoPos) if innerErr != nil { // return the error for GetParsedFile since it's more relevant in this situation. - return nil, nil, fmt.Errorf("getting file %s for Completion: %w (package completions: %v)", fh.URI(), err, innerErr) + return nil, nil, fmt.Errorf("getting file %s for Completion: %v (package completions: %v)", fh.URI(), err, innerErr) } return items, surrounding, nil } diff --git a/gopls/internal/server/diagnostics.go b/gopls/internal/server/diagnostics.go index 37c430ae956..d5808d42f30 100644 --- a/gopls/internal/server/diagnostics.go +++ b/gopls/internal/server/diagnostics.go @@ -831,7 +831,6 @@ func (s *server) updateOrphanedFileDiagnostics(ctx context.Context, modID uint64 // // If the publication succeeds, it updates f.publishedHash and f.mustPublish. func (s *server) publishFileDiagnosticsLocked(ctx context.Context, views viewSet, uri protocol.DocumentURI, version int32, f *fileDiagnostics) error { - // We add a disambiguating suffix (e.g. " [darwin,arm64]") to // each diagnostic that doesn't occur in the default view; // see golang/go#65496. @@ -851,6 +850,8 @@ func (s *server) publishFileDiagnosticsLocked(ctx context.Context, views viewSet for _, diag := range f.orphanedFileDiagnostics { add(diag, "") } + + var allViews []*cache.View for view, viewDiags := range f.byView { if _, ok := views[view]; !ok { delete(f.byView, view) // view no longer exists @@ -859,7 +860,34 @@ func (s *server) publishFileDiagnosticsLocked(ctx context.Context, views viewSet if viewDiags.version != version { continue // a payload of diagnostics applies to a specific file version } + allViews = append(allViews, view) + } + + // Only report diagnostics from the best views for a file. This avoids + // spurious import errors when a view has only a partial set of dependencies + // for a package (golang/go#66425). + // + // It's ok to use the session to derive the eligible views, because we + // publish diagnostics following any state change, so the set of best views + // is eventually consistent. + bestViews, err := cache.BestViews(ctx, s.session, uri, allViews) + if err != nil { + return err + } + + if len(bestViews) == 0 { + // If we have no preferred diagnostics for a given file (i.e., the file is + // not naturally nested within a view), then all diagnostics should be + // considered valid. + // + // This could arise if the user jumps to definition outside the workspace. + // There is no view that owns the file, so its diagnostics are valid from + // any view. + bestViews = allViews + } + for _, view := range bestViews { + viewDiags := f.byView[view] // Compute the view's suffix (e.g. " [darwin,arm64]"). var suffix string {