From 85bf7a8fb4750610858ce72f1ebd39fb94013b27 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Fri, 18 Nov 2022 12:03:11 -0500 Subject: [PATCH] gopls/internal/lsp/source: eliminate Metadata interface This change merges the source.Metadata interface with its sole implementation, cache.Metadata. One possible concern: the struct cannot have private fields used only by the cache-package logic that constructs these structs. We are ok with that. Change-Id: I93c112f92dc812bd0da07d36e7244d5d77978312 Reviewed-on: https://go-review.googlesource.com/c/tools/+/452035 Run-TryBot: Alan Donovan TryBot-Result: Gopher Robot Reviewed-by: Robert Findley --- gopls/internal/lsp/cache/check.go | 12 +-- gopls/internal/lsp/cache/graph.go | 6 +- gopls/internal/lsp/cache/load.go | 18 ++-- gopls/internal/lsp/cache/metadata.go | 88 ------------------- gopls/internal/lsp/cache/pkg.go | 10 ++- gopls/internal/lsp/cache/snapshot.go | 12 +-- gopls/internal/lsp/source/format.go | 2 +- gopls/internal/lsp/source/rename.go | 54 ++++++------ gopls/internal/lsp/source/view.go | 63 ++++++++++--- gopls/internal/lsp/source/workspace_symbol.go | 32 +++---- 10 files changed, 128 insertions(+), 169 deletions(-) delete mode 100644 gopls/internal/lsp/cache/metadata.go diff --git a/gopls/internal/lsp/cache/check.go b/gopls/internal/lsp/cache/check.go index 1f3622a73d..a22e285b6a 100644 --- a/gopls/internal/lsp/cache/check.go +++ b/gopls/internal/lsp/cache/check.go @@ -46,7 +46,7 @@ type packageHandle struct { promise *memoize.Promise // [typeCheckResult] // m is the metadata associated with the package. - m *Metadata + m *source.Metadata // key is the hashed key for the package. // @@ -184,7 +184,7 @@ func (s *snapshot) buildPackageHandle(ctx context.Context, id PackageID, mode so // readGoFiles reads the content of Metadata.GoFiles and // Metadata.CompiledGoFiles, in parallel. -func readGoFiles(ctx context.Context, s *snapshot, m *Metadata) (goFiles, compiledGoFiles []source.FileHandle, err error) { +func readGoFiles(ctx context.Context, s *snapshot, m *source.Metadata) (goFiles, compiledGoFiles []source.FileHandle, err error) { var group errgroup.Group getFileHandles := func(files []span.URI) []source.FileHandle { fhs := make([]source.FileHandle, len(files)) @@ -221,7 +221,7 @@ func (s *snapshot) workspaceParseMode(id PackageID) source.ParseMode { // computePackageKey returns a key representing the act of type checking // a package named id containing the specified files, metadata, and // combined dependency hash. -func computePackageKey(id PackageID, files []source.FileHandle, m *Metadata, depsKey source.Hash, mode source.ParseMode, experimentalKey bool) packageHandleKey { +func computePackageKey(id PackageID, files []source.FileHandle, m *source.Metadata, depsKey source.Hash, mode source.ParseMode, experimentalKey bool) packageHandleKey { // TODO(adonovan): opt: no need to materalize the bytes; hash them directly. // Also, use field separators to avoid spurious collisions. b := bytes.NewBuffer(nil) @@ -304,7 +304,7 @@ func (ph *packageHandle) cached() (*pkg, error) { // typeCheckImpl type checks the parsed source files in compiledGoFiles. // (The resulting pkg also holds the parsed but not type-checked goFiles.) // deps holds the future results of type-checking the direct dependencies. -func typeCheckImpl(ctx context.Context, snapshot *snapshot, goFiles, compiledGoFiles []source.FileHandle, m *Metadata, mode source.ParseMode, deps map[PackageID]*packageHandle) (*pkg, error) { +func typeCheckImpl(ctx context.Context, snapshot *snapshot, goFiles, compiledGoFiles []source.FileHandle, m *source.Metadata, mode source.ParseMode, deps map[PackageID]*packageHandle) (*pkg, error) { // Start type checking of direct dependencies, // in parallel and asynchronously. // As the type checker imports each of these @@ -439,7 +439,7 @@ func typeCheckImpl(ctx context.Context, snapshot *snapshot, goFiles, compiledGoF var goVersionRx = regexp.MustCompile(`^go([1-9][0-9]*)\.(0|[1-9][0-9]*)$`) -func doTypeCheck(ctx context.Context, snapshot *snapshot, goFiles, compiledGoFiles []source.FileHandle, m *Metadata, mode source.ParseMode, deps map[PackageID]*packageHandle, astFilter *unexportedFilter) (*pkg, error) { +func doTypeCheck(ctx context.Context, snapshot *snapshot, goFiles, compiledGoFiles []source.FileHandle, m *source.Metadata, mode source.ParseMode, deps map[PackageID]*packageHandle, astFilter *unexportedFilter) (*pkg, error) { ctx, done := event.Start(ctx, "cache.typeCheck", tag.Package.Of(string(m.ID))) defer done() @@ -628,7 +628,7 @@ func (s *snapshot) depsErrors(ctx context.Context, pkg *pkg) ([]*source.Diagnost // Select packages that can't be found, and were imported in non-workspace packages. // Workspace packages already show their own errors. var relevantErrors []*packagesinternal.PackageError - for _, depsError := range pkg.m.depsErrors { + for _, depsError := range pkg.m.DepsErrors { // Up to Go 1.15, the missing package was included in the stack, which // was presumably a bug. We want the next one up. directImporterIdx := len(depsError.ImportStack) - 1 diff --git a/gopls/internal/lsp/cache/graph.go b/gopls/internal/lsp/cache/graph.go index be5a0618ca..709f1abfb2 100644 --- a/gopls/internal/lsp/cache/graph.go +++ b/gopls/internal/lsp/cache/graph.go @@ -15,7 +15,7 @@ import ( // graph of Go packages, as obtained from go/packages. type metadataGraph struct { // metadata maps package IDs to their associated metadata. - metadata map[PackageID]*Metadata + metadata map[PackageID]*source.Metadata // importedBy maps package IDs to the list of packages that import them. importedBy map[PackageID][]PackageID @@ -27,12 +27,12 @@ type metadataGraph struct { // Clone creates a new metadataGraph, applying the given updates to the // receiver. -func (g *metadataGraph) Clone(updates map[PackageID]*Metadata) *metadataGraph { +func (g *metadataGraph) Clone(updates map[PackageID]*source.Metadata) *metadataGraph { if len(updates) == 0 { // Optimization: since the graph is immutable, we can return the receiver. return g } - result := &metadataGraph{metadata: make(map[PackageID]*Metadata, len(g.metadata))} + result := &metadataGraph{metadata: make(map[PackageID]*source.Metadata, len(g.metadata))} // Copy metadata. for id, m := range g.metadata { result.metadata[id] = m diff --git a/gopls/internal/lsp/cache/load.go b/gopls/internal/lsp/cache/load.go index 46e2e891ac..5f31958a15 100644 --- a/gopls/internal/lsp/cache/load.go +++ b/gopls/internal/lsp/cache/load.go @@ -157,7 +157,7 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...loadSc moduleErrs := make(map[string][]packages.Error) // module path -> errors filterer := buildFilterer(s.view.rootURI.Filename(), s.view.gomodcache, s.view.Options()) - newMetadata := make(map[PackageID]*Metadata) + newMetadata := make(map[PackageID]*source.Metadata) for _, pkg := range pkgs { // The Go command returns synthetic list results for module queries that // encountered module errors. @@ -222,7 +222,7 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...loadSc // // TODO(rfindley): perform a sanity check that metadata matches here. If not, // we have an invalidation bug elsewhere. - updates := make(map[PackageID]*Metadata) + updates := make(map[PackageID]*source.Metadata) var updatedIDs []PackageID for _, m := range newMetadata { if existing := s.meta.metadata[m.ID]; existing == nil { @@ -475,7 +475,7 @@ func makeWorkspaceDir(ctx context.Context, workspace *workspace, fs source.FileS // buildMetadata populates the updates map with metadata updates to // apply, based on the given pkg. It recurs through pkg.Imports to ensure that // metadata exists for all dependencies. -func buildMetadata(ctx context.Context, pkg *packages.Package, cfg *packages.Config, query []string, updates map[PackageID]*Metadata, path []PackageID) error { +func buildMetadata(ctx context.Context, pkg *packages.Package, cfg *packages.Config, query []string, updates map[PackageID]*source.Metadata, path []PackageID) error { // Allow for multiple ad-hoc packages in the workspace (see #47584). pkgPath := PackagePath(pkg.PkgPath) id := PackageID(pkg.ID) @@ -507,7 +507,7 @@ func buildMetadata(ctx context.Context, pkg *packages.Package, cfg *packages.Con } // Recreate the metadata rather than reusing it to avoid locking. - m := &Metadata{ + m := &source.Metadata{ ID: id, PkgPath: pkgPath, Name: PackageName(pkg.Name), @@ -515,7 +515,7 @@ func buildMetadata(ctx context.Context, pkg *packages.Package, cfg *packages.Con TypesSizes: pkg.TypesSizes, Config: cfg, Module: pkg.Module, - depsErrors: packagesinternal.GetDepsErrors(pkg), + DepsErrors: packagesinternal.GetDepsErrors(pkg), } updates[id] = m @@ -606,7 +606,7 @@ func buildMetadata(ctx context.Context, pkg *packages.Package, cfg *packages.Con // snapshot s. // // s.mu must be held while calling this function. -func containsPackageLocked(s *snapshot, m *Metadata) bool { +func containsPackageLocked(s *snapshot, m *source.Metadata) bool { // In legacy workspace mode, or if a package does not have an associated // module, a package is considered inside the workspace if any of its files // are under the workspace root (and not excluded). @@ -647,7 +647,7 @@ func containsPackageLocked(s *snapshot, m *Metadata) bool { // the snapshot s. // // s.mu must be held while calling this function. -func containsOpenFileLocked(s *snapshot, m *Metadata) bool { +func containsOpenFileLocked(s *snapshot, m *source.Metadata) bool { uris := map[span.URI]struct{}{} for _, uri := range m.CompiledGoFiles { uris[uri] = struct{}{} @@ -668,7 +668,7 @@ func containsOpenFileLocked(s *snapshot, m *Metadata) bool { // workspace of the snapshot s. // // s.mu must be held while calling this function. -func containsFileInWorkspaceLocked(s *snapshot, m *Metadata) bool { +func containsFileInWorkspaceLocked(s *snapshot, m *source.Metadata) bool { uris := map[span.URI]struct{}{} for _, uri := range m.CompiledGoFiles { uris[uri] = struct{}{} @@ -738,7 +738,7 @@ func computeWorkspacePackagesLocked(s *snapshot, meta *metadataGraph) map[Packag // function returns false. // // If m is not a command-line-arguments package, this is trivially true. -func allFilesHaveRealPackages(g *metadataGraph, m *Metadata) bool { +func allFilesHaveRealPackages(g *metadataGraph, m *source.Metadata) bool { n := len(m.CompiledGoFiles) checkURIs: for _, uri := range append(m.CompiledGoFiles[0:n:n], m.GoFiles...) { diff --git a/gopls/internal/lsp/cache/metadata.go b/gopls/internal/lsp/cache/metadata.go deleted file mode 100644 index 135623fc53..0000000000 --- a/gopls/internal/lsp/cache/metadata.go +++ /dev/null @@ -1,88 +0,0 @@ -// Copyright 2021 The Go Authors. All rights reserved. -// Use of this source code is governed by a BSD-style -// license that can be found in the LICENSE file. - -package cache - -import ( - "go/types" - - "golang.org/x/tools/go/packages" - "golang.org/x/tools/gopls/internal/lsp/source" - "golang.org/x/tools/gopls/internal/span" - "golang.org/x/tools/internal/packagesinternal" -) - -type ( - PackageID = source.PackageID - PackagePath = source.PackagePath - PackageName = source.PackageName - ImportPath = source.ImportPath -) - -// Metadata holds package Metadata extracted from a call to packages.Load. -type Metadata struct { - ID PackageID - PkgPath PackagePath - Name PackageName - GoFiles []span.URI - CompiledGoFiles []span.URI - ForTest PackagePath // package path under test, or "" - TypesSizes types.Sizes - Errors []packages.Error - DepsByImpPath map[ImportPath]PackageID // may contain dups; empty ID => missing - DepsByPkgPath map[PackagePath]PackageID // values are unique and non-empty - Module *packages.Module - depsErrors []*packagesinternal.PackageError - - // Config is the *packages.Config associated with the loaded package. - Config *packages.Config -} - -// PackageID implements the source.Metadata interface. -func (m *Metadata) PackageID() PackageID { return m.ID } - -// Name implements the source.Metadata interface. -func (m *Metadata) PackageName() PackageName { return m.Name } - -// PkgPath implements the source.Metadata interface. -func (m *Metadata) PackagePath() PackagePath { return m.PkgPath } - -// IsIntermediateTestVariant reports whether the given package is an -// intermediate test variant, e.g. "net/http [net/url.test]". -// -// Such test variants arise when an x_test package (in this case net/url_test) -// imports a package (in this case net/http) that itself imports the the -// non-x_test package (in this case net/url). -// -// This is done so that the forward transitive closure of net/url_test has -// only one package for the "net/url" import. -// The intermediate test variant exists to hold the test variant import: -// -// net/url_test [net/url.test] -// -// | "net/http" -> net/http [net/url.test] -// | "net/url" -> net/url [net/url.test] -// | ... -// -// net/http [net/url.test] -// -// | "net/url" -> net/url [net/url.test] -// | ... -// -// This restriction propagates throughout the import graph of net/http: for -// every package imported by net/http that imports net/url, there must be an -// intermediate test variant that instead imports "net/url [net/url.test]". -// -// As one can see from the example of net/url and net/http, intermediate test -// variants can result in many additional packages that are essentially (but -// not quite) identical. For this reason, we filter these variants wherever -// possible. -func (m *Metadata) IsIntermediateTestVariant() bool { - return m.ForTest != "" && m.ForTest != m.PkgPath && m.ForTest+"_test" != m.PkgPath -} - -// ModuleInfo implements the source.Metadata interface. -func (m *Metadata) ModuleInfo() *packages.Module { - return m.Module -} diff --git a/gopls/internal/lsp/cache/pkg.go b/gopls/internal/lsp/cache/pkg.go index 6e1233a37b..5c43a82ad5 100644 --- a/gopls/internal/lsp/cache/pkg.go +++ b/gopls/internal/lsp/cache/pkg.go @@ -18,9 +18,17 @@ import ( "golang.org/x/tools/internal/memoize" ) +// Convenient local aliases for typed strings. +type ( + PackageID = source.PackageID + PackagePath = source.PackagePath + PackageName = source.PackageName + ImportPath = source.ImportPath +) + // pkg contains the type information needed by the source package. type pkg struct { - m *Metadata + m *source.Metadata mode source.ParseMode fset *token.FileSet // for now, same as the snapshot's FileSet goFiles []*source.ParsedGoFile diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go index 281c5d9b08..ae5fe28825 100644 --- a/gopls/internal/lsp/cache/snapshot.go +++ b/gopls/internal/lsp/cache/snapshot.go @@ -1128,12 +1128,12 @@ func (s *snapshot) Symbols(ctx context.Context) map[span.URI][]source.Symbol { return result } -func (s *snapshot) MetadataForFile(ctx context.Context, uri span.URI) ([]source.Metadata, error) { +func (s *snapshot) MetadataForFile(ctx context.Context, uri span.URI) ([]*source.Metadata, error) { knownIDs, err := s.getOrLoadIDsForURI(ctx, uri) if err != nil { return nil, err } - var mds []source.Metadata + var mds []*source.Metadata for _, id := range knownIDs { md := s.getMetadata(id) // TODO(rfindley): knownIDs and metadata should be in sync, but existing @@ -1168,7 +1168,7 @@ func (s *snapshot) KnownPackages(ctx context.Context) ([]source.Package, error) return pkgs, nil } -func (s *snapshot) AllValidMetadata(ctx context.Context) ([]source.Metadata, error) { +func (s *snapshot) AllValidMetadata(ctx context.Context) ([]*source.Metadata, error) { if err := s.awaitLoaded(ctx); err != nil { return nil, err } @@ -1176,7 +1176,7 @@ func (s *snapshot) AllValidMetadata(ctx context.Context) ([]source.Metadata, err s.mu.Lock() defer s.mu.Unlock() - var meta []source.Metadata + var meta []*source.Metadata for _, m := range s.meta.metadata { meta = append(meta, m) } @@ -1233,7 +1233,7 @@ func moduleForURI(modFiles map[span.URI]struct{}, uri span.URI) span.URI { return match } -func (s *snapshot) getMetadata(id PackageID) *Metadata { +func (s *snapshot) getMetadata(id PackageID) *source.Metadata { s.mu.Lock() defer s.mu.Unlock() @@ -1922,7 +1922,7 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC // Compute which metadata updates are required. We only need to invalidate // packages directly containing the affected file, and only if it changed in // a relevant way. - metadataUpdates := make(map[PackageID]*Metadata) + metadataUpdates := make(map[PackageID]*source.Metadata) for k, v := range s.meta.metadata { invalidateMetadata := idsToInvalidate[k] diff --git a/gopls/internal/lsp/source/format.go b/gopls/internal/lsp/source/format.go index a83c9e471e..2605cbb0a8 100644 --- a/gopls/internal/lsp/source/format.go +++ b/gopls/internal/lsp/source/format.go @@ -74,7 +74,7 @@ func Format(ctx context.Context, snapshot Snapshot, fh FileHandle) ([]protocol.T var langVersion, modulePath string mds, err := snapshot.MetadataForFile(ctx, fh.URI()) if err == nil && len(mds) > 0 { - if mi := mds[0].ModuleInfo(); mi != nil { + if mi := mds[0].Module; mi != nil { langVersion = mi.GoVersion modulePath = mi.Path } diff --git a/gopls/internal/lsp/source/rename.go b/gopls/internal/lsp/source/rename.go index 9586d8418c..51555b0ba1 100644 --- a/gopls/internal/lsp/source/rename.go +++ b/gopls/internal/lsp/source/rename.go @@ -87,27 +87,27 @@ func PrepareRename(ctx context.Context, snapshot Snapshot, f FileHandle, pp prot meta := fileMeta[0] - if meta.PackageName() == "main" { + if meta.Name == "main" { err := errors.New("can't rename package \"main\"") return nil, err, err } - if strings.HasSuffix(string(meta.PackageName()), "_test") { + if strings.HasSuffix(string(meta.Name), "_test") { err := errors.New("can't rename x_test packages") return nil, err, err } - if meta.ModuleInfo() == nil { - err := fmt.Errorf("can't rename package: missing module information for package %q", meta.PackagePath()) + if meta.Module == nil { + err := fmt.Errorf("can't rename package: missing module information for package %q", meta.PkgPath) return nil, err, err } - if meta.ModuleInfo().Path == string(meta.PackagePath()) { - err := fmt.Errorf("can't rename package: package path %q is the same as module path %q", meta.PackagePath(), meta.ModuleInfo().Path) + if meta.Module.Path == string(meta.PkgPath) { + err := fmt.Errorf("can't rename package: package path %q is the same as module path %q", meta.PkgPath, meta.Module.Path) return nil, err, err } // TODO(rfindley): we should not need the package here. - pkg, err := snapshot.WorkspacePackageByID(ctx, meta.PackageID()) + pkg, err := snapshot.WorkspacePackageByID(ctx, meta.ID) if err != nil { err = fmt.Errorf("error building package to rename: %v", err) return nil, err, err @@ -200,10 +200,10 @@ func Rename(ctx context.Context, s Snapshot, f FileHandle, pp protocol.Position, // TODO(rfindley): we mix package path and import path here haphazardly. // Fix this. meta := fileMeta[0] - oldPath := meta.PackagePath() + oldPath := meta.PkgPath var modulePath PackagePath - if mi := meta.ModuleInfo(); mi == nil { - return nil, true, fmt.Errorf("cannot rename package: missing module information for package %q", meta.PackagePath()) + if mi := meta.Module; mi == nil { + return nil, true, fmt.Errorf("cannot rename package: missing module information for package %q", meta.PkgPath) } else { modulePath = PackagePath(mi.Path) } @@ -244,7 +244,7 @@ func Rename(ctx context.Context, s Snapshot, f FileHandle, pp protocol.Position, // It updates package clauses and import paths for the renamed package as well // as any other packages affected by the directory renaming among packages // described by allMetadata. -func renamePackage(ctx context.Context, s Snapshot, modulePath, oldPath PackagePath, newName PackageName, allMetadata []Metadata) (map[span.URI][]protocol.TextEdit, error) { +func renamePackage(ctx context.Context, s Snapshot, modulePath, oldPath PackagePath, newName PackageName, allMetadata []*Metadata) (map[span.URI][]protocol.TextEdit, error) { if modulePath == oldPath { return nil, fmt.Errorf("cannot rename package: module path %q is the same as the package path, so renaming the package directory would have no effect", modulePath) } @@ -259,7 +259,7 @@ func renamePackage(ctx context.Context, s Snapshot, modulePath, oldPath PackageP // Special case: x_test packages for the renamed package will not have the // package path as as a dir prefix, but still need their package clauses // renamed. - if m.PackagePath() == oldPath+"_test" { + if m.PkgPath == oldPath+"_test" { newTestName := newName + "_test" if err := renamePackageClause(ctx, m, s, newTestName, seen, edits); err != nil { @@ -271,24 +271,24 @@ func renamePackage(ctx context.Context, s Snapshot, modulePath, oldPath PackageP // Subtle: check this condition before checking for valid module info // below, because we should not fail this operation if unrelated packages // lack module info. - if !strings.HasPrefix(string(m.PackagePath())+"/", string(oldPath)+"/") { + if !strings.HasPrefix(string(m.PkgPath)+"/", string(oldPath)+"/") { continue // not affected by the package renaming } - if m.ModuleInfo() == nil { - return nil, fmt.Errorf("cannot rename package: missing module information for package %q", m.PackagePath()) + if m.Module == nil { + return nil, fmt.Errorf("cannot rename package: missing module information for package %q", m.PkgPath) } - if modulePath != PackagePath(m.ModuleInfo().Path) { + if modulePath != PackagePath(m.Module.Path) { continue // don't edit imports if nested package and renaming package have different module paths } // Renaming a package consists of changing its import path and package name. - suffix := strings.TrimPrefix(string(m.PackagePath()), string(oldPath)) + suffix := strings.TrimPrefix(string(m.PkgPath), string(oldPath)) newPath := newPathPrefix + suffix - pkgName := m.PackageName() - if m.PackagePath() == PackagePath(oldPath) { + pkgName := m.Name + if m.PkgPath == PackagePath(oldPath) { pkgName = newName if err := renamePackageClause(ctx, m, s, newName, seen, edits); err != nil { @@ -336,15 +336,15 @@ func (s seenPackageRename) add(uri span.URI, path PackagePath) bool { // package clause has already been updated, to prevent duplicate edits. // // Edits are written into the edits map. -func renamePackageClause(ctx context.Context, m Metadata, s Snapshot, newName PackageName, seen seenPackageRename, edits map[span.URI][]protocol.TextEdit) error { - pkg, err := s.WorkspacePackageByID(ctx, m.PackageID()) +func renamePackageClause(ctx context.Context, m *Metadata, s Snapshot, newName PackageName, seen seenPackageRename, edits map[span.URI][]protocol.TextEdit) error { + pkg, err := s.WorkspacePackageByID(ctx, m.ID) if err != nil { return err } // Rename internal references to the package in the renaming package. for _, f := range pkg.CompiledGoFiles() { - if seen.add(f.URI, m.PackagePath()) { + if seen.add(f.URI, m.PkgPath) { continue } @@ -370,11 +370,11 @@ func renamePackageClause(ctx context.Context, m Metadata, s Snapshot, newName Pa // newPath and name newName. // // Edits are written into the edits map. -func renameImports(ctx context.Context, s Snapshot, m Metadata, newPath ImportPath, newName PackageName, seen seenPackageRename, edits map[span.URI][]protocol.TextEdit) error { +func renameImports(ctx context.Context, s Snapshot, m *Metadata, newPath ImportPath, newName PackageName, seen seenPackageRename, edits map[span.URI][]protocol.TextEdit) error { // TODO(rfindley): we should get reverse dependencies as metadata first, // rather then building the package immediately. We don't need reverse // dependencies if they are intermediate test variants. - rdeps, err := s.GetReverseDependencies(ctx, m.PackageID()) + rdeps, err := s.GetReverseDependencies(ctx, m.ID) if err != nil { return err } @@ -394,13 +394,13 @@ func renameImports(ctx context.Context, s Snapshot, m Metadata, newPath ImportPa } for _, f := range dep.CompiledGoFiles() { - if seen.add(f.URI, m.PackagePath()) { + if seen.add(f.URI, m.PkgPath) { continue } for _, imp := range f.File.Imports { // TODO(adonovan): what if RHS has "vendor/" prefix? - if UnquoteImportPath(imp) != ImportPath(m.PackagePath()) { + if UnquoteImportPath(imp) != ImportPath(m.PkgPath) { continue // not the import we're looking for } @@ -418,7 +418,7 @@ func renameImports(ctx context.Context, s Snapshot, m Metadata, newPath ImportPa // If the package name of an import has not changed or if its import // path already has a local package name, then we don't need to update // the local package name. - if newName == m.PackageName() || imp.Name != nil { + if newName == m.Name || imp.Name != nil { continue } diff --git a/gopls/internal/lsp/source/view.go b/gopls/internal/lsp/source/view.go index 4178197e48..7466484952 100644 --- a/gopls/internal/lsp/source/view.go +++ b/gopls/internal/lsp/source/view.go @@ -28,6 +28,7 @@ import ( "golang.org/x/tools/internal/event/tag" "golang.org/x/tools/internal/gocommand" "golang.org/x/tools/internal/imports" + "golang.org/x/tools/internal/packagesinternal" ) // A GlobalSnapshotID uniquely identifies a snapshot within this process and @@ -196,7 +197,7 @@ type Snapshot interface { ActivePackages(ctx context.Context) ([]Package, error) // AllValidMetadata returns all valid metadata loaded for the snapshot. - AllValidMetadata(ctx context.Context) ([]Metadata, error) + AllValidMetadata(ctx context.Context) ([]*Metadata, error) // WorkspacePackageByID returns the workspace package with id, type checked // in 'workspace' mode. @@ -206,7 +207,7 @@ type Snapshot interface { Symbols(ctx context.Context) map[span.URI][]Symbol // Metadata returns package metadata associated with the given file URI. - MetadataForFile(ctx context.Context, uri span.URI) ([]Metadata, error) + MetadataForFile(ctx context.Context, uri span.URI) ([]*Metadata, error) // GetCriticalError returns any critical errors in the workspace. GetCriticalError(ctx context.Context) *CriticalError @@ -373,18 +374,56 @@ type TidiedModule struct { } // Metadata represents package metadata retrieved from go/packages. -type Metadata interface { - // PackageID is the unique package id. - PackageID() PackageID +type Metadata struct { + ID PackageID + PkgPath PackagePath + Name PackageName + GoFiles []span.URI + CompiledGoFiles []span.URI + ForTest PackagePath // package path under test, or "" + TypesSizes types.Sizes + Errors []packages.Error + DepsByImpPath map[ImportPath]PackageID // may contain dups; empty ID => missing + DepsByPkgPath map[PackagePath]PackageID // values are unique and non-empty + Module *packages.Module + DepsErrors []*packagesinternal.PackageError - // PackageName is the package name. - PackageName() PackageName + // Config is the *packages.Config associated with the loaded package. + Config *packages.Config +} - // PackagePath is the package path. - PackagePath() PackagePath - - // ModuleInfo returns the go/packages module information for the given package. - ModuleInfo() *packages.Module +// IsIntermediateTestVariant reports whether the given package is an +// intermediate test variant, e.g. "net/http [net/url.test]". +// +// Such test variants arise when an x_test package (in this case net/url_test) +// imports a package (in this case net/http) that itself imports the the +// non-x_test package (in this case net/url). +// +// This is done so that the forward transitive closure of net/url_test has +// only one package for the "net/url" import. +// The intermediate test variant exists to hold the test variant import: +// +// net/url_test [net/url.test] +// +// | "net/http" -> net/http [net/url.test] +// | "net/url" -> net/url [net/url.test] +// | ... +// +// net/http [net/url.test] +// +// | "net/url" -> net/url [net/url.test] +// | ... +// +// This restriction propagates throughout the import graph of net/http: for +// every package imported by net/http that imports net/url, there must be an +// intermediate test variant that instead imports "net/url [net/url.test]". +// +// As one can see from the example of net/url and net/http, intermediate test +// variants can result in many additional packages that are essentially (but +// not quite) identical. For this reason, we filter these variants wherever +// possible. +func (m *Metadata) IsIntermediateTestVariant() bool { + return m.ForTest != "" && m.ForTest != m.PkgPath && m.ForTest+"_test" != m.PkgPath } var ErrViewExists = errors.New("view already exists for session") diff --git a/gopls/internal/lsp/source/workspace_symbol.go b/gopls/internal/lsp/source/workspace_symbol.go index dfb5f39dc1..dc5abd211f 100644 --- a/gopls/internal/lsp/source/workspace_symbol.go +++ b/gopls/internal/lsp/source/workspace_symbol.go @@ -88,17 +88,17 @@ type matcherFunc func(chunks []string) (int, float64) // // The space argument is an empty slice with spare capacity that may be used // to allocate the result. -type symbolizer func(space []string, name string, pkg Metadata, m matcherFunc) ([]string, float64) +type symbolizer func(space []string, name string, pkg *Metadata, m matcherFunc) ([]string, float64) -func fullyQualifiedSymbolMatch(space []string, name string, pkg Metadata, matcher matcherFunc) ([]string, float64) { +func fullyQualifiedSymbolMatch(space []string, name string, pkg *Metadata, matcher matcherFunc) ([]string, float64) { if _, score := dynamicSymbolMatch(space, name, pkg, matcher); score > 0 { - return append(space, string(pkg.PackagePath()), ".", name), score + return append(space, string(pkg.PkgPath), ".", name), score } return nil, 0 } -func dynamicSymbolMatch(space []string, name string, pkg Metadata, matcher matcherFunc) ([]string, float64) { - if IsCommandLineArguments(pkg.PackageID()) { +func dynamicSymbolMatch(space []string, name string, pkg *Metadata, matcher matcherFunc) ([]string, float64) { + if IsCommandLineArguments(pkg.ID) { // command-line-arguments packages have a non-sensical package path, so // just use their package name. return packageSymbolMatch(space, name, pkg, matcher) @@ -106,14 +106,14 @@ func dynamicSymbolMatch(space []string, name string, pkg Metadata, matcher match var score float64 - endsInPkgName := strings.HasSuffix(string(pkg.PackagePath()), string(pkg.PackageName())) + endsInPkgName := strings.HasSuffix(string(pkg.PkgPath), string(pkg.Name)) // If the package path does not end in the package name, we need to check the // package-qualified symbol as an extra pass first. if !endsInPkgName { - pkgQualified := append(space, string(pkg.PackageName()), ".", name) + pkgQualified := append(space, string(pkg.Name), ".", name) idx, score := matcher(pkgQualified) - nameStart := len(pkg.PackageName()) + 1 + nameStart := len(pkg.Name) + 1 if score > 0 { // If our match is contained entirely within the unqualified portion, // just return that. @@ -126,11 +126,11 @@ func dynamicSymbolMatch(space []string, name string, pkg Metadata, matcher match } // Now try matching the fully qualified symbol. - fullyQualified := append(space, string(pkg.PackagePath()), ".", name) + fullyQualified := append(space, string(pkg.PkgPath), ".", name) idx, score := matcher(fullyQualified) // As above, check if we matched just the unqualified symbol name. - nameStart := len(pkg.PackagePath()) + 1 + nameStart := len(pkg.PkgPath) + 1 if idx >= nameStart { return append(space, name), score } @@ -139,9 +139,9 @@ func dynamicSymbolMatch(space []string, name string, pkg Metadata, matcher match // initial pass above, so check if we matched just the package-qualified // name. if endsInPkgName && idx >= 0 { - pkgStart := len(pkg.PackagePath()) - len(pkg.PackageName()) + pkgStart := len(pkg.PkgPath) - len(pkg.Name) if idx >= pkgStart { - return append(space, string(pkg.PackageName()), ".", name), score + return append(space, string(pkg.Name), ".", name), score } } @@ -150,8 +150,8 @@ func dynamicSymbolMatch(space []string, name string, pkg Metadata, matcher match return fullyQualified, score * 0.6 } -func packageSymbolMatch(space []string, name string, pkg Metadata, matcher matcherFunc) ([]string, float64) { - qualified := append(space, string(pkg.PackageName()), ".", name) +func packageSymbolMatch(space []string, name string, pkg *Metadata, matcher matcherFunc) ([]string, float64) { + qualified := append(space, string(pkg.Name), ".", name) if _, s := matcher(qualified); s > 0 { return qualified, s } @@ -441,7 +441,7 @@ func convertFilterToRegexp(filter string) *regexp.Regexp { // symbolFile holds symbol information for a single file. type symbolFile struct { uri span.URI - md Metadata + md *Metadata syms []Symbol } @@ -526,7 +526,7 @@ func matchFile(store *symbolStore, symbolizer symbolizer, matcher matcherFunc, r kind: sym.Kind, uri: i.uri, rng: sym.Range, - container: string(i.md.PackagePath()), + container: string(i.md.PkgPath), } store.store(si) }