From 3d17549cdc6bfc8f924b817a94ec54bec5596488 Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Thu, 23 May 2019 15:03:11 -0400 Subject: [PATCH] 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 TryBot-Result: Gobot Gobot Reviewed-by: Ian Cottrell --- internal/lsp/cache/file.go | 107 +-------------------------- internal/lsp/cache/gofile.go | 113 +++++++++++++++++++++++++++++ internal/lsp/cache/modfile.go | 16 ++++ internal/lsp/cache/sumfile.go | 16 ++++ internal/lsp/cache/view.go | 36 ++++++--- internal/lsp/diagnostics.go | 20 +++-- internal/lsp/lsp_test.go | 10 ++- internal/lsp/source/analysis.go | 6 +- internal/lsp/source/diagnostics.go | 20 ++--- internal/lsp/source/source_test.go | 6 +- internal/lsp/source/view.go | 8 ++ 11 files changed, 216 insertions(+), 142 deletions(-) create mode 100644 internal/lsp/cache/gofile.go create mode 100644 internal/lsp/cache/modfile.go create mode 100644 internal/lsp/cache/sumfile.go diff --git a/internal/lsp/cache/file.go b/internal/lsp/cache/file.go index d0683ebe..f71a15e5 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 00000000..aab3d829 --- /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 00000000..b912a6cc --- /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 00000000..03c11a00 --- /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 7908f0b6..96983b1c 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 4e4f7fc3..ffc7c82b 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 7d20a5d3..61156f25 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 f7021b20..46e64b41 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 b3d78689..922840f1 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 f89ec0c6..fa220918 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 b4380317..e00cff87 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 {