From b35949e23814650f4c5caf482adbf63b6550fead Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Thu, 20 Apr 2023 14:53:41 -0400 Subject: [PATCH] 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 gopls-CI: kokoro TryBot-Result: Gopher Robot Run-TryBot: Alan Donovan --- gopls/internal/lsp/cache/snapshot.go | 14 +++- gopls/internal/lsp/code_action.go | 2 +- gopls/internal/lsp/command.go | 24 +++---- gopls/internal/lsp/diagnostics.go | 6 +- gopls/internal/lsp/link.go | 4 +- gopls/internal/lsp/semantic.go | 2 +- gopls/internal/lsp/source/call_hierarchy.go | 6 +- gopls/internal/lsp/source/code_lens.go | 2 +- .../lsp/source/completion/completion.go | 2 +- gopls/internal/lsp/source/definition.go | 2 +- gopls/internal/lsp/source/diagnostics.go | 4 +- gopls/internal/lsp/source/fix.go | 2 +- gopls/internal/lsp/source/format.go | 6 +- gopls/internal/lsp/source/highlight.go | 2 +- gopls/internal/lsp/source/hover.go | 2 +- gopls/internal/lsp/source/implementation.go | 4 +- gopls/internal/lsp/source/inlay_hint.go | 2 +- gopls/internal/lsp/source/known_packages.go | 8 +-- gopls/internal/lsp/source/references.go | 7 +- gopls/internal/lsp/source/rename.go | 64 ++++++++++------- gopls/internal/lsp/source/signature_help.go | 2 +- gopls/internal/lsp/source/stub.go | 2 +- gopls/internal/lsp/source/type_definition.go | 2 +- gopls/internal/lsp/source/view.go | 70 +++++++++++-------- gopls/internal/lsp/source/workspace_symbol.go | 8 +-- 25 files changed, 135 insertions(+), 114 deletions(-) diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go index 510431a6908..294995a98a4 100644 --- a/gopls/internal/lsp/cache/snapshot.go +++ b/gopls/internal/lsp/cache/snapshot.go @@ -811,14 +811,24 @@ func (s *snapshot) MetadataForFile(ctx context.Context, uri span.URI) ([]*source 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 12a275d6b08..5891cd2d643 100644 --- a/gopls/internal/lsp/code_action.go +++ b/gopls/internal/lsp/code_action.go @@ -183,7 +183,7 @@ func (s *Server) codeAction(ctx context.Context, params *protocol.CodeActionPara // 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 272913b6cf8..19e492911b1 100644 --- a/gopls/internal/lsp/command.go +++ b/gopls/internal/lsp/command.go @@ -438,15 +438,11 @@ func (c *commandHandler) RunTests(ctx context.Context, args command.RunTestsArgs 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 @@ func (c *commandHandler) ToggleGCDetails(ctx context.Context, args command.URIAr 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 @@ func (c *commandHandler) ListImports(ctx context.Context, args command.URIArg) ( }) } } - 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 20c797d85cb..a71e40ae4d1 100644 --- a/gopls/internal/lsp/diagnostics.go +++ b/gopls/internal/lsp/diagnostics.go @@ -245,10 +245,8 @@ func (s *Server) diagnoseChangedFiles(ctx context.Context, snapshot source.Snaps // 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 @@ func (s *Server) checkForOrphanedFile(ctx context.Context, snapshot source.Snaps 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 d787b830729..31e35311047 100644 --- a/gopls/internal/lsp/link.go +++ b/gopls/internal/lsp/link.go @@ -119,8 +119,8 @@ func goLinks(ctx context.Context, snapshot source.Snapshot, fh source.FileHandle // 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 7421bf54849..021fa5efabb 100644 --- a/gopls/internal/lsp/semantic.go +++ b/gopls/internal/lsp/semantic.go @@ -91,7 +91,7 @@ func (s *Server) computeSemanticTokens(ctx context.Context, td protocol.TextDocu 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 710c6bd65ac..f66d9364a91 100644 --- a/gopls/internal/lsp/source/call_hierarchy.go +++ b/gopls/internal/lsp/source/call_hierarchy.go @@ -27,7 +27,7 @@ func PrepareCallHierarchy(ctx context.Context, snapshot Snapshot, fh FileHandle, 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 @@ func OutgoingCalls(ctx context.Context, snapshot Snapshot, fh FileHandle, pp pro 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 @@ func OutgoingCalls(ctx context.Context, snapshot Snapshot, fh FileHandle, pp pro } // 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 ef1c3aa5427..f095e8b0a8c 100644 --- a/gopls/internal/lsp/source/code_lens.go +++ b/gopls/internal/lsp/source/code_lens.go @@ -100,7 +100,7 @@ func TestsAndBenchmarks(ctx context.Context, snapshot Snapshot, fh FileHandle) ( 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 c11b02454e5..e6e53e012fa 100644 --- a/gopls/internal/lsp/source/completion/completion.go +++ b/gopls/internal/lsp/source/completion/completion.go @@ -446,7 +446,7 @@ func Completion(ctx context.Context, snapshot source.Snapshot, fh source.FileHan 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 e3423131c13..eb9118cd69b 100644 --- a/gopls/internal/lsp/source/definition.go +++ b/gopls/internal/lsp/source/definition.go @@ -22,7 +22,7 @@ func Definition(ctx context.Context, snapshot Snapshot, fh FileHandle, position 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 38321fad996..fc08dcfa14c 100644 --- a/gopls/internal/lsp/source/diagnostics.go +++ b/gopls/internal/lsp/source/diagnostics.go @@ -60,7 +60,7 @@ func Analyze(ctx context.Context, snapshot Snapshot, pkgid PackageID, includeCon // 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 @@ func FileDiagnostics(ctx context.Context, snapshot Snapshot, uri span.URI) (File 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 1b62bf4373d..08abdd00597 100644 --- a/gopls/internal/lsp/source/fix.go +++ b/gopls/internal/lsp/source/fix.go @@ -55,7 +55,7 @@ var suggestedFixes = map[string]SuggestedFixFunc{ // 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 809b3113122..ac73c76e5a7 100644 --- a/gopls/internal/lsp/source/format.go +++ b/gopls/internal/lsp/source/format.go @@ -72,9 +72,9 @@ func Format(ctx context.Context, snapshot Snapshot, fh FileHandle) ([]protocol.T // 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 a190f489671..ad13d253df9 100644 --- a/gopls/internal/lsp/source/highlight.go +++ b/gopls/internal/lsp/source/highlight.go @@ -23,7 +23,7 @@ func Highlight(ctx context.Context, snapshot Snapshot, fh FileHandle, position p // 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 08bfa78912a..fe0bc9a3111 100644 --- a/gopls/internal/lsp/source/hover.go +++ b/gopls/internal/lsp/source/hover.go @@ -88,7 +88,7 @@ func Hover(ctx context.Context, snapshot Snapshot, fh FileHandle, position proto // 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 26fae0461b3..d37ca4bf173 100644 --- a/gopls/internal/lsp/source/implementation.go +++ b/gopls/internal/lsp/source/implementation.go @@ -88,6 +88,7 @@ func implementations(ctx context.Context, snapshot Snapshot, fh FileHandle, pp p 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 @@ func implementations(ctx context.Context, snapshot Snapshot, fh FileHandle, pp p } 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 offsetToLocation(ctx context.Context, snapshot Snapshot, filename string, s 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 671d405dc60..0d0bb82ebe1 100644 --- a/gopls/internal/lsp/source/inlay_hint.go +++ b/gopls/internal/lsp/source/inlay_hint.go @@ -82,7 +82,7 @@ func InlayHint(ctx context.Context, snapshot Snapshot, fh FileHandle, pRng proto 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 ba4e9dcd31a..4414852510d 100644 --- a/gopls/internal/lsp/source/known_packages.go +++ b/gopls/internal/lsp/source/known_packages.go @@ -6,7 +6,6 @@ package source import ( "context" - "fmt" "go/parser" "go/token" "sort" @@ -28,15 +27,10 @@ func KnownPackagePaths(ctx context.Context, snapshot Snapshot, fh FileHandle) ([ // 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 5b3978fce07..3b69359d665 100644 --- a/gopls/internal/lsp/source/references.go +++ b/gopls/internal/lsp/source/references.go @@ -171,7 +171,7 @@ func packageReferences(ctx context.Context, snapshot Snapshot, uri span.URI) ([] // 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 @@ func ordinaryReferences(ctx context.Context, snapshot Snapshot, uri span.URI, pp // 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 @@ func ordinaryReferences(ctx context.Context, snapshot Snapshot, uri span.URI, pp 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 @@ func ordinaryReferences(ctx context.Context, snapshot Snapshot, uri span.URI, pp 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 @@ func expandMethodSearch(ctx context.Context, snapshot Snapshot, method *types.Fu 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 02dd0100e73..317de2f484f 100644 --- a/gopls/internal/lsp/source/rename.go +++ b/gopls/internal/lsp/source/rename.go @@ -116,7 +116,7 @@ func PrepareRename(ctx context.Context, snapshot Snapshot, f FileHandle, pp prot // 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 @@ func prepareRenamePackageName(ctx context.Context, snapshot Snapshot, pgf *Parse } // 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 @@ func Rename(ctx context.Context, snapshot Snapshot, f FileHandle, pp protocol.Po // 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 @@ func typeCheckReverseDependencies(ctx context.Context, snapshot Snapshot, declUR 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 @@ func renamePackage(ctx context.Context, s Snapshot, f FileHandle, newName Packag // 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 716de2dd9b3..ce9b2678e46 100644 --- a/gopls/internal/lsp/source/signature_help.go +++ b/gopls/internal/lsp/source/signature_help.go @@ -23,7 +23,7 @@ func SignatureHelp(ctx context.Context, snapshot Snapshot, fh FileHandle, positi // 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 f361f4fdabb..c772469066b 100644 --- a/gopls/internal/lsp/source/stub.go +++ b/gopls/internal/lsp/source/stub.go @@ -30,7 +30,7 @@ import ( // 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 104b7accfe7..73fc9659983 100644 --- a/gopls/internal/lsp/source/type_definition.go +++ b/gopls/internal/lsp/source/type_definition.go @@ -18,7 +18,7 @@ func TypeDefinition(ctx context.Context, snapshot Snapshot, fh FileHandle, posit 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 affe556e831..8b0bcdfee22 100644 --- a/gopls/internal/lsp/source/view.go +++ b/gopls/internal/lsp/source/view.go @@ -178,7 +178,8 @@ type Snapshot interface { // 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 @@ type Snapshot interface { // 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 @@ type Snapshot interface { 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 @@ func SnapshotLabels(snapshot Snapshot) []label.Label { 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 @@ func PackageForFile(ctx context.Context, snapshot Snapshot, uri span.URI, pkgSel 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 17c3a24fbb0..6ad1bf63584 100644 --- a/gopls/internal/lsp/source/workspace_symbol.go +++ b/gopls/internal/lsp/source/workspace_symbol.go @@ -331,17 +331,13 @@ func collectSymbols(ctx context.Context, views []View, matcherType SymbolMatcher 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}) } }