gopls/internal/lsp/source: filter intermediate test variants
This change ensures that intermediate test variant (ITV) packages
are filtered out before type checking in all calls to PackageForFile
or TypeCheck. There should never be any need to type check ITVs
in practice: they contain identical syntax to the regular package,
but differ in their import metadata. Technically this may affect
types of selectors (fields/methods) but we choose to ignore that
as it is bad programming and expensive to accommodate.
Details:
- MetadataForFile now sorts primarily by narrowest and
secondarily by ITVs.
- source.NarrowestMetadataForFile is a convenience wrapper
that selects the first non-ITV element.
- PackageForFile now always chooses the narrowest,
and is renamed NarrowestPackageForFile.
The sole use of widest, from renameOrdinary, was inlined.
(One other was replaced by narrowest.)
- The PackageSelector enum (narrowes/widest) is gone.
- TODOs added to think about ITVs in the implicitly
type checking methods.
Fixes golang/go#57795
Change-Id: Id1e95990bcbc830594d2d5acec7b4f93bc01a501
Reviewed-on: https://go-review.googlesource.com/c/tools/+/487095
Reviewed-by: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Alan Donovan <adonovan@google.com>
diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go
index 510431a..294995a 100644
--- a/gopls/internal/lsp/cache/snapshot.go
+++ b/gopls/internal/lsp/cache/snapshot.go
@@ -811,14 +811,24 @@
s.unloadableFiles[uri] = struct{}{}
}
- // Sort packages "narrowest" to "widest" (in practice: non-tests before tests).
+ // 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 {
- return len(metas[i].CompiledGoFiles) < len(metas[j].CompiledGoFiles)
+ x, y := metas[i], metas[j]
+ xfiles, yfiles := len(x.CompiledGoFiles), len(y.CompiledGoFiles)
+ if xfiles != yfiles {
+ return xfiles < yfiles
+ }
+ return boolLess(x.IsIntermediateTestVariant(), y.IsIntermediateTestVariant())
})
return metas, nil
}
+func boolLess(x, y bool) bool { return !x && y } // false < true
+
func (s *snapshot) ReverseDependencies(ctx context.Context, id PackageID, transitive bool) (map[PackageID]*source.Metadata, error) {
if err := s.awaitLoaded(ctx); err != nil {
return nil, err
diff --git a/gopls/internal/lsp/code_action.go b/gopls/internal/lsp/code_action.go
index 12a275d..5891cd2 100644
--- a/gopls/internal/lsp/code_action.go
+++ b/gopls/internal/lsp/code_action.go
@@ -183,7 +183,7 @@
// Type-check the package and also run analysis,
// then combine their diagnostics.
- pkg, _, err := source.PackageForFile(ctx, snapshot, fh.URI(), source.NarrowestPackage)
+ pkg, _, err := source.NarrowestPackageForFile(ctx, snapshot, fh.URI())
if err != nil {
return nil, err
}
diff --git a/gopls/internal/lsp/command.go b/gopls/internal/lsp/command.go
index 272913b..19e4929 100644
--- a/gopls/internal/lsp/command.go
+++ b/gopls/internal/lsp/command.go
@@ -438,15 +438,11 @@
func (c *commandHandler) runTests(ctx context.Context, snapshot source.Snapshot, work *progress.WorkDone, uri protocol.DocumentURI, tests, benchmarks []string) error {
// TODO: fix the error reporting when this runs async.
- metas, err := snapshot.MetadataForFile(ctx, uri.SpanURI())
+ meta, err := source.NarrowestMetadataForFile(ctx, snapshot, uri.SpanURI())
if err != nil {
return err
}
- metas = source.RemoveIntermediateTestVariants(metas)
- if len(metas) == 0 {
- return fmt.Errorf("package could not be found for file: %s", uri.SpanURI().Filename())
- }
- pkgPath := string(metas[0].ForTest)
+ pkgPath := string(meta.ForTest)
// create output
buf := &bytes.Buffer{}
@@ -704,17 +700,16 @@
progress: "Toggling GC Details",
forURI: args.URI,
}, func(ctx context.Context, deps commandDeps) error {
- metas, err := deps.snapshot.MetadataForFile(ctx, deps.fh.URI())
+ meta, err := source.NarrowestMetadataForFile(ctx, deps.snapshot, deps.fh.URI())
if err != nil {
return err
}
- id := metas[0].ID // 0 => narrowest package
c.s.gcOptimizationDetailsMu.Lock()
- if _, ok := c.s.gcOptimizationDetails[id]; ok {
- delete(c.s.gcOptimizationDetails, id)
+ if _, ok := c.s.gcOptimizationDetails[meta.ID]; ok {
+ delete(c.s.gcOptimizationDetails, meta.ID)
c.s.clearDiagnosticSource(gcDetailsSource)
} else {
- c.s.gcOptimizationDetails[id] = struct{}{}
+ c.s.gcOptimizationDetails[meta.ID] = struct{}{}
}
c.s.gcOptimizationDetailsMu.Unlock()
c.s.diagnoseSnapshot(deps.snapshot, nil, false)
@@ -766,14 +761,11 @@
})
}
}
- metas, err := deps.snapshot.MetadataForFile(ctx, args.URI.SpanURI())
+ meta, err := source.NarrowestMetadataForFile(ctx, deps.snapshot, args.URI.SpanURI())
if err != nil {
return err // e.g. cancelled
}
- if len(metas) == 0 {
- return fmt.Errorf("no package containing %v", args.URI.SpanURI())
- }
- for pkgPath := range metas[0].DepsByPkgPath { // 0 => narrowest package
+ for pkgPath := range meta.DepsByPkgPath {
result.PackageImports = append(result.PackageImports,
command.PackageImport{Path: string(pkgPath)})
}
diff --git a/gopls/internal/lsp/diagnostics.go b/gopls/internal/lsp/diagnostics.go
index 20c797d..a71e40a 100644
--- a/gopls/internal/lsp/diagnostics.go
+++ b/gopls/internal/lsp/diagnostics.go
@@ -245,10 +245,8 @@
// noisy to log (and we'll handle things later in the slow pass).
continue
}
+ source.RemoveIntermediateTestVariants(metas)
for _, m := range metas {
- if m.IsIntermediateTestVariant() {
- continue
- }
toDiagnose[m.ID] = m
}
}
@@ -708,7 +706,7 @@
metas, _ := snapshot.MetadataForFile(ctx, fh.URI())
if len(metas) > 0 || ctx.Err() != nil {
- return nil // no package, or cancelled
+ return nil // file has a package (or cancelled)
}
// Inv: file does not belong to a package we know about.
pgf, err := snapshot.ParseGo(ctx, fh, source.ParseHeader)
diff --git a/gopls/internal/lsp/link.go b/gopls/internal/lsp/link.go
index d787b83..31e3531 100644
--- a/gopls/internal/lsp/link.go
+++ b/gopls/internal/lsp/link.go
@@ -119,8 +119,8 @@
// This requires the import map from the package metadata. Ignore errors.
var depsByImpPath map[source.ImportPath]source.PackageID
if strings.ToLower(view.Options().LinkTarget) == "pkg.go.dev" {
- if metas, _ := snapshot.MetadataForFile(ctx, fh.URI()); len(metas) > 0 {
- depsByImpPath = metas[0].DepsByImpPath // 0 => narrowest package
+ if meta, err := source.NarrowestMetadataForFile(ctx, snapshot, fh.URI()); err == nil {
+ depsByImpPath = meta.DepsByImpPath
}
}
diff --git a/gopls/internal/lsp/semantic.go b/gopls/internal/lsp/semantic.go
index 7421bf5..021fa5e 100644
--- a/gopls/internal/lsp/semantic.go
+++ b/gopls/internal/lsp/semantic.go
@@ -91,7 +91,7 @@
if kind != source.Go {
return nil, nil
}
- pkg, pgf, err := source.PackageForFile(ctx, snapshot, fh.URI(), source.NarrowestPackage)
+ pkg, pgf, err := source.NarrowestPackageForFile(ctx, snapshot, fh.URI())
if err != nil {
return nil, err
}
diff --git a/gopls/internal/lsp/source/call_hierarchy.go b/gopls/internal/lsp/source/call_hierarchy.go
index 710c6bd..f66d936 100644
--- a/gopls/internal/lsp/source/call_hierarchy.go
+++ b/gopls/internal/lsp/source/call_hierarchy.go
@@ -27,7 +27,7 @@
ctx, done := event.Start(ctx, "source.PrepareCallHierarchy")
defer done()
- pkg, pgf, err := PackageForFile(ctx, snapshot, fh.URI(), NarrowestPackage)
+ pkg, pgf, err := NarrowestPackageForFile(ctx, snapshot, fh.URI())
if err != nil {
return nil, err
}
@@ -182,7 +182,7 @@
ctx, done := event.Start(ctx, "source.OutgoingCalls")
defer done()
- pkg, pgf, err := PackageForFile(ctx, snapshot, fh.URI(), NarrowestPackage)
+ pkg, pgf, err := NarrowestPackageForFile(ctx, snapshot, fh.URI())
if err != nil {
return nil, err
}
@@ -221,7 +221,7 @@
}
// Use TypecheckFull as we want to inspect the body of the function declaration.
- declPkg, declPGF, err := PackageForFile(ctx, snapshot, uri, NarrowestPackage)
+ declPkg, declPGF, err := NarrowestPackageForFile(ctx, snapshot, uri)
if err != nil {
return nil, err
}
diff --git a/gopls/internal/lsp/source/code_lens.go b/gopls/internal/lsp/source/code_lens.go
index ef1c3aa..f095e8b 100644
--- a/gopls/internal/lsp/source/code_lens.go
+++ b/gopls/internal/lsp/source/code_lens.go
@@ -100,7 +100,7 @@
if !strings.HasSuffix(fh.URI().Filename(), "_test.go") {
return out, nil
}
- pkg, pgf, err := PackageForFile(ctx, snapshot, fh.URI(), NarrowestPackage)
+ pkg, pgf, err := NarrowestPackageForFile(ctx, snapshot, fh.URI())
if err != nil {
return out, err
}
diff --git a/gopls/internal/lsp/source/completion/completion.go b/gopls/internal/lsp/source/completion/completion.go
index c11b024..e6e53e0 100644
--- a/gopls/internal/lsp/source/completion/completion.go
+++ b/gopls/internal/lsp/source/completion/completion.go
@@ -446,7 +446,7 @@
startTime := time.Now()
- pkg, pgf, err := source.PackageForFile(ctx, snapshot, fh.URI(), source.NarrowestPackage)
+ pkg, pgf, err := source.NarrowestPackageForFile(ctx, snapshot, fh.URI())
if err != nil || pgf.File.Package == token.NoPos {
// If we can't parse this file or find position for the package
// keyword, it may be missing a package declaration. Try offering
diff --git a/gopls/internal/lsp/source/definition.go b/gopls/internal/lsp/source/definition.go
index e342313..eb9118c 100644
--- a/gopls/internal/lsp/source/definition.go
+++ b/gopls/internal/lsp/source/definition.go
@@ -22,7 +22,7 @@
ctx, done := event.Start(ctx, "source.Definition")
defer done()
- pkg, pgf, err := PackageForFile(ctx, snapshot, fh.URI(), NarrowestPackage)
+ pkg, pgf, err := NarrowestPackageForFile(ctx, snapshot, fh.URI())
if err != nil {
return nil, err
}
diff --git a/gopls/internal/lsp/source/diagnostics.go b/gopls/internal/lsp/source/diagnostics.go
index 38321fa..fc08dcf 100644
--- a/gopls/internal/lsp/source/diagnostics.go
+++ b/gopls/internal/lsp/source/diagnostics.go
@@ -60,7 +60,7 @@
// as used by the "gopls check" command.
//
// TODO(adonovan): factor in common with (*Server).codeAction, which
-// executes { PackageForFile; Analyze } too?
+// executes { NarrowestPackageForFile; Analyze } too?
//
// TODO(adonovan): opt: this function is called in a loop from the
// "gopls/diagnoseFiles" nonstandard request handler. It would be more
@@ -71,7 +71,7 @@
if err != nil {
return nil, nil, err
}
- pkg, _, err := PackageForFile(ctx, snapshot, uri, NarrowestPackage)
+ pkg, _, err := NarrowestPackageForFile(ctx, snapshot, uri)
if err != nil {
return nil, nil, err
}
diff --git a/gopls/internal/lsp/source/fix.go b/gopls/internal/lsp/source/fix.go
index 1b62bf4..08abdd0 100644
--- a/gopls/internal/lsp/source/fix.go
+++ b/gopls/internal/lsp/source/fix.go
@@ -55,7 +55,7 @@
// singleFile calls analyzers that expect inputs for a single file
func singleFile(sf singleFileFixFunc) SuggestedFixFunc {
return func(ctx context.Context, snapshot Snapshot, fh FileHandle, pRng protocol.Range) (*token.FileSet, *analysis.SuggestedFix, error) {
- pkg, pgf, err := PackageForFile(ctx, snapshot, fh.URI(), NarrowestPackage)
+ pkg, pgf, err := NarrowestPackageForFile(ctx, snapshot, fh.URI())
if err != nil {
return nil, nil, err
}
diff --git a/gopls/internal/lsp/source/format.go b/gopls/internal/lsp/source/format.go
index 809b311..ac73c76 100644
--- a/gopls/internal/lsp/source/format.go
+++ b/gopls/internal/lsp/source/format.go
@@ -72,9 +72,9 @@
// Can this, for example, result in inconsistent formatting across saves,
// due to pending calls to packages.Load?
var langVersion, modulePath string
- mds, err := snapshot.MetadataForFile(ctx, fh.URI())
- if err == nil && len(mds) > 0 {
- if mi := mds[0].Module; mi != nil {
+ meta, err := NarrowestMetadataForFile(ctx, snapshot, fh.URI())
+ if err == nil {
+ if mi := meta.Module; mi != nil {
langVersion = mi.GoVersion
modulePath = mi.Path
}
diff --git a/gopls/internal/lsp/source/highlight.go b/gopls/internal/lsp/source/highlight.go
index a190f48..ad13d25 100644
--- a/gopls/internal/lsp/source/highlight.go
+++ b/gopls/internal/lsp/source/highlight.go
@@ -23,7 +23,7 @@
// We always want fully parsed files for highlight, regardless
// of whether the file belongs to a workspace package.
- pkg, pgf, err := PackageForFile(ctx, snapshot, fh.URI(), NarrowestPackage)
+ pkg, pgf, err := NarrowestPackageForFile(ctx, snapshot, fh.URI())
if err != nil {
return nil, fmt.Errorf("getting package for Highlight: %w", err)
}
diff --git a/gopls/internal/lsp/source/hover.go b/gopls/internal/lsp/source/hover.go
index 08bfa78..fe0bc9a 100644
--- a/gopls/internal/lsp/source/hover.go
+++ b/gopls/internal/lsp/source/hover.go
@@ -88,7 +88,7 @@
// hovering at the position, it returns _, nil, nil: an error is only returned
// if the position is valid but we fail to compute hover information.
func hover(ctx context.Context, snapshot Snapshot, fh FileHandle, pp protocol.Position) (protocol.Range, *HoverJSON, error) {
- pkg, pgf, err := PackageForFile(ctx, snapshot, fh.URI(), NarrowestPackage)
+ pkg, pgf, err := NarrowestPackageForFile(ctx, snapshot, fh.URI())
if err != nil {
return protocol.Range{}, nil, err
}
diff --git a/gopls/internal/lsp/source/implementation.go b/gopls/internal/lsp/source/implementation.go
index 26fae04..d37ca4b 100644
--- a/gopls/internal/lsp/source/implementation.go
+++ b/gopls/internal/lsp/source/implementation.go
@@ -88,6 +88,7 @@
if err != nil {
return nil, err
}
+ RemoveIntermediateTestVariants(declMetas)
if len(declMetas) == 0 {
return nil, fmt.Errorf("no packages for file %s", declURI)
}
@@ -170,6 +171,7 @@
}
globalIDs = append(globalIDs, m.ID)
}
+ // TODO(adonovan): filter out ITVs?
indexes, err := snapshot.MethodSets(ctx, globalIDs...)
if err != nil {
return nil, fmt.Errorf("querying method sets: %v", err)
@@ -244,7 +246,7 @@
func typeDeclPosition(ctx context.Context, snapshot Snapshot, uri span.URI, ppos protocol.Position) (token.Position, error) {
var noPosn token.Position
- pkg, pgf, err := PackageForFile(ctx, snapshot, uri, WidestPackage)
+ pkg, pgf, err := NarrowestPackageForFile(ctx, snapshot, uri)
if err != nil {
return noPosn, err
}
diff --git a/gopls/internal/lsp/source/inlay_hint.go b/gopls/internal/lsp/source/inlay_hint.go
index 671d405..0d0bb82 100644
--- a/gopls/internal/lsp/source/inlay_hint.go
+++ b/gopls/internal/lsp/source/inlay_hint.go
@@ -82,7 +82,7 @@
ctx, done := event.Start(ctx, "source.InlayHint")
defer done()
- pkg, pgf, err := PackageForFile(ctx, snapshot, fh.URI(), NarrowestPackage)
+ pkg, pgf, err := NarrowestPackageForFile(ctx, snapshot, fh.URI())
if err != nil {
return nil, fmt.Errorf("getting file for InlayHint: %w", err)
}
diff --git a/gopls/internal/lsp/source/known_packages.go b/gopls/internal/lsp/source/known_packages.go
index ba4e9dc..4414852 100644
--- a/gopls/internal/lsp/source/known_packages.go
+++ b/gopls/internal/lsp/source/known_packages.go
@@ -6,7 +6,6 @@
import (
"context"
- "fmt"
"go/parser"
"go/token"
"sort"
@@ -28,15 +27,10 @@
// This algorithm is expressed in terms of Metadata, not Packages,
// so it doesn't cause or wait for type checking.
- // Find a Metadata containing the file.
- metas, err := snapshot.MetadataForFile(ctx, fh.URI())
+ current, err := NarrowestMetadataForFile(ctx, snapshot, fh.URI())
if err != nil {
return nil, err // e.g. context cancelled
}
- if len(metas) == 0 {
- return nil, fmt.Errorf("no loaded package contain file %s", fh.URI())
- }
- current := metas[0] // pick one arbitrarily (they should all have the same package path)
// Parse the file's imports so we can compute which
// PackagePaths are imported by this specific file.
diff --git a/gopls/internal/lsp/source/references.go b/gopls/internal/lsp/source/references.go
index 5b3978f..3b69359 100644
--- a/gopls/internal/lsp/source/references.go
+++ b/gopls/internal/lsp/source/references.go
@@ -171,7 +171,7 @@
// The widest package (possibly a test variant) has the
// greatest number of files and thus we choose it for the
// "internal" references.
- widest := metas[len(metas)-1]
+ widest := metas[len(metas)-1] // may include _test.go files
for _, uri := range widest.CompiledGoFiles {
fh, err := snapshot.ReadFile(ctx, uri)
if err != nil {
@@ -204,7 +204,7 @@
// declaration (e.g. because the _test.go files can change the
// meaning of a field or method selection), but the narrower
// package reports the more broadly referenced object.
- pkg, pgf, err := PackageForFile(ctx, snapshot, uri, NarrowestPackage)
+ pkg, pgf, err := NarrowestPackageForFile(ctx, snapshot, uri)
if err != nil {
return nil, err
}
@@ -252,6 +252,7 @@
if len(variants) == 0 {
return nil, fmt.Errorf("no packages for file %q", declURI) // can't happen
}
+ // (variants must include ITVs for reverse depedency computation below.)
// Is object exported?
// If so, compute scope and targets of the global search.
@@ -415,6 +416,7 @@
for id := range globalScope {
globalIDs = append(globalIDs, id)
}
+ // TODO(adonovan): filter out ITVs?
indexes, err := snapshot.References(ctx, globalIDs...)
if err != nil {
return err
@@ -457,6 +459,7 @@
allIDs = append(allIDs, m.ID)
}
// Search the methodset index of each package in the workspace.
+ // TODO(adonovan): filter out ITVs?
indexes, err := snapshot.MethodSets(ctx, allIDs...)
if err != nil {
return err
diff --git a/gopls/internal/lsp/source/rename.go b/gopls/internal/lsp/source/rename.go
index 02dd010..317de2f 100644
--- a/gopls/internal/lsp/source/rename.go
+++ b/gopls/internal/lsp/source/rename.go
@@ -116,7 +116,7 @@
// which means we return (nil, nil) at the protocol
// layer. This seems like a bug, or at best an exploitation of
// knowledge of VSCode-specific behavior. Can we avoid that?
- pkg, pgf, err := PackageForFile(ctx, snapshot, f.URI(), NarrowestPackage)
+ pkg, pgf, err := NarrowestPackageForFile(ctx, snapshot, f.URI())
if err != nil {
return nil, nil, err
}
@@ -163,14 +163,10 @@
}
// Check validity of the metadata for the file's containing package.
- fileMeta, err := snapshot.MetadataForFile(ctx, pgf.URI)
+ meta, err := NarrowestMetadataForFile(ctx, snapshot, pgf.URI)
if err != nil {
return nil, err
}
- if len(fileMeta) == 0 {
- return nil, fmt.Errorf("no packages found for file %q", pgf.URI)
- }
- meta := fileMeta[0]
if meta.Name == "main" {
return nil, fmt.Errorf("can't rename package \"main\"")
}
@@ -293,19 +289,41 @@
// renameOrdinary renames an ordinary (non-package) name throughout the workspace.
func renameOrdinary(ctx context.Context, snapshot Snapshot, f FileHandle, pp protocol.Position, newName string) (map[span.URI][]diff.Edit, error) {
// Type-check the referring package and locate the object(s).
- // We choose the widest variant as, for non-exported
- // identifiers, it is the only package we need.
- pkg, pgf, err := PackageForFile(ctx, snapshot, f.URI(), WidestPackage)
- if err != nil {
- return nil, err
- }
- pos, err := pgf.PositionPos(pp)
- if err != nil {
- return nil, err
- }
- targets, _, err := objectsAt(pkg.GetTypesInfo(), pgf.File, pos)
- if err != nil {
- return nil, err
+ //
+ // Unlike PackageForFile, we choose the widest variant as,
+ // for non-exported identifiers, it is the 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.)
+ var targets map[types.Object]ast.Node
+ var pkg Package
+ {
+ metas, err := snapshot.MetadataForFile(ctx, f.URI())
+ if err != nil {
+ return nil, err
+ }
+ RemoveIntermediateTestVariants(metas)
+ if len(metas) == 0 {
+ return nil, fmt.Errorf("no package metadata for file %s", f.URI())
+ }
+ widest := metas[len(metas)-1] // widest variant may include _test.go files
+ pkgs, err := snapshot.TypeCheck(ctx, widest.ID)
+ if err != nil {
+ return nil, err
+ }
+ pkg = pkgs[0]
+ pgf, err := pkg.File(f.URI())
+ if err != nil {
+ return nil, err // "can't happen"
+ }
+ pos, err := pgf.PositionPos(pp)
+ if err != nil {
+ return nil, err
+ }
+ objects, _, err := objectsAt(pkg.GetTypesInfo(), pgf.File, pos)
+ if err != nil {
+ return nil, err
+ }
+ targets = objects
}
// Pick a representative object arbitrarily.
@@ -444,6 +462,8 @@
if err != nil {
return nil, err
}
+ // variants must include ITVs for the reverse dependency
+ // computation, but they are filtered out before we typecheck.
allRdeps := make(map[PackageID]*Metadata)
for _, variant := range variants {
rdeps, err := snapshot.ReverseDependencies(ctx, variant.ID, transitive)
@@ -705,14 +725,10 @@
// We need metadata for the relevant package and module paths.
// These should be the same for all packages containing the file.
- metas, err := s.MetadataForFile(ctx, f.URI())
+ meta, err := NarrowestMetadataForFile(ctx, s, f.URI())
if err != nil {
return nil, err
}
- if len(metas) == 0 {
- return nil, fmt.Errorf("no packages found for file %q", f.URI())
- }
- meta := metas[0] // narrowest
oldPkgPath := meta.PkgPath
if meta.Module == nil {
diff --git a/gopls/internal/lsp/source/signature_help.go b/gopls/internal/lsp/source/signature_help.go
index 716de2d..ce9b267 100644
--- a/gopls/internal/lsp/source/signature_help.go
+++ b/gopls/internal/lsp/source/signature_help.go
@@ -23,7 +23,7 @@
// We need full type-checking here, as we must type-check function bodies in
// order to provide signature help at the requested position.
- pkg, pgf, err := PackageForFile(ctx, snapshot, fh.URI(), NarrowestPackage)
+ pkg, pgf, err := NarrowestPackageForFile(ctx, snapshot, fh.URI())
if err != nil {
return nil, 0, fmt.Errorf("getting file for SignatureHelp: %w", err)
}
diff --git a/gopls/internal/lsp/source/stub.go b/gopls/internal/lsp/source/stub.go
index f361f4f..c772469 100644
--- a/gopls/internal/lsp/source/stub.go
+++ b/gopls/internal/lsp/source/stub.go
@@ -30,7 +30,7 @@
// methods of the concrete type that is assigned to an interface type
// at the cursor position.
func stubSuggestedFixFunc(ctx context.Context, snapshot Snapshot, fh FileHandle, rng protocol.Range) (*token.FileSet, *analysis.SuggestedFix, error) {
- pkg, pgf, err := PackageForFile(ctx, snapshot, fh.URI(), NarrowestPackage)
+ pkg, pgf, err := NarrowestPackageForFile(ctx, snapshot, fh.URI())
if err != nil {
return nil, nil, fmt.Errorf("GetTypedFile: %w", err)
}
diff --git a/gopls/internal/lsp/source/type_definition.go b/gopls/internal/lsp/source/type_definition.go
index 104b7ac..73fc965 100644
--- a/gopls/internal/lsp/source/type_definition.go
+++ b/gopls/internal/lsp/source/type_definition.go
@@ -18,7 +18,7 @@
ctx, done := event.Start(ctx, "source.TypeDefinition")
defer done()
- pkg, pgf, err := PackageForFile(ctx, snapshot, fh.URI(), NarrowestPackage)
+ pkg, pgf, err := NarrowestPackageForFile(ctx, snapshot, fh.URI())
if err != nil {
return nil, err
}
diff --git a/gopls/internal/lsp/source/view.go b/gopls/internal/lsp/source/view.go
index affe556..8b0bcdf 100644
--- a/gopls/internal/lsp/source/view.go
+++ b/gopls/internal/lsp/source/view.go
@@ -178,7 +178,8 @@
// MetadataForFile returns a new slice containing metadata for each
// package containing the Go file identified by uri, ordered by the
- // number of CompiledGoFiles (i.e. "narrowest" to "widest" package).
+ // 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.
// It returns an error if the context was cancelled.
@@ -188,6 +189,11 @@
// and returns them in the same order as the ids.
// The resulting packages' types may belong to different importers,
// so types from different packages are incommensurable.
+ //
+ // There should never be any need to type-check an
+ // intermediate test variant (ITV) package. Callers should
+ // apply RemoveIntermediateTestVariants (or equivalent) before
+ // this method, or any of the potentially type-checking methods below.
TypeCheck(ctx context.Context, ids ...PackageID) ([]Package, error)
// PackageDiagnostics returns diagnostics for files contained in specified
@@ -215,6 +221,21 @@
GetCriticalError(ctx context.Context) *CriticalError
}
+// NarrowestMetadataForFile returns metadata for the narrowest package
+// (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 NarrowestMetadataForFile(ctx context.Context, snapshot Snapshot, uri span.URI) (*Metadata, error) {
+ metas, err := snapshot.MetadataForFile(ctx, uri)
+ if err != nil {
+ return nil, err
+ }
+ RemoveIntermediateTestVariants(metas)
+ if len(metas) == 0 {
+ return nil, fmt.Errorf("no package metadata for file %s", uri)
+ }
+ return metas[0], nil
+}
+
type XrefIndex interface {
Lookup(targets map[PackagePath]map[objectpath.Path]struct{}) (locs []protocol.Location)
}
@@ -225,28 +246,34 @@
return []label.Label{tag.Snapshot.Of(snapshot.SequenceID()), tag.Directory.Of(snapshot.View().Folder())}
}
-// PackageForFile is a convenience function that selects a package to
-// which this file belongs (narrowest or widest), type-checks it in
-// the requested mode (full or workspace), and returns it, along with
-// the parse tree of that file.
+// NarrowestPackageForFile is a convenience function that selects the
+// narrowest non-ITV package to which this file belongs, type-checks
+// it in the requested mode (full or workspace), and returns it, along
+// with the parse tree of that file.
+//
+// The "narrowest" package is the one with the fewest number of files
+// that includes the given file. This solves the problem of test
+// variants, as the test will have more files than the non-test package.
+// (Historically the preference was a parameter but widest was almost
+// never needed.)
+//
+// An intermediate test variant (ITV) package has identical source
+// to a regular package but resolves imports differently.
+// gopls should never need to type-check them.
//
// Type-checking is expensive. Call snapshot.ParseGo if all you need
// is a parse tree, or snapshot.MetadataForFile if you only need metadata.
-func PackageForFile(ctx context.Context, snapshot Snapshot, uri span.URI, pkgSel PackageSelector) (Package, *ParsedGoFile, error) {
+func NarrowestPackageForFile(ctx context.Context, snapshot Snapshot, uri span.URI) (Package, *ParsedGoFile, error) {
metas, err := snapshot.MetadataForFile(ctx, uri)
if err != nil {
return nil, nil, err
}
+ RemoveIntermediateTestVariants(metas)
if len(metas) == 0 {
return nil, nil, fmt.Errorf("no package metadata for file %s", uri)
}
- switch pkgSel {
- case NarrowestPackage:
- metas = metas[:1]
- case WidestPackage:
- metas = metas[len(metas)-1:]
- }
- pkgs, err := snapshot.TypeCheck(ctx, metas[0].ID)
+ narrowest := metas[0]
+ pkgs, err := snapshot.TypeCheck(ctx, narrowest.ID)
if err != nil {
return nil, nil, err
}
@@ -258,23 +285,6 @@
return pkg, pgf, err
}
-// PackageSelector sets how a package is selected out from a set of packages
-// containing a given file.
-type PackageSelector int
-
-const (
- // NarrowestPackage picks the "narrowest" package for a given file.
- // By "narrowest" package, we mean the package with the fewest number of
- // files that includes the given file. This solves the problem of test
- // variants, as the test will have more files than the non-test package.
- NarrowestPackage PackageSelector = iota
-
- // WidestPackage returns the Package containing the most files.
- // This is useful for something like diagnostics, where we'd prefer to
- // offer diagnostics for as many files as possible.
- WidestPackage
-)
-
// InvocationFlags represents the settings of a particular go command invocation.
// It is a mode, plus a set of flag bits.
type InvocationFlags int
diff --git a/gopls/internal/lsp/source/workspace_symbol.go b/gopls/internal/lsp/source/workspace_symbol.go
index 17c3a24..6ad1bf6 100644
--- a/gopls/internal/lsp/source/workspace_symbol.go
+++ b/gopls/internal/lsp/source/workspace_symbol.go
@@ -331,17 +331,13 @@
if seen[uri] {
continue
}
- mds, err := snapshot.MetadataForFile(ctx, uri)
+ meta, err := NarrowestMetadataForFile(ctx, snapshot, uri)
if err != nil {
event.Error(ctx, fmt.Sprintf("missing metadata for %q", uri), err)
continue
}
- if len(mds) == 0 {
- // TODO: should use the bug reporting API
- continue
- }
seen[uri] = true
- work = append(work, symbolFile{uri, mds[0], syms})
+ work = append(work, symbolFile{uri, meta, syms})
}
}
