From 12d73424210d5399ae74d7e94f5c464d70d02406 Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Wed, 29 May 2019 14:59:35 -0400 Subject: [PATCH] internal/lsp: refactor to separate pieces of type-checking Change-Id: Idab49286e59803e4ae0f749eb9f2990b611ea689 Reviewed-on: https://go-review.googlesource.com/c/tools/+/179439 Run-TryBot: Rebecca Stambler TryBot-Result: Gobot Gobot Reviewed-by: Ian Cottrell --- internal/lsp/cache/check.go | 152 --------------------------- internal/lsp/cache/file.go | 5 - internal/lsp/cache/gofile.go | 18 ++-- internal/lsp/cache/load.go | 160 +++++++++++++++++++++++++++++ internal/lsp/cache/pkg.go | 8 +- internal/lsp/source/diagnostics.go | 4 +- 6 files changed, 175 insertions(+), 172 deletions(-) create mode 100644 internal/lsp/cache/load.go diff --git a/internal/lsp/cache/check.go b/internal/lsp/cache/check.go index e6d8c27849..e6feadd2f9 100644 --- a/internal/lsp/cache/check.go +++ b/internal/lsp/cache/check.go @@ -8,7 +8,6 @@ import ( "context" "fmt" "go/ast" - "go/parser" "go/scanner" "go/token" "go/types" @@ -18,157 +17,6 @@ import ( "golang.org/x/tools/internal/span" ) -func (v *view) parse(ctx context.Context, f *goFile) ([]packages.Error, error) { - v.mcache.mu.Lock() - defer v.mcache.mu.Unlock() - - // Apply any queued-up content changes. - if err := v.applyContentChanges(ctx); err != nil { - return nil, err - } - - // If the package for the file has not been invalidated by the application - // of the pending changes, there is no need to continue. - if f.isPopulated() { - return nil, nil - } - // Check if the file's imports have changed. If they have, update the - // metadata by calling packages.Load. - if errs, err := v.checkMetadata(ctx, f); err != nil { - return errs, err - } - if f.meta == nil { - return nil, fmt.Errorf("no metadata found for %v", f.filename()) - } - imp := &importer{ - view: v, - seen: make(map[string]struct{}), - ctx: ctx, - fset: f.FileSet(), - } - // Start prefetching direct imports. - for importPath := range f.meta.children { - go imp.Import(importPath) - } - // Type-check package. - pkg, err := imp.getPkg(f.meta.pkgPath) - if pkg == nil || pkg.GetTypes() == nil { - return nil, err - } - - // If we still have not found the package for the file, something is wrong. - if f.pkg == nil { - return nil, fmt.Errorf("parse: no package found for %v", f.filename()) - } - return nil, nil -} - -func (v *view) checkMetadata(ctx context.Context, f *goFile) ([]packages.Error, error) { - if v.reparseImports(ctx, f, f.filename()) { - cfg := v.buildConfig() - pkgs, err := packages.Load(cfg, fmt.Sprintf("file=%s", f.filename())) - if len(pkgs) == 0 { - if err == nil { - err = fmt.Errorf("%s: no packages found", f.filename()) - } - // Return this error as a diagnostic to the user. - return []packages.Error{ - { - Msg: err.Error(), - Kind: packages.ListError, - }, - }, err - } - for _, pkg := range pkgs { - // If the package comes back with errors from `go list`, don't bother - // type-checking it. - if len(pkg.Errors) > 0 { - return pkg.Errors, fmt.Errorf("package %s has errors, skipping type-checking", pkg.PkgPath) - } - v.link(ctx, pkg.PkgPath, pkg, nil) - } - } - return nil, nil -} - -// reparseImports reparses a file's import declarations to determine if they -// have changed. -func (v *view) reparseImports(ctx context.Context, f *goFile, filename string) bool { - if f.meta == nil { - return true - } - // Get file content in case we don't already have it. - f.read(ctx) - if f.fc.Error != nil { - return true - } - parsed, _ := parser.ParseFile(f.FileSet(), filename, f.fc.Data, parser.ImportsOnly) - if parsed == nil { - return true - } - // If the package name has changed, re-run `go list`. - if f.meta.name != parsed.Name.Name { - return true - } - // If the package's imports have changed, re-run `go list`. - if len(f.imports) != len(parsed.Imports) { - return true - } - for i, importSpec := range f.imports { - if importSpec.Path.Value != f.imports[i].Path.Value { - return true - } - } - return false -} - -func (v *view) link(ctx context.Context, pkgPath string, pkg *packages.Package, parent *metadata) *metadata { - m, ok := v.mcache.packages[pkgPath] - if !ok { - m = &metadata{ - pkgPath: pkgPath, - id: pkg.ID, - typesSizes: pkg.TypesSizes, - parents: make(map[string]bool), - children: make(map[string]bool), - } - v.mcache.packages[pkgPath] = m - } - // Reset any field that could have changed across calls to packages.Load. - m.name = pkg.Name - m.files = pkg.CompiledGoFiles - for _, filename := range m.files { - if f, _ := v.getFile(span.FileURI(filename)); f != nil { - gof, ok := f.(*goFile) - if !ok { - v.Session().Logger().Errorf(ctx, "not a Go file: %v", f.URI()) - continue - } - gof.meta = m - } - } - // Connect the import graph. - if parent != nil { - m.parents[parent.pkgPath] = true - parent.children[pkgPath] = true - } - for importPath, importPkg := range pkg.Imports { - if _, ok := m.children[importPath]; !ok { - v.link(ctx, importPath, importPkg, m) - } - } - // Clear out any imports that have been removed. - for importPath := range m.children { - if _, ok := pkg.Imports[importPath]; !ok { - delete(m.children, importPath) - if child, ok := v.mcache.packages[importPath]; ok { - delete(child.parents, pkgPath) - } - } - } - return m -} - type importer struct { view *view diff --git a/internal/lsp/cache/file.go b/internal/lsp/cache/file.go index 36ec9e4eb5..d7a2b91fc4 100644 --- a/internal/lsp/cache/file.go +++ b/internal/lsp/cache/file.go @@ -87,8 +87,3 @@ func (f *fileBase) read(ctx context.Context) { // We don't know the content yet, so read it. f.fc = f.view.Session().ReadFile(f.URI()) } - -// isPopulated returns true if all of the computed fields of the file are set. -func (f *goFile) isPopulated() bool { - return f.ast != nil && f.token != nil && f.pkg != nil && f.meta != nil && f.imports != nil -} diff --git a/internal/lsp/cache/gofile.go b/internal/lsp/cache/gofile.go index aab3d82900..4a8c0d15fe 100644 --- a/internal/lsp/cache/gofile.go +++ b/internal/lsp/cache/gofile.go @@ -22,8 +22,8 @@ type goFile struct { 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 { + if f.isDirty() { + if _, err := f.view.loadParseTypecheck(ctx, f); err != nil { f.View().Session().Logger().Errorf(ctx, "unable to check package for %s: %v", f.URI(), err) return nil } @@ -35,8 +35,8 @@ 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 { + if f.isDirty() { + if _, err := f.view.loadParseTypecheck(ctx, f); err != nil { f.View().Session().Logger().Errorf(ctx, "unable to check package for %s: %v", f.URI(), err) return nil } @@ -48,8 +48,8 @@ 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 { + if f.isDirty() { + if errs, err := f.view.loadParseTypecheck(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. @@ -62,6 +62,12 @@ func (f *goFile) GetPackage(ctx context.Context) source.Package { return f.pkg } +// isDirty is true if the file needs to be type-checked. +// It assumes that the file's view's mutex is held by the caller. +func (f *goFile) isDirty() bool { + return f.meta == nil || f.imports == nil || f.token == nil || f.ast == nil || f.pkg == nil || len(f.view.contentChanges) > 0 +} + func (f *goFile) GetActiveReverseDeps(ctx context.Context) []source.GoFile { pkg := f.GetPackage(ctx) if pkg == nil { diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go new file mode 100644 index 0000000000..aebe5f233a --- /dev/null +++ b/internal/lsp/cache/load.go @@ -0,0 +1,160 @@ +package cache + +import ( + "context" + "fmt" + "go/parser" + + "golang.org/x/tools/go/packages" + "golang.org/x/tools/internal/span" +) + +func (v *view) loadParseTypecheck(ctx context.Context, f *goFile) ([]packages.Error, error) { + v.mcache.mu.Lock() + defer v.mcache.mu.Unlock() + + // Apply any queued-up content changes. + if err := v.applyContentChanges(ctx); err != nil { + return nil, err + } + + // If the package for the file has not been invalidated by the application + // of the pending changes, there is no need to continue. + if !f.isDirty() { + return nil, nil + } + // Check if the file's imports have changed. If they have, update the + // metadata by calling packages.Load. + if errs, err := v.checkMetadata(ctx, f); err != nil { + return errs, err + } + if f.meta == nil { + return nil, fmt.Errorf("no metadata found for %v", f.filename()) + } + imp := &importer{ + view: v, + seen: make(map[string]struct{}), + ctx: ctx, + fset: f.FileSet(), + } + // Start prefetching direct imports. + for importPath := range f.meta.children { + go imp.Import(importPath) + } + // Type-check package. + pkg, err := imp.getPkg(f.meta.pkgPath) + if pkg == nil || pkg.IsIllTyped() { + return nil, err + } + // If we still have not found the package for the file, something is wrong. + if f.pkg == nil { + return nil, fmt.Errorf("parse: no package found for %v", f.filename()) + } + return nil, nil +} + +func (v *view) checkMetadata(ctx context.Context, f *goFile) ([]packages.Error, error) { + if v.reparseImports(ctx, f, f.filename()) { + cfg := v.buildConfig() + pkgs, err := packages.Load(cfg, fmt.Sprintf("file=%s", f.filename())) + if len(pkgs) == 0 { + if err == nil { + err = fmt.Errorf("%s: no packages found", f.filename()) + } + // Return this error as a diagnostic to the user. + return []packages.Error{ + { + Msg: err.Error(), + Kind: packages.ListError, + }, + }, err + } + for _, pkg := range pkgs { + // If the package comes back with errors from `go list`, don't bother + // type-checking it. + if len(pkg.Errors) > 0 { + return pkg.Errors, fmt.Errorf("package %s has errors, skipping type-checking", pkg.PkgPath) + } + v.link(ctx, pkg.PkgPath, pkg, nil) + } + } + return nil, nil +} + +// reparseImports reparses a file's import declarations to determine if they +// have changed. +func (v *view) reparseImports(ctx context.Context, f *goFile, filename string) bool { + if f.meta == nil { + return true + } + // Get file content in case we don't already have it. + f.read(ctx) + if f.fc.Error != nil { + return true + } + parsed, _ := parser.ParseFile(f.FileSet(), filename, f.fc.Data, parser.ImportsOnly) + if parsed == nil { + return true + } + // If the package name has changed, re-run `go list`. + if f.meta.name != parsed.Name.Name { + return true + } + // If the package's imports have changed, re-run `go list`. + if len(f.imports) != len(parsed.Imports) { + return true + } + for i, importSpec := range f.imports { + if importSpec.Path.Value != f.imports[i].Path.Value { + return true + } + } + return false +} + +func (v *view) link(ctx context.Context, pkgPath string, pkg *packages.Package, parent *metadata) *metadata { + m, ok := v.mcache.packages[pkgPath] + if !ok { + m = &metadata{ + pkgPath: pkgPath, + id: pkg.ID, + typesSizes: pkg.TypesSizes, + parents: make(map[string]bool), + children: make(map[string]bool), + } + v.mcache.packages[pkgPath] = m + } + // Reset any field that could have changed across calls to packages.Load. + m.name = pkg.Name + m.files = pkg.CompiledGoFiles + for _, filename := range m.files { + if f, _ := v.getFile(span.FileURI(filename)); f != nil { + gof, ok := f.(*goFile) + if !ok { + v.Session().Logger().Errorf(ctx, "not a go file: %v", f.URI()) + continue + } + gof.meta = m + } + } + // Connect the import graph. + if parent != nil { + m.parents[parent.pkgPath] = true + parent.children[pkgPath] = true + } + for importPath, importPkg := range pkg.Imports { + if _, ok := m.children[importPath]; !ok { + v.link(ctx, importPath, importPkg, m) + } + } + // Clear out any imports that have been removed. + for importPath := range m.children { + if _, ok := pkg.Imports[importPath]; !ok { + delete(m.children, importPath) + if child, ok := v.mcache.packages[importPath]; ok { + delete(child.parents, pkgPath) + } + } + } + return m +} diff --git a/internal/lsp/cache/pkg.go b/internal/lsp/cache/pkg.go index 6fc66228a8..eeb1403306 100644 --- a/internal/lsp/cache/pkg.go +++ b/internal/lsp/cache/pkg.go @@ -161,11 +161,5 @@ func (pkg *pkg) IsIllTyped() bool { } func (pkg *pkg) GetImport(pkgPath string) source.Package { - imported := pkg.imports[pkgPath] - // Be careful not to return a nil pointer because that still satisfies the - // interface. - if imported != nil { - return imported - } - return nil + return pkg.imports[pkgPath] } diff --git a/internal/lsp/source/diagnostics.go b/internal/lsp/source/diagnostics.go index 922840f194..3f88a884c0 100644 --- a/internal/lsp/source/diagnostics.go +++ b/internal/lsp/source/diagnostics.go @@ -56,7 +56,7 @@ func Diagnostics(ctx context.Context, v View, f GoFile) (map[span.URI][]Diagnost if pkg == nil { return singleDiagnostic(f.URI(), "%s is not part of a package", f.URI()), nil } - // Prepare the reports we will send for this package. + // Prepare the reports we will send for the files in this package. reports := make(map[span.URI][]Diagnostic) for _, filename := range pkg.GetFilenames() { uri := span.FileURI(filename) @@ -66,7 +66,7 @@ func Diagnostics(ctx context.Context, v View, f GoFile) (map[span.URI][]Diagnost reports[uri] = []Diagnostic{} } - // Prepare reports for package errors + // Prepare any additional reports for the errors in this package. for _, pkgErr := range pkg.GetErrors() { reports[packageErrorSpan(pkgErr).URI()] = []Diagnostic{} }