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>

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 {
+// read is the internal part of GetContent. It assumes that the caller is
-	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
 // 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)
-	}
-}

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)
+	}
+}

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 }

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 }

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

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

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

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 {

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) {
+func Diagnostics(ctx context.Context, v View, f GoFile) (map[span.URI][]Diagnostic, error) {
-	f, err := v.GetFile(ctx, uri)
+	pkg := f.GetPackage(ctx)
-	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)
 	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)
 }
 

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

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 {