From 3c3713e6a543fc1870ade379dd0acb6bcc3c0c24 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Thu, 10 Nov 2022 13:02:38 -0500 Subject: [PATCH] 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 Auto-Submit: Alan Donovan Run-TryBot: Alan Donovan gopls-CI: kokoro Reviewed-by: Robert Findley --- gopls/internal/lsp/cache/analysis.go | 8 +-- gopls/internal/lsp/cache/check.go | 8 +-- gopls/internal/lsp/cache/graph.go | 6 +-- gopls/internal/lsp/cache/load.go | 14 ++--- gopls/internal/lsp/cache/metadata.go | 25 +++------ gopls/internal/lsp/cache/pkg.go | 35 +++++-------- gopls/internal/lsp/cache/snapshot.go | 17 +++---- gopls/internal/lsp/command.go | 10 ++-- gopls/internal/lsp/diagnostics.go | 8 +-- gopls/internal/lsp/link.go | 13 +++-- gopls/internal/lsp/semantic.go | 7 ++- gopls/internal/lsp/server.go | 4 +- .../lsp/source/completion/completion.go | 24 +++++---- .../internal/lsp/source/completion/package.go | 24 ++++----- .../lsp/source/completion/package_test.go | 8 ++- .../lsp/source/completion/postfix_snippets.go | 7 +-- gopls/internal/lsp/source/hover.go | 4 +- gopls/internal/lsp/source/identifier.go | 2 +- gopls/internal/lsp/source/implementation.go | 2 +- gopls/internal/lsp/source/known_packages.go | 21 ++++---- gopls/internal/lsp/source/references.go | 2 +- gopls/internal/lsp/source/rename.go | 51 ++++++++++--------- gopls/internal/lsp/source/rename_check.go | 5 +- gopls/internal/lsp/source/stub.go | 6 +-- gopls/internal/lsp/source/util.go | 20 ++++---- gopls/internal/lsp/source/view.go | 39 ++++++++------ gopls/internal/lsp/source/workspace_symbol.go | 14 ++--- 27 files changed, 193 insertions(+), 191 deletions(-) diff --git a/gopls/internal/lsp/cache/analysis.go b/gopls/internal/lsp/cache/analysis.go index e15b43e91c..cbb6f04e76 100644 --- a/gopls/internal/lsp/cache/analysis.go +++ b/gopls/internal/lsp/cache/analysis.go @@ -24,7 +24,7 @@ import ( "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 @@ func (s *snapshot) Analyze(ctx context.Context, id string, analyzers []*source.A 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 @@ func actionImpl(ctx context.Context, snapshot *snapshot, deps []*actionHandle, a 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 @@ func (s *snapshot) DiagnosePackage(ctx context.Context, spkg source.Package) (ma 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 17943efb70..580ed0c87e 100644 --- a/gopls/internal/lsp/cache/check.go +++ b/gopls/internal/lsp/cache/check.go @@ -378,7 +378,7 @@ func typeCheckImpl(ctx context.Context, snapshot *snapshot, goFiles, compiledGoF 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 @@ func typeCheckImpl(ctx context.Context, snapshot *snapshot, goFiles, compiledGoF 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 @@ func typeCheckImpl(ctx context.Context, snapshot *snapshot, goFiles, compiledGoF 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 @@ func doTypeCheck(ctx context.Context, snapshot *snapshot, goFiles, compiledGoFil 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 1dac0767af..83336a23e1 100644 --- a/gopls/internal/lsp/cache/graph.go +++ b/gopls/internal/lsp/cache/graph.go @@ -93,8 +93,8 @@ func (g *metadataGraph) build() { } // 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 @@ func (g *metadataGraph) build() { } // 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 9cabffc0d0..5250a67f14 100644 --- a/gopls/internal/lsp/cache/load.go +++ b/gopls/internal/lsp/cache/load.go @@ -50,7 +50,9 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...loadSc 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 @@ func buildMetadata(ctx context.Context, pkg *packages.Package, cfg *packages.Con // 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 @@ func computeWorkspacePackagesLocked(s *snapshot, meta *metadataGraph) map[Packag 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 @@ func allFilesHaveRealPackages(g *metadataGraph, m *KnownMetadata) bool { 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 c8b0a53722..03037f0b09 100644 --- a/gopls/internal/lsp/cache/metadata.go +++ b/gopls/internal/lsp/cache/metadata.go @@ -8,19 +8,16 @@ 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" ) -// 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 @@ type Metadata struct { } // 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 ddfb9ea7e9..ef8177871b 100644 --- a/gopls/internal/lsp/cache/pkg.go +++ b/gopls/internal/lsp/cache/pkg.go @@ -36,7 +36,7 @@ type pkg struct { 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 (packageLoadScope) aScope() {} 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 @@ func (p *pkg) ForTest() string { // 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 @@ func (p *pkg) DirectDep(pkgPath string) (source.Package, error) { // 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 @@ func (p *pkg) ResolveImportPath(importPath string) (source.Package, error) { 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 @@ func (p *pkg) MissingDependencies() []string { // 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 d6599b1cee..f28030431a 100644 --- a/gopls/internal/lsp/cache/snapshot.go +++ b/gopls/internal/lsp/cache/snapshot.go @@ -811,17 +811,17 @@ func (s *snapshot) useInvalidMetadata() bool { 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 @@ func (s *snapshot) AllValidMetadata(ctx context.Context) ([]source.Metadata, err 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 @@ func (s *snapshot) CachedImportPaths(ctx context.Context) (map[string]source.Pac 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 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC // 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) } diff --git a/gopls/internal/lsp/command.go b/gopls/internal/lsp/command.go index 9722b7864e..3b0d3b849d 100644 --- a/gopls/internal/lsp/command.go +++ b/gopls/internal/lsp/command.go @@ -735,8 +735,10 @@ func (c *commandHandler) ListKnownPackages(ctx context.Context, args command.URI progress: "Listing packages", forURI: args.URI, }, func(ctx context.Context, deps commandDeps) error { - var err error - result.Packages, err = source.KnownPackages(ctx, deps.snapshot, deps.fh) + pkgs, err := source.KnownPackages(ctx, deps.snapshot, deps.fh) + for _, pkg := range pkgs { + result.Packages = append(result.Packages, string(pkg)) + } return err }) return result, err @@ -765,14 +767,14 @@ func (c *commandHandler) ListImports(ctx context.Context, args command.URIArg) ( name = imp.Name.Name } result.Imports = append(result.Imports, command.FileImport{ - Path: source.ImportPath(imp), + Path: string(source.UnquoteImportPath(imp)), Name: name, }) } } for _, imp := range pkg.Imports() { result.PackageImports = append(result.PackageImports, command.PackageImport{ - Path: imp.PkgPath(), // This might be the vendored path under GOPATH vendoring, in which case it's a bug. + Path: string(imp.PkgPath()), // This might be the vendored path under GOPATH vendoring, in which case it's a bug. }) } sort.Slice(result.PackageImports, func(i, j int) bool { diff --git a/gopls/internal/lsp/diagnostics.go b/gopls/internal/lsp/diagnostics.go index a01898b26a..bbc57ffaba 100644 --- a/gopls/internal/lsp/diagnostics.go +++ b/gopls/internal/lsp/diagnostics.go @@ -346,7 +346,7 @@ func (s *Server) diagnose(ctx context.Context, snapshot source.Snapshot, forceAn } func (s *Server) diagnosePkg(ctx context.Context, snapshot source.Snapshot, pkg source.Package, alwaysAnalyze bool) { - ctx, done := event.Start(ctx, "Server.diagnosePkg", tag.Snapshot.Of(snapshot.ID()), tag.Package.Of(pkg.ID())) + ctx, done := event.Start(ctx, "Server.diagnosePkg", tag.Snapshot.Of(snapshot.ID()), tag.Package.Of(string(pkg.ID()))) defer done() enableDiagnostics := false includeAnalysis := alwaysAnalyze // only run analyses for packages with open files @@ -361,7 +361,7 @@ func (s *Server) diagnosePkg(ctx context.Context, snapshot source.Snapshot, pkg pkgDiagnostics, err := snapshot.DiagnosePackage(ctx, pkg) if err != nil { - event.Error(ctx, "warning: diagnosing package", err, tag.Snapshot.Of(snapshot.ID()), tag.Package.Of(pkg.ID())) + event.Error(ctx, "warning: diagnosing package", err, tag.Snapshot.Of(snapshot.ID()), tag.Package.Of(string(pkg.ID()))) return } for _, cgf := range pkg.CompiledGoFiles() { @@ -374,7 +374,7 @@ func (s *Server) diagnosePkg(ctx context.Context, snapshot source.Snapshot, pkg if includeAnalysis && !pkg.HasListOrParseErrors() { reports, err := source.Analyze(ctx, snapshot, pkg, false) if err != nil { - event.Error(ctx, "warning: analyzing package", err, tag.Snapshot.Of(snapshot.ID()), tag.Package.Of(pkg.ID())) + event.Error(ctx, "warning: analyzing package", err, tag.Snapshot.Of(snapshot.ID()), tag.Package.Of(string(pkg.ID()))) return } for _, cgf := range pkg.CompiledGoFiles() { @@ -390,7 +390,7 @@ func (s *Server) diagnosePkg(ctx context.Context, snapshot source.Snapshot, pkg if enableGCDetails { gcReports, err := source.GCOptimizationDetails(ctx, snapshot, pkg) if err != nil { - event.Error(ctx, "warning: gc details", err, tag.Snapshot.Of(snapshot.ID()), tag.Package.Of(pkg.ID())) + event.Error(ctx, "warning: gc details", err, tag.Snapshot.Of(snapshot.ID()), tag.Package.Of(string(pkg.ID()))) } s.gcOptimizationDetailsMu.Lock() _, enableGCDetails := s.gcOptimizationDetails[pkg.ID()] diff --git a/gopls/internal/lsp/link.go b/gopls/internal/lsp/link.go index b26bfb33c8..7eb561fa00 100644 --- a/gopls/internal/lsp/link.go +++ b/gopls/internal/lsp/link.go @@ -12,7 +12,6 @@ import ( "go/token" "net/url" "regexp" - "strconv" "strings" "sync" @@ -133,21 +132,21 @@ func goLinks(ctx context.Context, snapshot source.Snapshot, fh source.FileHandle // https://pkg.go.dev. if view.Options().ImportShortcut.ShowLinks() { for _, imp := range imports { - target, err := strconv.Unquote(imp.Path.Value) - if err != nil { + target := source.UnquoteImportPath(imp) + if target == "" { continue } // See golang/go#36998: don't link to modules matching GOPRIVATE. - if view.IsGoPrivatePath(target) { + if view.IsGoPrivatePath(string(target)) { continue } if mod, version, ok := moduleAtVersion(target, pkg); ok && strings.ToLower(view.Options().LinkTarget) == "pkg.go.dev" { - target = strings.Replace(target, mod, mod+"@"+version, 1) + target = source.ImportPath(strings.Replace(string(target), mod, mod+"@"+version, 1)) } // Account for the quotation marks in the positions. start := imp.Path.Pos() + 1 end := imp.Path.End() - 1 - targetURL := source.BuildLink(view.Options().LinkTarget, target, "") + targetURL := source.BuildLink(view.Options().LinkTarget, string(target), "") l, err := toProtocolLink(pgf.Tok, pgf.Mapper, targetURL, start, end) if err != nil { return nil, err @@ -174,7 +173,7 @@ func goLinks(ctx context.Context, snapshot source.Snapshot, fh source.FileHandle return links, nil } -func moduleAtVersion(targetImportPath string, pkg source.Package) (string, string, bool) { +func moduleAtVersion(targetImportPath source.ImportPath, pkg source.Package) (string, string, bool) { impPkg, err := pkg.ResolveImportPath(targetImportPath) if err != nil { return "", "", false diff --git a/gopls/internal/lsp/semantic.go b/gopls/internal/lsp/semantic.go index a8b38f044a..75f88b3efa 100644 --- a/gopls/internal/lsp/semantic.go +++ b/gopls/internal/lsp/semantic.go @@ -15,7 +15,6 @@ import ( "log" "path/filepath" "sort" - "strconv" "strings" "time" @@ -905,8 +904,8 @@ func (e *encoded) importSpec(d *ast.ImportSpec) { } return // don't mark anything for . or _ } - importPath, err := strconv.Unquote(d.Path.Value) - if err != nil { + importPath := source.UnquoteImportPath(d) + if importPath == "" { return } // Import strings are implementation defined. Try to match with parse information. @@ -916,7 +915,7 @@ func (e *encoded) importSpec(d *ast.ImportSpec) { return } // Check whether the original literal contains the package's declared name. - j := strings.LastIndex(d.Path.Value, imported.Name()) + j := strings.LastIndex(d.Path.Value, string(imported.Name())) if j == -1 { // name doesn't show up, for whatever reason, so nothing to report return diff --git a/gopls/internal/lsp/server.go b/gopls/internal/lsp/server.go index 693afaedab..9f7059141d 100644 --- a/gopls/internal/lsp/server.go +++ b/gopls/internal/lsp/server.go @@ -27,7 +27,7 @@ func NewServer(session *cache.Session, client protocol.ClientCloser) *Server { session.SetProgressTracker(tracker) return &Server{ diagnostics: map[span.URI]*fileReports{}, - gcOptimizationDetails: make(map[string]struct{}), + gcOptimizationDetails: make(map[source.PackageID]struct{}), watchedGlobPatterns: make(map[string]struct{}), changedFiles: make(map[span.URI]struct{}), session: session, @@ -97,7 +97,7 @@ type Server struct { // optimization details to be included in the diagnostics. The key is the // ID of the package. gcOptimizationDetailsMu sync.Mutex - gcOptimizationDetails map[string]struct{} + gcOptimizationDetails map[source.PackageID]struct{} // diagnosticsSema limits the concurrency of diagnostics runs, which can be // expensive. diff --git a/gopls/internal/lsp/source/completion/completion.go b/gopls/internal/lsp/source/completion/completion.go index c3b7c2b461..1f8dd929b2 100644 --- a/gopls/internal/lsp/source/completion/completion.go +++ b/gopls/internal/lsp/source/completion/completion.go @@ -1089,7 +1089,7 @@ func (c *completer) selector(ctx context.Context, sel *ast.SelectorExpr) error { if pkgName, ok := c.pkg.GetTypesInfo().Uses[id].(*types.PkgName); ok { var pkg source.Package for _, imp := range c.pkg.Imports() { - if imp.PkgPath() == pkgName.Imported().Path() { + if imp.PkgPath() == source.PackagePath(pkgName.Imported().Path()) { pkg = imp } } @@ -1140,7 +1140,7 @@ func (c *completer) unimportedMembers(ctx context.Context, id *ast.Ident) error if pkg.GetTypes().Name() != id.Name { continue } - paths = append(paths, path) + paths = append(paths, string(path)) } var relevances map[string]float64 @@ -1158,7 +1158,7 @@ func (c *completer) unimportedMembers(ctx context.Context, id *ast.Ident) error }) for _, path := range paths { - pkg := known[path] + pkg := known[source.PackagePath(path)] if pkg.GetTypes().Name() != id.Name { continue } @@ -1182,7 +1182,8 @@ func (c *completer) unimportedMembers(ctx context.Context, id *ast.Ident) error add := func(pkgExport imports.PackageExport) { mu.Lock() defer mu.Unlock() - if _, ok := known[pkgExport.Fix.StmtInfo.ImportPath]; ok { + // TODO(adonovan): what if the actual package has a vendor/ prefix? + if _, ok := known[source.PackagePath(pkgExport.Fix.StmtInfo.ImportPath)]; ok { return // We got this one above. } @@ -1379,7 +1380,8 @@ func (c *completer) lexical(ctx context.Context) error { // Make sure the package name isn't already in use by another // object, and that this file doesn't import the package yet. - if _, ok := seen[pkg.Name()]; !ok && pkg != c.pkg.GetTypes() && !alreadyImports(c.file, pkg.Path()) { + // TODO(adonovan): what if pkg.Path has vendor/ prefix? + if _, ok := seen[pkg.Name()]; !ok && pkg != c.pkg.GetTypes() && !alreadyImports(c.file, source.ImportPath(pkg.Path())) { seen[pkg.Name()] = struct{}{} obj := types.NewPkgName(0, nil, pkg.Name(), pkg) imp := &importInfo{ @@ -1481,12 +1483,12 @@ func (c *completer) unimportedPackages(ctx context.Context, seen map[string]stru if err != nil { return err } - var paths []string + var paths []string // actually PackagePaths for path, pkg := range known { if !strings.HasPrefix(pkg.GetTypes().Name(), prefix) { continue } - paths = append(paths, path) + paths = append(paths, string(path)) } var relevances map[string]float64 @@ -1511,7 +1513,7 @@ func (c *completer) unimportedPackages(ctx context.Context, seen map[string]stru }) for _, path := range paths { - pkg := known[path] + pkg := known[source.PackagePath(path)] if _, ok := seen[pkg.GetTypes().Name()]; ok { continue } @@ -1526,7 +1528,7 @@ func (c *completer) unimportedPackages(ctx context.Context, seen map[string]stru } c.deepState.enqueue(candidate{ // Pass an empty *types.Package to disable deep completions. - obj: types.NewPkgName(0, nil, pkg.GetTypes().Name(), types.NewPackage(path, pkg.Name())), + obj: types.NewPkgName(0, nil, pkg.GetTypes().Name(), types.NewPackage(path, string(pkg.Name()))), score: unimportedScore(relevances[path]), imp: imp, }) @@ -1573,9 +1575,9 @@ func (c *completer) unimportedPackages(ctx context.Context, seen map[string]stru } // alreadyImports reports whether f has an import with the specified path. -func alreadyImports(f *ast.File, path string) bool { +func alreadyImports(f *ast.File, path source.ImportPath) bool { for _, s := range f.Imports { - if source.ImportPath(s) == path { + if source.UnquoteImportPath(s) == path { return true } } diff --git a/gopls/internal/lsp/source/completion/package.go b/gopls/internal/lsp/source/completion/package.go index b7fad0fa51..1b7c731fd8 100644 --- a/gopls/internal/lsp/source/completion/package.go +++ b/gopls/internal/lsp/source/completion/package.go @@ -240,7 +240,7 @@ func packageSuggestions(ctx context.Context, snapshot source.Snapshot, fileURI s } pkgName := convertDirNameToPkgName(dirName) - seenPkgs := make(map[string]struct{}) + seenPkgs := make(map[source.PackageName]struct{}) // The `go` command by default only allows one package per directory but we // support multiple package suggestions since gopls is build system agnostic. @@ -267,30 +267,30 @@ func packageSuggestions(ctx context.Context, snapshot source.Snapshot, fileURI s // Add a found package used in current directory as a high relevance // suggestion and the test package for it as a medium relevance // suggestion. - if score := float64(matcher.Score(pkg.Name())); score > 0 { - packages = append(packages, toCandidate(pkg.Name(), score*highScore)) + if score := float64(matcher.Score(string(pkg.Name()))); score > 0 { + packages = append(packages, toCandidate(string(pkg.Name()), score*highScore)) } seenPkgs[pkg.Name()] = struct{}{} testPkgName := pkg.Name() + "_test" - if _, ok := seenPkgs[testPkgName]; ok || strings.HasSuffix(pkg.Name(), "_test") { + if _, ok := seenPkgs[testPkgName]; ok || strings.HasSuffix(string(pkg.Name()), "_test") { continue } - if score := float64(matcher.Score(testPkgName)); score > 0 { - packages = append(packages, toCandidate(testPkgName, score*stdScore)) + if score := float64(matcher.Score(string(testPkgName))); score > 0 { + packages = append(packages, toCandidate(string(testPkgName), score*stdScore)) } seenPkgs[testPkgName] = struct{}{} } // Add current directory name as a low relevance suggestion. if _, ok := seenPkgs[pkgName]; !ok { - if score := float64(matcher.Score(pkgName)); score > 0 { - packages = append(packages, toCandidate(pkgName, score*lowScore)) + if score := float64(matcher.Score(string(pkgName))); score > 0 { + packages = append(packages, toCandidate(string(pkgName), score*lowScore)) } testPkgName := pkgName + "_test" - if score := float64(matcher.Score(testPkgName)); score > 0 { - packages = append(packages, toCandidate(testPkgName, score*lowScore)) + if score := float64(matcher.Score(string(testPkgName))); score > 0 { + packages = append(packages, toCandidate(string(testPkgName), score*lowScore)) } } @@ -330,7 +330,7 @@ func isValidDirName(dirName string) bool { // convertDirNameToPkgName converts a valid directory name to a valid package name. // It leaves only letters and digits. All letters are mapped to lower case. -func convertDirNameToPkgName(dirName string) string { +func convertDirNameToPkgName(dirName string) source.PackageName { var buf bytes.Buffer for _, ch := range dirName { switch { @@ -341,7 +341,7 @@ func convertDirNameToPkgName(dirName string) string { buf.WriteRune(ch) } } - return buf.String() + return source.PackageName(buf.String()) } // isLetter and isDigit allow only ASCII characters because diff --git a/gopls/internal/lsp/source/completion/package_test.go b/gopls/internal/lsp/source/completion/package_test.go index 6436984fdc..614359fa5d 100644 --- a/gopls/internal/lsp/source/completion/package_test.go +++ b/gopls/internal/lsp/source/completion/package_test.go @@ -4,7 +4,11 @@ package completion -import "testing" +import ( + "testing" + + "golang.org/x/tools/gopls/internal/lsp/source" +) func TestIsValidDirName(t *testing.T) { tests := []struct { @@ -51,7 +55,7 @@ func TestIsValidDirName(t *testing.T) { func TestConvertDirNameToPkgName(t *testing.T) { tests := []struct { dirName string - pkgName string + pkgName source.PackageName }{ {dirName: "a", pkgName: "a"}, {dirName: "abcdef", pkgName: "abcdef"}, diff --git a/gopls/internal/lsp/source/completion/postfix_snippets.go b/gopls/internal/lsp/source/completion/postfix_snippets.go index 414db42592..3c805a77e1 100644 --- a/gopls/internal/lsp/source/completion/postfix_snippets.go +++ b/gopls/internal/lsp/source/completion/postfix_snippets.go @@ -16,11 +16,11 @@ import ( "sync" "text/template" - "golang.org/x/tools/internal/event" - "golang.org/x/tools/internal/imports" "golang.org/x/tools/gopls/internal/lsp/protocol" "golang.org/x/tools/gopls/internal/lsp/snippet" "golang.org/x/tools/gopls/internal/lsp/source" + "golang.org/x/tools/internal/event" + "golang.org/x/tools/internal/imports" ) // Postfix snippets are artificial methods that allow the user to @@ -442,7 +442,8 @@ func (c *completer) importIfNeeded(pkgPath string, scope *types.Scope) (string, // Check if file already imports pkgPath. for _, s := range c.file.Imports { - if source.ImportPath(s) == pkgPath { + // TODO(adonovan): what if pkgPath has a vendor/ prefix? + if source.UnquoteImportPath(s) == source.ImportPath(pkgPath) { if s.Name == nil { return defaultName, nil, nil } diff --git a/gopls/internal/lsp/source/hover.go b/gopls/internal/lsp/source/hover.go index 09f7224c80..0bd5a3055e 100644 --- a/gopls/internal/lsp/source/hover.go +++ b/gopls/internal/lsp/source/hover.go @@ -448,7 +448,7 @@ func moduleAtVersion(path string, i *IdentifierInfo) (string, string, bool) { if strings.ToLower(i.Snapshot.View().Options().LinkTarget) != "pkg.go.dev" { return "", "", false } - impPkg, err := i.pkg.DirectDep(path) + impPkg, err := i.pkg.DirectDep(PackagePath(path)) if err != nil { return "", "", false } @@ -539,7 +539,7 @@ func FindHoverContext(ctx context.Context, s Snapshot, pkg Package, obj types.Ob if err != nil { return nil, err } - imp, err := pkg.ResolveImportPath(importPath) + imp, err := pkg.ResolveImportPath(ImportPath(importPath)) if err != nil { return nil, err } diff --git a/gopls/internal/lsp/source/identifier.go b/gopls/internal/lsp/source/identifier.go index f11817f586..526c108470 100644 --- a/gopls/internal/lsp/source/identifier.go +++ b/gopls/internal/lsp/source/identifier.go @@ -456,7 +456,7 @@ func importSpec(snapshot Snapshot, pkg Package, file *ast.File, pos token.Pos) ( if err != nil { return nil, fmt.Errorf("import path not quoted: %s (%v)", imp.Path.Value, err) } - imported, err := pkg.ResolveImportPath(importPath) + imported, err := pkg.ResolveImportPath(ImportPath(importPath)) if err != nil { return nil, err } diff --git a/gopls/internal/lsp/source/implementation.go b/gopls/internal/lsp/source/implementation.go index 2da488dd67..87f435108d 100644 --- a/gopls/internal/lsp/source/implementation.go +++ b/gopls/internal/lsp/source/implementation.go @@ -323,7 +323,7 @@ func qualifiedObjsAtLocation(ctx context.Context, s Snapshot, key positionKey, s // Look up the implicit *types.PkgName. obj := searchpkg.GetTypesInfo().Implicits[leaf] if obj == nil { - return nil, fmt.Errorf("%w for import %q", errNoObjectFound, ImportPath(leaf)) + return nil, fmt.Errorf("%w for import %s", errNoObjectFound, UnquoteImportPath(leaf)) } objs = append(objs, obj) } diff --git a/gopls/internal/lsp/source/known_packages.go b/gopls/internal/lsp/source/known_packages.go index e2d950ba00..efaaee57ae 100644 --- a/gopls/internal/lsp/source/known_packages.go +++ b/gopls/internal/lsp/source/known_packages.go @@ -19,14 +19,15 @@ import ( // KnownPackages returns a list of all known packages // in the package graph that could potentially be imported // by the given file. -func KnownPackages(ctx context.Context, snapshot Snapshot, fh VersionedFileHandle) ([]string, error) { +func KnownPackages(ctx context.Context, snapshot Snapshot, fh VersionedFileHandle) ([]PackagePath, error) { pkg, pgf, err := GetParsedFile(ctx, snapshot, fh, NarrowestPackage) if err != nil { return nil, fmt.Errorf("GetParsedFile: %w", err) } - alreadyImported := map[string]struct{}{} + alreadyImported := map[PackagePath]struct{}{} for _, imp := range pgf.File.Imports { - alreadyImported[imp.Path.Value] = struct{}{} + // TODO(adonovan): the correct PackagePath might need a "vendor/" prefix. + alreadyImported[PackagePath(imp.Path.Value)] = struct{}{} } // TODO(adonovan): this whole algorithm could be more // simply expressed in terms of Metadata, not Packages. @@ -35,8 +36,8 @@ func KnownPackages(ctx context.Context, snapshot Snapshot, fh VersionedFileHandl return nil, err } var ( - seen = make(map[string]struct{}) - paths []string + seen = make(map[PackagePath]struct{}) + paths []PackagePath ) for path, knownPkg := range pkgs { gofiles := knownPkg.CompiledGoFiles() @@ -79,10 +80,12 @@ func KnownPackages(ctx context.Context, snapshot Snapshot, fh VersionedFileHandl return imports.GetAllCandidates(ctx, func(ifix imports.ImportFix) { mu.Lock() defer mu.Unlock() - if _, ok := seen[ifix.StmtInfo.ImportPath]; ok { + // TODO(adonovan): what if the actual package path has a vendor/ prefix? + path := PackagePath(ifix.StmtInfo.ImportPath) + if _, ok := seen[path]; ok { return } - paths = append(paths, ifix.StmtInfo.ImportPath) + paths = append(paths, path) }, "", pgf.URI.Filename(), pkg.GetTypes().Name(), o.Env) }) if err != nil { @@ -92,8 +95,8 @@ func KnownPackages(ctx context.Context, snapshot Snapshot, fh VersionedFileHandl } sort.Slice(paths, func(i, j int) bool { importI, importJ := paths[i], paths[j] - iHasDot := strings.Contains(importI, ".") - jHasDot := strings.Contains(importJ, ".") + iHasDot := strings.Contains(string(importI), ".") + jHasDot := strings.Contains(string(importJ), ".") if iHasDot && !jHasDot { return false } diff --git a/gopls/internal/lsp/source/references.go b/gopls/internal/lsp/source/references.go index e26091c9fe..4db716e6dd 100644 --- a/gopls/internal/lsp/source/references.go +++ b/gopls/internal/lsp/source/references.go @@ -78,7 +78,7 @@ func References(ctx context.Context, s Snapshot, f FileHandle, pp protocol.Posit for _, dep := range rdeps { for _, f := range dep.CompiledGoFiles() { for _, imp := range f.File.Imports { - if path, err := strconv.Unquote(imp.Path.Value); err == nil && path == renamingPkg.PkgPath() { + if path, err := strconv.Unquote(imp.Path.Value); err == nil && path == string(renamingPkg.PkgPath()) { refs = append(refs, &ReferenceInfo{ Name: packageName, MappedRange: NewMappedRange(f.Tok, f.Mapper, imp.Pos(), imp.End()), diff --git a/gopls/internal/lsp/source/rename.go b/gopls/internal/lsp/source/rename.go index 2d4a188b8a..3a8d7cfb6b 100644 --- a/gopls/internal/lsp/source/rename.go +++ b/gopls/internal/lsp/source/rename.go @@ -93,7 +93,7 @@ func PrepareRename(ctx context.Context, snapshot Snapshot, f FileHandle, pp prot return nil, err, err } - if strings.HasSuffix(meta.PackageName(), "_test") { + if strings.HasSuffix(string(meta.PackageName()), "_test") { err := errors.New("can't rename x_test packages") return nil, err, err } @@ -103,7 +103,7 @@ func PrepareRename(ctx context.Context, snapshot Snapshot, f FileHandle, pp prot return nil, err, err } - if meta.ModuleInfo().Path == meta.PackagePath() { + 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) return nil, err, err } @@ -113,7 +113,7 @@ func PrepareRename(ctx context.Context, snapshot Snapshot, f FileHandle, pp prot err = fmt.Errorf("error building package to rename: %v", err) return nil, err, err } - result, err := computePrepareRenameResp(snapshot, pkg, pgf.File.Name, pkg.Name()) + result, err := computePrepareRenameResp(snapshot, pkg, pgf.File.Name, string(pkg.Name())) if err != nil { return nil, nil, err } @@ -202,11 +202,11 @@ func Rename(ctx context.Context, s Snapshot, f FileHandle, pp protocol.Position, // Fix this. meta := fileMeta[0] oldPath := meta.PackagePath() - var modulePath string + 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()) } else { - modulePath = mi.Path + modulePath = PackagePath(mi.Path) } if strings.HasSuffix(newName, "_test") { @@ -218,7 +218,7 @@ func Rename(ctx context.Context, s Snapshot, f FileHandle, pp protocol.Position, return nil, true, err } - renamingEdits, err := renamePackage(ctx, s, modulePath, oldPath, newName, metadata) + renamingEdits, err := renamePackage(ctx, s, modulePath, oldPath, PackageName(newName), metadata) if err != nil { return nil, true, err } @@ -245,12 +245,12 @@ 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, newName string, 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) } - newPathPrefix := path.Join(path.Dir(oldPath), newName) + newPathPrefix := path.Join(path.Dir(string(oldPath)), string(newName)) edits := make(map[span.URI][]protocol.TextEdit) seen := make(seenPackageRename) // track per-file import renaming we've already processed @@ -272,7 +272,7 @@ func renamePackage(ctx context.Context, s Snapshot, modulePath, oldPath, newName // 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(m.PackagePath()+"/", oldPath+"/") { + if !strings.HasPrefix(string(m.PackagePath())+"/", string(oldPath)+"/") { continue // not affected by the package renaming } @@ -280,16 +280,16 @@ func renamePackage(ctx context.Context, s Snapshot, modulePath, oldPath, newName return nil, fmt.Errorf("cannot rename package: missing module information for package %q", m.PackagePath()) } - if modulePath != m.ModuleInfo().Path { + if modulePath != PackagePath(m.ModuleInfo().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(m.PackagePath(), oldPath) + suffix := strings.TrimPrefix(string(m.PackagePath()), string(oldPath)) newPath := newPathPrefix + suffix pkgName := m.PackageName() - if m.PackagePath() == oldPath { + if m.PackagePath() == PackagePath(oldPath) { pkgName = newName if err := renamePackageClause(ctx, m, s, newName, seen, edits); err != nil { @@ -297,7 +297,8 @@ func renamePackage(ctx context.Context, s Snapshot, modulePath, oldPath, newName } } - if err := renameImports(ctx, s, m, newPath, pkgName, seen, edits); err != nil { + imp := ImportPath(newPath) // TODO(adonovan): what if newPath has vendor/ prefix? + if err := renameImports(ctx, s, m, imp, pkgName, seen, edits); err != nil { return nil, err } } @@ -314,14 +315,14 @@ func renamePackage(ctx context.Context, s Snapshot, modulePath, oldPath, newName // However, in all cases the resulting edits will be the same. type seenPackageRename map[seenPackageKey]bool type seenPackageKey struct { - uri span.URI - importPath string + uri span.URI + path PackagePath } // add reports whether uri and importPath have been seen, and records them as // seen if not. -func (s seenPackageRename) add(uri span.URI, importPath string) bool { - key := seenPackageKey{uri, importPath} +func (s seenPackageRename) add(uri span.URI, path PackagePath) bool { + key := seenPackageKey{uri, path} seen := s[key] if !seen { s[key] = true @@ -336,7 +337,7 @@ func (s seenPackageRename) add(uri span.URI, importPath string) 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 string, seen seenPackageRename, edits map[span.URI][]protocol.TextEdit) error { +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()) if err != nil { return err @@ -358,7 +359,7 @@ func renamePackageClause(ctx context.Context, m Metadata, s Snapshot, newName st } edits[f.URI] = append(edits[f.URI], protocol.TextEdit{ Range: rng, - NewText: newName, + NewText: string(newName), }) } @@ -370,7 +371,7 @@ func renamePackageClause(ctx context.Context, m Metadata, s Snapshot, newName st // newPath and name newName. // // Edits are written into the edits map. -func renameImports(ctx context.Context, s Snapshot, m Metadata, newPath, newName string, 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. @@ -399,7 +400,8 @@ func renameImports(ctx context.Context, s Snapshot, m Metadata, newPath, newName } for _, imp := range f.File.Imports { - if impPath, _ := strconv.Unquote(imp.Path.Value); impPath != m.PackagePath() { + // TODO(adonovan): what if RHS has "vendor/" prefix? + if UnquoteImportPath(imp) != ImportPath(m.PackagePath()) { continue // not the import we're looking for } @@ -409,10 +411,9 @@ func renameImports(ctx context.Context, s Snapshot, m Metadata, newPath, newName if err != nil { return err } - newText := strconv.Quote(newPath) edits[f.URI] = append(edits[f.URI], protocol.TextEdit{ Range: rng, - NewText: newText, + NewText: strconv.Quote(string(newPath)), }) // If the package name of an import has not changed or if its import @@ -430,7 +431,7 @@ func renameImports(ctx context.Context, s Snapshot, m Metadata, newPath, newName fileScope := dep.GetTypesInfo().Scopes[f.File] var changes map[span.URI][]protocol.TextEdit - localName := newName + localName := string(newName) try := 0 // Keep trying with fresh names until one succeeds. @@ -446,7 +447,7 @@ func renameImports(ctx context.Context, s Snapshot, m Metadata, newPath, newName // If the chosen local package name matches the package's new name, delete the // change that would have inserted an explicit local name, which is always // the lexically first change. - if localName == newName { + if localName == string(newName) { v := changes[f.URI] sort.Slice(v, func(i, j int) bool { return protocol.CompareRange(v[i].Range, v[j].Range) < 0 diff --git a/gopls/internal/lsp/source/rename_check.go b/gopls/internal/lsp/source/rename_check.go index c4d5709bf7..b81b6f6856 100644 --- a/gopls/internal/lsp/source/rename_check.go +++ b/gopls/internal/lsp/source/rename_check.go @@ -12,7 +12,6 @@ import ( "go/token" "go/types" "reflect" - "strconv" "strings" "unicode" @@ -834,8 +833,8 @@ func pathEnclosingInterval(fset *token.FileSet, pkg Package, start, end token.Po if imp == nil { continue } - importPath, err := strconv.Unquote(imp.Path.Value) - if err != nil { + importPath := UnquoteImportPath(imp) + if importPath == "" { continue } imported, err := pkg.ResolveImportPath(importPath) diff --git a/gopls/internal/lsp/source/stub.go b/gopls/internal/lsp/source/stub.go index 28a2e2b23d..8e3d8567d0 100644 --- a/gopls/internal/lsp/source/stub.go +++ b/gopls/internal/lsp/source/stub.go @@ -194,7 +194,7 @@ func deducePkgFromTypes(ctx context.Context, snapshot Snapshot, ifaceObj types.O return nil, err } for _, p := range pkgs { - if p.PkgPath() == ifaceObj.Pkg().Path() { + if p.PkgPath() == PackagePath(ifaceObj.Pkg().Path()) { return p, nil } } @@ -254,11 +254,11 @@ func missingMethods(ctx context.Context, snapshot Snapshot, concMS *types.Method for i := 0; i < iface.NumEmbeddeds(); i++ { eiface := iface.Embedded(i).Obj() depPkg := ifacePkg - if eiface.Pkg().Path() != ifacePkg.PkgPath() { + if path := PackagePath(eiface.Pkg().Path()); path != ifacePkg.PkgPath() { // TODO(adonovan): I'm not sure what this is trying to do, but it // looks wrong the in case of type aliases. var err error - depPkg, err = ifacePkg.DirectDep(eiface.Pkg().Path()) + depPkg, err = ifacePkg.DirectDep(path) if err != nil { return nil, err } diff --git a/gopls/internal/lsp/source/util.go b/gopls/internal/lsp/source/util.go index c933fbadd0..3ad17c0a84 100644 --- a/gopls/internal/lsp/source/util.go +++ b/gopls/internal/lsp/source/util.go @@ -314,7 +314,7 @@ func CompareDiagnostic(a, b *Diagnostic) int { // findFileInDeps finds uri in pkg or its dependencies. func findFileInDeps(pkg Package, uri span.URI) (*ParsedGoFile, Package, error) { queue := []Package{pkg} - seen := make(map[string]bool) + seen := make(map[PackageID]bool) for len(queue) > 0 { pkg := queue[0] @@ -333,14 +333,14 @@ func findFileInDeps(pkg Package, uri span.URI) (*ParsedGoFile, Package, error) { return nil, nil, fmt.Errorf("no file for %s in package %s", uri, pkg.ID()) } -// ImportPath returns the unquoted import path of s, +// UnquoteImportPath returns the unquoted import path of s, // or "" if the path is not properly quoted. -func ImportPath(s *ast.ImportSpec) string { - t, err := strconv.Unquote(s.Path.Value) +func UnquoteImportPath(s *ast.ImportSpec) ImportPath { + path, err := strconv.Unquote(s.Path.Value) if err != nil { return "" } - return t + return ImportPath(path) } // NodeContains returns true if a node encloses a given position pos. @@ -532,14 +532,14 @@ func InDirLex(dir, path string) bool { // IsValidImport returns whether importPkgPath is importable // by pkgPath -func IsValidImport(pkgPath, importPkgPath string) bool { +func IsValidImport(pkgPath, importPkgPath PackagePath) bool { i := strings.LastIndex(string(importPkgPath), "/internal/") if i == -1 { return true } // TODO(rfindley): this looks wrong: IsCommandLineArguments is meant to // operate on package IDs, not package paths. - if IsCommandLineArguments(string(pkgPath)) { + if IsCommandLineArguments(PackageID(pkgPath)) { return true } // TODO(rfindley): this is wrong. mod.testx/p should not be able to @@ -551,10 +551,8 @@ func IsValidImport(pkgPath, importPkgPath string) bool { // "command-line-arguments" package, which is a package with an unknown ID // created by the go command. It can have a test variant, which is why callers // should not check that a value equals "command-line-arguments" directly. -// -// TODO(rfindley): this should accept a PackageID. -func IsCommandLineArguments(s string) bool { - return strings.Contains(s, "command-line-arguments") +func IsCommandLineArguments(id PackageID) bool { + return strings.Contains(string(id), "command-line-arguments") } // RecvIdent returns the type identifier of a method receiver. diff --git a/gopls/internal/lsp/source/view.go b/gopls/internal/lsp/source/view.go index 54c942f1d3..be2b7f6a42 100644 --- a/gopls/internal/lsp/source/view.go +++ b/gopls/internal/lsp/source/view.go @@ -84,7 +84,7 @@ type Snapshot interface { DiagnosePackage(ctx context.Context, pkg Package) (map[span.URI][]*Diagnostic, error) // Analyze runs the analyses for the given package at this snapshot. - Analyze(ctx context.Context, pkgID string, analyzers []*Analyzer) ([]*Diagnostic, error) + Analyze(ctx context.Context, pkgID PackageID, analyzers []*Analyzer) ([]*Diagnostic, error) // RunGoCommandPiped runs the given `go` command, writing its output // to stdout and stderr. Verb, Args, and WorkingDir must be specified. @@ -149,12 +149,12 @@ type Snapshot interface { // GetActiveReverseDeps returns the active files belonging to the reverse // dependencies of this file's package, checked in TypecheckWorkspace mode. - GetReverseDependencies(ctx context.Context, id string) ([]Package, error) + GetReverseDependencies(ctx context.Context, id PackageID) ([]Package, error) // CachedImportPaths returns all the imported packages loaded in this // snapshot, indexed by their package path (not import path, despite the name) // and checked in TypecheckWorkspace mode. - CachedImportPaths(ctx context.Context) (map[string]Package, error) + CachedImportPaths(ctx context.Context) (map[PackagePath]Package, error) // KnownPackages returns all the packages loaded in this snapshot, checked // in TypecheckWorkspace mode. @@ -171,7 +171,7 @@ type Snapshot interface { // WorkspacePackageByID returns the workspace package with id, type checked // in 'workspace' mode. - WorkspacePackageByID(ctx context.Context, id string) (Package, error) + WorkspacePackageByID(ctx context.Context, id PackageID) (Package, error) // Symbols returns all symbols in the snapshot. Symbols(ctx context.Context) map[span.URI][]Symbol @@ -338,17 +338,15 @@ type TidiedModule struct { } // Metadata represents package metadata retrieved from go/packages. -// -// TODO(rfindley): move the strongly typed strings from the cache package here. type Metadata interface { // PackageID is the unique package id. - PackageID() string + PackageID() PackageID // PackageName is the package name. - PackageName() string + PackageName() PackageName // PackagePath is the package path. - PackagePath() string + PackagePath() PackagePath // ModuleInfo returns the go/packages module information for the given package. ModuleInfo() *packages.Module @@ -593,12 +591,23 @@ func (a Analyzer) IsEnabled(view View) bool { return a.Enabled } +// 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") +) + // Package represents a Go package that has been type-checked. It maintains // only the relevant fields of a *go/packages.Package. type Package interface { - ID() string // logically a cache.PackageID - Name() string // logically a cache.PackageName - PkgPath() string // logically a cache.PackagePath + ID() PackageID + Name() PackageName + PkgPath() PackagePath CompiledGoFiles() []*ParsedGoFile File(uri span.URI) (*ParsedGoFile, error) GetSyntax() []*ast.File @@ -606,9 +615,9 @@ type Package interface { GetTypesInfo() *types.Info GetTypesSizes() types.Sizes ForTest() string - DirectDep(packagePath string) (Package, error) // logically a cache.PackagePath - ResolveImportPath(importPath string) (Package, error) // logically a cache.ImportPath - MissingDependencies() []string // unordered; logically cache.ImportPaths + DirectDep(path PackagePath) (Package, error) + ResolveImportPath(path ImportPath) (Package, error) + MissingDependencies() []ImportPath // unordered Imports() []Package Version() *module.Version HasListOrParseErrors() bool diff --git a/gopls/internal/lsp/source/workspace_symbol.go b/gopls/internal/lsp/source/workspace_symbol.go index ee4e020e25..dfb5f39dc1 100644 --- a/gopls/internal/lsp/source/workspace_symbol.go +++ b/gopls/internal/lsp/source/workspace_symbol.go @@ -92,7 +92,7 @@ type symbolizer func(space []string, name string, pkg Metadata, m matcherFunc) ( func fullyQualifiedSymbolMatch(space []string, name string, pkg Metadata, matcher matcherFunc) ([]string, float64) { if _, score := dynamicSymbolMatch(space, name, pkg, matcher); score > 0 { - return append(space, pkg.PackagePath(), ".", name), score + return append(space, string(pkg.PackagePath()), ".", name), score } return nil, 0 } @@ -106,12 +106,12 @@ func dynamicSymbolMatch(space []string, name string, pkg Metadata, matcher match var score float64 - endsInPkgName := strings.HasSuffix(pkg.PackagePath(), pkg.PackageName()) + endsInPkgName := strings.HasSuffix(string(pkg.PackagePath()), string(pkg.PackageName())) // 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, pkg.PackageName(), ".", name) + pkgQualified := append(space, string(pkg.PackageName()), ".", name) idx, score := matcher(pkgQualified) nameStart := len(pkg.PackageName()) + 1 if score > 0 { @@ -126,7 +126,7 @@ func dynamicSymbolMatch(space []string, name string, pkg Metadata, matcher match } // Now try matching the fully qualified symbol. - fullyQualified := append(space, pkg.PackagePath(), ".", name) + fullyQualified := append(space, string(pkg.PackagePath()), ".", name) idx, score := matcher(fullyQualified) // As above, check if we matched just the unqualified symbol name. @@ -141,7 +141,7 @@ func dynamicSymbolMatch(space []string, name string, pkg Metadata, matcher match if endsInPkgName && idx >= 0 { pkgStart := len(pkg.PackagePath()) - len(pkg.PackageName()) if idx >= pkgStart { - return append(space, pkg.PackageName(), ".", name), score + return append(space, string(pkg.PackageName()), ".", name), score } } @@ -151,7 +151,7 @@ func dynamicSymbolMatch(space []string, name string, pkg Metadata, matcher match } func packageSymbolMatch(space []string, name string, pkg Metadata, matcher matcherFunc) ([]string, float64) { - qualified := append(space, pkg.PackageName(), ".", name) + qualified := append(space, string(pkg.PackageName()), ".", name) if _, s := matcher(qualified); s > 0 { return qualified, s } @@ -526,7 +526,7 @@ func matchFile(store *symbolStore, symbolizer symbolizer, matcher matcherFunc, r kind: sym.Kind, uri: i.uri, rng: sym.Range, - container: i.md.PackagePath(), + container: string(i.md.PackagePath()), } store.store(si) }