internal/lsp/cache: invalidate metadata when parsing issues resolve
Package loading (at least via go list) fails when the import header
doesn't parse, so we need to invalidate metadata upon a state change
from non parsing->parsing. Refactor metadata invalidation to implement
this feature, add additional documentation, and avoid unnecessary work.
This change revealed a latent bug (via TestDeleteDirectory): when
statting a deleted directory failed, we could fail to invalidate any
package IDs, even those we already knew about. This bug was masked by
the somewhat complicated semantics of pkgNameChanged. The semantics of
pkgNameChanged are simplified to encapsulate any change to a
package->file association.
Also refactor the parsing API somewhat, and add a bug report if we don't
get a ParseGoFile while inspecting for metadata changes.
Update most regtests to panic upon receiving bug reports.
Fixes golang/go#52981
Change-Id: I1838963ecc9c01e316f887aa9d8f1260662281ab
Reviewed-on: https://go-review.googlesource.com/c/tools/+/407501
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
diff --git a/gopls/internal/regtest/bench/bench_test.go b/gopls/internal/regtest/bench/bench_test.go
index 44ab457..f37c2f6 100644
--- a/gopls/internal/regtest/bench/bench_test.go
+++ b/gopls/internal/regtest/bench/bench_test.go
@@ -12,6 +12,7 @@
"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 @@
)
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 3e15271..a64f9c4 100644
--- a/gopls/internal/regtest/codelens/codelens_test.go
+++ b/gopls/internal/regtest/codelens/codelens_test.go
@@ -11,6 +11,7 @@
"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 @@
)
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 c0b4736..1ffb000 100644
--- a/gopls/internal/regtest/completion/completion_test.go
+++ b/gopls/internal/regtest/completion/completion_test.go
@@ -10,6 +10,7 @@
"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 @@
)
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 a2707ef..6ce0cdb 100644
--- a/gopls/internal/regtest/diagnostics/diagnostics_test.go
+++ b/gopls/internal/regtest/diagnostics/diagnostics_test.go
@@ -11,6 +11,7 @@
"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 @@
)
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 3694b07..c553bdb 100644
--- a/gopls/internal/regtest/misc/misc_test.go
+++ b/gopls/internal/regtest/misc/misc_test.go
@@ -8,9 +8,11 @@
"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 cca3b3c..93d4325 100644
--- a/gopls/internal/regtest/modfile/modfile_test.go
+++ b/gopls/internal/regtest/modfile/modfile_test.go
@@ -11,6 +11,7 @@
"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 @@
)
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 b0acdfe..9489e9b 100644
--- a/gopls/internal/regtest/template/template_test.go
+++ b/gopls/internal/regtest/template/template_test.go
@@ -9,11 +9,13 @@
"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 5b432e1..e66d08a 100644
--- a/gopls/internal/regtest/watch/watch_test.go
+++ b/gopls/internal/regtest/watch/watch_test.go
@@ -8,6 +8,7 @@
"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 @@
)
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 0000000..28291a2
--- /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 9098bd4..b283cfa 100644
--- a/gopls/internal/regtest/workspace/workspace_test.go
+++ b/gopls/internal/regtest/workspace/workspace_test.go
@@ -12,15 +12,17 @@
"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 147b8e8..a419a9c 100644
--- a/internal/lsp/bug/bug.go
+++ b/internal/lsp/bug/bug.go
@@ -19,6 +19,11 @@
"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 @@
// 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 @@
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 fdc4908..b8a3655 100644
--- a/internal/lsp/cache/check.go
+++ b/internal/lsp/cache/check.go
@@ -570,8 +570,7 @@
// 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 5d751eb..1ab1fa9 100644
--- a/internal/lsp/cache/parse.go
+++ b/internal/lsp/cache/parse.go
@@ -72,27 +72,19 @@
}
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 @@
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 5cc534a..9beb9e6 100644
--- a/internal/lsp/cache/snapshot.go
+++ b/internal/lsp/cache/snapshot.go
@@ -29,6 +29,7 @@
"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 @@
}
}
- 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 @@
// 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 @@
// 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 @@
// 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 @@
}
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 @@
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 @@
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 087e211..db68912 100644
--- a/internal/lsp/cache/symbols.go
+++ b/internal/lsp/cache/symbols.go
@@ -40,10 +40,10 @@
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 @@
// 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 @@
// 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,
