mirror
/
go
mirror of https://github.com/golang/go.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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>
This commit is contained in:
Heschi Kreinick 2020-10-06 13:20:32 -04:00
parent 567bb5a4fa
commit 23a3aa10a2
5 changed files with 67 additions and 48 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

25
gopls/internal/regtest/workspace_test.go
View File

@ -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`))
})
}

38
internal/lsp/cache/session.go vendored
View File

@ -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) {

2
internal/lsp/general.go
View File

@ -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{}{}

2
internal/lsp/source/view.go
View File

@ -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

48
internal/lsp/text_synchronization.go
View File

@ -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.