From 8110780cfa068905824bca5103f95cac20c79fc8 Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Mon, 11 Mar 2019 17:14:55 -0400 Subject: [PATCH] internal/lsp: add correct handling for circular imports This change brings back handling for circular imports, which was removed because I originally thought that go/packages would handle that. However, since we are type-checking from source, we still end up having to deal with that. Additionally, we propagate the errors of type-checking to the diagnostics so that the user can actually see some of the problems. Change-Id: I0139bcaae461f1bcaf95706532bc5026f2430101 Reviewed-on: https://go-review.googlesource.com/c/tools/+/166882 Run-TryBot: Rebecca Stambler TryBot-Result: Gobot Gobot Reviewed-by: Ian Cottrell --- internal/lsp/cache/check.go | 104 ++++++++++++++++---------- internal/lsp/cache/file.go | 11 ++- internal/lsp/cache/pkg.go | 4 + internal/lsp/cache/view.go | 1 + internal/lsp/source/completion.go | 3 + internal/lsp/source/definition.go | 3 + internal/lsp/source/diagnostics.go | 9 ++- internal/lsp/source/signature_help.go | 3 + internal/lsp/source/view.go | 1 + 9 files changed, 94 insertions(+), 45 deletions(-) diff --git a/internal/lsp/cache/check.go b/internal/lsp/cache/check.go index 7b58c6309f5..2082b47d1df 100644 --- a/internal/lsp/cache/check.go +++ b/internal/lsp/cache/check.go @@ -19,51 +19,55 @@ import ( "golang.org/x/tools/internal/span" ) -func (v *View) parse(ctx context.Context, uri span.URI) error { +func (v *View) parse(ctx context.Context, uri span.URI) ([]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 err + return nil, err } f := v.files[uri] // This should never happen. if f == nil { - return fmt.Errorf("no file for %v", uri) + return nil, fmt.Errorf("no file for %v", uri) } // 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 + return nil, nil } // Check if the file's imports have changed. If they have, update the // metadata by calling packages.Load. - if err := v.checkMetadata(ctx, f); err != nil { - return err + if errs, err := v.checkMetadata(ctx, f); err != nil { + return errs, err } if f.meta == nil { - return fmt.Errorf("no metadata found for %v", uri) + return nil, fmt.Errorf("no metadata found for %v", uri) + } + imp := &importer{ + view: v, + circular: make(map[string]struct{}), } // Start prefetching direct imports. for importPath := range f.meta.children { - go v.Import(importPath) + go imp.Import(importPath) } // Type-check package. - pkg, err := v.typeCheck(f.meta.pkgPath) + pkg, err := imp.typeCheck(f.meta.pkgPath) if pkg == nil || pkg.GetTypes() == nil { - return err + return nil, err } // Add every file in this package to our cache. v.cachePackage(pkg) // If we still have not found the package for the file, something is wrong. if f.pkg == nil { - return fmt.Errorf("no package found for %v", uri) + return nil, fmt.Errorf("no package found for %v", uri) } - return nil + return nil, nil } func (v *View) cachePackage(pkg *Package) { @@ -87,10 +91,10 @@ func (v *View) cachePackage(pkg *Package) { } } -func (v *View) checkMetadata(ctx context.Context, f *File) error { +func (v *View) checkMetadata(ctx context.Context, f *File) ([]packages.Error, error) { filename, err := f.uri.Filename() if err != nil { - return err + return nil, err } if v.reparseImports(ctx, f, filename) { cfg := v.Config @@ -100,21 +104,18 @@ func (v *View) checkMetadata(ctx context.Context, f *File) error { if err == nil { err = fmt.Errorf("no packages found for %s", filename) } - return err + return nil, err } for _, pkg := range pkgs { // If the package comes back with errors from `go list`, don't bother // type-checking it. - for _, err := range pkg.Errors { - switch err.Kind { - case packages.UnknownError, packages.ListError: - return err - } + if len(pkg.Errors) > 0 { + return pkg.Errors, fmt.Errorf("package %s has errors, skipping type-checking", pkg.PkgPath) } v.link(pkg.PkgPath, pkg, nil) } } - return nil + return nil, nil } // reparseImports reparses a file's import declarations to determine if they @@ -181,23 +182,34 @@ func (v *View) link(pkgPath string, pkg *packages.Package, parent *metadata) *me return m } -func (v *View) Import(pkgPath string) (*types.Package, error) { - v.pcache.mu.Lock() - e, ok := v.pcache.packages[pkgPath] +type importer struct { + view *View + + // circular maintains the set of previously imported packages. + // If we have seen a package that is already in this map, we have a circular import. + circular map[string]struct{} +} + +func (imp *importer) Import(pkgPath string) (*types.Package, error) { + if _, ok := imp.circular[pkgPath]; ok { + return nil, fmt.Errorf("circular import detected") + } + imp.view.pcache.mu.Lock() + e, ok := imp.view.pcache.packages[pkgPath] if ok { // cache hit - v.pcache.mu.Unlock() + imp.view.pcache.mu.Unlock() // wait for entry to become ready <-e.ready } else { // cache miss e = &entry{ready: make(chan struct{})} - v.pcache.packages[pkgPath] = e - v.pcache.mu.Unlock() + imp.view.pcache.packages[pkgPath] = e + imp.view.pcache.mu.Unlock() // This goroutine becomes responsible for populating // the entry and broadcasting its readiness. - e.pkg, e.err = v.typeCheck(pkgPath) + e.pkg, e.err = imp.typeCheck(pkgPath) close(e.ready) } if e.err != nil { @@ -206,8 +218,8 @@ func (v *View) Import(pkgPath string) (*types.Package, error) { return e.pkg.types, nil } -func (v *View) typeCheck(pkgPath string) (*Package, error) { - meta, ok := v.mcache.packages[pkgPath] +func (imp *importer) typeCheck(pkgPath string) (*Package, error) { + meta, ok := imp.view.mcache.packages[pkgPath] if !ok { return nil, fmt.Errorf("no metadata for %v", pkgPath) } @@ -235,28 +247,36 @@ func (v *View) typeCheck(pkgPath string) (*Package, error) { analyses: make(map[*analysis.Analyzer]*analysisEntry), } appendError := func(err error) { - v.appendPkgError(pkg, err) + imp.view.appendPkgError(pkg, err) } - files, errs := v.parseFiles(meta.files) + files, errs := imp.view.parseFiles(meta.files) for _, err := range errs { appendError(err) } pkg.syntax = files + + // Handle circular imports by copying previously seen imports. + newCircular := copySet(imp.circular) + newCircular[pkgPath] = struct{}{} + cfg := &types.Config{ - Error: appendError, - Importer: v, + Error: appendError, + Importer: &importer{ + view: imp.view, + circular: newCircular, + }, } - check := types.NewChecker(cfg, v.Config.Fset, pkg.types, pkg.typesInfo) + check := types.NewChecker(cfg, imp.view.Config.Fset, pkg.types, pkg.typesInfo) check.Files(pkg.syntax) // Set imports of package to correspond to cached packages. // We lock the package cache, but we shouldn't get any inconsistencies // because we are still holding the lock on the view. - v.pcache.mu.Lock() - defer v.pcache.mu.Unlock() + imp.view.pcache.mu.Lock() + defer imp.view.pcache.mu.Unlock() for importPath := range meta.children { - if importEntry, ok := v.pcache.packages[importPath]; ok { + if importEntry, ok := imp.view.pcache.packages[importPath]; ok { pkg.imports[importPath] = importEntry.pkg } } @@ -264,6 +284,14 @@ func (v *View) typeCheck(pkgPath string) (*Package, error) { return pkg, nil } +func copySet(m map[string]struct{}) map[string]struct{} { + result := make(map[string]struct{}) + for k, v := range m { + result[k] = v + } + return result +} + func (v *View) appendPkgError(pkg *Package, err error) { if err == nil { return diff --git a/internal/lsp/cache/file.go b/internal/lsp/cache/file.go index 8b3bceb838b..3b1269e3b33 100644 --- a/internal/lsp/cache/file.go +++ b/internal/lsp/cache/file.go @@ -52,7 +52,7 @@ func (f *File) GetToken(ctx context.Context) *token.File { defer f.view.mu.Unlock() if f.token == nil || len(f.view.contentChanges) > 0 { - if err := f.view.parse(ctx, f.uri); err != nil { + if _, err := f.view.parse(ctx, f.uri); err != nil { return nil } } @@ -64,7 +64,7 @@ func (f *File) GetAST(ctx context.Context) *ast.File { defer f.view.mu.Unlock() if f.ast == nil || len(f.view.contentChanges) > 0 { - if err := f.view.parse(ctx, f.uri); err != nil { + if _, err := f.view.parse(ctx, f.uri); err != nil { return nil } } @@ -76,7 +76,12 @@ func (f *File) GetPackage(ctx context.Context) source.Package { defer f.view.mu.Unlock() if f.pkg == nil || len(f.view.contentChanges) > 0 { - if err := f.view.parse(ctx, f.uri); err != nil { + errs, err := f.view.parse(ctx, f.uri) + if err != nil { + // Create diagnostics for errors if we are able to. + if len(errs) > 0 { + return &Package{errors: errs} + } return nil } } diff --git a/internal/lsp/cache/pkg.go b/internal/lsp/cache/pkg.go index 6fa35cc0540..0a3c660b788 100644 --- a/internal/lsp/cache/pkg.go +++ b/internal/lsp/cache/pkg.go @@ -117,3 +117,7 @@ func (pkg *Package) GetTypes() *types.Package { func (pkg *Package) GetTypesInfo() *types.Info { return pkg.typesInfo } + +func (pkg *Package) IsIllTyped() bool { + return pkg.types == nil && pkg.typesInfo == nil +} diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index 1817bd1a904..ba7b221a911 100644 --- a/internal/lsp/cache/view.go +++ b/internal/lsp/cache/view.go @@ -43,6 +43,7 @@ type View struct { // mcache caches metadata for the packages of the opened files in a view. mcache *metadataCache + // pcache caches type information for the packages of the opened files in a view. pcache *packageCache } diff --git a/internal/lsp/source/completion.go b/internal/lsp/source/completion.go index b776e5d9656..86c1b6d6464 100644 --- a/internal/lsp/source/completion.go +++ b/internal/lsp/source/completion.go @@ -49,6 +49,9 @@ type finder func(types.Object, float64, []CompletionItem) []CompletionItem func Completion(ctx context.Context, f File, pos token.Pos) (items []CompletionItem, prefix string, err error) { file := f.GetAST(ctx) pkg := f.GetPackage(ctx) + if pkg.IsIllTyped() { + return nil, "", fmt.Errorf("package for %s is ill typed", f.URI()) + } path, _ := astutil.PathEnclosingInterval(file, pos, pos) if path == nil { return nil, "", fmt.Errorf("cannot find node enclosing position") diff --git a/internal/lsp/source/definition.go b/internal/lsp/source/definition.go index 5b862683b2b..64da3983a65 100644 --- a/internal/lsp/source/definition.go +++ b/internal/lsp/source/definition.go @@ -62,6 +62,9 @@ func (i *IdentifierInfo) Hover(ctx context.Context, q types.Qualifier) (string, func identifier(ctx context.Context, v View, f File, pos token.Pos) (*IdentifierInfo, error) { fAST := f.GetAST(ctx) pkg := f.GetPackage(ctx) + if pkg.IsIllTyped() { + return nil, fmt.Errorf("package for %s is ill typed", f.URI()) + } path, _ := astutil.PathEnclosingInterval(fAST, pos, pos) result := &IdentifierInfo{ File: f, diff --git a/internal/lsp/source/diagnostics.go b/internal/lsp/source/diagnostics.go index f3c347584b9..a0408138bb7 100644 --- a/internal/lsp/source/diagnostics.go +++ b/internal/lsp/source/diagnostics.go @@ -64,7 +64,7 @@ func Diagnostics(ctx context.Context, v View, uri span.URI) (map[span.URI][]Diag for _, filename := range pkg.GetFilenames() { reports[span.FileURI(filename)] = []Diagnostic{} } - var parseErrors, typeErrors []packages.Error + var listErrors, parseErrors, typeErrors []packages.Error for _, err := range pkg.GetErrors() { switch err.Kind { case packages.ParseError: @@ -72,14 +72,15 @@ func Diagnostics(ctx context.Context, v View, uri span.URI) (map[span.URI][]Diag case packages.TypeError: typeErrors = append(typeErrors, err) default: - // ignore other types of errors - continue + listErrors = append(listErrors, err) } } - // Don't report type errors if there are parse errors. + // Don't report type errors if there are parse errors or list errors. diags := typeErrors if len(parseErrors) > 0 { diags = parseErrors + } else if len(listErrors) > 0 { + diags = listErrors } for _, diag := range diags { spn := span.Parse(diag.Pos) diff --git a/internal/lsp/source/signature_help.go b/internal/lsp/source/signature_help.go index dda57fe8631..f64088f7797 100644 --- a/internal/lsp/source/signature_help.go +++ b/internal/lsp/source/signature_help.go @@ -27,6 +27,9 @@ type ParameterInformation struct { func SignatureHelp(ctx context.Context, f File, pos token.Pos) (*SignatureInformation, error) { fAST := f.GetAST(ctx) pkg := f.GetPackage(ctx) + if pkg.IsIllTyped() { + return nil, fmt.Errorf("package for %s is ill typed", f.URI()) + } // Find a call expression surrounding the query position. var callExpr *ast.CallExpr diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index 1930734c85e..54da56fd036 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -45,6 +45,7 @@ type Package interface { GetErrors() []packages.Error GetTypes() *types.Package GetTypesInfo() *types.Info + IsIllTyped() bool GetActionGraph(ctx context.Context, a *analysis.Analyzer) (*Action, error) }