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 <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>

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

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

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

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
 }
 

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

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,

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)

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

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