internal/lsp: add modfile, sumfile structs, require Go files for diagnostics

This change adds a stub modFile struct for use in the future. It also
moves the singleDiagnostic function out into the lsp package, so that
the source package does not make decisions about what to show to the
user as a diagnostic.

Fixes golang/go#32221

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

11 files changed, 216 insertions, 142 deletions

diff --git a/internal/lsp/cache/file.go b/internal/lsp/cache/file.go
index d0683ebe8..f71a15e57 100644
--- a/internal/lsp/cache/file.go
+++ b/internal/lsp/cache/file.go
@@ -6,7 +6,6 @@ package cache
 
 import (
 	"context"
-	"go/ast"
 	"go/token"
 	"path/filepath"
 	"strings"
@@ -18,6 +17,7 @@ import (
 // viewFile extends source.File with helper methods for the view package.
 type viewFile interface {
 	source.File
+
 	filename() string
 	addURI(uri span.URI) int
 }
@@ -33,16 +33,6 @@ type fileBase struct {
 	token *token.File
 }
 
-// goFile holds all the information we know about a go file.
-type goFile struct {
-	fileBase
-
-	ast     *ast.File
-	pkg     *pkg
-	meta    *metadata
-	imports []*ast.ImportSpec
-}
-
 func basename(filename string) string {
 	return strings.ToLower(filepath.Base(filename))
 }
@@ -72,50 +62,7 @@ func (f *fileBase) FileSet() *token.FileSet {
 	return f.view.Session().Cache().FileSet()
 }
 
-func (f *goFile) GetToken(ctx context.Context) *token.File {
-	f.view.mu.Lock()
-	defer f.view.mu.Unlock()
-	if f.token == nil || len(f.view.contentChanges) > 0 {
-		if _, err := f.view.parse(ctx, f); err != nil {
-			f.View().Session().Logger().Errorf(ctx, "unable to check package for %s: %v", f.URI(), err)
-			return nil
-		}
-	}
-	return f.token
-}
-
-func (f *goFile) GetAST(ctx context.Context) *ast.File {
-	f.view.mu.Lock()
-	defer f.view.mu.Unlock()
-
-	if f.ast == nil || len(f.view.contentChanges) > 0 {
-		if _, err := f.view.parse(ctx, f); err != nil {
-			f.View().Session().Logger().Errorf(ctx, "unable to check package for %s: %v", f.URI(), err)
-			return nil
-		}
-	}
-	return f.ast
-}
-
-func (f *goFile) GetPackage(ctx context.Context) source.Package {
-	f.view.mu.Lock()
-	defer f.view.mu.Unlock()
-
-	if f.pkg == nil || len(f.view.contentChanges) > 0 {
-		if errs, err := f.view.parse(ctx, f); err != nil {
-			f.View().Session().Logger().Errorf(ctx, "unable to check package for %s: %v", f.URI(), err)
-
-			// Create diagnostics for errors if we are able to.
-			if len(errs) > 0 {
-				return &pkg{errors: errs}
-			}
-			return nil
-		}
-	}
-	return f.pkg
-}
-
-// read is the internal part of Content. It assumes that the caller is
+// 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 {
@@ -144,53 +91,3 @@ func (f *fileBase) read(ctx context.Context) {
 func (f *goFile) isPopulated() bool {
 	return f.ast != nil && f.token != nil && f.pkg != nil && f.meta != nil && f.imports != nil
 }
-
-func (f *goFile) GetActiveReverseDeps(ctx context.Context) []source.GoFile {
-	pkg := f.GetPackage(ctx)
-	if pkg == nil {
-		return nil
-	}
-
-	f.view.mu.Lock()
-	defer f.view.mu.Unlock()
-
-	f.view.mcache.mu.Lock()
-	defer f.view.mcache.mu.Unlock()
-
-	seen := make(map[string]struct{}) // visited packages
-	results := make(map[*goFile]struct{})
-	f.view.reverseDeps(ctx, seen, results, pkg.PkgPath())
-
-	var files []source.GoFile
-	for rd := range results {
-		if rd == nil {
-			continue
-		}
-		// Don't return any of the active files in this package.
-		if rd.pkg != nil && rd.pkg == pkg {
-			continue
-		}
-		files = append(files, rd)
-	}
-	return files
-}
-
-func (v *view) reverseDeps(ctx context.Context, seen map[string]struct{}, results map[*goFile]struct{}, pkgPath string) {
-	if _, ok := seen[pkgPath]; ok {
-		return
-	}
-	seen[pkgPath] = struct{}{}
-	m, ok := v.mcache.packages[pkgPath]
-	if !ok {
-		return
-	}
-	for _, filename := range m.files {
-		uri := span.FileURI(filename)
-		if f, err := v.getFile(uri); err == nil && v.session.IsOpen(uri) {
-			results[f.(*goFile)] = struct{}{}
-		}
-	}
-	for parentPkgPath := range m.parents {
-		v.reverseDeps(ctx, seen, results, parentPkgPath)
-	}
-}
diff --git a/internal/lsp/cache/gofile.go b/internal/lsp/cache/gofile.go
new file mode 100644
index 000000000..aab3d8290
--- /dev/null
+++ b/internal/lsp/cache/gofile.go
@@ -0,0 +1,113 @@
+package cache
+
+import (
+	"context"
+	"go/ast"
+	"go/token"
+
+	"golang.org/x/tools/internal/lsp/source"
+	"golang.org/x/tools/internal/span"
+)
+
+// goFile holds all of the information we know about a Go file.
+type goFile struct {
+	fileBase
+
+	ast     *ast.File
+	pkg     *pkg
+	meta    *metadata
+	imports []*ast.ImportSpec
+}
+
+func (f *goFile) GetToken(ctx context.Context) *token.File {
+	f.view.mu.Lock()
+	defer f.view.mu.Unlock()
+	if f.token == nil || len(f.view.contentChanges) > 0 {
+		if _, err := f.view.parse(ctx, f); err != nil {
+			f.View().Session().Logger().Errorf(ctx, "unable to check package for %s: %v", f.URI(), err)
+			return nil
+		}
+	}
+	return f.token
+}
+
+func (f *goFile) GetAST(ctx context.Context) *ast.File {
+	f.view.mu.Lock()
+	defer f.view.mu.Unlock()
+
+	if f.ast == nil || len(f.view.contentChanges) > 0 {
+		if _, err := f.view.parse(ctx, f); err != nil {
+			f.View().Session().Logger().Errorf(ctx, "unable to check package for %s: %v", f.URI(), err)
+			return nil
+		}
+	}
+	return f.ast
+}
+
+func (f *goFile) GetPackage(ctx context.Context) source.Package {
+	f.view.mu.Lock()
+	defer f.view.mu.Unlock()
+
+	if f.pkg == nil || len(f.view.contentChanges) > 0 {
+		if errs, err := f.view.parse(ctx, f); err != nil {
+			f.View().Session().Logger().Errorf(ctx, "unable to check package for %s: %v", f.URI(), err)
+
+			// Create diagnostics for errors if we are able to.
+			if len(errs) > 0 {
+				return &pkg{errors: errs}
+			}
+			return nil
+		}
+	}
+	return f.pkg
+}
+
+func (f *goFile) GetActiveReverseDeps(ctx context.Context) []source.GoFile {
+	pkg := f.GetPackage(ctx)
+	if pkg == nil {
+		return nil
+	}
+
+	f.view.mu.Lock()
+	defer f.view.mu.Unlock()
+
+	f.view.mcache.mu.Lock()
+	defer f.view.mcache.mu.Unlock()
+
+	seen := make(map[string]struct{}) // visited packages
+	results := make(map[*goFile]struct{})
+	f.view.reverseDeps(ctx, seen, results, pkg.PkgPath())
+
+	var files []source.GoFile
+	for rd := range results {
+		if rd == nil {
+			continue
+		}
+		// Don't return any of the active files in this package.
+		if rd.pkg != nil && rd.pkg == pkg {
+			continue
+		}
+		files = append(files, rd)
+	}
+	return files
+}
+
+func (v *view) reverseDeps(ctx context.Context, seen map[string]struct{}, results map[*goFile]struct{}, pkgPath string) {
+	if _, ok := seen[pkgPath]; ok {
+		return
+	}
+	seen[pkgPath] = struct{}{}
+	m, ok := v.mcache.packages[pkgPath]
+	if !ok {
+		return
+	}
+	for _, filename := range m.files {
+		uri := span.FileURI(filename)
+		if f, err := v.getFile(uri); err == nil && v.session.IsOpen(uri) {
+			results[f.(*goFile)] = struct{}{}
+		}
+	}
+	for parentPkgPath := range m.parents {
+		v.reverseDeps(ctx, seen, results, parentPkgPath)
+	}
+}
diff --git a/internal/lsp/cache/modfile.go b/internal/lsp/cache/modfile.go
new file mode 100644
index 000000000..b912a6cc2
--- /dev/null
+++ b/internal/lsp/cache/modfile.go
@@ -0,0 +1,16 @@
+package cache
+
+import (
+	"context"
+	"go/token"
+)
+
+// modFile holds all of the information we know about a mod file.
+type modFile struct {
+	fileBase
+}
+
+func (*modFile) GetToken(context.Context) *token.File { return nil }
+func (*modFile) setContent(content []byte)            {}
+func (*modFile) filename() string                     { return "" }
+func (*modFile) isActive() bool                       { return false }
diff --git a/internal/lsp/cache/sumfile.go b/internal/lsp/cache/sumfile.go
new file mode 100644
index 000000000..03c11a00a
--- /dev/null
+++ b/internal/lsp/cache/sumfile.go
@@ -0,0 +1,16 @@
+package cache
+
+import (
+	"context"
+	"go/token"
+)
+
+// sumFile holds all of the information we know about a sum file.
+type sumFile struct {
+	fileBase
+}
+
+func (*sumFile) GetToken(context.Context) *token.File { return nil }
+func (*sumFile) setContent(content []byte)            {}
+func (*sumFile) filename() string                     { return "" }
+func (*sumFile) isActive() bool                       { return false }
diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go
index 7908f0b62..96983b1ce 100644
--- a/internal/lsp/cache/view.go
+++ b/internal/lsp/cache/view.go
@@ -159,6 +159,13 @@ func (v *view) shutdown(context.Context) {
 	}
 }
 
+// Ignore checks if the given URI is a URI we ignore.
+// As of right now, we only ignore files in the "builtin" package.
+func (v *view) Ignore(uri span.URI) bool {
+	_, ok := v.ignoredURIs[uri]
+	return ok
+}
+
 func (v *view) BackgroundContext() context.Context {
 	v.mu.Lock()
 	defer v.mu.Unlock()
@@ -306,7 +313,7 @@ func (v *view) getFile(uri span.URI) (viewFile, error) {
 	if err != nil {
 		return nil, err
 	}
-	var f *goFile
+	var f viewFile
 	switch ext := filepath.Ext(filename); ext {
 	case ".go":
 		f = &goFile{
@@ -315,25 +322,30 @@ func (v *view) getFile(uri span.URI) (viewFile, error) {
 				fname: filename,
 			},
 		}
+		v.session.filesWatchMap.Watch(uri, func() {
+			f.(*goFile).invalidate()
+		})
 	case ".mod":
-		return nil, fmt.Errorf("mod files are not yet supported")
+		f = &modFile{
+			fileBase: fileBase{
+				view:  v,
+				fname: filename,
+			},
+		}
+	case ".sum":
+		f = &sumFile{
+			fileBase: fileBase{
+				view:  v,
+				fname: filename,
+			},
+		}
 	default:
 		return nil, fmt.Errorf("unsupported file extension: %s", ext)
 	}
-	v.session.filesWatchMap.Watch(uri, func() {
-		f.invalidate()
-	})
 	v.mapFile(uri, f)
 	return f, nil
 }
 
-// Ignore checks if the given URI is a URI we ignore.
-// As of right now, we only ignore files in the "builtin" package.
-func (v *view) Ignore(uri span.URI) bool {
-	_, ok := v.ignoredURIs[uri]
-	return ok
-}
-
 // findFile checks the cache for any file matching the given uri.
 //
 // An error is only returned for an irreparable failure, for example, if the
diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go
index 4e4f7fc34..ffc7c82ba 100644
--- a/internal/lsp/diagnostics.go
+++ b/internal/lsp/diagnostics.go
@@ -13,14 +13,24 @@ import (
 	"golang.org/x/tools/internal/span"
 )
 
-func (s *Server) Diagnostics(ctx context.Context, view source.View, uri span.URI) {
+func (s *Server) Diagnostics(ctx context.Context, v source.View, uri span.URI) {
 	if ctx.Err() != nil {
 		s.session.Logger().Errorf(ctx, "canceling diagnostics for %s: %v", uri, ctx.Err())
 		return
 	}
-	reports, err := source.Diagnostics(ctx, view, uri)
+	f, err := v.GetFile(ctx, uri)
 	if err != nil {
-		s.session.Logger().Errorf(ctx, "failed to compute diagnostics for %s: %v", uri, err)
+		s.session.Logger().Errorf(ctx, "no file for %s: %v", uri, err)
+		return
+	}
+	// For non-Go files, don't return any diagnostics.
+	gof, ok := f.(source.GoFile)
+	if !ok {
+		return
+	}
+	reports, err := source.Diagnostics(ctx, v, gof)
+	if err != nil {
+		s.session.Logger().Errorf(ctx, "failed to compute diagnostics for %s: %v", gof.URI(), err)
 		return
 	}
 
@@ -28,7 +38,7 @@ func (s *Server) Diagnostics(ctx context.Context, view source.View, uri span.URI
 	defer s.undeliveredMu.Unlock()
 
 	for uri, diagnostics := range reports {
-		if err := s.publishDiagnostics(ctx, view, uri, diagnostics); err != nil {
+		if err := s.publishDiagnostics(ctx, v, uri, diagnostics); err != nil {
 			if s.undelivered == nil {
 				s.undelivered = make(map[span.URI][]source.Diagnostic)
 			}
@@ -41,7 +51,7 @@ func (s *Server) Diagnostics(ctx context.Context, view source.View, uri span.URI
 	// Anytime we compute diagnostics, make sure to also send along any
 	// undelivered ones (only for remaining URIs).
 	for uri, diagnostics := range s.undelivered {
-		err := s.publishDiagnostics(ctx, view, uri, diagnostics)
+		err := s.publishDiagnostics(ctx, v, uri, diagnostics)
 		if err != nil {
 			s.session.Logger().Errorf(ctx, "failed to deliver diagnostic for %s: %v", uri, err)
 		}
diff --git a/internal/lsp/lsp_test.go b/internal/lsp/lsp_test.go
index 7d20a5d30..61156f25b 100644
--- a/internal/lsp/lsp_test.go
+++ b/internal/lsp/lsp_test.go
@@ -61,7 +61,15 @@ func testLSP(t *testing.T, exporter packagestest.Exporter) {
 func (r *runner) Diagnostics(t *testing.T, data tests.Diagnostics) {
 	v := r.server.session.View(viewName)
 	for uri, want := range data {
-		results, err := source.Diagnostics(context.Background(), v, uri)
+		f, err := v.GetFile(context.Background(), uri)
+		if err != nil {
+			t.Fatalf("no file for %s: %v", f, err)
+		}
+		gof, ok := f.(source.GoFile)
+		if !ok {
+			t.Fatalf("%s is not a Go file: %v", uri, err)
+		}
+		results, err := source.Diagnostics(context.Background(), v, gof)
 		if err != nil {
 			t.Fatal(err)
 		}
diff --git a/internal/lsp/source/analysis.go b/internal/lsp/source/analysis.go
index f7021b20b..46e64b412 100644
--- a/internal/lsp/source/analysis.go
+++ b/internal/lsp/source/analysis.go
@@ -80,7 +80,7 @@ type packageFactKey struct {
 }
 
 func (act *Action) String() string {
-	return fmt.Sprintf("%s@%s", act.Analyzer, act.Pkg)
+	return fmt.Sprintf("%s@%s", act.Analyzer, act.Pkg.PkgPath())
 }
 
 func execAll(ctx context.Context, fset *token.FileSet, actions []*Action) error {
@@ -112,7 +112,7 @@ func (act *Action) execOnce(ctx context.Context, fset *token.FileSet) error {
 	var failed []string
 	for _, dep := range act.Deps {
 		if dep.err != nil {
-			failed = append(failed, dep.String())
+			failed = append(failed, fmt.Sprintf("%s: %v", dep.String(), dep.err))
 		}
 	}
 	if failed != nil {
@@ -159,7 +159,7 @@ func (act *Action) execOnce(ctx context.Context, fset *token.FileSet) error {
 	act.pass = pass
 
 	if len(act.Pkg.GetErrors()) > 0 && !pass.Analyzer.RunDespiteErrors {
-		act.err = fmt.Errorf("analysis skipped due to errors in package")
+		act.err = fmt.Errorf("analysis skipped due to errors in package: %v", act.Pkg.GetErrors())
 	} else {
 		act.result, act.err = pass.Analyzer.Run(pass)
 		if act.err == nil {
diff --git a/internal/lsp/source/diagnostics.go b/internal/lsp/source/diagnostics.go
index b3d78689a..922840f19 100644
--- a/internal/lsp/source/diagnostics.go
+++ b/internal/lsp/source/diagnostics.go
@@ -51,19 +51,10 @@ const (
 	SeverityError
 )
 
-func Diagnostics(ctx context.Context, v View, uri span.URI) (map[span.URI][]Diagnostic, error) {
-	f, err := v.GetFile(ctx, uri)
-	if err != nil {
-		return singleDiagnostic(uri, "no file found for %s", uri), nil
-	}
-	// For non-Go files, don't return any diagnostics.
-	gof, ok := f.(GoFile)
-	if !ok {
-		return nil, nil
-	}
-	pkg := gof.GetPackage(ctx)
+func Diagnostics(ctx context.Context, v View, f GoFile) (map[span.URI][]Diagnostic, error) {
+	pkg := f.GetPackage(ctx)
 	if pkg == nil {
-		return singleDiagnostic(uri, "%s is not part of a package", uri), nil
+		return singleDiagnostic(f.URI(), "%s is not part of a package", f.URI()), nil
 	}
 	// Prepare the reports we will send for this package.
 	reports := make(map[span.URI][]Diagnostic)
@@ -84,11 +75,11 @@ func Diagnostics(ctx context.Context, v View, uri span.URI) (map[span.URI][]Diag
 	if !diagnostics(ctx, v, pkg, reports) {
 		// If we don't have any list, parse, or type errors, run analyses.
 		if err := analyses(ctx, v, pkg, reports); err != nil {
-			return singleDiagnostic(uri, "failed to run analyses for %s: %v", uri, err), nil
+			v.Session().Logger().Errorf(ctx, "failed to run analyses for %s: %v", f.URI(), err)
 		}
 	}
 	// Updates to the diagnostics for this package may need to be propagated.
-	for _, f := range gof.GetActiveReverseDeps(ctx) {
+	for _, f := range f.GetActiveReverseDeps(ctx) {
 		pkg := f.GetPackage(ctx)
 		if pkg == nil {
 			continue
@@ -184,7 +175,6 @@ func packageErrorSpan(pkgErr packages.Error) span.Span {
 	if pkgErr.Pos == "" {
 		return parseDiagnosticMessage(pkgErr.Msg)
 	}
-
 	return span.Parse(pkgErr.Pos)
 }
 
diff --git a/internal/lsp/source/source_test.go b/internal/lsp/source/source_test.go
index f89ec0c63..fa2209188 100644
--- a/internal/lsp/source/source_test.go
+++ b/internal/lsp/source/source_test.go
@@ -52,7 +52,11 @@ func testSource(t *testing.T, exporter packagestest.Exporter) {
 
 func (r *runner) Diagnostics(t *testing.T, data tests.Diagnostics) {
 	for uri, want := range data {
-		results, err := source.Diagnostics(context.Background(), r.view, uri)
+		f, err := r.view.GetFile(context.Background(), uri)
+		if err != nil {
+			t.Fatal(err)
+		}
+		results, err := source.Diagnostics(context.Background(), r.view, f.(source.GoFile))
 		if err != nil {
 			t.Fatal(err)
 		}
diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go
index b43803172..e00cff87a 100644
--- a/internal/lsp/source/view.go
+++ b/internal/lsp/source/view.go
@@ -156,6 +156,14 @@ type GoFile interface {
 	GetActiveReverseDeps(ctx context.Context) []GoFile
 }
 
+type ModFile interface {
+	File
+}
+
+type SumFile interface {
+	File
+}
+
 // Package represents a Go package that has been type-checked. It maintains
 // only the relevant fields of a *go/packages.Package.
 type Package interface {