From 23a3aa10a23ce7f9a01e441b2f451a918d5e1546 Mon Sep 17 00:00:00 2001 From: Heschi Kreinick Date: Tue, 6 Oct 2020 13:20:32 -0400 Subject: [PATCH] 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 Run-TryBot: Heschi Kreinick gopls-CI: kokoro TryBot-Result: Go Bot Reviewed-by: Rebecca Stambler --- gopls/internal/regtest/workspace_test.go | 25 ++++++++++++ internal/lsp/cache/session.go | 38 +++++++++++++------ internal/lsp/general.go | 2 +- internal/lsp/source/view.go | 2 +- internal/lsp/text_synchronization.go | 48 +++++++----------------- 5 files changed, 67 insertions(+), 48 deletions(-) diff --git a/gopls/internal/regtest/workspace_test.go b/gopls/internal/regtest/workspace_test.go index 9b76436a52..5a4c029a2c 100644 --- a/gopls/internal/regtest/workspace_test.go +++ b/gopls/internal/regtest/workspace_test.go @@ -10,6 +10,8 @@ import ( "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 @@ replace b.com => %s/modb } }) } + +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 e5978737ce..e74a0d192b 100644 --- a/internal/lsp/cache/session.go +++ b/internal/lsp/cache/session.go @@ -430,16 +430,16 @@ func (s *Session) dropView(ctx context.Context, v *View) (int, error) { } 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 @@ func (s *Session) DidModifyFiles(ctx context.Context, changes []source.FileModif 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 @@ func (s *Session) DidModifyFiles(ctx context.Context, changes []source.FileModif 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 @@ func (s *Session) DidModifyFiles(ctx context.Context, changes []source.FileModif } 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 @@ func (s *Session) DidModifyFiles(ctx context.Context, changes []source.FileModif } } } - 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 1c0ebf2598..ff76d58dac 100644 --- a/internal/lsp/general.go +++ b/internal/lsp/general.go @@ -269,7 +269,7 @@ func (s *Server) addFolders(ctx context.Context, folders []protocol.WorkspaceFol // 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 90a5f4d09f..bbe95c4d94 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -269,7 +269,7 @@ type Session interface { // 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 0a781147ee..805393c57e 100644 --- a/internal/lsp/text_synchronization.go +++ b/internal/lsp/text_synchronization.go @@ -186,7 +186,7 @@ func (s *Server) didModifyFiles(ctx context.Context, modifications []source.File }() }() } - 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 @@ func (s *Server) didModifyFiles(ctx context.Context, modifications []source.File 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 @@ func (s *Server) didModifyFiles(ctx context.Context, modifications []source.File } } - 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 @@ func (s *Server) didModifyFiles(ctx context.Context, modifications []source.File 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.