internal/lsp: improve handling of files not in views
didModifyFiles and DidModifyFiles were tightly coupled but also repeated
each other's work a bit, and inconsistencies in the implementation led
to golang/go#41777.
Push all the work of assigning a "best view" down to the Session, and
always assign it somewhere, matching the logic in ViewOf. This would
in principle allow us to diagnose random files, but we only diagnose
workspace packages.
Fixes golang/go#41777.
Change-Id: I6dab32b98bdff6edd07032d84a8fec1b82ecd283
Reviewed-on: https://go-review.googlesource.com/c/tools/+/259877
Trust: Heschi Kreinick <heschi@google.com>
Run-TryBot: Heschi Kreinick <heschi@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
diff --git a/gopls/internal/regtest/workspace_test.go b/gopls/internal/regtest/workspace_test.go
index 9b76436..5a4c029 100644
--- a/gopls/internal/regtest/workspace_test.go
+++ b/gopls/internal/regtest/workspace_test.go
@@ -10,6 +10,8 @@
"testing"
"golang.org/x/tools/internal/lsp"
+ "golang.org/x/tools/internal/lsp/fake"
+ "golang.org/x/tools/internal/testenv"
)
const workspaceProxy = `
@@ -457,3 +459,26 @@
}
})
}
+
+func TestNonWorkspaceFileCreation(t *testing.T) {
+ testenv.NeedsGo1Point(t, 13)
+
+ const files = `
+-- go.mod --
+module mod.com
+
+-- x.go --
+package x
+`
+
+ const code = `
+package foo
+import "fmt"
+var _ = fmt.Printf
+`
+ run(t, files, func(t *testing.T, env *Env) {
+ env.CreateBuffer("/tmp/foo.go", "")
+ env.EditBuffer("/tmp/foo.go", fake.NewEdit(0, 0, 0, 0, code))
+ env.GoToDefinition("/tmp/foo.go", env.RegexpSearch("/tmp/foo.go", `Printf`))
+ })
+}
diff --git a/internal/lsp/cache/session.go b/internal/lsp/cache/session.go
index e597873..e74a0d1 100644
--- a/internal/lsp/cache/session.go
+++ b/internal/lsp/cache/session.go
@@ -430,16 +430,16 @@
}
func (s *Session) ModifyFiles(ctx context.Context, changes []source.FileModification) error {
- _, releases, _, err := s.DidModifyFiles(ctx, changes)
+ _, _, releases, _, err := s.DidModifyFiles(ctx, changes)
for _, release := range releases {
release()
}
return err
}
-func (s *Session) DidModifyFiles(ctx context.Context, changes []source.FileModification) ([]source.Snapshot, []func(), []span.URI, error) {
+func (s *Session) DidModifyFiles(ctx context.Context, changes []source.FileModification) (map[span.URI]source.View, map[source.View]source.Snapshot, []func(), []span.URI, error) {
views := make(map[*View]map[span.URI]source.VersionedFileHandle)
-
+ bestViews := map[span.URI]source.View{}
// Keep track of deleted files so that we can clear their diagnostics.
// A file might be re-created after deletion, so only mark files that
// have truly been deleted.
@@ -447,7 +447,7 @@
overlays, err := s.updateOverlays(ctx, changes)
if err != nil {
- return nil, nil, nil, err
+ return nil, nil, nil, nil, err
}
var forceReloadMetadata bool
for _, c := range changes {
@@ -455,17 +455,32 @@
forceReloadMetadata = true
}
- // Look through all of the session's views, invalidating the file for
- // all of the views to which it is known.
+ // Build the list of affected views.
+ bestView, err := s.viewOf(c.URI)
+ if err != nil {
+ return nil, nil, nil, nil, err
+ }
+ bestViews[c.URI] = bestView
+
+ var changedViews []*View
for _, view := range s.views {
// Don't propagate changes that are outside of the view's scope
// or knowledge.
if !view.relevantChange(c) {
continue
}
+ changedViews = append(changedViews, view)
+ }
+ // If no view matched the change, assign it to the best view.
+ if len(changedViews) == 0 {
+ changedViews = append(changedViews, bestView)
+ }
+
+ // Apply the changes to all affected views.
+ for _, view := range changedViews {
// Make sure that the file is added to the view.
if _, err := view.getFile(c.URI); err != nil {
- return nil, nil, nil, err
+ return nil, nil, nil, nil, err
}
if _, ok := views[view]; !ok {
views[view] = make(map[span.URI]source.VersionedFileHandle)
@@ -480,7 +495,7 @@
} else {
fsFile, err := s.cache.getFile(ctx, c.URI)
if err != nil {
- return nil, nil, nil, err
+ return nil, nil, nil, nil, err
}
fh = &closedFile{fsFile}
views[view][c.URI] = fh
@@ -490,18 +505,19 @@
}
}
}
- var snapshots []source.Snapshot
+
+ snapshots := map[source.View]source.Snapshot{}
var releases []func()
for view, uris := range views {
snapshot, release := view.invalidateContent(ctx, uris, forceReloadMetadata)
- snapshots = append(snapshots, snapshot)
+ snapshots[view] = snapshot
releases = append(releases, release)
}
var deletionsSlice []span.URI
for uri := range deletions {
deletionsSlice = append(deletionsSlice, uri)
}
- return snapshots, releases, deletionsSlice, nil
+ return bestViews, snapshots, releases, deletionsSlice, nil
}
func (s *Session) updateOverlays(ctx context.Context, changes []source.FileModification) (map[span.URI]*overlay, error) {
diff --git a/internal/lsp/general.go b/internal/lsp/general.go
index 1c0ebf2..ff76d58 100644
--- a/internal/lsp/general.go
+++ b/internal/lsp/general.go
@@ -269,7 +269,7 @@
// with the previously registered set of directories. If the set of directories
// has changed, we unregister and re-register for file watching notifications.
// updatedSnapshots is the set of snapshots that have been updated.
-func (s *Server) updateWatchedDirectories(ctx context.Context, updatedSnapshots []source.Snapshot) error {
+func (s *Server) updateWatchedDirectories(ctx context.Context, updatedSnapshots map[source.View]source.Snapshot) error {
dirsToWatch := map[span.URI]struct{}{}
seenViews := map[source.View]struct{}{}
diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go
index 90a5f4d..bbe95c4 100644
--- a/internal/lsp/source/view.go
+++ b/internal/lsp/source/view.go
@@ -269,7 +269,7 @@
// DidModifyFile reports a file modification to the session. It returns the
// resulting snapshots, a guaranteed one per view.
- DidModifyFiles(ctx context.Context, changes []FileModification) ([]Snapshot, []func(), []span.URI, error)
+ DidModifyFiles(ctx context.Context, changes []FileModification) (map[span.URI]View, map[View]Snapshot, []func(), []span.URI, error)
// Overlays returns a slice of file overlays for the session.
Overlays() []Overlay
diff --git a/internal/lsp/text_synchronization.go b/internal/lsp/text_synchronization.go
index 0a78114..805393c 100644
--- a/internal/lsp/text_synchronization.go
+++ b/internal/lsp/text_synchronization.go
@@ -186,7 +186,7 @@
}()
}()
}
- snapshots, releases, deletions, err := s.session.DidModifyFiles(ctx, modifications)
+ views, snapshots, releases, deletions, err := s.session.DidModifyFiles(ctx, modifications)
if err != nil {
return err
}
@@ -201,43 +201,15 @@
return err
}
}
- snapshotByURI := make(map[span.URI]source.Snapshot)
- for _, c := range modifications {
- snapshotByURI[c.URI] = nil
- }
- // Avoid diagnosing the same snapshot twice.
- snapshotSet := make(map[source.Snapshot][]span.URI)
- for uri := range snapshotByURI {
- view, err := s.session.ViewOf(uri)
- if err != nil {
- return err
- }
- var snapshot source.Snapshot
- for _, s := range snapshots {
- if s.View() == view {
- if snapshot != nil {
- return errors.New("duplicate snapshots for the same view")
- }
- snapshot = s
- }
- }
- // If the file isn't in any known views (for example, if it's in a dependency),
- // we may not have a snapshot to map it to. As a result, we won't try to
- // diagnose it. TODO(rstambler): Figure out how to handle this better.
- if snapshot == nil {
- continue
- }
- snapshotSet[snapshot] = append(snapshotSet[snapshot], uri)
- snapshotByURI[uri] = snapshot
- }
+ // Check if the user is trying to modify a generated file.
for _, mod := range modifications {
if mod.OnDisk || mod.Action != source.Change {
continue
}
- snapshot, ok := snapshotByURI[mod.URI]
- if !ok {
- continue
+ snapshot := snapshots[views[mod.URI]]
+ if snapshot == nil {
+ panic("no snapshot assigned for file " + mod.URI)
}
// Ideally, we should be able to specify that a generated file should be opened as read-only.
// Tell the user that they should not be editing a generated file.
@@ -251,12 +223,17 @@
}
}
- for snapshot, uris := range snapshotSet {
+ // Group files by best view and diagnose them.
+ viewURIs := map[source.View][]span.URI{}
+ for uri, view := range views {
+ viewURIs[view] = append(viewURIs[view], uri)
+ }
+ for view, uris := range viewURIs {
diagnosticWG.Add(1)
go func(snapshot source.Snapshot, uris []span.URI) {
defer diagnosticWG.Done()
s.diagnoseSnapshot(snapshot, uris)
- }(snapshot, uris)
+ }(snapshots[view], uris)
}
go func() {
@@ -265,6 +242,7 @@
release()
}
}()
+
// After any file modifications, we need to update our watched files,
// in case something changed. Compute the new set of directories to watch,
// and if it differs from the current set, send updated registrations.
