gopls/internal/lsp/cache: use typed strings (PackagePath et al) throughout
This changes uses the string types to enforce better hygiene
of conversions. Fishy conversions were flagged with TODO comments.
Perhaps some of these contribute to bugs in our support for vendoring.
Also, the function formerly known as ImportPath is now UnquoteImportPath.
Change-Id: Ia6bf8749908d881fb0a62cb6c98f7dd00563a69e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/449497
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Alan Donovan <adonovan@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
diff --git a/gopls/internal/lsp/cache/analysis.go b/gopls/internal/lsp/cache/analysis.go
index e15b43e..cbb6f04 100644
--- a/gopls/internal/lsp/cache/analysis.go
+++ b/gopls/internal/lsp/cache/analysis.go
@@ -24,7 +24,7 @@
"golang.org/x/tools/internal/memoize"
)
-func (s *snapshot) Analyze(ctx context.Context, id string, analyzers []*source.Analyzer) ([]*source.Diagnostic, error) {
+func (s *snapshot) Analyze(ctx context.Context, id PackageID, analyzers []*source.Analyzer) ([]*source.Diagnostic, error) {
// TODO(adonovan): merge these two loops. There's no need to
// construct all the root action handles before beginning
// analysis. Operations should be concurrent (though that first
@@ -35,7 +35,7 @@
if !a.IsEnabled(s.view) {
continue
}
- ah, err := s.actionHandle(ctx, PackageID(id), a.Analyzer)
+ ah, err := s.actionHandle(ctx, id, a.Analyzer)
if err != nil {
return nil, err
}
@@ -416,7 +416,7 @@
for _, diag := range rawDiagnostics {
srcDiags, err := analysisDiagnosticDiagnostics(snapshot, pkg, analyzer, &diag)
if err != nil {
- event.Error(ctx, "unable to compute analysis error position", err, tag.Category.Of(diag.Category), tag.Package.Of(pkg.ID()))
+ event.Error(ctx, "unable to compute analysis error position", err, tag.Category.Of(diag.Category), tag.Package.Of(string(pkg.ID())))
continue
}
diagnostics = append(diagnostics, srcDiags...)
@@ -477,7 +477,7 @@
errorAnalyzerDiag, err = s.Analyze(ctx, pkg.ID(), analyzers)
if err != nil {
// Keep going: analysis failures should not block diagnostics.
- event.Error(ctx, "type error analysis failed", err, tag.Package.Of(pkg.ID()))
+ event.Error(ctx, "type error analysis failed", err, tag.Package.Of(string(pkg.ID())))
}
}
diags := map[span.URI][]*source.Diagnostic{}
diff --git a/gopls/internal/lsp/cache/check.go b/gopls/internal/lsp/cache/check.go
index 17943ef..580ed0c 100644
--- a/gopls/internal/lsp/cache/check.go
+++ b/gopls/internal/lsp/cache/check.go
@@ -378,7 +378,7 @@
for _, e := range m.Errors {
diags, err := goPackagesErrorDiagnostics(snapshot, pkg, e)
if err != nil {
- event.Error(ctx, "unable to compute positions for list errors", err, tag.Package.Of(pkg.ID()))
+ event.Error(ctx, "unable to compute positions for list errors", err, tag.Package.Of(string(pkg.ID())))
continue
}
pkg.diagnostics = append(pkg.diagnostics, diags...)
@@ -400,7 +400,7 @@
for _, e := range pkg.parseErrors {
diags, err := parseErrorDiagnostics(snapshot, pkg, e)
if err != nil {
- event.Error(ctx, "unable to compute positions for parse errors", err, tag.Package.Of(pkg.ID()))
+ event.Error(ctx, "unable to compute positions for parse errors", err, tag.Package.Of(string(pkg.ID())))
continue
}
for _, diag := range diags {
@@ -418,7 +418,7 @@
for _, e := range expandErrors(unexpanded, snapshot.View().Options().RelatedInformationSupported) {
diags, err := typeErrorDiagnostics(snapshot, pkg, e)
if err != nil {
- event.Error(ctx, "unable to compute positions for type errors", err, tag.Package.Of(pkg.ID()))
+ event.Error(ctx, "unable to compute positions for type errors", err, tag.Package.Of(string(pkg.ID())))
continue
}
pkg.typeErrors = append(pkg.typeErrors, e.primary)
@@ -535,7 +535,7 @@
if !ok {
return nil, snapshot.missingPkgError(path)
}
- if !source.IsValidImport(string(m.PkgPath), string(dep.m.PkgPath)) {
+ if !source.IsValidImport(m.PkgPath, dep.m.PkgPath) {
return nil, fmt.Errorf("invalid use of internal package %s", path)
}
depPkg, err := dep.await(ctx, snapshot)
diff --git a/gopls/internal/lsp/cache/graph.go b/gopls/internal/lsp/cache/graph.go
index 1dac076..83336a2 100644
--- a/gopls/internal/lsp/cache/graph.go
+++ b/gopls/internal/lsp/cache/graph.go
@@ -93,8 +93,8 @@
}
// 2. command-line-args packages appear later.
- cli := source.IsCommandLineArguments(string(ids[i]))
- clj := source.IsCommandLineArguments(string(ids[j]))
+ cli := source.IsCommandLineArguments(ids[i])
+ clj := source.IsCommandLineArguments(ids[j])
if cli != clj {
return clj
}
@@ -121,7 +121,7 @@
}
// If we've seen *anything* prior to command-line arguments package, take
// it. Note that ids[0] may itself be command-line-arguments.
- if i > 0 && source.IsCommandLineArguments(string(id)) {
+ if i > 0 && source.IsCommandLineArguments(id) {
g.ids[uri] = ids[:i]
break
}
diff --git a/gopls/internal/lsp/cache/load.go b/gopls/internal/lsp/cache/load.go
index 9cabffc..5250a67 100644
--- a/gopls/internal/lsp/cache/load.go
+++ b/gopls/internal/lsp/cache/load.go
@@ -50,7 +50,9 @@
for _, scope := range scopes {
switch scope := scope.(type) {
case packageLoadScope:
- if source.IsCommandLineArguments(string(scope)) {
+ // TODO(adonovan): is this cast sound?? A
+ // packageLoadScope is really a PackagePath I think.
+ if source.IsCommandLineArguments(PackageID(scope)) {
panic("attempted to load command-line-arguments")
}
// The only time we pass package paths is when we're doing a
@@ -480,10 +482,10 @@
// Allow for multiple ad-hoc packages in the workspace (see #47584).
pkgPath := PackagePath(pkg.PkgPath)
id := PackageID(pkg.ID)
- if source.IsCommandLineArguments(pkg.ID) {
+ if source.IsCommandLineArguments(id) {
suffix := ":" + strings.Join(query, ",")
- id = PackageID(string(id) + suffix)
- pkgPath = PackagePath(string(pkgPath) + suffix)
+ id = PackageID(pkg.ID + suffix)
+ pkgPath = PackagePath(pkg.PkgPath + suffix)
}
if _, ok := updates[id]; ok {
@@ -712,7 +714,7 @@
continue
}
- if source.IsCommandLineArguments(string(m.ID)) {
+ if source.IsCommandLineArguments(m.ID) {
// If all the files contained in m have a real package, we don't need to
// keep m as a workspace package.
if allFilesHaveRealPackages(meta, m) {
@@ -754,7 +756,7 @@
checkURIs:
for _, uri := range append(m.CompiledGoFiles[0:n:n], m.GoFiles...) {
for _, id := range g.ids[uri] {
- if !source.IsCommandLineArguments(string(id)) && (g.metadata[id].Valid || !m.Valid) {
+ if !source.IsCommandLineArguments(id) && (g.metadata[id].Valid || !m.Valid) {
continue checkURIs
}
}
diff --git a/gopls/internal/lsp/cache/metadata.go b/gopls/internal/lsp/cache/metadata.go
index c8b0a53..03037f0 100644
--- a/gopls/internal/lsp/cache/metadata.go
+++ b/gopls/internal/lsp/cache/metadata.go
@@ -8,19 +8,16 @@
"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"
)
-// Declare explicit types for package paths, names, and IDs to ensure that we
-// never use an ID where a path belongs, and vice versa. If we confused these,
-// it would result in confusing errors because package IDs often look like
-// package paths.
type (
- PackageID string // go list's unique identifier for a package (e.g. "vendor/example.com/foo [vendor/example.com/bar.test]")
- PackagePath string // name used to prefix linker symbols (e.g. "vendor/example.com/foo")
- PackageName string // identifier in 'package' declaration (e.g. "foo")
- ImportPath string // path that appears in an import declaration (e.g. "example.com/foo")
+ PackageID = source.PackageID
+ PackagePath = source.PackagePath
+ PackageName = source.PackageName
+ ImportPath = source.ImportPath
)
// Metadata holds package Metadata extracted from a call to packages.Load.
@@ -43,19 +40,13 @@
}
// PackageID implements the source.Metadata interface.
-func (m *Metadata) PackageID() string {
- return string(m.ID)
-}
+func (m *Metadata) PackageID() PackageID { return m.ID }
// Name implements the source.Metadata interface.
-func (m *Metadata) PackageName() string {
- return string(m.Name)
-}
+func (m *Metadata) PackageName() PackageName { return m.Name }
// PkgPath implements the source.Metadata interface.
-func (m *Metadata) PackagePath() string {
- return string(m.PkgPath)
-}
+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]".
diff --git a/gopls/internal/lsp/cache/pkg.go b/gopls/internal/lsp/cache/pkg.go
index ddfb9ea..ef81778 100644
--- a/gopls/internal/lsp/cache/pkg.go
+++ b/gopls/internal/lsp/cache/pkg.go
@@ -36,7 +36,7 @@
analyses memoize.Store // maps analyzer.Name to Promise[actionResult]
}
-func (p *pkg) String() string { return p.ID() }
+func (p *pkg) String() string { return string(p.ID()) }
// A loadScope defines a package loading scope for use with go/packages.
type loadScope interface {
@@ -56,17 +56,9 @@
func (moduleLoadScope) aScope() {}
func (viewLoadScope) aScope() {}
-func (p *pkg) ID() string {
- return string(p.m.ID)
-}
-
-func (p *pkg) Name() string {
- return string(p.m.Name)
-}
-
-func (p *pkg) PkgPath() string {
- return string(p.m.PkgPath)
-}
+func (p *pkg) ID() PackageID { return p.m.ID }
+func (p *pkg) Name() PackageName { return p.m.Name }
+func (p *pkg) PkgPath() PackagePath { return p.m.PkgPath }
func (p *pkg) ParseMode() source.ParseMode {
return p.mode
@@ -118,8 +110,8 @@
// given its PackagePath. (If you have an ImportPath, e.g. a string
// from an import declaration, use ResolveImportPath instead.
// They may differ in case of vendoring.)
-func (p *pkg) DirectDep(pkgPath string) (source.Package, error) {
- if id, ok := p.m.DepsByPkgPath[PackagePath(pkgPath)]; ok {
+func (p *pkg) DirectDep(pkgPath PackagePath) (source.Package, error) {
+ if id, ok := p.m.DepsByPkgPath[pkgPath]; ok {
if imp := p.deps[id]; imp != nil {
return imp, nil
}
@@ -129,8 +121,8 @@
// ResolveImportPath returns the directly imported dependency of this package,
// given its ImportPath. See also DirectDep.
-func (p *pkg) ResolveImportPath(importPath string) (source.Package, error) {
- if id, ok := p.m.DepsByImpPath[ImportPath(importPath)]; ok && id != "" {
+func (p *pkg) ResolveImportPath(importPath ImportPath) (source.Package, error) {
+ if id, ok := p.m.DepsByImpPath[importPath]; ok && id != "" {
if imp := p.deps[id]; imp != nil {
return imp, nil
}
@@ -138,7 +130,7 @@
return nil, fmt.Errorf("package does not import %s", importPath)
}
-func (p *pkg) MissingDependencies() []string {
+func (p *pkg) MissingDependencies() []ImportPath {
// We don't invalidate metadata for import deletions,
// so check the package imports via the *types.Package.
//
@@ -162,13 +154,14 @@
// should be fast) then we can simply return the blank entries
// in DepsByImpPath. (They are PackageIDs not PackagePaths,
// but the caller only cares whether the set is empty!)
- var missing []string
+ var missing []ImportPath
for _, pkg := range p.types.Imports() {
- if id, ok := p.m.DepsByImpPath[ImportPath(pkg.Path())]; ok && id == "" {
- missing = append(missing, pkg.Path())
+ importPath := ImportPath(pkg.Path())
+ if id, ok := p.m.DepsByImpPath[importPath]; ok && id == "" {
+ missing = append(missing, importPath)
}
}
- sort.Strings(missing)
+ sort.Slice(missing, func(i, j int) bool { return missing[i] < missing[j] })
return missing
}
diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go
index d6599b1..f280304 100644
--- a/gopls/internal/lsp/cache/snapshot.go
+++ b/gopls/internal/lsp/cache/snapshot.go
@@ -811,17 +811,17 @@
return s.view.goversion >= 13 && s.view.Options().ExperimentalUseInvalidMetadata
}
-func (s *snapshot) GetReverseDependencies(ctx context.Context, id string) ([]source.Package, error) {
+func (s *snapshot) GetReverseDependencies(ctx context.Context, id PackageID) ([]source.Package, error) {
if err := s.awaitLoaded(ctx); err != nil {
return nil, err
}
s.mu.Lock()
meta := s.meta
s.mu.Unlock()
- ids := meta.reverseTransitiveClosure(s.useInvalidMetadata(), PackageID(id))
+ ids := meta.reverseTransitiveClosure(s.useInvalidMetadata(), id)
// Make sure to delete the original package ID from the map.
- delete(ids, PackageID(id))
+ delete(ids, id)
var pkgs []source.Package
for id := range ids {
@@ -1212,12 +1212,11 @@
return meta, nil
}
-func (s *snapshot) WorkspacePackageByID(ctx context.Context, id string) (source.Package, error) {
- packageID := PackageID(id)
- return s.checkedPackage(ctx, packageID, s.workspaceParseMode(packageID))
+func (s *snapshot) WorkspacePackageByID(ctx context.Context, id PackageID) (source.Package, error) {
+ return s.checkedPackage(ctx, id, s.workspaceParseMode(id))
}
-func (s *snapshot) CachedImportPaths(ctx context.Context) (map[string]source.Package, error) {
+func (s *snapshot) CachedImportPaths(ctx context.Context) (map[PackagePath]source.Package, error) {
// Don't reload workspace package metadata.
// This function is meant to only return currently cached information.
s.AwaitInitialized(ctx)
@@ -1225,7 +1224,7 @@
s.mu.Lock()
defer s.mu.Unlock()
- results := map[string]source.Package{}
+ results := map[PackagePath]source.Package{}
s.packages.Range(func(_, v interface{}) {
cachedPkg, err := v.(*packageHandle).cached()
if err != nil {
@@ -1983,7 +1982,7 @@
// For metadata that has been newly invalidated, capture package paths
// requiring reloading in the shouldLoad map.
- if invalidateMetadata && !source.IsCommandLineArguments(string(v.ID)) {
+ if invalidateMetadata && !source.IsCommandLineArguments(v.ID) {
if result.shouldLoad == nil {
result.shouldLoad = make(map[PackageID][]PackagePath)
}
