internal/lsp: set file contents through the source.View, not File

Refactor code as a follow-up to
https://go-review.googlesource.com/c/tools/+/154742.
Also, change every instance of "source.URI()" to "fromProtocolURI", so
that we can add a better implementation of that later on (for Windows
support).

Change-Id: Ifa24ffd7e1aebf1f7d05df6f65742769ead0922f
Reviewed-on: https://go-review.googlesource.com/c/154741
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

internal/lsp/cache/file.go

@@ -25,35 +25,6 @@ type File struct {
 	pkg     *packages.Package
 }
 
-// SetContent sets the overlay contents for a file.
-// Setting it to nil will revert it to the on disk contents, and remove it
-// from the active set.
-func (f *File) SetContent(content []byte) {
-	f.view.mu.Lock()
-	defer f.view.mu.Unlock()
-	f.content = content
-	// the ast and token fields are invalid
-	f.ast = nil
-	f.token = nil
-	f.pkg = nil
-	// and we might need to update the overlay
-	switch {
-	case f.active && content == nil:
-		// we were active, and want to forget the content
-		f.active = false
-		if filename, err := f.URI.Filename(); err == nil {
-			delete(f.view.Config.Overlay, filename)
-		}
-		f.content = nil
-	case content != nil:
-		// an active overlay, update the map
-		f.active = true
-		if filename, err := f.URI.Filename(); err == nil {
-			f.view.Config.Overlay[filename] = f.content
-		}
-	}
-}
-
 // Read returns the contents of the file, reading it from file system if needed.
 func (f *File) Read() ([]byte, error) {
 	f.view.mu.Lock()
@@ -62,9 +33,6 @@ func (f *File) Read() ([]byte, error) {
 }
 
 func (f *File) GetFileSet() (*token.FileSet, error) {
-	if f.view.Config == nil {
-		return nil, fmt.Errorf("no config for file view")
-	}
 	if f.view.Config.Fset == nil {
 		return nil, fmt.Errorf("no fileset for file view config")
 	}

internal/lsp/cache/view.go

@@ -5,6 +5,7 @@
 package cache
 
 import (
+	"context"
 	"fmt"
 	"go/token"
 	"sync"
@@ -16,31 +17,66 @@ import (
 type View struct {
 	mu sync.Mutex // protects all mutable state of the view
 
-	Config *packages.Config
+	Config packages.Config
 
 	files map[source.URI]*File
 }
 
-func NewView(rootPath string) *View {
+// NewView creates a new View, given a root path and go/packages configuration.
+// If config is nil, one is created with the directory set to the rootPath.
+func NewView(config *packages.Config) *View {
 	return &View{
-		Config: &packages.Config{
+		Config: *config,
-			Dir:     rootPath,
-			Mode:    packages.LoadSyntax,
-			Fset:    token.NewFileSet(),
-			Tests:   true,
-			Overlay: make(map[string][]byte),
-		},
 		files:  make(map[source.URI]*File),
 	}
 }
 
+func (v *View) FileSet() *token.FileSet {
+	return v.Config.Fset
+}
+
+// SetContent sets the overlay contents for a file. A nil content value will
+// remove the file from the active set and revert it to its on-disk contents.
+func (v *View) SetContent(ctx context.Context, uri source.URI, content []byte) (source.View, error) {
+	v.mu.Lock()
+	defer v.mu.Unlock()
+
+	f := v.getFile(uri)
+	f.content = content
+
+	// Resetting the contents invalidates the ast, token, and pkg fields.
+	f.ast = nil
+	f.token = nil
+	f.pkg = nil
+
+	// We might need to update the overlay.
+	switch {
+	case f.active && content == nil:
+		// The file was active, so we need to forget its content.
+		f.active = false
+		if filename, err := f.URI.Filename(); err == nil {
+			delete(f.view.Config.Overlay, filename)
+		}
+		f.content = nil
+	case content != nil:
+		// This is an active overlay, so we update the map.
+		f.active = true
+		if filename, err := f.URI.Filename(); err == nil {
+			f.view.Config.Overlay[filename] = f.content
+		}
+	}
+
+	// TODO(rstambler): We should really return a new, updated view.
+	return v, nil
+}
+
 // GetFile returns a File for the given URI. It will always succeed because it
 // adds the file to the managed set if needed.
-func (v *View) GetFile(uri source.URI) source.File {
+func (v *View) GetFile(ctx context.Context, uri source.URI) (source.File, error) {
 	v.mu.Lock()
 	f := v.getFile(uri)
 	v.mu.Unlock()
-	return f
+	return f, nil
 }
 
 // getFile is the unlocked internal implementation of GetFile.
@@ -51,7 +87,7 @@ func (v *View) getFile(uri source.URI) *File {
 			URI:  uri,
 			view: v,
 		}
-		v.files[f.URI] = f
+		v.files[uri] = f
 	}
 	return f
 }
@@ -61,7 +97,7 @@ func (v *View) parse(uri source.URI) error {
 	if err != nil {
 		return err
 	}
-	pkgs, err := packages.Load(v.Config, fmt.Sprintf("file=%s", path))
+	pkgs, err := packages.Load(&v.Config, fmt.Sprintf("file=%s", path))
 	if len(pkgs) == 0 {
 		if err == nil {
 			err = fmt.Errorf("no packages found for %s", path)
@@ -69,9 +105,9 @@ func (v *View) parse(uri source.URI) error {
 		return err
 	}
 	for _, pkg := range pkgs {
-		// add everything we find to the files cache
+		// Add every file in this package to our cache.
 		for _, fAST := range pkg.Syntax {
-			// if a file was in multiple packages, which token/ast/pkg do we store
+			// TODO: If a file is in multiple packages, which package do we store?
 			fToken := v.Config.Fset.File(fAST.Pos())
 			fURI := source.ToURI(fToken.Name())
 			f := v.getFile(fURI)

internal/lsp/diagnostics.go

@@ -8,18 +8,17 @@ import (
 	"context"
 	"sort"
 
-	"golang.org/x/tools/internal/lsp/cache"
 	"golang.org/x/tools/internal/lsp/protocol"
 	"golang.org/x/tools/internal/lsp/source"
 )
 
-func (s *server) CacheAndDiagnose(ctx context.Context, uri protocol.DocumentURI, text string) {
+func (s *server) cacheAndDiagnose(ctx context.Context, uri protocol.DocumentURI, content string) {
-	f := s.view.GetFile(source.URI(uri))
+	sourceURI := fromProtocolURI(uri)
-	if f, ok := f.(*cache.File); ok {
+	if err := s.setContent(ctx, sourceURI, []byte(content)); err != nil {
-		f.SetContent([]byte(text))
+		return // handle error?
 	}
 	go func() {
-		reports, err := source.Diagnostics(ctx, s.view, f)
+		reports, err := source.Diagnostics(ctx, s.view, sourceURI)
 		if err != nil {
 			return // handle error?
 		}
@@ -27,16 +26,32 @@ func (s *server) CacheAndDiagnose(ctx context.Context, uri protocol.DocumentURI,
 			uri := source.ToURI(filename)
 			s.client.PublishDiagnostics(ctx, &protocol.PublishDiagnosticsParams{
 				URI:         protocol.DocumentURI(uri),
-				Diagnostics: toProtocolDiagnostics(s.view, uri, diagnostics),
+				Diagnostics: toProtocolDiagnostics(ctx, s.view, uri, diagnostics),
 			})
 		}
 	}()
 }
 
-func toProtocolDiagnostics(v *cache.View, uri source.URI, diagnostics []source.Diagnostic) []protocol.Diagnostic {
+func (s *server) setContent(ctx context.Context, uri source.URI, content []byte) error {
+	v, err := s.view.SetContent(ctx, uri, content)
+	if err != nil {
+		return err
+	}
+
+	s.viewMu.Lock()
+	s.view = v
+	s.viewMu.Unlock()
+
+	return nil
+}
+
+func toProtocolDiagnostics(ctx context.Context, v source.View, uri source.URI, diagnostics []source.Diagnostic) []protocol.Diagnostic {
 	reports := []protocol.Diagnostic{}
 	for _, diag := range diagnostics {
-		f := v.GetFile(uri)
+		f, err := v.GetFile(ctx, uri)
+		if err != nil {
+			continue // handle error?
+		}
 		tok, err := f.GetToken()
 		if err != nil {
 			continue // handle error?

internal/lsp/format.go

@@ -4,14 +4,16 @@ import (
 	"context"
 	"go/token"
 
-	"golang.org/x/tools/internal/lsp/cache"
 	"golang.org/x/tools/internal/lsp/protocol"
 	"golang.org/x/tools/internal/lsp/source"
 )
 
 // formatRange formats a document with a given range.
-func formatRange(ctx context.Context, v *cache.View, uri protocol.DocumentURI, rng *protocol.Range) ([]protocol.TextEdit, error) {
+func formatRange(ctx context.Context, v source.View, uri protocol.DocumentURI, rng *protocol.Range) ([]protocol.TextEdit, error) {
-	f := v.GetFile(source.URI(uri))
+	f, err := v.GetFile(ctx, fromProtocolURI(uri))
+	if err != nil {
+		return nil, err
+	}
 	tok, err := f.GetToken()
 	if err != nil {
 		return nil, err

internal/lsp/imports.go

@@ -7,13 +7,15 @@ package lsp
 import (
 	"context"
 
-	"golang.org/x/tools/internal/lsp/cache"
 	"golang.org/x/tools/internal/lsp/protocol"
 	"golang.org/x/tools/internal/lsp/source"
 )
 
-func organizeImports(ctx context.Context, v *cache.View, uri protocol.DocumentURI) ([]protocol.TextEdit, error) {
+func organizeImports(ctx context.Context, v source.View, uri protocol.DocumentURI) ([]protocol.TextEdit, error) {
-	f := v.GetFile(source.URI(uri))
+	f, err := v.GetFile(ctx, fromProtocolURI(uri))
+	if err != nil {
+		return nil, err
+	}
 	tok, err := f.GetToken()
 	if err != nil {
 		return nil, err

internal/lsp/lsp_test.go

@@ -56,15 +56,14 @@ func testLSP(t *testing.T, exporter packagestest.Exporter) {
 	exported := packagestest.Export(t, exporter, modules)
 	defer exported.Cleanup()
 
-	s := &server{
-		view: cache.NewView(exported.Config.Dir),
-	}
 	// Merge the exported.Config with the view.Config.
 	cfg := *exported.Config
-	cfg.Fset = s.view.Config.Fset
+	cfg.Fset = token.NewFileSet()
 	cfg.Mode = packages.LoadSyntax
-	s.view.Config = &cfg
 
+	s := &server{
+		view: cache.NewView(&cfg),
+	}
 	// Do a first pass to collect special markers for completion.
 	if err := exported.Expect(map[string]interface{}{
 		"item": func(name string, r packagestest.Range, _, _ string) {
@@ -150,15 +149,15 @@ type completions map[token.Position][]token.Pos
 type formats map[string]string
 type definitions map[protocol.Location]protocol.Location
 
-func (d diagnostics) test(t *testing.T, exported *packagestest.Exported, v *cache.View) int {
+func (d diagnostics) test(t *testing.T, exported *packagestest.Exported, v source.View) int {
 	count := 0
+	ctx := context.Background()
 	for filename, want := range d {
-		f := v.GetFile(source.ToURI(filename))
+		sourceDiagnostics, err := source.Diagnostics(context.Background(), v, source.ToURI(filename))
-		sourceDiagnostics, err := source.Diagnostics(context.Background(), v, f)
 		if err != nil {
 			t.Fatal(err)
 		}
-		got := toProtocolDiagnostics(v, source.ToURI(filename), sourceDiagnostics[filename])
+		got := toProtocolDiagnostics(ctx, v, source.ToURI(filename), sourceDiagnostics[filename])
 		sorted(got)
 		if diff := diffDiagnostics(filename, want, got); diff != "" {
 			t.Error(diff)

internal/lsp/position.go

@@ -5,6 +5,7 @@
 package lsp
 
 import (
+	"context"
 	"go/token"
 
 	"golang.org/x/tools/internal/lsp/cache"
@@ -12,11 +13,20 @@ import (
 	"golang.org/x/tools/internal/lsp/source"
 )
 
+// fromProtocolURI converts a protocol.DocumentURI to a source.URI.
+// TODO(rstambler): Add logic here to support Windows.
+func fromProtocolURI(uri protocol.DocumentURI) source.URI {
+	return source.URI(uri)
+}
+
 // fromProtocolLocation converts from a protocol location to a source range.
 // It will return an error if the file of the location was not valid.
 // It uses fromProtocolRange to convert the start and end positions.
-func fromProtocolLocation(v *cache.View, loc protocol.Location) (source.Range, error) {
+func fromProtocolLocation(ctx context.Context, v *cache.View, loc protocol.Location) (source.Range, error) {
-	f := v.GetFile(source.URI(loc.URI))
+	f, err := v.GetFile(ctx, fromProtocolURI(loc.URI))
+	if err != nil {
+		return source.Range{}, err
+	}
 	tok, err := f.GetToken()
 	if err != nil {
 		return source.Range{}, err

internal/lsp/server.go

@@ -6,9 +6,11 @@ package lsp
 
 import (
 	"context"
+	"go/token"
 	"os"
 	"sync"
 
+	"golang.org/x/tools/go/packages"
 	"golang.org/x/tools/internal/jsonrpc2"
 	"golang.org/x/tools/internal/lsp/cache"
 	"golang.org/x/tools/internal/lsp/protocol"
@@ -33,7 +35,8 @@ type server struct {
 	signatureHelpEnabled bool
 	snippetsSupported    bool
 
-	view *cache.View
+	viewMu sync.Mutex
+	view   source.View
 }
 
 func (s *server) Initialize(ctx context.Context, params *protocol.InitializeParams) (*protocol.InitializeResult, error) {
@@ -48,11 +51,17 @@ func (s *server) Initialize(ctx context.Context, params *protocol.InitializePara
 	s.snippetsSupported = params.Capabilities.TextDocument.Completion.CompletionItem.SnippetSupport
 	s.signatureHelpEnabled = true
 
-	rootPath, err := source.URI(*params.RootURI).Filename()
+	rootPath, err := fromProtocolURI(*params.RootURI).Filename()
 	if err != nil {
 		return nil, err
 	}
-	s.view = cache.NewView(rootPath)
+	s.view = cache.NewView(&packages.Config{
+		Dir:     rootPath,
+		Mode:    packages.LoadSyntax,
+		Fset:    token.NewFileSet(),
+		Tests:   true,
+		Overlay: make(map[string][]byte),
+	})
 
 	return &protocol.InitializeResult{
 		Capabilities: protocol.ServerCapabilities{
@@ -119,7 +128,7 @@ func (s *server) ExecuteCommand(context.Context, *protocol.ExecuteCommandParams)
 }
 
 func (s *server) DidOpen(ctx context.Context, params *protocol.DidOpenTextDocumentParams) error {
-	s.CacheAndDiagnose(ctx, params.TextDocument.URI, params.TextDocument.Text)
+	s.cacheAndDiagnose(ctx, params.TextDocument.URI, params.TextDocument.Text)
 	return nil
 }
 
@@ -129,7 +138,7 @@ func (s *server) DidChange(ctx context.Context, params *protocol.DidChangeTextDo
 	}
 	// We expect the full content of file, i.e. a single change with no range.
 	if change := params.ContentChanges[0]; change.RangeLength == 0 {
-		s.CacheAndDiagnose(ctx, params.TextDocument.URI, change.Text)
+		s.cacheAndDiagnose(ctx, params.TextDocument.URI, change.Text)
 	}
 	return nil
 }
@@ -147,15 +156,15 @@ func (s *server) DidSave(context.Context, *protocol.DidSaveTextDocumentParams) e
 }
 
 func (s *server) DidClose(ctx context.Context, params *protocol.DidCloseTextDocumentParams) error {
-	f := s.view.GetFile(source.URI(params.TextDocument.URI))
+	s.setContent(ctx, fromProtocolURI(params.TextDocument.URI), nil)
-	if f, ok := f.(*cache.File); ok {
-		f.SetContent(nil)
-	}
 	return nil
 }
 
 func (s *server) Completion(ctx context.Context, params *protocol.CompletionParams) (*protocol.CompletionList, error) {
-	f := s.view.GetFile(source.URI(params.TextDocument.URI))
+	f, err := s.view.GetFile(ctx, fromProtocolURI(params.TextDocument.URI))
+	if err != nil {
+		return nil, err
+	}
 	tok, err := f.GetToken()
 	if err != nil {
 		return nil, err
@@ -176,7 +185,10 @@ func (s *server) CompletionResolve(context.Context, *protocol.CompletionItem) (*
 }
 
 func (s *server) Hover(ctx context.Context, params *protocol.TextDocumentPositionParams) (*protocol.Hover, error) {
-	f := s.view.GetFile(source.URI(params.TextDocument.URI))
+	f, err := s.view.GetFile(ctx, fromProtocolURI(params.TextDocument.URI))
+	if err != nil {
+		return nil, err
+	}
 	tok, err := f.GetToken()
 	if err != nil {
 		return nil, err
@@ -196,7 +208,10 @@ func (s *server) Hover(ctx context.Context, params *protocol.TextDocumentPositio
 }
 
 func (s *server) SignatureHelp(ctx context.Context, params *protocol.TextDocumentPositionParams) (*protocol.SignatureHelp, error) {
-	f := s.view.GetFile(source.URI(params.TextDocument.URI))
+	f, err := s.view.GetFile(ctx, fromProtocolURI(params.TextDocument.URI))
+	if err != nil {
+		return nil, err
+	}
 	tok, err := f.GetToken()
 	if err != nil {
 		return nil, err
@@ -210,7 +225,10 @@ func (s *server) SignatureHelp(ctx context.Context, params *protocol.TextDocumen
 }
 
 func (s *server) Definition(ctx context.Context, params *protocol.TextDocumentPositionParams) ([]protocol.Location, error) {
-	f := s.view.GetFile(source.URI(params.TextDocument.URI))
+	f, err := s.view.GetFile(ctx, fromProtocolURI(params.TextDocument.URI))
+	if err != nil {
+		return nil, err
+	}
 	tok, err := f.GetToken()
 	if err != nil {
 		return nil, err
@@ -220,11 +238,14 @@ func (s *server) Definition(ctx context.Context, params *protocol.TextDocumentPo
 	if err != nil {
 		return nil, err
 	}
-	return []protocol.Location{toProtocolLocation(s.view.Config.Fset, r)}, nil
+	return []protocol.Location{toProtocolLocation(s.view.FileSet(), r)}, nil
 }
 
 func (s *server) TypeDefinition(ctx context.Context, params *protocol.TextDocumentPositionParams) ([]protocol.Location, error) {
-	f := s.view.GetFile(source.URI(params.TextDocument.URI))
+	f, err := s.view.GetFile(ctx, fromProtocolURI(params.TextDocument.URI))
+	if err != nil {
+		return nil, err
+	}
 	tok, err := f.GetToken()
 	if err != nil {
 		return nil, err
@@ -234,7 +255,7 @@ func (s *server) TypeDefinition(ctx context.Context, params *protocol.TextDocume
 	if err != nil {
 		return nil, err
 	}
-	return []protocol.Location{toProtocolLocation(s.view.Config.Fset, r)}, nil
+	return []protocol.Location{toProtocolLocation(s.view.FileSet(), r)}, nil
 }
 
 func (s *server) Implementation(context.Context, *protocol.TextDocumentPositionParams) ([]protocol.Location, error) {

internal/lsp/source/definition.go

@@ -48,7 +48,7 @@ func Definition(ctx context.Context, v View, f File, pos token.Pos) (Range, erro
 	if err != nil {
 		return Range{}, err
 	}
-	return objToRange(v, fset, obj), nil
+	return objToRange(ctx, v, fset, obj), nil
 }
 
 func TypeDefinition(ctx context.Context, v View, f File, pos token.Pos) (Range, error) {
@@ -79,7 +79,7 @@ func TypeDefinition(ctx context.Context, v View, f File, pos token.Pos) (Range,
 	if err != nil {
 		return Range{}, err
 	}
-	return objToRange(v, fset, obj), nil
+	return objToRange(ctx, v, fset, obj), nil
 }
 
 func typeToObject(typ types.Type) (obj types.Object) {
@@ -137,7 +137,7 @@ func checkIdentifier(f *ast.File, pos token.Pos) (ident, error) {
 	return result, nil
 }
 
-func objToRange(v View, fset *token.FileSet, obj types.Object) Range {
+func objToRange(ctx context.Context, v View, fset *token.FileSet, obj types.Object) Range {
 	p := obj.Pos()
 	f := fset.File(p)
 	pos := f.Position(p)
@@ -148,7 +148,10 @@ func objToRange(v View, fset *token.FileSet, obj types.Object) Range {
 		// column to match its offset.
 		//
 		// TODO: If we parse from source, we will never need this hack.
-		f := v.GetFile(ToURI(pos.Filename))
+		f, err := v.GetFile(ctx, ToURI(pos.Filename))
+		if err != nil {
+			goto Return
+		}
 		tok, err := f.GetToken()
 		if err != nil {
 			goto Return

internal/lsp/source/diagnostics.go

@@ -20,7 +20,11 @@ type Diagnostic struct {
 	Message string
 }
 
-func Diagnostics(ctx context.Context, v View, f File) (map[string][]Diagnostic, error) {
+func Diagnostics(ctx context.Context, v View, uri URI) (map[string][]Diagnostic, error) {
+	f, err := v.GetFile(ctx, uri)
+	if err != nil {
+		return nil, err
+	}
 	pkg, err := f.GetPackage()
 	if err != nil {
 		return nil, err
@@ -49,7 +53,10 @@ func Diagnostics(ctx context.Context, v View, f File) (map[string][]Diagnostic,
 	}
 	for _, diag := range diags {
 		pos := errorPos(diag)
-		diagFile := v.GetFile(ToURI(pos.Filename))
+		diagFile, err := v.GetFile(ctx, ToURI(pos.Filename))
+		if err != nil {
+			continue
+		}
 		diagTok, err := diagFile.GetToken()
 		if err != nil {
 			continue

internal/lsp/source/view.go

@@ -5,6 +5,7 @@
 package source
 
 import (
+	"context"
 	"go/ast"
 	"go/token"
 
@@ -15,8 +16,9 @@ import (
 // package. The view provides access to files and their contents, so the source
 // package does not directly access the file system.
 type View interface {
-	// Consider adding an error to this method, if users require it.
+	GetFile(ctx context.Context, uri URI) (File, error)
-	GetFile(uri URI) File
+	SetContent(ctx context.Context, uri URI, content []byte) (View, error)
+	FileSet() *token.FileSet
 }
 
 // File represents a Go source file that has been type-checked. It is the input