gopls/internal/lsp: eliminate more unnecessary typechecking
This change downgrades type-checking operations to mere
metadata queries or calls to the parser in these places:
- cmd toggle-gc-details
- cmd list-imports
- references
- rename (3 places)
The parsePackageNameDecl function (formerly isInPackageName)
only parses the package and imports declarations.
Also, eliminate the last two uses of Snapshot.WorkspacePackageByID
and the method itself.
Change-Id: I2bd24c24c9a2c14c9ba15e9624222b4aaf231c79
Reviewed-on: https://go-review.googlesource.com/c/tools/+/456040
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go
index 8b02dea..52c5a26 100644
--- a/gopls/internal/lsp/cache/snapshot.go
+++ b/gopls/internal/lsp/cache/snapshot.go
@@ -1197,10 +1197,6 @@
return meta, nil
}
-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[PackagePath]source.Package, error) {
// Don't reload workspace package metadata.
// This function is meant to only return currently cached information.
diff --git a/gopls/internal/lsp/command.go b/gopls/internal/lsp/command.go
index 747adc8..4bbf0a9 100644
--- a/gopls/internal/lsp/command.go
+++ b/gopls/internal/lsp/command.go
@@ -691,17 +691,17 @@
progress: "Toggling GC Details",
forURI: args.URI,
}, func(ctx context.Context, deps commandDeps) error {
- // TODO(adonovan): opt: avoid loading type-checked package; only Metadata is needed.
- pkg, err := deps.snapshot.PackageForFile(ctx, deps.fh.URI(), source.TypecheckWorkspace, source.NarrowestPackage)
+ metas, err := deps.snapshot.MetadataForFile(ctx, deps.fh.URI())
if err != nil {
return err
}
+ id := metas[0].ID // 0 => narrowest package
c.s.gcOptimizationDetailsMu.Lock()
- if _, ok := c.s.gcOptimizationDetails[pkg.ID()]; ok {
- delete(c.s.gcOptimizationDetails, pkg.ID())
+ if _, ok := c.s.gcOptimizationDetails[id]; ok {
+ delete(c.s.gcOptimizationDetails, id)
c.s.clearDiagnosticSource(gcDetailsSource)
} else {
- c.s.gcOptimizationDetails[pkg.ID()] = struct{}{}
+ c.s.gcOptimizationDetails[id] = struct{}{}
}
c.s.gcOptimizationDetailsMu.Unlock()
c.s.diagnoseSnapshot(deps.snapshot, nil, false)
@@ -758,16 +758,15 @@
err := c.run(ctx, commandConfig{
forURI: args.URI,
}, func(ctx context.Context, deps commandDeps) error {
- // TODO(adonovan): opt: avoid loading type-checked package; only Metadata is needed.
- pkg, err := deps.snapshot.PackageForFile(ctx, args.URI.SpanURI(), source.TypecheckWorkspace, source.NarrowestPackage)
+ fh, err := deps.snapshot.GetFile(ctx, args.URI.SpanURI())
if err != nil {
return err
}
- pgf, err := pkg.File(args.URI.SpanURI())
+ pgf, err := deps.snapshot.ParseGo(ctx, fh, source.ParseHeader)
if err != nil {
return err
}
- for _, group := range astutil.Imports(pkg.FileSet(), pgf.File) {
+ for _, group := range astutil.Imports(deps.snapshot.FileSet(), pgf.File) {
for _, imp := range group {
if imp.Path == nil {
continue
@@ -782,10 +781,16 @@
})
}
}
- for _, imp := range pkg.Imports() {
- result.PackageImports = append(result.PackageImports, command.PackageImport{
- Path: string(imp.PkgPath()), // This might be the vendored path under GOPATH vendoring, in which case it's a bug.
- })
+ metas, err := deps.snapshot.MetadataForFile(ctx, args.URI.SpanURI())
+ if err != nil {
+ return err // e.g. cancelled
+ }
+ if len(metas) == 0 {
+ return fmt.Errorf("no package containing %v", args.URI.SpanURI())
+ }
+ for pkgPath := range metas[0].DepsByPkgPath { // 0 => narrowest package
+ result.PackageImports = append(result.PackageImports,
+ command.PackageImport{Path: string(pkgPath)})
}
sort.Slice(result.PackageImports, func(i, j int) bool {
return result.PackageImports[i].Path < result.PackageImports[j].Path
diff --git a/gopls/internal/lsp/source/references.go b/gopls/internal/lsp/source/references.go
index bf3784b..49b8294 100644
--- a/gopls/internal/lsp/source/references.go
+++ b/gopls/internal/lsp/source/references.go
@@ -9,12 +9,10 @@
"errors"
"fmt"
"go/ast"
- "strings"
-
"go/token"
"go/types"
"sort"
- "strconv"
+ "strings"
"golang.org/x/tools/gopls/internal/lsp/protocol"
"golang.org/x/tools/gopls/internal/span"
@@ -32,36 +30,17 @@
isDeclaration bool
}
-// isInPackageName reports whether the file's package name surrounds the
-// given position pp (e.g. "foo" surrounds the cursor in "package foo").
-func isInPackageName(ctx context.Context, s Snapshot, f FileHandle, pgf *ParsedGoFile, pp protocol.Position) (bool, error) {
- // Find position of the package name declaration
- cursorPos, err := pgf.Mapper.Pos(pp)
- if err != nil {
- return false, err
- }
-
- return pgf.File.Name.Pos() <= cursorPos && cursorPos <= pgf.File.Name.End(), nil
-}
-
// References returns a list of references for a given identifier within the packages
// containing i.File. Declarations appear first in the result.
func References(ctx context.Context, s Snapshot, f FileHandle, pp protocol.Position, includeDeclaration bool) ([]*ReferenceInfo, error) {
ctx, done := event.Start(ctx, "source.References")
defer done()
- // Find position of the package name declaration
- pgf, err := s.ParseGo(ctx, f, ParseFull)
+ // Is the cursor within the package name declaration?
+ pgf, inPackageName, err := parsePackageNameDecl(ctx, s, f, pp)
if err != nil {
return nil, err
}
-
- packageName := pgf.File.Name.Name // from package decl
- inPackageName, err := isInPackageName(ctx, s, f, pgf, pp)
- if err != nil {
- return nil, err
- }
-
if inPackageName {
// TODO(rfindley): this is inaccurate, excluding test variants, and
// redundant with package renaming. Refactor to share logic.
@@ -79,13 +58,12 @@
for _, dep := range rdeps {
for _, f := range dep.CompiledGoFiles() {
for _, imp := range f.File.Imports {
- // TODO(adonovan): using UnquoteImport() here would
- // reveal that there's an ImportPath==PackagePath
+ // TODO(adonovan): there's an ImportPath==PackagePath
// comparison that doesn't account for vendoring.
// Use dep.Metadata().DepsByPkgPath instead.
- if path, err := strconv.Unquote(imp.Path.Value); err == nil && path == string(renamingPkg.PkgPath()) {
+ if string(UnquoteImportPath(imp)) == string(renamingPkg.PkgPath()) {
refs = append(refs, &ReferenceInfo{
- Name: packageName,
+ Name: pgf.File.Name.Name,
MappedRange: NewMappedRange(f.Tok, f.Mapper, imp.Pos(), imp.End()),
})
}
@@ -96,7 +74,7 @@
// Find internal references to the package within the package itself
for _, f := range renamingPkg.CompiledGoFiles() {
refs = append(refs, &ReferenceInfo{
- Name: packageName,
+ Name: pgf.File.Name.Name,
MappedRange: NewMappedRange(f.Tok, f.Mapper, f.File.Name.Pos(), f.File.Name.End()),
})
}
@@ -132,6 +110,20 @@
return refs, nil
}
+// parsePackageNameDecl is a convenience function that parses and
+// returns the package name declaration of file fh, and reports
+// whether the position ppos lies within it.
+func parsePackageNameDecl(ctx context.Context, snapshot Snapshot, fh FileHandle, ppos protocol.Position) (*ParsedGoFile, bool, error) {
+ pgf, err := snapshot.ParseGo(ctx, fh, ParseHeader)
+ if err != nil {
+ return nil, false, err
+ }
+ // Careful: because we used ParseHeader,
+ // Mapper.Pos(ppos) may be beyond EOF => (0, err).
+ pos, _ := pgf.Mapper.Pos(ppos)
+ return pgf, pgf.File.Name.Pos() <= pos && pos <= pgf.File.Name.End(), nil
+}
+
// references is a helper function to avoid recomputing qualifiedObjsAtProtocolPos.
// The first element of qos is considered to be the declaration;
// if isDeclaration, the first result is an extra item for it.
diff --git a/gopls/internal/lsp/source/rename.go b/gopls/internal/lsp/source/rename.go
index 51555b0..6855e20 100644
--- a/gopls/internal/lsp/source/rename.go
+++ b/gopls/internal/lsp/source/rename.go
@@ -53,15 +53,12 @@
// Find position of the package name declaration.
ctx, done := event.Start(ctx, "source.PrepareRename")
defer done()
- pgf, err := snapshot.ParseGo(ctx, f, ParseFull)
- if err != nil {
- return nil, err, err
- }
- inPackageName, err := isInPackageName(ctx, snapshot, f, pgf, pp)
- if err != nil {
- return nil, err, err
- }
+ // Is the cursor within the package name declaration?
+ pgf, inPackageName, err := parsePackageNameDecl(ctx, snapshot, f, pp)
+ if err != nil {
+ return nil, err, err
+ }
if inPackageName {
fileRenameSupported := false
for _, op := range snapshot.View().Options().SupportedResourceOperations {
@@ -106,17 +103,16 @@
err := fmt.Errorf("can't rename package: package path %q is the same as module path %q", meta.PkgPath, meta.Module.Path)
return nil, err, err
}
- // TODO(rfindley): we should not need the package here.
- pkg, err := snapshot.WorkspacePackageByID(ctx, meta.ID)
+
+ // Return the location of the package declaration.
+ rng, err := pgf.Mapper.PosRange(pgf.File.Name.Pos(), pgf.File.Name.End())
if err != nil {
- err = fmt.Errorf("error building package to rename: %v", err)
return nil, err, err
}
- result, err := computePrepareRenameResp(snapshot, pkg, pgf.File.Name, string(pkg.Name()))
- if err != nil {
- return nil, nil, err
- }
- return result, nil, nil
+ return &PrepareItem{
+ Range: rng,
+ Text: string(meta.Name),
+ }, nil, nil
}
qos, err := qualifiedObjsAtProtocolPos(ctx, snapshot, f.URI(), pp)
@@ -171,15 +167,11 @@
ctx, done := event.Start(ctx, "source.Rename")
defer done()
- pgf, err := s.ParseGo(ctx, f, ParseFull)
+ // Is the cursor within the package name declaration?
+ _, inPackageName, err := parsePackageNameDecl(ctx, s, f, pp)
if err != nil {
return nil, false, err
}
- inPackageName, err := isInPackageName(ctx, s, f, pgf, pp)
- if err != nil {
- return nil, false, err
- }
-
if inPackageName {
if !isValidIdentifier(newName) {
return nil, true, fmt.Errorf("%q is not a valid identifier", newName)
@@ -336,23 +328,24 @@
// package clause has already been updated, to prevent duplicate edits.
//
// Edits are written into the edits map.
-func renamePackageClause(ctx context.Context, m *Metadata, s Snapshot, newName PackageName, seen seenPackageRename, edits map[span.URI][]protocol.TextEdit) error {
- pkg, err := s.WorkspacePackageByID(ctx, m.ID)
- if err != nil {
- return err
- }
-
+func renamePackageClause(ctx context.Context, m *Metadata, snapshot Snapshot, newName PackageName, seen seenPackageRename, edits map[span.URI][]protocol.TextEdit) error {
// Rename internal references to the package in the renaming package.
- for _, f := range pkg.CompiledGoFiles() {
- if seen.add(f.URI, m.PkgPath) {
+ for _, uri := range m.CompiledGoFiles {
+ if seen.add(uri, m.PkgPath) {
continue
}
-
+ fh, err := snapshot.GetFile(ctx, uri)
+ if err != nil {
+ return err
+ }
+ f, err := snapshot.ParseGo(ctx, fh, ParseHeader)
+ if err != nil {
+ return err
+ }
if f.File.Name == nil {
- continue
+ continue // no package declaration
}
- pkgNameMappedRange := NewMappedRange(f.Tok, f.Mapper, f.File.Name.Pos(), f.File.Name.End())
- rng, err := pkgNameMappedRange.Range()
+ rng, err := f.Mapper.PosRange(f.File.Name.Pos(), f.File.Name.End())
if err != nil {
return err
}
diff --git a/gopls/internal/lsp/source/view.go b/gopls/internal/lsp/source/view.go
index 1084c74..1168370 100644
--- a/gopls/internal/lsp/source/view.go
+++ b/gopls/internal/lsp/source/view.go
@@ -104,6 +104,7 @@
// ParseGo returns the parsed AST for the file.
// If the file is not available, returns nil and an error.
+ // Position information is added to FileSet().
ParseGo(ctx context.Context, fh FileHandle, mode ParseMode) (*ParsedGoFile, error)
// DiagnosePackage returns basic diagnostics, including list, parse, and type errors
@@ -206,10 +207,6 @@
// AllValidMetadata returns all valid metadata loaded for the snapshot.
AllValidMetadata(ctx context.Context) ([]*Metadata, error)
- // WorkspacePackageByID returns the workspace package with id, type checked
- // in 'workspace' mode.
- WorkspacePackageByID(ctx context.Context, id PackageID) (Package, error)
-
// Symbols returns all symbols in the snapshot.
Symbols(ctx context.Context) map[span.URI][]Symbol
