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 <adonovan@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

Author: findleyr

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()

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
 }

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
 {