gopls/internal/cache: add removeIntermediateTestVariants parameter

This CL adds a removeIntermediateTestVariants parameter to
snapshot.MetadataForFile method.

As noted in the TODO:

"in nearly all cases the caller must use
[metadata.RemoveIntermediateTestVariants]. Make this a parameter to
force the caller to consider it (and reduce code)."

Also allows earlier removal before sorting.

Replaces sort.Slice with the more efficient slices.SortFunc.

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

Author: skewb1k

gopls/internal/cache/metadata/metadata.go

@@ -151,7 +151,7 @@ func (mp *Package) String() string { return string(mp.ID) }
 // intermediate test variant of p, of which there could be many.
 // Good code doesn't rely on such trickery.
 //
-// Most callers of MetadataForFile call RemoveIntermediateTestVariants
+// Most callers of MetadataForFile set removeIntermediateTestVariants parameter
 // to discard them before requesting type checking, or the products of
 // type-checking such as the cross-reference index or method set index.
 //

gopls/internal/cache/snapshot.go

@@ -6,6 +6,7 @@ package cache
 
 import (
 	"bytes"
+	"cmp"
 	"context"
 	"errors"
 	"fmt"
@@ -18,7 +19,6 @@ import (
 	"path/filepath"
 	"regexp"
 	"slices"
-	"sort"
 	"strconv"
 	"strings"
 	"sync"
@@ -652,11 +652,10 @@ func (s *Snapshot) Tests(ctx context.Context, ids ...PackageID) ([]*testfuncs.In
 // (the one with the fewest files) that encloses the specified file.
 // The result may be a test variant, but never an intermediate test variant.
 func (snapshot *Snapshot) NarrowestMetadataForFile(ctx context.Context, uri protocol.DocumentURI) (*metadata.Package, error) {
-	mps, err := snapshot.MetadataForFile(ctx, uri)
+	mps, err := snapshot.MetadataForFile(ctx, uri, true)
 	if err != nil {
 		return nil, err
 	}
-	metadata.RemoveIntermediateTestVariants(&mps)
 	if len(mps) == 0 {
 		return nil, fmt.Errorf("no package metadata for file %s", uri)
 	}
@@ -668,13 +667,10 @@ func (snapshot *Snapshot) NarrowestMetadataForFile(ctx context.Context, uri prot
 // number of CompiledGoFiles (i.e. "narrowest" to "widest" package),
 // and secondarily by IsIntermediateTestVariant (false < true).
 // The result may include tests and intermediate test variants of
-// importable packages.
+// importable packages. If removeIntermediateTestVariants is provided,
+// intermediate test variants will be excluded.
 // It returns an error if the context was cancelled.
-//
-// TODO(adonovan): in nearly all cases the caller must use
-// [metadata.RemoveIntermediateTestVariants]. Make this a parameter to
-// force the caller to consider it (and reduce code).
-func (s *Snapshot) MetadataForFile(ctx context.Context, uri protocol.DocumentURI) ([]*metadata.Package, error) {
+func (s *Snapshot) MetadataForFile(ctx context.Context, uri protocol.DocumentURI, removeIntermediateTestVariants bool) ([]*metadata.Package, error) {
 	if s.view.typ == AdHocView {
 		// As described in golang/go#57209, in ad-hoc workspaces (where we load ./
 		// rather than ./...), preempting the directory load with file loads can
@@ -712,7 +708,6 @@ func (s *Snapshot) MetadataForFile(ctx context.Context, uri protocol.DocumentURI
 		scope := fileLoadScope(uri)
 		err := s.load(ctx, NoNetwork, scope)
 
-		//
 		// Return the context error here as the current operation is no longer
 		// valid.
 		if err != nil {
@@ -752,23 +747,42 @@ func (s *Snapshot) MetadataForFile(ctx context.Context, uri protocol.DocumentURI
 		s.unloadableFiles.Add(uri)
 	}
 
+	if removeIntermediateTestVariants {
+		metadata.RemoveIntermediateTestVariants(&metas)
+	}
+
 	// Sort packages "narrowest" to "widest" (in practice:
 	// non-tests before tests), and regular packages before
 	// their intermediate test variants (which have the same
 	// files but different imports).
-	sort.Slice(metas, func(i, j int) bool {
-		x, y := metas[i], metas[j]
-		xfiles, yfiles := len(x.CompiledGoFiles), len(y.CompiledGoFiles)
-		if xfiles != yfiles {
-			return xfiles < yfiles
+	slices.SortFunc(metas, func(x, y *metadata.Package) int {
+		if sign := cmp.Compare(len(x.CompiledGoFiles), len(y.CompiledGoFiles)); sign != 0 {
+			return sign
+		}
+		// Skip ITV-specific ordering if they were removed.
+		if removeIntermediateTestVariants {
+			return 0
 		}
-		return boolLess(x.IsIntermediateTestVariant(), y.IsIntermediateTestVariant())
+		return boolCompare(x.IsIntermediateTestVariant(), y.IsIntermediateTestVariant())
 	})
 
 	return metas, nil
 }
 
-func boolLess(x, y bool) bool { return !x && y } // false < true
+// btoi returns int(b) as proposed in #64825.
+func btoi(b bool) int {
+	if b {
+		return 1
+	} else {
+		return 0
+	}
+}
+
+// boolCompare is a comparison function for booleans, returning -1 if x < y, 0
+// if x == y, and 1 if x > y, where false < true.
+func boolCompare(x, y bool) int {
+	return btoi(x) - btoi(y)
+}
 
 // ReverseDependencies returns a new mapping whose entries are
 // the ID and Metadata of each package in the workspace that
@@ -1252,7 +1266,7 @@ searchOverlays:
 		if s.IsBuiltin(uri) || s.FileKind(o) != file.Go {
 			continue
 		}
-		mps, err := s.MetadataForFile(ctx, uri)
+		mps, err := s.MetadataForFile(ctx, uri, true)
 		if err != nil {
 			return nil, err
 		}
@@ -1261,7 +1275,6 @@ searchOverlays:
 				continue searchOverlays
 			}
 		}
-		metadata.RemoveIntermediateTestVariants(&mps)
 
 		// With zero-config gopls (golang/go#57979), orphaned file diagnostics
 		// include diagnostics for orphaned files -- not just diagnostics relating
@@ -1341,6 +1354,7 @@ searchOverlays:
 				if s.view.folder.Env.GoVersion >= 18 {
 					if s.view.gowork != "" {
 						fix = fmt.Sprintf("To fix this problem, you can add this module to your go.work file (%s)", s.view.gowork)
+
 						cmd := command.NewRunGoWorkCommandCommand("Run `go work use`", command.RunGoWorkArgs{
 							ViewID: s.view.ID(),
 							Args:   []string{"use", modDir},

gopls/internal/cache/source.go

@@ -176,7 +176,7 @@ type found struct {
 
 func (s *goplsSource) resolveWorkspaceReferences(filename string, missing imports.References) ([]*imports.Result, error) {
 	uri := protocol.URIFromPath(filename)
-	mypkgs, err := s.snapshot.MetadataForFile(s.ctx, uri)
+	mypkgs, err := s.snapshot.MetadataForFile(s.ctx, uri, false)
 	if err != nil {
 		return nil, err
 	}

gopls/internal/golang/completion/package.go

@@ -215,13 +215,14 @@ func packageSuggestions(ctx context.Context, snapshot *cache.Snapshot, fileURI p
 		return candidate{obj: obj, name: name, detail: name, score: score}
 	}
 
-	matcher := fuzzy.NewMatcher(prefix)
 	var currentPackageName string
-	if variants, err := snapshot.MetadataForFile(ctx, fileURI); err == nil &&
-		len(variants) != 0 {
-		currentPackageName = string(variants[0].Name)
+	// TODO: consider propagating error.
+	if md, err := snapshot.NarrowestMetadataForFile(ctx, fileURI); err == nil {
+		currentPackageName = string(md.Name)
 	}
 
+	matcher := fuzzy.NewMatcher(prefix)
+
 	// Always try to suggest a main package
 	defer func() {
 		mainScore := lowScore

gopls/internal/golang/implementation.go

@@ -143,11 +143,10 @@ func implementationsMsets(ctx context.Context, snapshot *cache.Snapshot, pkg *ca
 		// enumerate all types within the package that satisfy the
 		// query type, even those defined local to a function.
 		declURI = protocol.URIFromPath(declPosn.Filename)
-		declMPs, err := snapshot.MetadataForFile(ctx, declURI)
+		declMPs, err := snapshot.MetadataForFile(ctx, declURI, true)
 		if err != nil {
 			return err
 		}
-		metadata.RemoveIntermediateTestVariants(&declMPs)
 		if len(declMPs) == 0 {
 			return fmt.Errorf("no packages for file %s", declURI)
 		}

gopls/internal/golang/references.go

@@ -110,7 +110,7 @@ func references(ctx context.Context, snapshot *cache.Snapshot, f file.Handle, pp
 // import declarations of all packages that directly import the target
 // package.
 func packageReferences(ctx context.Context, snapshot *cache.Snapshot, uri protocol.DocumentURI) ([]reference, error) {
-	metas, err := snapshot.MetadataForFile(ctx, uri)
+	metas, err := snapshot.MetadataForFile(ctx, uri, false)
 	if err != nil {
 		return nil, err
 	}
@@ -260,7 +260,7 @@ func ordinaryReferences(ctx context.Context, snapshot *cache.Snapshot, uri proto
 	// This may include the query pkg, and possibly other variants.
 	declPosn := safetoken.StartPosition(pkg.FileSet(), obj.Pos())
 	declURI := protocol.URIFromPath(declPosn.Filename)
-	variants, err := snapshot.MetadataForFile(ctx, declURI)
+	variants, err := snapshot.MetadataForFile(ctx, declURI, false)
 	if err != nil {
 		return nil, err
 	}

gopls/internal/golang/rename.go

@@ -494,11 +494,10 @@ func renameOrdinary(ctx context.Context, snapshot *cache.Snapshot, uri protocol.
 	// only package we need. (In case you're wondering why
 	// 'references' doesn't also want the widest variant: it
 	// computes the union across all variants.)
-	mps, err := snapshot.MetadataForFile(ctx, uri)
+	mps, err := snapshot.MetadataForFile(ctx, uri, true)
 	if err != nil {
 		return nil, err
 	}
-	metadata.RemoveIntermediateTestVariants(&mps)
 	if len(mps) == 0 {
 		return nil, fmt.Errorf("no package metadata for file %s", uri)
 	}
@@ -745,7 +744,7 @@ func renameReceivers(pkg *cache.Package, recv *types.Var, newName string, editMa
 // selectors used only in an ITV, but life is short. Also sin must be
 // punished.)
 func typeCheckReverseDependencies(ctx context.Context, snapshot *cache.Snapshot, declURI protocol.DocumentURI, transitive bool) ([]*cache.Package, error) {
-	variants, err := snapshot.MetadataForFile(ctx, declURI)
+	variants, err := snapshot.MetadataForFile(ctx, declURI, false)
 	if err != nil {
 		return nil, err
 	}

gopls/internal/golang/snapshot.go

@@ -52,11 +52,10 @@ func WidestPackageForFile(ctx context.Context, snapshot *cache.Snapshot, uri pro
 }
 
 func selectPackageForFile(ctx context.Context, snapshot *cache.Snapshot, uri protocol.DocumentURI, selector func([]*metadata.Package) *metadata.Package) (*cache.Package, *parsego.File, error) {
-	mps, err := snapshot.MetadataForFile(ctx, uri)
+	mps, err := snapshot.MetadataForFile(ctx, uri, true)
 	if err != nil {
 		return nil, nil, err
 	}
-	metadata.RemoveIntermediateTestVariants(&mps)
 	if len(mps) == 0 {
 		return nil, nil, fmt.Errorf("no package metadata for file %s", uri)
 	}