gopls/internal/cache: use a better view in viewOfLocked

The 'bestView' function was used in two places, where the meaning of
'best' differed. When re-evaluating view definitions in selectViewDefs,
we may want to create a new view if none of them matched build tags.
When operating on a file in viewOfLocked, we want to choose the most
relevant view out of the existing view definitions.

In golang/go#60776, we see that the latter concern was poorly handled by
the 'bestView' abstraction. Returning nil was not, in fact, best,
because it resulted in the file being associated with the default AdHoc
view, which doesn't know about modules.

Refactor so that viewOfLocked chooses the most relevant view, even if
none match build tags. This causes the orphaned file diagnostic to more
accurately report that the file is excluded due to build tags.

Fixes golang/go#60776

Change-Id: I40f236b3b63468faa1dfe6ae6aeac590c952594f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/588941
Reviewed-by: Alan Donovan <[email protected]>
Auto-Submit: Robert Findley <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>

Author: findleyr

gopls/internal/cache/session.go

@@ -427,11 +427,12 @@ func (s *Session) SnapshotOf(ctx context.Context, uri protocol.DocumentURI) (*Sn
 // we have no view containing a file.
 var errNoViews = errors.New("no views")
 
-// viewOfLocked wraps bestViewForURI, memoizing its result.
+// viewOfLocked evaluates the best view for uri, memoizing its result in
+// s.viewMap.
 //
 // Precondition: caller holds s.viewMu lock.
 //
-// May return (nil, nil).
+// May return (nil, nil) if no best view can be determined.
 func (s *Session) viewOfLocked(ctx context.Context, uri protocol.DocumentURI) (*View, error) {
 	v, hit := s.viewMap[uri]
 	if !hit {
@@ -440,10 +441,19 @@ func (s *Session) viewOfLocked(ctx context.Context, uri protocol.DocumentURI) (*
 		if err != nil {
 			return nil, err
 		}
-		v, err = bestView(ctx, s, fh, s.views)
+		bestViews, err := BestViews(ctx, s, fh.URI(), s.views)
 		if err != nil {
 			return nil, err
 		}
+		v = matchingView(fh, bestViews)
+		if v == nil && len(bestViews) > 0 {
+			// If we have candidate views, but none of them matched the file's build
+			// constraints, then we are still better off using one of them here.
+			// Otherwise, logic may fall back to an inferior view, which lacks
+			// relevant module information, leading to misleading diagnostics.
+			// (as in golang/go#60776).
+			v = bestViews[0]
+		}
 		if s.viewMap == nil {
 			return nil, errors.New("session is shut down")
 		}
@@ -517,12 +527,13 @@ checkFiles:
 		if err != nil {
 			return nil, err
 		}
-		def, err := bestView(ctx, fs, fh, defs)
+		bestViews, err := BestViews(ctx, fs, fh.URI(), defs)
 		if err != nil {
 			// We should never call selectViewDefs with a cancellable context, so
 			// this should never fail.
 			return nil, bug.Errorf("failed to find best view for open file: %v", err)
 		}
+		def := matchingView(fh, bestViews)
 		if def != nil {
 			continue // file covered by an existing view
 		}
@@ -646,30 +657,28 @@ func BestViews[V viewDefiner](ctx context.Context, fs file.Source, uri protocol.
 	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.
+// matchingView returns the View or viewDefinition out of bestViews that
+// matches the given file's build constraints, or nil if no match is found.
 //
 // 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) {
+func matchingView[V viewDefiner](fh file.Handle, bestViews []V) V {
 	var zero V
-	bestViews, err := BestViews(ctx, fs, fh.URI(), views)
-	if err != nil || len(bestViews) == 0 {
-		return zero, err
+
+	if len(bestViews) == 0 {
+		return zero
 	}
 
 	content, err := fh.Content()
+
 	// Port matching doesn't apply to non-go files, or files that no longer exist.
 	// Note that the behavior here on non-existent files shouldn't matter much,
-	// since there will be a subsequent failure. But it is simpler to preserve
-	// the invariant that bestView only fails on context cancellation.
+	// since there will be a subsequent failure.
 	if fileKind(fh) != file.Go || err != nil {
-		return bestViews[0], nil
+		return bestViews[0]
 	}
 
 	// Find the first view that matches constraints.
@@ -680,11 +689,11 @@ func bestView[V viewDefiner](ctx context.Context, fs file.Source, fh file.Handle
 		def := v.definition()
 		viewPort := port{def.GOOS(), def.GOARCH()}
 		if viewPort.matches(path, content) {
-			return v, nil
+			return v
 		}
 	}
 
-	return zero, nil // no view found
+	return zero // no view found
 }
 
 // updateViewLocked recreates the view with the given options.

gopls/internal/cache/snapshot.go

@@ -1424,11 +1424,18 @@ searchOverlays:
 		if initialErr != nil {
 			msg = fmt.Sprintf("initialization failed: %v", initialErr.MainError)
 		} else if goMod, err := nearestModFile(ctx, fh.URI(), s); err == nil && goMod != "" {
+			// Check if the file's module should be loadable by considering both
+			// loaded modules and workspace modules. The former covers cases where
+			// the file is outside of a workspace folder. The latter covers cases
+			// where the file is inside a workspace module, but perhaps no packages
+			// were loaded for that module.
+			_, loadedMod := loadedModFiles[goMod]
+			_, workspaceMod := s.view.viewDefinition.workspaceModFiles[goMod]
 			// If we have a relevant go.mod file, check whether the file is orphaned
 			// due to its go.mod file being inactive. We could also offer a
-			// prescriptive diagnostic in the case that there is no go.mod file, but it
-			// is harder to be precise in that case, and less important.
-			if _, ok := loadedModFiles[goMod]; !ok {
+			// prescriptive diagnostic in the case that there is no go.mod file, but
+			// it is harder to be precise in that case, and less important.
+			if !(loadedMod || workspaceMod) {
 				modDir := filepath.Dir(goMod.Path())
 				viewDir := s.view.folder.Dir.Path()
 

gopls/internal/test/marker/testdata/zeroconfig/nested.txt

@@ -36,6 +36,14 @@ func _() {
 	fmt.Println(undef) //@diag("undef", re"undefined|undeclared")
 }
 
+-- mod1/a/tagged.go --
+//go:build tag1
+
+// golang/go#60776: verify that we get an accurate error about build tags
+// here, rather than an inaccurate error suggesting to add a go.work
+// file (which won't help).
+package a //@diag(re`package (a)`, re`excluded due to its build tags`)
+
 -- mod1/b/b.go --
 package b