internal/lsp: remove logic for re-creating a view when a go.mod changes
This is no longer necessary now that we modify the modules field based
on newly created/deleted modules.
There was a race condition setting the metadata--we were reusing old
metadata that may have already been cached in some instances. Now, we
always override metadata.
Also, disabled the TestUseGoplsMod test -- there seems to be an issue
with it. Will discuss this offline in the AM.
Change-Id: Ie8c97557d4f0a319051256e5f130b9cdae479928
Reviewed-on: https://go-review.googlesource.com/c/tools/+/258121
Trust: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
diff --git a/gopls/internal/regtest/workspace_test.go b/gopls/internal/regtest/workspace_test.go
index 704f598..77d55be 100644
--- a/gopls/internal/regtest/workspace_test.go
+++ b/gopls/internal/regtest/workspace_test.go
@@ -286,6 +286,7 @@
if want := "b.com@v1.2.3/b/b.go"; !strings.HasSuffix(original, want) {
t.Errorf("expected %s, got %v", want, original)
}
+ env.CloseBuffer(original)
env.WriteWorkspaceFiles(map[string]string{
"modb/go.mod": "module b.com",
"modb/b/b.go": `package b
diff --git a/internal/lsp/cache/session.go b/internal/lsp/cache/session.go
index 61595a2..4986c00 100644
--- a/internal/lsp/cache/session.go
+++ b/internal/lsp/cache/session.go
@@ -483,13 +483,6 @@
deletions[c.URI] = struct{}{}
}
}
- // If the file change is to a go.mod file, and initialization for
- // the view has previously failed, we should attempt to retry.
- // TODO(rstambler): We can use unsaved contents with -modfile, so
- // maybe we should do that and retry on any change?
- if fh.Kind() == source.Mod && (c.OnDisk || c.Action == source.Save) {
- view.maybeReinitialize()
- }
}
}
var snapshots []source.Snapshot
diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go
index 759cb33..d08bc78 100644
--- a/internal/lsp/cache/snapshot.go
+++ b/internal/lsp/cache/snapshot.go
@@ -126,10 +126,11 @@
}
func (s *snapshot) ModFiles() []span.URI {
- if s.view.modURI == "" {
- return nil
+ var uris []span.URI
+ for _, m := range s.modules {
+ uris = append(uris, m.modURI)
}
- return []span.URI{s.view.modURI}
+ return uris
}
func (s *snapshot) ValidBuildConfiguration() bool {
@@ -928,7 +929,7 @@
return fmt.Sprintf("v%v/%v", v.id, snapshotID)
}
-func (s *snapshot) clone(ctx context.Context, withoutURIs map[span.URI]source.VersionedFileHandle, forceReloadMetadata bool) *snapshot {
+func (s *snapshot) clone(ctx context.Context, withoutURIs map[span.URI]source.VersionedFileHandle, forceReloadMetadata bool) (*snapshot, reinitializeView) {
s.mu.Lock()
defer s.mu.Unlock()
@@ -1062,8 +1063,8 @@
// If the view's go.mod file's contents have changed, invalidate
// the metadata for every known package in the snapshot.
if invalidateMetadata {
- for k := range s.packages {
- directIDs[k.id] = struct{}{}
+ for k := range s.metadata {
+ directIDs[k] = struct{}{}
}
// If a go.mod file in the workspace has changed, we need to
// rebuild the workspace module.
@@ -1159,7 +1160,6 @@
// When modules change, we need to recompute their workspace directories,
// as replace directives may have changed.
if modulesChanged {
- // Use the new snapshot, as it can be locked without causing issues.
result.workspaceDirectories = result.findWorkspaceDirectories(ctx)
}
@@ -1200,7 +1200,11 @@
}
// Copy the set of initally loaded packages.
for id, pkgPath := range s.workspacePackages {
- if id == "command-line-arguments" {
+ // Packages with the id "command-line-arguments" or with a package path
+ // prefixed with "_/" are generated by the go command when the user is
+ // outside of GOPATH and outside of a module. Do not cache them as
+ // workspace packages for longer than necessary.
+ if id == "command-line-arguments" || strings.HasPrefix(string(pkgPath), "_/") {
if invalidateMetadata, ok := transitiveIDs[id]; invalidateMetadata && ok {
continue
}
@@ -1224,10 +1228,6 @@
result.workspacePackages[id] = pkgPath
}
- if shouldReinitializeView {
- s.view.definitelyReinitialize()
- }
-
// Inherit all of the go.mod-related handles.
for _, v := range result.modTidyHandles {
newGen.Inherit(v.handle)
@@ -1244,12 +1244,27 @@
if result.workspaceModuleHandle != nil {
newGen.Inherit(result.workspaceModuleHandle.handle)
}
-
// Don't bother copying the importedBy graph,
// as it changes each time we update metadata.
- return result
+
+ var reinitialize reinitializeView
+ if modulesChanged {
+ reinitialize = maybeReinit
+ }
+ if shouldReinitializeView {
+ reinitialize = definitelyReinit
+ }
+ return result, reinitialize
}
+type reinitializeView int
+
+const (
+ doNotReinit = reinitializeView(iota)
+ maybeReinit
+ definitelyReinit
+)
+
// fileWasSaved reports whether the FileHandle passed in has been saved. It
// accomplishes this by checking to see if the original and current FileHandles
// are both overlays, and if the current FileHandle is saved while the original
@@ -1270,7 +1285,7 @@
// determine if the file requires a metadata reload.
func (s *snapshot) shouldInvalidateMetadata(ctx context.Context, newSnapshot *snapshot, originalFH, currentFH source.FileHandle) bool {
if originalFH == nil {
- return currentFH.Kind() == source.Go
+ return true
}
// If the file hasn't changed, there's no need to reload.
if originalFH.FileIdentity() == currentFH.FileIdentity() {
diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go
index fca69e8..01b9526 100644
--- a/internal/lsp/cache/view.go
+++ b/internal/lsp/cache/view.go
@@ -801,9 +801,14 @@
defer v.snapshotMu.Unlock()
oldSnapshot := v.snapshot
- v.snapshot = oldSnapshot.clone(ctx, uris, forceReloadMetadata)
+ var reinitialize reinitializeView
+ v.snapshot, reinitialize = oldSnapshot.clone(ctx, uris, forceReloadMetadata)
go oldSnapshot.generation.Destroy()
+ if reinitialize == maybeReinit || reinitialize == definitelyReinit {
+ v.reinitialize(reinitialize == definitelyReinit)
+ }
+
return v.snapshot, v.snapshot.generation.Acquire(ctx)
}
@@ -818,14 +823,6 @@
v.backgroundCtx, v.cancel = context.WithCancel(v.baseCtx)
}
-func (v *View) maybeReinitialize() {
- v.reinitialize(false)
-}
-
-func (v *View) definitelyReinitialize() {
- v.reinitialize(true)
-}
-
func (v *View) reinitialize(force bool) {
v.initializationSema <- struct{}{}
defer func() {
diff --git a/internal/lsp/text_synchronization.go b/internal/lsp/text_synchronization.go
index 89aea44..e1a80eb 100644
--- a/internal/lsp/text_synchronization.go
+++ b/internal/lsp/text_synchronization.go
@@ -251,31 +251,6 @@
}
for snapshot, uris := range snapshotSet {
- // If a modification comes in for the view's go.mod file and the view
- // was never properly initialized, or the view does not have
- // a go.mod file, try to recreate the associated view.
- if len(snapshot.ModFiles()) == 0 {
- for _, uri := range uris {
- // Don't rebuild the view until the go.mod is on disk.
- if !snapshot.IsSaved(uri) {
- continue
- }
- fh, err := snapshot.GetFile(ctx, uri)
- if err != nil {
- return err
- }
- switch fh.Kind() {
- case source.Mod:
- newSnapshot, release, err := snapshot.View().Rebuild(ctx)
- releases = append(releases, release)
- if err != nil {
- return err
- }
- // Update the snapshot to the rebuilt one.
- snapshot = newSnapshot
- }
- }
- }
diagnosticWG.Add(1)
go func(snapshot source.Snapshot, uris []span.URI) {
defer diagnosticWG.Done()
