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) }