internal/lsp: remove source.FileContent

On FileHandle Read now just returns the data hash and error
This makes it more obvious that you should handle the error, rather than hiding
it all in a struct.
We also change the way we get and return content, the main source.File
constructs now hold a FileHandle that then updates on invalidation

Change-Id: I20be1b995355e948244342130eafec056df10081
Reviewed-on: https://go-review.googlesource.com/c/tools/+/180417
Reviewed-by: Rebecca Stambler <rstambler@golang.org>

internal/lsp/cache/external.go

@@ -41,16 +41,14 @@ func (h *nativeFileHandle) Identity() source.FileIdentity {
 	return h.identity
 }
 
-func (h *nativeFileHandle) Read(ctx context.Context) *source.FileContent {
+func (h *nativeFileHandle) Read(ctx context.Context) ([]byte, string, error) {
-	r := &source.FileContent{}
 	filename, err := h.identity.URI.Filename()
 	if err != nil {
-		r.Error = err
+		return nil, "", err
-		return r
 	}
-	r.Data, r.Error = ioutil.ReadFile(filename)
+	data, err := ioutil.ReadFile(filename)
-	if r.Error != nil {
+	if err != nil {
-		r.Hash = hashContents(r.Data)
+		return nil, "", err
 	}
-	return r
+	return data, hashContents(data), nil
 }

internal/lsp/cache/file.go

@@ -9,6 +9,7 @@ import (
 	"go/token"
 	"path/filepath"
 	"strings"
+	"sync"
 
 	"golang.org/x/tools/internal/lsp/source"
 	"golang.org/x/tools/internal/span"
@@ -29,8 +30,8 @@ type fileBase struct {
 	fname string
 
 	view     *view
-	fh    source.FileHandle
+	handleMu sync.Mutex
-	fc    *source.FileContent
+	handle   source.FileHandle
 	token    *token.File
 }
 
@@ -51,32 +52,16 @@ func (f *fileBase) View() source.View {
 	return f.view
 }
 
-// Content returns the contents of the file, reading it from file system if needed.
+// Content returns a handle for the contents of the file.
-func (f *fileBase) Content(ctx context.Context) *source.FileContent {
+func (f *fileBase) Handle(ctx context.Context) source.FileHandle {
-	f.view.mu.Lock()
+	f.handleMu.Lock()
-	defer f.view.mu.Unlock()
+	defer f.handleMu.Unlock()
-
+	if f.handle == nil {
-	f.read(ctx)
+		f.handle = f.view.Session().GetFile(f.URI())
-	return f.fc
+	}
+	return f.handle
 }
 
 func (f *fileBase) FileSet() *token.FileSet {
 	return f.view.Session().Cache().FileSet()
 }
-
-// read is the internal part of GetContent. It assumes that the caller is
-// holding the mutex of the file's view.
-func (f *fileBase) read(ctx context.Context) {
-	if err := ctx.Err(); err != nil {
-		f.fc = &source.FileContent{Error: err}
-		return
-	}
-	oldFH := f.fh
-	f.fh = f.view.Session().GetFile(f.URI())
-	// do we already have the right contents?
-	if f.fc != nil && f.fh.Identity() == oldFH.Identity() {
-		return
-	}
-	// update the contents
-	f.fc = f.fh.Read(ctx)
-}

internal/lsp/cache/load.go

@@ -90,11 +90,11 @@ func (v *view) reparseImports(ctx context.Context, f *goFile) bool {
 		return true
 	}
 	// Get file content in case we don't already have it.
-	f.read(ctx)
+	data, _, err := f.Handle(ctx).Read(ctx)
-	if f.fc.Error != nil {
+	if err != nil {
 		return true
 	}
-	parsed, _ := parser.ParseFile(f.FileSet(), f.filename(), f.fc.Data, parser.ImportsOnly)
+	parsed, _ := parser.ParseFile(f.FileSet(), f.filename(), data, parser.ImportsOnly)
 	if parsed == nil {
 		return true
 	}

internal/lsp/cache/parse.go

@@ -74,11 +74,11 @@ func (imp *importer) parseFiles(filenames []string, ignoreFuncBodies bool) ([]*a
 			}
 
 			// We don't have a cached AST for this file, so we read its content and parse it.
-			gof.read(imp.ctx)
+			data, _, err := gof.Handle(imp.ctx).Read(imp.ctx)
-			if gof.fc.Error != nil {
+			if err != nil {
 				return
 			}
-			src := gof.fc.Data
+			src := data
 			if src == nil {
 				parsed[i], errors[i] = nil, fmt.Errorf("no source for %v", filename)
 				return

internal/lsp/cache/session.go

@@ -40,7 +40,8 @@ type session struct {
 type overlay struct {
 	session *session
 	uri     span.URI
-	content source.FileContent
+	data    []byte
+	hash    string
 }
 
 func (s *session) Shutdown(ctx context.Context) {
@@ -213,10 +214,8 @@ func (s *session) SetOverlay(uri span.URI, data []byte) {
 	s.overlays[uri] = &overlay{
 		session: s,
 		uri:     uri,
-		content: source.FileContent{
+		data:    data,
-			Data: data,
+		hash:    hashContents(data),
-			Hash: hashContents(data),
-		},
 	}
 }
 
@@ -237,11 +236,8 @@ func (s *session) buildOverlay() map[string][]byte {
 
 	overlays := make(map[string][]byte)
 	for uri, overlay := range s.overlays {
-		if overlay.content.Error != nil {
-			continue
-		}
 		if filename, err := uri.Filename(); err == nil {
-			overlays[filename] = overlay.content.Data
+			overlays[filename] = overlay.data
 		}
 	}
 	return overlays
@@ -254,12 +250,12 @@ func (o *overlay) FileSystem() source.FileSystem {
 func (o *overlay) Identity() source.FileIdentity {
 	return source.FileIdentity{
 		URI:     o.uri,
-		Version: o.content.Hash,
+		Version: o.hash,
 	}
 }
 
-func (o *overlay) Read(ctx context.Context) *source.FileContent {
+func (o *overlay) Read(ctx context.Context) ([]byte, string, error) {
-	return &o.content
+	return o.data, o.hash, nil
 }
 
 type debugSession struct{ *session }
@@ -287,9 +283,9 @@ func (s debugSession) Files() []*debug.File {
 			seen[overlay.uri] = f
 			files = append(files, f)
 		}
-		f.Data = string(overlay.content.Data)
+		f.Data = string(overlay.data)
-		f.Error = overlay.content.Error
+		f.Error = nil
-		f.Hash = overlay.content.Hash
+		f.Hash = overlay.hash
 	}
 	sort.Slice(files, func(i int, j int) bool {
 		return files[i].URI < files[j].URI
@@ -301,13 +297,13 @@ func (s debugSession) File(hash string) *debug.File {
 	s.overlayMu.Lock()
 	defer s.overlayMu.Unlock()
 	for _, overlay := range s.overlays {
-		if overlay.content.Hash == hash {
+		if overlay.hash == hash {
 			return &debug.File{
 				Session: s,
 				URI:     overlay.uri,
-				Data:    string(overlay.content.Data),
+				Data:    string(overlay.data),
-				Error:   overlay.content.Error,
+				Error:   nil,
-				Hash:    overlay.content.Hash,
+				Hash:    overlay.hash,
 			}
 		}
 	}

internal/lsp/cache/view.go

@@ -233,7 +233,7 @@ func (f *goFile) invalidate() {
 	if f.pkg != nil {
 		f.view.remove(f.pkg.pkgPath, map[string]struct{}{})
 	}
-	f.fc = nil
+	f.handle = nil
 }
 
 // remove invalidates a package and its reverse dependencies in the view's

internal/lsp/source/diagnostics.go

@@ -202,8 +202,8 @@ func pointToSpan(ctx context.Context, v View, spn span.Span) span.Span {
 		v.Session().Logger().Errorf(ctx, "Could not find tokens for diagnostic: %v", spn.URI())
 		return spn
 	}
-	fc := diagFile.Content(ctx)
+	data, _, err := diagFile.Handle(ctx).Read(ctx)
-	if fc.Error != nil {
+	if err != nil {
 		v.Session().Logger().Errorf(ctx, "Could not find content for diagnostic: %v", spn.URI())
 		return spn
 	}
@@ -216,7 +216,7 @@ func pointToSpan(ctx context.Context, v View, spn span.Span) span.Span {
 	}
 	start := s.Start()
 	offset := start.Offset()
-	width := bytes.IndexAny(fc.Data[offset:], " \n,():;[]")
+	width := bytes.IndexAny(data[offset:], " \n,():;[]")
 	if width <= 0 {
 		return spn
 	}

internal/lsp/source/format.go

@@ -56,15 +56,15 @@ func hasParseErrors(errors []packages.Error) bool {
 
 // Imports formats a file using the goimports tool.
 func Imports(ctx context.Context, f GoFile, rng span.Range) ([]TextEdit, error) {
-	fc := f.Content(ctx)
+	data, _, err := f.Handle(ctx).Read(ctx)
-	if fc.Error != nil {
+	if err != nil {
-		return nil, fc.Error
+		return nil, err
 	}
 	tok := f.GetToken(ctx)
 	if tok == nil {
 		return nil, fmt.Errorf("no token file for %s", f.URI())
 	}
-	formatted, err := imports.Process(tok.Name(), fc.Data, nil)
+	formatted, err := imports.Process(tok.Name(), data, nil)
 	if err != nil {
 		return nil, err
 	}
@@ -72,12 +72,12 @@ func Imports(ctx context.Context, f GoFile, rng span.Range) ([]TextEdit, error)
 }
 
 func computeTextEdits(ctx context.Context, file File, formatted string) (edits []TextEdit) {
-	fc := file.Content(ctx)
+	data, _, err := file.Handle(ctx).Read(ctx)
-	if fc.Error != nil {
+	if err != nil {
-		file.View().Session().Logger().Errorf(ctx, "Cannot compute text edits: %v", fc.Error)
+		file.View().Session().Logger().Errorf(ctx, "Cannot compute text edits: %v", err)
 		return nil
 	}
-	u := diff.SplitLines(string(fc.Data))
+	u := diff.SplitLines(string(data))
 	f := diff.SplitLines(formatted)
 	return DiffToEdits(file.URI(), diff.Operations(u, f))
 }

internal/lsp/source/source_test.go

@@ -296,12 +296,12 @@ func (r *runner) Format(t *testing.T, data tests.Formats) {
 			continue
 		}
 		ops := source.EditsToDiff(edits)
-		fc := f.Content(ctx)
+		data, _, err := f.Handle(ctx).Read(ctx)
-		if fc.Error != nil {
+		if err != nil {
 			t.Error(err)
 			continue
 		}
-		got := strings.Join(diff.ApplyEdits(diff.SplitLines(string(fc.Data)), ops), "")
+		got := strings.Join(diff.ApplyEdits(diff.SplitLines(string(data)), ops), "")
 		if gofmted != got {
 			t.Errorf("format failed for %s, expected:\n%v\ngot:\n%v", filename, gofmted, got)
 		}
@@ -337,12 +337,12 @@ func (r *runner) Import(t *testing.T, data tests.Imports) {
 			continue
 		}
 		ops := source.EditsToDiff(edits)
-		fc := f.Content(ctx)
+		data, _, err := f.Handle(ctx).Read(ctx)
-		if fc.Error != nil {
+		if err != nil {
 			t.Error(err)
 			continue
 		}
-		got := strings.Join(diff.ApplyEdits(diff.SplitLines(string(fc.Data)), ops), "")
+		got := strings.Join(diff.ApplyEdits(diff.SplitLines(string(data)), ops), "")
 		if goimported != got {
 			t.Errorf("import failed for %s, expected:\n%v\ngot:\n%v", filename, goimported, got)
 		}

internal/lsp/source/view.go

@@ -18,14 +18,6 @@ import (
 	"golang.org/x/tools/internal/span"
 )
 
-// FileContent is returned from FileSystem implementation to represent the
-// contents of a file.
-type FileContent struct {
-	Data  []byte
-	Error error
-	Hash  string
-}
-
 // FileIdentity uniquely identifies a file at a version from a FileSystem.
 type FileIdentity struct {
 	URI     span.URI
@@ -35,14 +27,14 @@ type FileIdentity struct {
 // FileHandle represents a handle to a specific version of a single file from
 // a specific file system.
 type FileHandle interface {
-	// FileSystem returns the file system this handle was aquired from.
+	// FileSystem returns the file system this handle was acquired from.
 	FileSystem() FileSystem
 	// Return the Identity for the file.
 	Identity() FileIdentity
-	// Read reads the contents of a file and returns it.
+	// Read reads the contents of a file and returns it along with its hash
-	// If the file is not available, the returned FileContent will have no
+	// value.
-	// data and an error.
+	// If the file is not available, retruns a nil slice and an error.
-	Read(ctx context.Context) *FileContent
+	Read(ctx context.Context) ([]byte, string, error)
 }
 
 // FileSystem is the interface to something that provides file contents.
@@ -161,7 +153,7 @@ type View interface {
 type File interface {
 	URI() span.URI
 	View() View
-	Content(ctx context.Context) *FileContent
+	Handle(ctx context.Context) FileHandle
 	FileSet() *token.FileSet
 	GetToken(ctx context.Context) *token.File
 }

internal/lsp/text_synchronization.go

@@ -66,11 +66,10 @@ func (s *Server) applyChanges(ctx context.Context, params *protocol.DidChangeTex
 	}
 
 	uri := span.NewURI(params.TextDocument.URI)
-	fc := s.session.GetFile(uri).Read(ctx)
+	content, _, err := s.session.GetFile(uri).Read(ctx)
-	if fc.Error != nil {
+	if err != nil {
 		return "", jsonrpc2.NewErrorf(jsonrpc2.CodeInternalError, "file not found")
 	}
-	content := fc.Data
 	fset := s.session.Cache().FileSet()
 	filename, err := uri.Filename()
 	if err != nil {

internal/lsp/util.go

@@ -22,11 +22,11 @@ func getSourceFile(ctx context.Context, v source.View, uri span.URI) (source.Fil
 	if err != nil {
 		return nil, nil, err
 	}
-	fc := f.Content(ctx)
+	data, _, err := f.Handle(ctx).Read(ctx)
-	if fc.Error != nil {
+	if err != nil {
-		return nil, nil, fc.Error
+		return nil, nil, err
 	}
-	m := protocol.NewColumnMapper(f.URI(), filename, f.FileSet(), f.GetToken(ctx), fc.Data)
+	m := protocol.NewColumnMapper(f.URI(), filename, f.FileSet(), f.GetToken(ctx), data)
 
 	return f, m, nil
 }