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 <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>

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
 		{