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>
diff --git a/internal/lsp/cache/file.go b/internal/lsp/cache/file.go
index d0683eb..f71a15e 100644
--- a/internal/lsp/cache/file.go
+++ b/internal/lsp/cache/file.go
@@ -6,7 +6,6 @@
import (
"context"
- "go/ast"
"go/token"
"path/filepath"
"strings"
@@ -18,6 +17,7 @@
// 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 @@
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 @@
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 *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 0000000..aab3d82
--- /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 0000000..b912a6c
--- /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 0000000..03c11a0
--- /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 7908f0b..96983b1 100644
--- a/internal/lsp/cache/view.go
+++ b/internal/lsp/cache/view.go
@@ -159,6 +159,13 @@
}
}
+// 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 @@
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 @@
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 4e4f7fc..ffc7c82 100644
--- a/internal/lsp/diagnostics.go
+++ b/internal/lsp/diagnostics.go
@@ -13,14 +13,24 @@
"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 @@
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 @@
// 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 7d20a5d..61156f2 100644
--- a/internal/lsp/lsp_test.go
+++ b/internal/lsp/lsp_test.go
@@ -61,7 +61,15 @@
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 f7021b2..46e64b4 100644
--- a/internal/lsp/source/analysis.go
+++ b/internal/lsp/source/analysis.go
@@ -80,7 +80,7 @@
}
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 @@
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 @@
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 b3d7868..922840f 100644
--- a/internal/lsp/source/diagnostics.go
+++ b/internal/lsp/source/diagnostics.go
@@ -51,19 +51,10 @@
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 @@
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 @@
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 f89ec0c..fa22091 100644
--- a/internal/lsp/source/source_test.go
+++ b/internal/lsp/source/source_test.go
@@ -52,7 +52,11 @@
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 b438031..e00cff8 100644
--- a/internal/lsp/source/view.go
+++ b/internal/lsp/source/view.go
@@ -156,6 +156,14 @@
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 {
