diff --git a/gopls/internal/regtest/bench/bench_test.go b/gopls/internal/regtest/bench/bench_test.go index 44ab457095..f37c2f6da2 100644 --- a/gopls/internal/regtest/bench/bench_test.go +++ b/gopls/internal/regtest/bench/bench_test.go @@ -12,6 +12,7 @@ import ( "testing" "golang.org/x/tools/gopls/internal/hooks" + "golang.org/x/tools/internal/lsp/bug" "golang.org/x/tools/internal/lsp/fake" . "golang.org/x/tools/internal/lsp/regtest" @@ -19,6 +20,7 @@ import ( ) func TestMain(m *testing.M) { + bug.PanicOnBugs = true Main(m, hooks.Options) } diff --git a/gopls/internal/regtest/codelens/codelens_test.go b/gopls/internal/regtest/codelens/codelens_test.go index 3e15271147..a64f9c480a 100644 --- a/gopls/internal/regtest/codelens/codelens_test.go +++ b/gopls/internal/regtest/codelens/codelens_test.go @@ -11,6 +11,7 @@ import ( "testing" "golang.org/x/tools/gopls/internal/hooks" + "golang.org/x/tools/internal/lsp/bug" . "golang.org/x/tools/internal/lsp/regtest" "golang.org/x/tools/internal/lsp/command" @@ -21,6 +22,7 @@ import ( ) func TestMain(m *testing.M) { + bug.PanicOnBugs = true Main(m, hooks.Options) } diff --git a/gopls/internal/regtest/completion/completion_test.go b/gopls/internal/regtest/completion/completion_test.go index c0b4736e76..1ffb0000d3 100644 --- a/gopls/internal/regtest/completion/completion_test.go +++ b/gopls/internal/regtest/completion/completion_test.go @@ -10,6 +10,7 @@ import ( "testing" "golang.org/x/tools/gopls/internal/hooks" + "golang.org/x/tools/internal/lsp/bug" . "golang.org/x/tools/internal/lsp/regtest" "golang.org/x/tools/internal/lsp/fake" @@ -18,6 +19,7 @@ import ( ) func TestMain(m *testing.M) { + bug.PanicOnBugs = true Main(m, hooks.Options) } diff --git a/gopls/internal/regtest/diagnostics/diagnostics_test.go b/gopls/internal/regtest/diagnostics/diagnostics_test.go index a2707efdb5..6ce0cdbc74 100644 --- a/gopls/internal/regtest/diagnostics/diagnostics_test.go +++ b/gopls/internal/regtest/diagnostics/diagnostics_test.go @@ -11,6 +11,7 @@ import ( "testing" "golang.org/x/tools/gopls/internal/hooks" + "golang.org/x/tools/internal/lsp/bug" . "golang.org/x/tools/internal/lsp/regtest" "golang.org/x/tools/internal/lsp" @@ -20,6 +21,7 @@ import ( ) func TestMain(m *testing.M) { + bug.PanicOnBugs = true Main(m, hooks.Options) } diff --git a/gopls/internal/regtest/misc/misc_test.go b/gopls/internal/regtest/misc/misc_test.go index 3694b07fc1..c553bdb378 100644 --- a/gopls/internal/regtest/misc/misc_test.go +++ b/gopls/internal/regtest/misc/misc_test.go @@ -8,9 +8,11 @@ import ( "testing" "golang.org/x/tools/gopls/internal/hooks" + "golang.org/x/tools/internal/lsp/bug" "golang.org/x/tools/internal/lsp/regtest" ) func TestMain(m *testing.M) { + bug.PanicOnBugs = true regtest.Main(m, hooks.Options) } diff --git a/gopls/internal/regtest/modfile/modfile_test.go b/gopls/internal/regtest/modfile/modfile_test.go index cca3b3cba6..93d4325304 100644 --- a/gopls/internal/regtest/modfile/modfile_test.go +++ b/gopls/internal/regtest/modfile/modfile_test.go @@ -11,6 +11,7 @@ import ( "testing" "golang.org/x/tools/gopls/internal/hooks" + "golang.org/x/tools/internal/lsp/bug" . "golang.org/x/tools/internal/lsp/regtest" "golang.org/x/tools/internal/lsp/protocol" @@ -19,6 +20,7 @@ import ( ) func TestMain(m *testing.M) { + bug.PanicOnBugs = true Main(m, hooks.Options) } diff --git a/gopls/internal/regtest/template/template_test.go b/gopls/internal/regtest/template/template_test.go index b0acdfeb85..9489e9bf7f 100644 --- a/gopls/internal/regtest/template/template_test.go +++ b/gopls/internal/regtest/template/template_test.go @@ -9,11 +9,13 @@ import ( "testing" "golang.org/x/tools/gopls/internal/hooks" + "golang.org/x/tools/internal/lsp/bug" "golang.org/x/tools/internal/lsp/protocol" . "golang.org/x/tools/internal/lsp/regtest" ) func TestMain(m *testing.M) { + bug.PanicOnBugs = true Main(m, hooks.Options) } diff --git a/gopls/internal/regtest/watch/watch_test.go b/gopls/internal/regtest/watch/watch_test.go index 5b432e18a7..e66d08ab12 100644 --- a/gopls/internal/regtest/watch/watch_test.go +++ b/gopls/internal/regtest/watch/watch_test.go @@ -8,6 +8,7 @@ import ( "testing" "golang.org/x/tools/gopls/internal/hooks" + "golang.org/x/tools/internal/lsp/bug" . "golang.org/x/tools/internal/lsp/regtest" "golang.org/x/tools/internal/lsp/fake" @@ -16,6 +17,7 @@ import ( ) func TestMain(m *testing.M) { + bug.PanicOnBugs = true Main(m, hooks.Options) } diff --git a/gopls/internal/regtest/workspace/metadata_test.go b/gopls/internal/regtest/workspace/metadata_test.go new file mode 100644 index 0000000000..28291a2e23 --- /dev/null +++ b/gopls/internal/regtest/workspace/metadata_test.go @@ -0,0 +1,43 @@ +// Copyright 2022 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package workspace + +import ( + "testing" + + . "golang.org/x/tools/internal/lsp/regtest" + "golang.org/x/tools/internal/testenv" +) + +// TODO(rfindley): move workspace tests related to metadata bugs into this +// file. + +func TestFixImportDecl(t *testing.T) { + // It appears that older Go versions don't even see p.go from the initial + // workspace load. + testenv.NeedsGo1Point(t, 15) + const src = ` +-- go.mod -- +module mod.test + +go 1.12 +-- p.go -- +package p + +import ( + _ "fmt" + +const C = 42 +` + + Run(t, src, func(t *testing.T, env *Env) { + env.OpenFile("p.go") + env.RegexpReplace("p.go", "\"fmt\"", "\"fmt\"\n)") + env.Await(OnceMet( + env.DoneWithChange(), + EmptyDiagnostics("p.go"), + )) + }) +} diff --git a/gopls/internal/regtest/workspace/workspace_test.go b/gopls/internal/regtest/workspace/workspace_test.go index 9098bd4c5a..b283cfa727 100644 --- a/gopls/internal/regtest/workspace/workspace_test.go +++ b/gopls/internal/regtest/workspace/workspace_test.go @@ -12,15 +12,17 @@ import ( "testing" "golang.org/x/tools/gopls/internal/hooks" - . "golang.org/x/tools/internal/lsp/regtest" - "golang.org/x/tools/internal/lsp/source" - + "golang.org/x/tools/internal/lsp/bug" "golang.org/x/tools/internal/lsp/fake" "golang.org/x/tools/internal/lsp/protocol" + "golang.org/x/tools/internal/lsp/source" "golang.org/x/tools/internal/testenv" + + . "golang.org/x/tools/internal/lsp/regtest" ) func TestMain(m *testing.M) { + bug.PanicOnBugs = true Main(m, hooks.Options) } diff --git a/internal/lsp/bug/bug.go b/internal/lsp/bug/bug.go index 147b8e818c..a419a9c3a1 100644 --- a/internal/lsp/bug/bug.go +++ b/internal/lsp/bug/bug.go @@ -19,6 +19,11 @@ import ( "sync" ) +// PanicOnBugs controls whether to panic when bugs are reported. +// +// It may be set to true during testing. +var PanicOnBugs = false + var ( mu sync.Mutex exemplars map[string]Bug @@ -39,6 +44,11 @@ type Bug struct { // Data is additional metadata to record for a bug. type Data map[string]interface{} +// Reportf reports a formatted bug message. +func Reportf(format string, args ...interface{}) { + Report(fmt.Sprintf(format, args...), nil) +} + // Report records a new bug encountered on the server. // It uses reflection to report the position of the immediate caller. func Report(description string, data Data) { @@ -49,6 +59,10 @@ func Report(description string, data Data) { key = fmt.Sprintf("%s:%d", file, line) } + if PanicOnBugs { + panic(fmt.Sprintf("%s: %s", key, description)) + } + bug := Bug{ File: file, Line: line, diff --git a/internal/lsp/cache/check.go b/internal/lsp/cache/check.go index fdc49087ae..b8a3655a9d 100644 --- a/internal/lsp/cache/check.go +++ b/internal/lsp/cache/check.go @@ -570,8 +570,7 @@ func parseCompiledGoFiles(ctx context.Context, snapshot *snapshot, mode source.P // Only parse Full through the cache -- we need to own Exported ASTs // to prune them. if mode == source.ParseFull { - pgh := snapshot.parseGoHandle(ctx, fh, mode) - pgf, fixed, err = snapshot.parseGo(ctx, pgh) + pgf, fixed, err = snapshot.parseGo(ctx, fh, mode) } else { d := parseGo(ctx, snapshot.FileSet(), fh, mode) pgf, fixed, err = d.parsed, d.fixed, d.err diff --git a/internal/lsp/cache/parse.go b/internal/lsp/cache/parse.go index 5d751ebe5a..1ab1fa9e52 100644 --- a/internal/lsp/cache/parse.go +++ b/internal/lsp/cache/parse.go @@ -72,27 +72,19 @@ func (s *snapshot) parseGoHandle(ctx context.Context, fh source.FileHandle, mode } func (pgh *parseGoHandle) String() string { - return pgh.File().URI().Filename() -} - -func (pgh *parseGoHandle) File() source.FileHandle { - return pgh.file -} - -func (pgh *parseGoHandle) Mode() source.ParseMode { - return pgh.mode + return pgh.file.URI().Filename() } func (s *snapshot) ParseGo(ctx context.Context, fh source.FileHandle, mode source.ParseMode) (*source.ParsedGoFile, error) { - pgh := s.parseGoHandle(ctx, fh, mode) - pgf, _, err := s.parseGo(ctx, pgh) + pgf, _, err := s.parseGo(ctx, fh, mode) return pgf, err } -func (s *snapshot) parseGo(ctx context.Context, pgh *parseGoHandle) (*source.ParsedGoFile, bool, error) { - if pgh.mode == source.ParseExported { +func (s *snapshot) parseGo(ctx context.Context, fh source.FileHandle, mode source.ParseMode) (*source.ParsedGoFile, bool, error) { + if mode == source.ParseExported { panic("only type checking should use Exported") } + pgh := s.parseGoHandle(ctx, fh, mode) d, err := pgh.handle.Get(ctx, s.generation, s) if err != nil { return nil, false, err @@ -101,6 +93,22 @@ func (s *snapshot) parseGo(ctx context.Context, pgh *parseGoHandle) (*source.Par return data.parsed, data.fixed, data.err } +// cachedPGF returns the cached ParsedGoFile for the given ParseMode, if it +// has already been computed. Otherwise, it returns nil. +func (s *snapshot) cachedPGF(fh source.FileHandle, mode source.ParseMode) *source.ParsedGoFile { + key := parseKey{file: fh.FileIdentity(), mode: mode} + if pgh := s.getGoFile(key); pgh != nil { + cached := pgh.handle.Cached(s.generation) + if cached != nil { + cached := cached.(*parseGoData) + if cached.parsed != nil { + return cached.parsed + } + } + } + return nil +} + type astCacheKey struct { pkg packageHandleKey uri span.URI diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index 5cc534aa79..9beb9e6579 100644 --- a/internal/lsp/cache/snapshot.go +++ b/internal/lsp/cache/snapshot.go @@ -29,6 +29,7 @@ import ( "golang.org/x/tools/go/packages" "golang.org/x/tools/internal/event" "golang.org/x/tools/internal/gocommand" + "golang.org/x/tools/internal/lsp/bug" "golang.org/x/tools/internal/lsp/debug/log" "golang.org/x/tools/internal/lsp/debug/tag" "golang.org/x/tools/internal/lsp/source" @@ -1817,7 +1818,7 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC } } - changedPkgNames := map[PackageID]struct{}{} + changedPkgFiles := map[PackageID]struct{}{} // packages whose file set may have changed anyImportDeleted := false for uri, change := range changes { // Maybe reinitialize the view if we see a change in the vendor @@ -1829,23 +1830,26 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC // The original FileHandle for this URI is cached on the snapshot. originalFH := s.files[uri] - // Check if the file's package name or imports have changed, - // and if so, invalidate this file's packages' metadata. - var shouldInvalidateMetadata, pkgNameChanged, importDeleted bool - if !isGoMod(uri) { - shouldInvalidateMetadata, pkgNameChanged, importDeleted = checkForMetadataChanges(ctx, originalFH, change.fileHandle) + // If uri is a Go file, check if it has changed in a way that would + // invalidate metadata. Note that we can't use s.view.FileKind here, + // because the file type that matters is not what the *client* tells us, + // but what the Go command sees. + var invalidateMetadata, pkgFileChanged, importDeleted bool + if strings.HasSuffix(uri.Filename(), ".go") { + invalidateMetadata, pkgFileChanged, importDeleted = metadataChanges(ctx, s, originalFH, change.fileHandle) } - invalidateMetadata := forceReloadMetadata || workspaceReload || shouldInvalidateMetadata + + invalidateMetadata = invalidateMetadata || forceReloadMetadata || workspaceReload anyImportDeleted = anyImportDeleted || importDeleted // Mark all of the package IDs containing the given file. - filePackageIDs := invalidatedPackageIDs(uri, s.ids, originalFH == nil || pkgNameChanged) - if pkgNameChanged { - for _, id := range filePackageIDs { - changedPkgNames[id] = struct{}{} + filePackageIDs := invalidatedPackageIDs(uri, s.ids, pkgFileChanged) + if pkgFileChanged { + for id := range filePackageIDs { + changedPkgFiles[id] = struct{}{} } } - for _, id := range filePackageIDs { + for id := range filePackageIDs { directIDs[id] = directIDs[id] || invalidateMetadata } @@ -2050,7 +2054,7 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC // possible that the package ID may no longer exist. Delete it from // the set of workspace packages, on the assumption that we will add it // back when the relevant files are reloaded. - if _, ok := changedPkgNames[id]; ok { + if _, ok := changedPkgFiles[id]; ok { continue } @@ -2093,20 +2097,25 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC // directory. This is of course incorrect in build systems where packages are // not organized by directory. // -// If newPackageFile is set, the file is either a new file, or has a new +// If packageFileChanged is set, the file is either a new file, or has a new // package name. In this case, all known packages in the directory will be // invalidated. -func invalidatedPackageIDs(uri span.URI, known map[span.URI][]PackageID, newPackageFile bool) []PackageID { +func invalidatedPackageIDs(uri span.URI, known map[span.URI][]PackageID, packageFileChanged bool) map[PackageID]struct{} { + invalidated := make(map[PackageID]struct{}) + + // At a minimum, we invalidate packages known to contain uri. + for _, id := range known[uri] { + invalidated[id] = struct{}{} + } + // If the file didn't move to a new package, we should only invalidate the // packages it is currently contained inside. - if !newPackageFile { - if packages := known[uri]; len(packages) > 0 { - // We've seen this file before. - return packages - } + if !packageFileChanged && len(invalidated) > 0 { + return invalidated } - // This is a file we don't yet know about. Guess relevant packages by - // considering files in the same directory. + + // This is a file we don't yet know about, or which has moved packages. Guess + // relevant packages by considering files in the same directory. // Cache of FileInfo to avoid unnecessary stats for multiple files in the // same directory. @@ -2127,23 +2136,22 @@ func invalidatedPackageIDs(uri span.URI, known map[span.URI][]PackageID, newPack } dir := filepath.Dir(uri.Filename()) fi, err := getInfo(dir) - if err != nil { - return nil - } - - // Aggregate all possibly relevant package IDs. - var found []PackageID - for knownURI, ids := range known { - knownDir := filepath.Dir(knownURI.Filename()) - knownFI, err := getInfo(knownDir) - if err != nil { - continue - } - if os.SameFile(fi, knownFI) { - found = append(found, ids...) + if err == nil { + // Aggregate all possibly relevant package IDs. + for knownURI, ids := range known { + knownDir := filepath.Dir(knownURI.Filename()) + knownFI, err := getInfo(knownDir) + if err != nil { + continue + } + if os.SameFile(fi, knownFI) { + for _, id := range ids { + invalidated[id] = struct{}{} + } + } } } - return found + return invalidated } // fileWasSaved reports whether the FileHandle passed in has been saved. It @@ -2162,66 +2170,117 @@ func fileWasSaved(originalFH, currentFH source.FileHandle) bool { return !o.saved && c.saved } -// checkForMetadataChanges reparses the full file's AST to determine -// if the file requires a metadata reload. -func checkForMetadataChanges(ctx context.Context, originalFH, currentFH source.FileHandle) (invalidate, pkgNameChanged, importDeleted bool) { - if originalFH == nil { - return true, false, false +// metadataChanges detects features of the change from oldFH->newFH that may +// affect package metadata. +// +// It uses lockedSnapshot to access cached parse information. lockedSnapshot +// must be locked. +// +// The result parameters have the following meaning: +// - invalidate means that package metadata for packages containing the file +// should be invalidated. +// - pkgFileChanged means that the file->package associates for the file have +// changed (possibly because the file is new, or because its package name has +// changed). +// - importDeleted means that an import has been deleted, or we can't +// determine if an import was deleted due to errors. +func metadataChanges(ctx context.Context, lockedSnapshot *snapshot, oldFH, newFH source.FileHandle) (invalidate, pkgFileChanged, importDeleted bool) { + if oldFH == nil || newFH == nil { // existential changes + changed := (oldFH == nil) != (newFH == nil) + return changed, changed, (newFH == nil) // we don't know if an import was deleted } + // If the file hasn't changed, there's no need to reload. - if originalFH.FileIdentity() == currentFH.FileIdentity() { + if oldFH.FileIdentity() == newFH.FileIdentity() { return false, false, false } - // Get the original and current parsed files in order to check package name - // and imports. Don't parse through the cache as we don't know if we want to - // own the resulting file handle. - fset := token.NewFileSet() // We don't need positions, so can use a new file set. - original := parseGo(ctx, fset, originalFH, source.ParseFull) - current := parseGo(ctx, fset, currentFH, source.ParseFull) - if original.err != nil || current.err != nil { - return (original.err == nil) != (current.err == nil), false, (current.err != nil) // we don't know if an import was deleted - } - // Check if the package's metadata has changed. The cases handled are: - // 1. A package's name has changed - // 2. A file's imports have changed - if original.parsed.File.Name.Name != current.parsed.File.Name.Name { - invalidate = true - pkgNameChanged = true - } - origImportSet := make(map[string]struct{}) - for _, importSpec := range original.parsed.File.Imports { - origImportSet[importSpec.Path.Value] = struct{}{} - } - curImportSet := make(map[string]struct{}) - for _, importSpec := range current.parsed.File.Imports { - curImportSet[importSpec.Path.Value] = struct{}{} - } - // If any of the current imports were not in the original imports. - for path := range curImportSet { - if _, ok := origImportSet[path]; ok { - delete(origImportSet, path) - continue - } - // If the import path is obviously not valid, we can skip reloading - // metadata. For now, valid means properly quoted and without a - // terminal slash. - if isBadImportPath(path) { - continue - } - invalidate = true + + // Parse headers to compare package names and imports. + oldHead, oldErr := peekOrParse(ctx, lockedSnapshot, oldFH, source.ParseHeader) + newHead, newErr := peekOrParse(ctx, lockedSnapshot, newFH, source.ParseHeader) + + if oldErr != nil || newErr != nil { + // TODO(rfindley): we can get here if newFH does not exists. There is + // asymmetry here, in that newFH may be non-nil even if the underlying file + // does not exist. + // + // We should not produce a non-nil filehandle for a file that does not exist. + errChanged := (oldErr == nil) != (newErr == nil) + return errChanged, errChanged, (newErr != nil) // we don't know if an import was deleted } - for path := range origImportSet { - if !isBadImportPath(path) { - invalidate = true - importDeleted = true + // `go list` fails completely if the file header cannot be parsed. If we go + // from a non-parsing state to a parsing state, we should reload. + if oldHead.ParseErr != nil && newHead.ParseErr == nil { + return true, true, true // We don't know what changed, so fall back on full invalidation. + } + + // If a package name has changed, the set of package imports may have changed + // in ways we can't detect here. Assume an import has been deleted. + if oldHead.File.Name.Name != newHead.File.Name.Name { + return true, true, true + } + + // Check whether package imports have changed. Only consider potentially + // valid imports paths. + oldImports := validImports(oldHead.File.Imports) + newImports := validImports(newHead.File.Imports) + + for path := range newImports { + if _, ok := oldImports[path]; ok { + delete(oldImports, path) + } else { + invalidate = true // a new, potentially valid import was added } } + if len(oldImports) > 0 { + invalidate = true + importDeleted = true + } + + // If the change does not otherwise invalidate metadata, get the full ASTs in + // order to check magic comments. + // + // Note: if this affects performance we can probably avoid parsing in the + // common case by first scanning the source for potential comments. if !invalidate { - invalidate = magicCommentsChanged(original.parsed.File, current.parsed.File) + origFull, oldErr := peekOrParse(ctx, lockedSnapshot, oldFH, source.ParseFull) + currFull, newErr := peekOrParse(ctx, lockedSnapshot, newFH, source.ParseFull) + if oldErr == nil && newErr == nil { + invalidate = magicCommentsChanged(origFull.File, currFull.File) + } else { + // At this point, we shouldn't ever fail to produce a ParsedGoFile, as + // we're already past header parsing. + bug.Reportf("metadataChanges: unparseable file %v (old error: %v, new error: %v)", oldFH.URI(), oldErr, newErr) + } } - return invalidate, pkgNameChanged, importDeleted + + return invalidate, pkgFileChanged, importDeleted +} + +// peekOrParse returns the cached ParsedGoFile if it exists, otherwise parses +// without caching. +// +// It returns an error if the file could not be read (note that parsing errors +// are stored in ParsedGoFile.ParseErr). +// +// lockedSnapshot must be locked. +func peekOrParse(ctx context.Context, lockedSnapshot *snapshot, fh source.FileHandle, mode source.ParseMode) (*source.ParsedGoFile, error) { + key := parseKey{file: fh.FileIdentity(), mode: mode} + if pgh := lockedSnapshot.goFiles[key]; pgh != nil { + cached := pgh.handle.Cached(lockedSnapshot.generation) + if cached != nil { + cached := cached.(*parseGoData) + if cached.parsed != nil { + return cached.parsed, nil + } + } + } + + fset := token.NewFileSet() + data := parseGo(ctx, fset, fh, mode) + return data.parsed, data.err } func magicCommentsChanged(original *ast.File, current *ast.File) bool { @@ -2238,18 +2297,29 @@ func magicCommentsChanged(original *ast.File, current *ast.File) bool { return false } -func isBadImportPath(path string) bool { +// validImports extracts the set of valid import paths from imports. +func validImports(imports []*ast.ImportSpec) map[string]struct{} { + m := make(map[string]struct{}) + for _, spec := range imports { + if path := spec.Path.Value; validImportPath(path) { + m[path] = struct{}{} + } + } + return m +} + +func validImportPath(path string) bool { path, err := strconv.Unquote(path) if err != nil { - return true + return false } if path == "" { - return true + return false } if path[len(path)-1] == '/' { - return true + return false } - return false + return true } var buildConstraintOrEmbedRe = regexp.MustCompile(`^//(go:embed|go:build|\s*\+build).*`) diff --git a/internal/lsp/cache/symbols.go b/internal/lsp/cache/symbols.go index 087e21184b..db68912015 100644 --- a/internal/lsp/cache/symbols.go +++ b/internal/lsp/cache/symbols.go @@ -40,10 +40,10 @@ func (s *snapshot) buildSymbolHandle(ctx context.Context, fh source.FileHandle) return h } key := symbolHandleKey(fh.FileIdentity().Hash) - h := s.generation.Bind(key, func(ctx context.Context, arg memoize.Arg) interface{} { + h := s.generation.Bind(key, func(_ context.Context, arg memoize.Arg) interface{} { snapshot := arg.(*snapshot) data := &symbolData{} - data.symbols, data.err = symbolize(ctx, snapshot, fh) + data.symbols, data.err = symbolize(snapshot, fh) return data }, nil) @@ -57,7 +57,7 @@ func (s *snapshot) buildSymbolHandle(ctx context.Context, fh source.FileHandle) // symbolize extracts symbols from a file. It uses a parsed file already // present in the cache but otherwise does not populate the cache. -func symbolize(ctx context.Context, snapshot *snapshot, fh source.FileHandle) ([]source.Symbol, error) { +func symbolize(snapshot *snapshot, fh source.FileHandle) ([]source.Symbol, error) { src, err := fh.Read() if err != nil { return nil, err @@ -70,16 +70,9 @@ func symbolize(ctx context.Context, snapshot *snapshot, fh source.FileHandle) ([ // If the file has already been fully parsed through the cache, we can just // use the result. - key := parseKey{file: fh.FileIdentity(), mode: source.ParseFull} - if pgh := snapshot.getGoFile(key); pgh != nil { - cached := pgh.handle.Cached(snapshot.generation) - if cached != nil { - cached := cached.(*parseGoData) - if cached.parsed != nil { - file = cached.parsed.File - fileDesc = cached.parsed.Tok - } - } + if pgf := snapshot.cachedPGF(fh, source.ParseFull); pgf != nil { + file = pgf.File + fileDesc = pgf.Tok } // Otherwise, we parse the file ourselves. Notably we don't use parseGo here,