diff --git a/internal/lsp/cache/check.go b/internal/lsp/cache/check.go index b56be5f6e61..1ea134f35b4 100644 --- a/internal/lsp/cache/check.go +++ b/internal/lsp/cache/check.go @@ -119,7 +119,7 @@ func (imp *importer) typeCheck(id packageID) (*pkg, error) { ) for _, filename := range meta.files { uri := span.FileURI(filename) - f, err := imp.view.getFile(uri) + f, err := imp.view.getFile(imp.ctx, uri) if err != nil { continue } @@ -187,25 +187,25 @@ func (imp *importer) typeCheck(id packageID) (*pkg, error) { check.Files(pkg.GetSyntax()) // Add every file in this package to our cache. - imp.cachePackage(imp.ctx, pkg, meta, mode) + if err := imp.cachePackage(imp.ctx, pkg, meta, mode); err != nil { + return nil, err + } return pkg, nil } -func (imp *importer) cachePackage(ctx context.Context, pkg *pkg, meta *metadata, mode source.ParseMode) { +func (imp *importer) cachePackage(ctx context.Context, pkg *pkg, meta *metadata, mode source.ParseMode) error { for _, file := range pkg.files { - f, err := imp.view.getFile(file.uri) + f, err := imp.view.getFile(ctx, file.uri) if err != nil { - imp.view.session.log.Errorf(ctx, "no file: %v", err) - continue + return fmt.Errorf("no such file %s: %v", file.uri, err) } gof, ok := f.(*goFile) if !ok { - imp.view.session.log.Errorf(ctx, "%v is not a Go file", file.uri) - continue + return fmt.Errorf("non Go file %s", file.uri) } if err := imp.cachePerFile(gof, file, pkg); err != nil { - imp.view.session.log.Errorf(ctx, "failed to cache file %s: %v", gof.URI(), err) + return fmt.Errorf("failed to cache file %s: %v", gof.URI(), err) } } @@ -219,6 +219,8 @@ func (imp *importer) cachePackage(ctx context.Context, pkg *pkg, meta *metadata, } pkg.imports[importPkg.pkgPath] = importPkg } + + return nil } func (imp *importer) cachePerFile(gof *goFile, file *astFile, p *pkg) error { diff --git a/internal/lsp/cache/gofile.go b/internal/lsp/cache/gofile.go index 0351aa6857d..23ecac617aa 100644 --- a/internal/lsp/cache/gofile.go +++ b/internal/lsp/cache/gofile.go @@ -226,7 +226,7 @@ func (v *view) reverseDeps(ctx context.Context, seen map[packageID]struct{}, res } for _, filename := range m.files { uri := span.FileURI(filename) - if f, err := v.getFile(uri); err == nil && v.session.IsOpen(uri) { + if f, err := v.getFile(ctx, uri); err == nil && v.session.IsOpen(uri) { results[f.(*goFile)] = struct{}{} } } diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go index 2d707ed8762..73aeff6e665 100644 --- a/internal/lsp/cache/load.go +++ b/internal/lsp/cache/load.go @@ -21,7 +21,7 @@ func (v *view) loadParseTypecheck(ctx context.Context, f *goFile) ([]packages.Er // don't ignore function bodies. if f.astIsTrimmed() { v.pcache.mu.Lock() - f.invalidateAST() + f.invalidateAST(ctx) v.pcache.mu.Unlock() } @@ -75,25 +75,20 @@ func sameSet(x, y map[packagePath]struct{}) bool { // checkMetadata determines if we should run go/packages.Load for this file. // If yes, update the metadata for the file and its package. func (v *view) checkMetadata(ctx context.Context, f *goFile) (map[packageID]*metadata, []packages.Error, error) { - f.mu.Lock() - defer f.mu.Unlock() - - if !v.parseImports(ctx, f) { + filename, ok := v.runGopackages(ctx, f) + if !ok { return f.meta, nil, nil } - // Reset the file's metadata and type information if we are re-running `go list`. - for k := range f.meta { - delete(f.meta, k) - } - for k := range f.pkgs { - delete(f.pkgs, k) + // Check if the context has been canceled before calling packages.Load. + if ctx.Err() != nil { + return nil, nil, ctx.Err() } - pkgs, err := packages.Load(v.buildConfig(), fmt.Sprintf("file=%s", f.filename())) + pkgs, err := packages.Load(v.buildConfig(), fmt.Sprintf("file=%s", filename)) if len(pkgs) == 0 { if err == nil { - err = fmt.Errorf("go/packages.Load: no packages found for %s", f.filename()) + err = fmt.Errorf("go/packages.Load: no packages found for %s", filename) } // Return this error as a diagnostic to the user. return nil, []packages.Error{ @@ -103,7 +98,6 @@ func (v *view) checkMetadata(ctx context.Context, f *goFile) (map[packageID]*met }, }, err } - // Track missing imports as we look at the package's errors. missingImports := make(map[packagePath]struct{}) for _, pkg := range pkgs { @@ -119,52 +113,87 @@ func (v *view) checkMetadata(ctx context.Context, f *goFile) (map[packageID]*met } } // Build the import graph for this package. - v.link(ctx, packagePath(pkg.PkgPath), pkg, nil) + if err := v.link(ctx, packagePath(pkg.PkgPath), pkg, nil); err != nil { + return nil, nil, err + } } + m, err := validateMetadata(ctx, missingImports, f) + if err != nil { + return nil, nil, err + } + return m, nil, nil +} + +func validateMetadata(ctx context.Context, missingImports map[packagePath]struct{}, f *goFile) (map[packageID]*metadata, error) { + f.mu.Lock() + defer f.mu.Unlock() // If `go list` failed to get data for the file in question (this should never happen). if len(f.meta) == 0 { - return nil, nil, fmt.Errorf("loadParseTypecheck: no metadata found for %v", f.filename()) + return nil, fmt.Errorf("loadParseTypecheck: no metadata found for %v", f.filename()) } // If we have already seen these missing imports before, and we have type information, // there is no need to continue. if sameSet(missingImports, f.missingImports) && len(f.pkgs) != 0 { - return nil, nil, nil + return nil, nil } // Otherwise, update the missing imports map. f.missingImports = missingImports - - return f.meta, nil, nil + return f.meta, nil } // reparseImports reparses a file's package and import declarations to // determine if they have changed. -func (v *view) parseImports(ctx context.Context, f *goFile) bool { +func (v *view) runGopackages(ctx context.Context, f *goFile) (filename string, result bool) { + f.mu.Lock() + defer func() { + // Clear metadata if we are intending to re-run go/packages. + if result { + // Reset the file's metadata and type information if we are re-running `go list`. + for k := range f.meta { + delete(f.meta, k) + } + for k := range f.pkgs { + delete(f.pkgs, k) + } + } + + defer f.mu.Unlock() + }() + if len(f.meta) == 0 || len(f.missingImports) > 0 { - return true + return f.filename(), true } // Get file content in case we don't already have it. parsed, _ := v.session.cache.ParseGoHandle(f.Handle(ctx), source.ParseHeader).Parse(ctx) if parsed == nil { - return true + return f.filename(), true } - // TODO: Add support for re-running `go list` when the package name changes. - - // 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 != parsed.Imports[i].Path.Value { - return true + // Check if the package's name has changed, by checking if this is a filename + // we already know about, and if so, check if its package name has changed. + for _, m := range f.meta { + for _, filename := range m.files { + if filename == f.URI().Filename() { + if m.name != parsed.Name.Name { + return f.filename(), true + } + } } } - return false + // If the package's imports have changed, re-run `go list`. + if len(f.imports) != len(parsed.Imports) { + return f.filename(), true + } + for i, importSpec := range f.imports { + if importSpec.Path.Value != parsed.Imports[i].Path.Value { + return f.filename(), true + } + } + return f.filename(), false } -func (v *view) link(ctx context.Context, pkgPath packagePath, pkg *packages.Package, parent *metadata) *metadata { +func (v *view) link(ctx context.Context, pkgPath packagePath, pkg *packages.Package, parent *metadata) error { id := packageID(pkg.ID) m, ok := v.mcache.packages[id] @@ -172,7 +201,7 @@ func (v *view) link(ctx context.Context, pkgPath packagePath, pkg *packages.Pack // so relevant packages get parsed and type-checked again. if ok && !filenamesIdentical(m.files, pkg.CompiledGoFiles) { v.pcache.mu.Lock() - v.remove(id, make(map[packageID]struct{})) + v.remove(ctx, id, make(map[packageID]struct{})) v.pcache.mu.Unlock() } @@ -192,16 +221,18 @@ func (v *view) link(ctx context.Context, pkgPath packagePath, pkg *packages.Pack m.name = pkg.Name m.files = pkg.CompiledGoFiles for _, filename := range m.files { - if f, _ := v.getFile(span.FileURI(filename)); f != nil { - if gof, ok := f.(*goFile); ok { - if gof.meta == nil { - gof.meta = make(map[packageID]*metadata) - } - gof.meta[m.id] = m - } else { - v.Session().Logger().Errorf(ctx, "not a Go file: %s", f.URI()) - } + f, err := v.getFile(ctx, span.FileURI(filename)) + if err != nil { + return err } + gof, ok := f.(*goFile) + if !ok { + return fmt.Errorf("not a Go file: %s", f.URI()) + } + if gof.meta == nil { + gof.meta = make(map[packageID]*metadata) + } + gof.meta[m.id] = m } // Connect the import graph. if parent != nil { @@ -209,8 +240,15 @@ func (v *view) link(ctx context.Context, pkgPath packagePath, pkg *packages.Pack parent.children[id] = true } for importPath, importPkg := range pkg.Imports { + importPkgPath := packagePath(importPath) + if importPkgPath == pkgPath { + v.session.log.Errorf(ctx, "cycle detected in %s", importPath) + return nil + } if _, ok := m.children[packageID(importPkg.ID)]; !ok { - v.link(ctx, packagePath(importPath), importPkg, m) + if err := v.link(ctx, importPkgPath, importPkg, m); err != nil { + return err + } } } // Clear out any imports that have been removed. @@ -226,7 +264,7 @@ func (v *view) link(ctx context.Context, pkgPath packagePath, pkg *packages.Pack delete(m.children, importID) delete(child.parents, id) } - return m + return nil } // filenamesIdentical reports whether two sets of file names are identical. diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index ec848a0c1be..a56dcbb826a 100644 --- a/internal/lsp/cache/view.go +++ b/internal/lsp/cache/view.go @@ -224,7 +224,7 @@ func (v *view) SetContent(ctx context.Context, uri span.URI, content []byte) err // invalidateContent invalidates the content of a Go file, // including any position and type information that depends on it. -func (f *goFile) invalidateContent() { +func (f *goFile) invalidateContent(ctx context.Context) { f.handleMu.Lock() defer f.handleMu.Unlock() @@ -234,13 +234,13 @@ func (f *goFile) invalidateContent() { f.view.pcache.mu.Lock() defer f.view.pcache.mu.Unlock() - f.invalidateAST() + f.invalidateAST(ctx) f.handle = nil } // invalidateAST invalidates the AST of a Go file, // including any position and type information that depends on it. -func (f *goFile) invalidateAST() { +func (f *goFile) invalidateAST(ctx context.Context) { f.mu.Lock() f.ast = nil f.token = nil @@ -250,7 +250,7 @@ func (f *goFile) invalidateAST() { // Remove the package and all of its reverse dependencies from the cache. for id, pkg := range pkgs { if pkg != nil { - f.view.remove(id, map[packageID]struct{}{}) + f.view.remove(ctx, id, map[packageID]struct{}{}) } } } @@ -258,7 +258,7 @@ func (f *goFile) invalidateAST() { // remove invalidates a package and its reverse dependencies in the view's // package cache. It is assumed that the caller has locked both the mutexes // of both the mcache and the pcache. -func (v *view) remove(id packageID, seen map[packageID]struct{}) { +func (v *view) remove(ctx context.Context, id packageID, seen map[packageID]struct{}) { if _, ok := seen[id]; ok { return } @@ -268,20 +268,27 @@ func (v *view) remove(id packageID, seen map[packageID]struct{}) { } seen[id] = struct{}{} for parentID := range m.parents { - v.remove(parentID, seen) + v.remove(ctx, parentID, seen) } // All of the files in the package may also be holding a pointer to the // invalidated package. for _, filename := range m.files { - if f, _ := v.findFile(span.FileURI(filename)); f != nil { - if gof, ok := f.(*goFile); ok { - gof.mu.Lock() - delete(gof.pkgs, id) - gof.mu.Unlock() - } + f, err := v.findFile(span.FileURI(filename)) + if err != nil { + v.session.log.Errorf(ctx, "cannot find file %s: %v", f.URI(), err) + continue } + gof, ok := f.(*goFile) + if !ok { + v.session.log.Errorf(ctx, "non-Go file %v", f.URI()) + continue + } + gof.mu.Lock() + delete(gof.pkgs, id) + gof.mu.Unlock() } delete(v.pcache.packages, id) + return } // FindFile returns the file if the given URI is already a part of the view. @@ -301,11 +308,11 @@ func (v *view) GetFile(ctx context.Context, uri span.URI) (source.File, error) { v.mu.Lock() defer v.mu.Unlock() - return v.getFile(uri) + return v.getFile(ctx, uri) } // getFile is the unlocked internal implementation of GetFile. -func (v *view) getFile(uri span.URI) (viewFile, error) { +func (v *view) getFile(ctx context.Context, uri span.URI) (viewFile, error) { if f, err := v.findFile(uri); err != nil { return nil, err } else if f != nil { @@ -344,7 +351,7 @@ func (v *view) getFile(uri span.URI) (viewFile, error) { if !ok { return } - gof.invalidateContent() + gof.invalidateContent(ctx) }) } v.mapFile(uri, f) diff --git a/internal/lsp/source/format.go b/internal/lsp/source/format.go index 56be5e0ef2f..3a308a7eea7 100644 --- a/internal/lsp/source/format.go +++ b/internal/lsp/source/format.go @@ -25,7 +25,7 @@ func Format(ctx context.Context, f GoFile, rng span.Range) ([]TextEdit, error) { return nil, fmt.Errorf("no AST for %s", f.URI()) } pkg := f.GetPackage(ctx) - if hasParseErrors(pkg.GetErrors()) { + if hasListErrors(pkg.GetErrors()) || hasParseErrors(pkg.GetErrors()) { return nil, fmt.Errorf("%s has parse errors, not formatting", f.URI()) } path, exact := astutil.PathEnclosingInterval(file, rng.Start, rng.End) @@ -45,6 +45,23 @@ func Format(ctx context.Context, f GoFile, rng span.Range) ([]TextEdit, error) { return computeTextEdits(ctx, f, buf.String()), nil } +// Imports formats a file using the goimports tool. +func Imports(ctx context.Context, f GoFile, rng span.Range) ([]TextEdit, error) { + data, _, err := f.Handle(ctx).Read(ctx) + if err != nil { + return nil, err + } + pkg := f.GetPackage(ctx) + if hasListErrors(pkg.GetErrors()) { + return nil, fmt.Errorf("%s has list errors, not running goimports", f.URI()) + } + formatted, err := imports.Process(f.URI().Filename(), data, nil) + if err != nil { + return nil, err + } + return computeTextEdits(ctx, f, string(formatted)), nil +} + func hasParseErrors(errors []packages.Error) bool { for _, err := range errors { if err.Kind == packages.ParseError { @@ -54,21 +71,13 @@ func hasParseErrors(errors []packages.Error) bool { return false } -// Imports formats a file using the goimports tool. -func Imports(ctx context.Context, f GoFile, rng span.Range) ([]TextEdit, error) { - data, _, err := f.Handle(ctx).Read(ctx) - if err != nil { - return nil, err +func hasListErrors(errors []packages.Error) bool { + for _, err := range errors { + if err.Kind == packages.ListError { + return true + } } - tok := f.GetToken(ctx) - if tok == nil { - return nil, fmt.Errorf("no token file for %s", f.URI()) - } - formatted, err := imports.Process(tok.Name(), data, nil) - if err != nil { - return nil, err - } - return computeTextEdits(ctx, f, string(formatted)), nil + return false } func computeTextEdits(ctx context.Context, file File, formatted string) (edits []TextEdit) {