From 3b4f30a44f3bd52b676ee9e88db10d11971b7363 Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Mon, 9 Sep 2019 19:26:26 -0400 Subject: [PATCH] internal/lsp: remove helpers for getting packages We had too many options for functions to use to get type information for a package. Now we stick with having one option to get the check package handles, and then the caller can refine the results as needed. Change-Id: I81f69a670e1539854ee23b6f364159a7de9b782f Reviewed-on: https://go-review.googlesource.com/c/tools/+/194457 Run-TryBot: Rebecca Stambler TryBot-Result: Gobot Gobot Reviewed-by: Ian Cottrell --- internal/lsp/cache/check.go | 8 +- internal/lsp/cache/gofile.go | 237 ++++++----------------- internal/lsp/cache/load.go | 8 +- internal/lsp/cache/pkg.go | 2 +- internal/lsp/cache/view.go | 10 +- internal/lsp/code_action.go | 7 +- internal/lsp/source/completion.go | 9 +- internal/lsp/source/completion_format.go | 2 +- internal/lsp/source/diagnostics.go | 35 ++-- internal/lsp/source/format.go | 28 ++- internal/lsp/source/identifier.go | 41 ++-- internal/lsp/source/references.go | 94 +++++---- internal/lsp/source/rename.go | 29 +-- internal/lsp/source/signature_help.go | 7 +- internal/lsp/source/symbols.go | 9 +- internal/lsp/source/util.go | 35 +++- internal/lsp/source/view.go | 22 +-- internal/lsp/text_synchronization.go | 20 +- internal/lsp/watched_files.go | 29 ++- 19 files changed, 276 insertions(+), 356 deletions(-) diff --git a/internal/lsp/cache/check.go b/internal/lsp/cache/check.go index 2ccf62f4acd..d7249268810 100644 --- a/internal/lsp/cache/check.go +++ b/internal/lsp/cache/check.go @@ -153,7 +153,7 @@ func (cph *checkPackageHandle) ID() string { func (cph *checkPackageHandle) Cached(ctx context.Context) (source.Package, error) { v := cph.handle.Cached() if v == nil { - return nil, errors.Errorf("no cached value for %s", cph.m.pkgPath) + return nil, errors.Errorf("no cached type information for %s", cph.m.pkgPath) } data := v.(*checkPackageData) return data.pkg, data.err @@ -345,10 +345,10 @@ func (imp *importer) cachePerFile(ctx context.Context, gof *goFile, ph source.Pa defer gof.mu.Unlock() // Set the package even if we failed to parse the file. - if gof.pkgs == nil { - gof.pkgs = make(map[packageID]source.CheckPackageHandle) + if gof.cphs == nil { + gof.cphs = make(map[packageID]source.CheckPackageHandle) } - gof.pkgs[cph.m.id] = cph + gof.cphs[cph.m.id] = cph file, _, err := ph.Parse(ctx) if file == nil { diff --git a/internal/lsp/cache/gofile.go b/internal/lsp/cache/gofile.go index c8ffc1f913d..7bf6fd5d6d5 100644 --- a/internal/lsp/cache/gofile.go +++ b/internal/lsp/cache/gofile.go @@ -11,8 +11,6 @@ import ( "golang.org/x/tools/internal/lsp/source" "golang.org/x/tools/internal/lsp/telemetry" - "golang.org/x/tools/internal/span" - "golang.org/x/tools/internal/telemetry/log" errors "golang.org/x/xerrors" ) @@ -35,66 +33,11 @@ type goFile struct { imports []*ast.ImportSpec - pkgs map[packageID]source.CheckPackageHandle + cphs map[packageID]source.CheckPackageHandle meta map[packageID]*metadata } -// metadata assumes that the caller holds the f.mu lock. -func (f *goFile) metadata() []*metadata { - result := make([]*metadata, 0, len(f.meta)) - for _, m := range f.meta { - result = append(result, m) - } - return result -} - -func (cache *cache) cachedAST(fh source.FileHandle, mode source.ParseMode) (*ast.File, error) { - for _, m := range []source.ParseMode{ - source.ParseHeader, - source.ParseExported, - source.ParseFull, - } { - if m < mode { - continue - } - if v, ok := cache.store.Cached(parseKey{ - file: fh.Identity(), - mode: m, - }).(*parseGoData); ok { - return v.ast, v.err - } - } - return nil, nil -} - -func (f *goFile) GetPackages(ctx context.Context) ([]source.Package, error) { - cphs, err := f.GetCheckPackageHandles(ctx) - if err != nil { - return nil, err - } - var pkgs []source.Package - for _, cph := range cphs { - pkg, err := cph.Check(ctx) - if err != nil { - log.Error(ctx, "failed to check package", err) - } - pkgs = append(pkgs, pkg) - } - if len(pkgs) == 0 { - return nil, errors.Errorf("no packages for %s", f.URI()) - } - return pkgs, nil -} - -func (f *goFile) GetPackage(ctx context.Context) (source.Package, error) { - cph, err := f.GetCheckPackageHandle(ctx) - if err != nil { - return nil, err - } - return cph.Check(ctx) -} - -func (f *goFile) GetCheckPackageHandles(ctx context.Context) ([]source.CheckPackageHandle, error) { +func (f *goFile) CheckPackageHandles(ctx context.Context) ([]source.CheckPackageHandle, error) { ctx = telemetry.File.With(ctx, f.URI()) fh := f.Handle(ctx) @@ -107,126 +50,16 @@ func (f *goFile) GetCheckPackageHandles(ctx context.Context) ([]source.CheckPack f.mu.Lock() defer f.mu.Unlock() - var cphs []source.CheckPackageHandle - for _, cph := range f.pkgs { - cphs = append(cphs, cph) - } - if len(cphs) == 0 { + if len(f.cphs) == 0 { return nil, errors.Errorf("no CheckPackageHandles for %s", f.URI()) } + var cphs []source.CheckPackageHandle + for _, cph := range f.cphs { + cphs = append(cphs, cph) + } return cphs, nil } -func (f *goFile) GetCheckPackageHandle(ctx context.Context) (source.CheckPackageHandle, error) { - cphs, err := f.GetCheckPackageHandles(ctx) - if err != nil { - return nil, err - } - return bestCheckPackageHandle(f.URI(), cphs) -} - -func (f *goFile) GetCachedPackage(ctx context.Context) (source.Package, error) { - f.mu.Lock() - var cphs []source.CheckPackageHandle - for _, cph := range f.pkgs { - cphs = append(cphs, cph) - } - f.mu.Unlock() - - if len(cphs) == 0 { - return nil, errors.Errorf("no CheckPackageHandles for %s", f.URI()) - } - - cph, err := bestCheckPackageHandle(f.URI(), cphs) - if err != nil { - return nil, err - } - return cph.Cached(ctx) -} - -func (f *goFile) GetCachedPackages(ctx context.Context) ([]source.Package, error) { - f.mu.Lock() - defer f.mu.Unlock() - - var pkgs []source.Package - for _, cph := range f.pkgs { - pkg, err := cph.Cached(ctx) - if err != nil { - return nil, err - } - pkgs = append(pkgs, pkg) - } - if len(pkgs) == 0 { - return nil, errors.Errorf("no CheckPackageHandles for %s", f.URI()) - } - return pkgs, nil -} - -// bestCheckPackageHandle picks the "narrowest" package for a given file. -// -// By "narrowest" package, we mean the package with the fewest number of files -// that includes the given file. This solves the problem of test variants, -// as the test will have more files than the non-test package. -func bestCheckPackageHandle(uri span.URI, cphs []source.CheckPackageHandle) (source.CheckPackageHandle, error) { - var result source.CheckPackageHandle - for _, cph := range cphs { - if result == nil || len(cph.Files()) < len(result.Files()) { - result = cph - } - } - if result == nil { - return nil, errors.Errorf("no CheckPackageHandle for %s", uri) - } - return result, nil -} - -func (f *goFile) wrongParseMode(ctx context.Context, fh source.FileHandle, mode source.ParseMode) bool { - f.mu.Lock() - defer f.mu.Unlock() - - for _, cph := range f.pkgs { - for _, ph := range cph.Files() { - if fh.Identity() == ph.File().Identity() { - return ph.Mode() < mode - } - } - } - return true -} - -// 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(ctx context.Context, fh source.FileHandle) bool { - f.mu.Lock() - defer f.mu.Unlock() - - // If the the file has just been opened, - // it may be part of more packages than we are aware of. - // - // Note: This must be the first case, otherwise we may not reset the value of f.justOpened. - if f.justOpened { - f.meta = make(map[packageID]*metadata) - f.pkgs = make(map[packageID]source.CheckPackageHandle) - f.justOpened = false - return true - } - if len(f.meta) == 0 || len(f.pkgs) == 0 { - return true - } - if len(f.missingImports) > 0 { - return true - } - for _, cph := range f.pkgs { - for _, file := range cph.Files() { - // There is a type-checked package for the current file handle. - if file.File().Identity() == fh.Identity() { - return false - } - } - } - return true -} - func (f *goFile) GetActiveReverseDeps(ctx context.Context) (files []source.GoFile) { seen := make(map[packageID]struct{}) // visited packages results := make(map[*goFile]struct{}) @@ -276,3 +109,59 @@ func (v *view) reverseDeps(ctx context.Context, seen map[packageID]struct{}, res v.reverseDeps(ctx, seen, results, parentID) } } + +// metadata assumes that the caller holds the f.mu lock. +func (f *goFile) metadata() []*metadata { + result := make([]*metadata, 0, len(f.meta)) + for _, m := range f.meta { + result = append(result, m) + } + return result +} + +func (f *goFile) wrongParseMode(ctx context.Context, fh source.FileHandle, mode source.ParseMode) bool { + f.mu.Lock() + defer f.mu.Unlock() + + for _, cph := range f.cphs { + for _, ph := range cph.Files() { + if fh.Identity() == ph.File().Identity() { + return ph.Mode() < mode + } + } + } + return true +} + +// 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(ctx context.Context, fh source.FileHandle) bool { + f.mu.Lock() + defer f.mu.Unlock() + + // If the the file has just been opened, + // it may be part of more packages than we are aware of. + // + // Note: This must be the first case, otherwise we may not reset the value of f.justOpened. + if f.justOpened { + f.meta = make(map[packageID]*metadata) + f.cphs = make(map[packageID]source.CheckPackageHandle) + f.justOpened = false + return true + } + if len(f.meta) == 0 || len(f.cphs) == 0 { + return true + } + if len(f.missingImports) > 0 { + return true + } + for _, cph := range f.cphs { + for _, file := range cph.Files() { + // There is a type-checked package for the current file handle. + if file.File().Identity() == fh.Identity() { + return false + } + } + } + return true +} diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go index 14b58791b09..d153af2ce01 100644 --- a/internal/lsp/cache/load.go +++ b/internal/lsp/cache/load.go @@ -59,7 +59,7 @@ func (view *view) load(ctx context.Context, f *goFile, fh source.FileHandle) ([] var toDelete []packageID f.mu.Lock() - for id, cph := range f.pkgs { + for id, cph := range f.cphs { if cph != nil { toDelete = append(toDelete, id) } @@ -124,8 +124,8 @@ func (v *view) checkMetadata(ctx context.Context, f *goFile, fh source.FileHandl for k := range f.meta { delete(f.meta, k) } - for k := range f.pkgs { - delete(f.pkgs, k) + for k := range f.cphs { + delete(f.cphs, k) } f.mu.Unlock() @@ -160,7 +160,7 @@ func validateMetadata(ctx context.Context, missingImports map[packagePath]struct // 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 { + if sameSet(missingImports, f.missingImports) && len(f.cphs) != 0 { return nil, nil } diff --git a/internal/lsp/cache/pkg.go b/internal/lsp/cache/pkg.go index 8a0454f87fe..ba7fed3a6e8 100644 --- a/internal/lsp/cache/pkg.go +++ b/internal/lsp/cache/pkg.go @@ -147,7 +147,7 @@ func (pkg *pkg) PkgPath() string { return string(pkg.pkgPath) } -func (pkg *pkg) GetHandles() []source.ParseGoHandle { +func (pkg *pkg) Files() []source.ParseGoHandle { return pkg.files } diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index 0aa294bc830..b16e34edadd 100644 --- a/internal/lsp/cache/view.go +++ b/internal/lsp/cache/view.go @@ -330,7 +330,7 @@ func (f *goFile) invalidateContent(ctx context.Context) { var toDelete []packageID f.mu.Lock() - for id, cph := range f.pkgs { + for id, cph := range f.cphs { if cph != nil { toDelete = append(toDelete, id) } @@ -352,14 +352,14 @@ func (f *goFile) invalidateContent(ctx context.Context) { // package. This forces f's package's metadata to be reloaded next // time the package is checked. func (f *goFile) invalidateMeta(ctx context.Context) { - pkgs, err := f.GetPackages(ctx) + cphs, err := f.CheckPackageHandles(ctx) if err != nil { log.Error(ctx, "invalidateMeta: GetPackages", err, telemetry.File.Of(f.URI())) return } - for _, pkg := range pkgs { - for _, pgh := range pkg.GetHandles() { + for _, pkg := range cphs { + for _, pgh := range pkg.Files() { uri := pgh.File().Identity().URI if gof, _ := f.view.FindFile(ctx, uri).(*goFile); gof != nil { gof.mu.Lock() @@ -402,7 +402,7 @@ func (v *view) remove(ctx context.Context, id packageID, seen map[packageID]stru continue } gof.mu.Lock() - delete(gof.pkgs, id) + delete(gof.cphs, id) gof.mu.Unlock() } return diff --git a/internal/lsp/code_action.go b/internal/lsp/code_action.go index fe87a1a4977..a8c7e8c9637 100644 --- a/internal/lsp/code_action.go +++ b/internal/lsp/code_action.go @@ -201,7 +201,12 @@ func quickFixes(ctx context.Context, view source.View, gof source.GoFile) ([]pro // TODO: This is technically racy because the diagnostics provided by the code action // may not be the same as the ones that gopls is aware of. // We need to figure out some way to solve this problem. - pkg, err := gof.GetCachedPackage(ctx) + cphs, err := gof.CheckPackageHandles(ctx) + if err != nil { + return nil, err + } + cph := source.NarrowestCheckPackageHandle(cphs) + pkg, err := cph.Cached(ctx) if err != nil { return nil, err } diff --git a/internal/lsp/source/completion.go b/internal/lsp/source/completion.go index e9281c046cb..17a8bc5a73a 100644 --- a/internal/lsp/source/completion.go +++ b/internal/lsp/source/completion.go @@ -367,12 +367,17 @@ func Completion(ctx context.Context, view View, f GoFile, pos protocol.Position, startTime := time.Now() - pkg, err := f.GetPackage(ctx) + cphs, err := f.CheckPackageHandles(ctx) + if err != nil { + return nil, nil, err + } + cph := NarrowestCheckPackageHandle(cphs) + pkg, err := cph.Check(ctx) if err != nil { return nil, nil, err } var ph ParseGoHandle - for _, h := range pkg.GetHandles() { + for _, h := range pkg.Files() { if h.File().Identity().URI == f.URI() { ph = h } diff --git a/internal/lsp/source/completion_format.go b/internal/lsp/source/completion_format.go index 162ca2bd769..561e5d897ab 100644 --- a/internal/lsp/source/completion_format.go +++ b/internal/lsp/source/completion_format.go @@ -136,7 +136,7 @@ func (c *completer) item(cand candidate) (CompletionItem, error) { if !(file.Pos() <= obj.Pos() && obj.Pos() <= file.End()) { return CompletionItem{}, errors.Errorf("no file for %s", obj.Name()) } - ident, err := findIdentifier(c.ctx, c.view, []Package{pkg}, file, obj.Pos()) + ident, err := findIdentifier(c.ctx, c.view, pkg, file, obj.Pos()) if err != nil { return CompletionItem{}, err } diff --git a/internal/lsp/source/diagnostics.go b/internal/lsp/source/diagnostics.go index 79c2fb58cc4..009d8926911 100644 --- a/internal/lsp/source/diagnostics.go +++ b/internal/lsp/source/diagnostics.go @@ -69,22 +69,11 @@ func Diagnostics(ctx context.Context, view View, f GoFile, disabledAnalyses map[ ctx, done := trace.StartSpan(ctx, "source.Diagnostics", telemetry.File.Of(f.URI())) defer done() - cphs, err := f.GetCheckPackageHandles(ctx) + cphs, err := f.CheckPackageHandles(ctx) if err != nil { return nil, err } - // Use the "biggest" package we know about. - // If we know about a package and its in-package tests, - // we should send diagnostics for both. - var cph CheckPackageHandle - for _, h := range cphs { - if cph == nil || len(h.Files()) > len(cph.Files()) { - cph = h - } - } - if cph == nil { - return nil, errors.Errorf("no package for file %s", f.URI()) - } + cph := NarrowestCheckPackageHandle(cphs) pkg, err := cph.Check(ctx) if err != nil { log.Error(ctx, "no package for file", err) @@ -92,7 +81,7 @@ func Diagnostics(ctx context.Context, view View, f GoFile, disabledAnalyses map[ } // Prepare the reports we will send for the files in this package. reports := make(map[span.URI][]Diagnostic) - for _, fh := range pkg.GetHandles() { + for _, fh := range pkg.Files() { clearReports(view, reports, fh.File().Identity().URI) } @@ -114,11 +103,16 @@ func Diagnostics(ctx context.Context, view View, f GoFile, disabledAnalyses map[ // Updates to the diagnostics for this package may need to be propagated. revDeps := f.GetActiveReverseDeps(ctx) for _, f := range revDeps { - pkg, err := f.GetPackage(ctx) + cphs, err := f.CheckPackageHandles(ctx) if err != nil { return nil, err } - for _, fh := range pkg.GetHandles() { + cph := WidestCheckPackageHandle(cphs) + pkg, err := cph.Check(ctx) + if err != nil { + return nil, err + } + for _, fh := range pkg.Files() { clearReports(view, reports, fh.File().Identity().URI) } diagnostics(ctx, view, pkg, reports) @@ -193,7 +187,7 @@ func spanToRange(ctx context.Context, view View, pkg Package, spn span.Span, isT m *protocol.ColumnMapper err error ) - for _, ph := range pkg.GetHandles() { + for _, ph := range pkg.Files() { if ph.File().Identity().URI == spn.URI() { fh = ph.File() file, m, err = ph.Cached(ctx) @@ -255,7 +249,12 @@ func toDiagnostic(ctx context.Context, view View, diag analysis.Diagnostic, cate } // If the package has changed since these diagnostics were computed, // this may be incorrect. Should the package be associated with the diagnostic? - pkg, err := gof.GetCachedPackage(ctx) + cphs, err := gof.CheckPackageHandles(ctx) + if err != nil { + return Diagnostic{}, err + } + cph := NarrowestCheckPackageHandle(cphs) + pkg, err := cph.Cached(ctx) if err != nil { return Diagnostic{}, err } diff --git a/internal/lsp/source/format.go b/internal/lsp/source/format.go index 31722242efe..b750b540828 100644 --- a/internal/lsp/source/format.go +++ b/internal/lsp/source/format.go @@ -28,12 +28,17 @@ func Format(ctx context.Context, view View, f File) ([]protocol.TextEdit, error) if !ok { return nil, errors.Errorf("formatting is not supported for non-Go files") } - pkg, err := gof.GetPackage(ctx) + cphs, err := gof.CheckPackageHandles(ctx) + if err != nil { + return nil, err + } + cph := NarrowestCheckPackageHandle(cphs) + pkg, err := cph.Check(ctx) if err != nil { return nil, err } var ph ParseGoHandle - for _, h := range pkg.GetHandles() { + for _, h := range pkg.Files() { if h.File().Identity().URI == f.URI() { ph = h } @@ -84,7 +89,13 @@ func formatSource(ctx context.Context, file File) ([]byte, error) { func Imports(ctx context.Context, view View, f GoFile, rng span.Range) ([]protocol.TextEdit, error) { ctx, done := trace.StartSpan(ctx, "source.Imports") defer done() - pkg, err := f.GetPackage(ctx) + + cphs, err := f.CheckPackageHandles(ctx) + if err != nil { + return nil, err + } + cph := NarrowestCheckPackageHandle(cphs) + pkg, err := cph.Check(ctx) if err != nil { return nil, err } @@ -92,7 +103,7 @@ func Imports(ctx context.Context, view View, f GoFile, rng span.Range) ([]protoc return nil, errors.Errorf("%s has list errors, not running goimports", f.URI()) } var ph ParseGoHandle - for _, h := range pkg.GetHandles() { + for _, h := range pkg.Files() { if h.File().Identity().URI == f.URI() { ph = h } @@ -146,7 +157,12 @@ func AllImportsFixes(ctx context.Context, view View, f File) (edits []protocol.T if !ok { return nil, nil, errors.Errorf("no imports fixes for non-Go files: %v", err) } - pkg, err := gof.GetPackage(ctx) + cphs, err := gof.CheckPackageHandles(ctx) + if err != nil { + return nil, nil, err + } + cph := NarrowestCheckPackageHandle(cphs) + pkg, err := cph.Check(ctx) if err != nil { return nil, nil, err } @@ -164,7 +180,7 @@ func AllImportsFixes(ctx context.Context, view View, f File) (edits []protocol.T } importFn := func(opts *imports.Options) error { var ph ParseGoHandle - for _, h := range pkg.GetHandles() { + for _, h := range pkg.Files() { if h.File().Identity().URI == f.URI() { ph = h } diff --git a/internal/lsp/source/identifier.go b/internal/lsp/source/identifier.go index 7f15a992450..9088e9ab238 100644 --- a/internal/lsp/source/identifier.go +++ b/internal/lsp/source/identifier.go @@ -32,7 +32,7 @@ type IdentifierInfo struct { Declaration Declaration - pkgs []Package + pkg Package ident *ast.Ident wasEmbeddedField bool qf types.Qualifier @@ -48,16 +48,17 @@ type Declaration struct { // Identifier returns identifier information for a position // in a file, accounting for a potentially incomplete selector. func Identifier(ctx context.Context, view View, f GoFile, pos protocol.Position) (*IdentifierInfo, error) { - pkgs, err := f.GetPackages(ctx) + cphs, err := f.CheckPackageHandles(ctx) if err != nil { return nil, err } - pkg, err := bestPackage(f.URI(), pkgs) + cph := WidestCheckPackageHandle(cphs) + pkg, err := cph.Check(ctx) if err != nil { return nil, err } var ph ParseGoHandle - for _, h := range pkg.GetHandles() { + for _, h := range cph.Files() { if h.File().Identity().URI == f.URI() { ph = h break @@ -75,17 +76,17 @@ func Identifier(ctx context.Context, view View, f GoFile, pos protocol.Position) if err != nil { return nil, err } - return findIdentifier(ctx, view, pkgs, file, rng.Start) + return findIdentifier(ctx, view, pkg, file, rng.Start) } -func findIdentifier(ctx context.Context, view View, pkgs []Package, file *ast.File, pos token.Pos) (*IdentifierInfo, error) { - if result, err := identifier(ctx, view, pkgs, file, pos); err != nil || result != nil { +func findIdentifier(ctx context.Context, view View, pkg Package, file *ast.File, pos token.Pos) (*IdentifierInfo, error) { + if result, err := identifier(ctx, view, pkg, file, pos); err != nil || result != nil { return result, err } // If the position is not an identifier but immediately follows // an identifier or selector period (as is common when // requesting a completion), use the path to the preceding node. - ident, err := identifier(ctx, view, pkgs, file, pos-1) + ident, err := identifier(ctx, view, pkg, file, pos-1) if ident == nil && err == nil { err = errors.New("no identifier found") } @@ -93,14 +94,14 @@ func findIdentifier(ctx context.Context, view View, pkgs []Package, file *ast.Fi } // identifier checks a single position for a potential identifier. -func identifier(ctx context.Context, view View, pkgs []Package, file *ast.File, pos token.Pos) (*IdentifierInfo, error) { +func identifier(ctx context.Context, view View, pkg Package, file *ast.File, pos token.Pos) (*IdentifierInfo, error) { ctx, done := trace.StartSpan(ctx, "source.identifier") defer done() var err error // Handle import specs separately, as there is no formal position for a package declaration. - if result, err := importSpec(ctx, view, file, pkgs, pos); result != nil || err != nil { + if result, err := importSpec(ctx, view, file, pkg, pos); result != nil || err != nil { return result, err } path, _ := astutil.PathEnclosingInterval(file, pos, pos) @@ -108,12 +109,8 @@ func identifier(ctx context.Context, view View, pkgs []Package, file *ast.File, return nil, errors.Errorf("can't find node enclosing position") } uri := span.FileURI(view.Session().Cache().FileSet().Position(pos).Filename) - pkg, err := bestPackage(uri, pkgs) - if err != nil { - return nil, err - } var ph ParseGoHandle - for _, h := range pkg.GetHandles() { + for _, h := range pkg.Files() { if h.File().Identity().URI == uri { ph = h } @@ -122,7 +119,7 @@ func identifier(ctx context.Context, view View, pkgs []Package, file *ast.File, View: view, File: ph, qf: qualifier(file, pkg.GetTypes(), pkg.GetTypesInfo()), - pkgs: pkgs, + pkg: pkg, ident: searchForIdent(path[0]), } // No identifier at the given position. @@ -279,9 +276,9 @@ func objToNode(ctx context.Context, view View, pkg Package, obj types.Object) (a } // importSpec handles positions inside of an *ast.ImportSpec. -func importSpec(ctx context.Context, view View, fAST *ast.File, pkgs []Package, pos token.Pos) (*IdentifierInfo, error) { +func importSpec(ctx context.Context, view View, file *ast.File, pkg Package, pos token.Pos) (*IdentifierInfo, error) { var imp *ast.ImportSpec - for _, spec := range fAST.Imports { + for _, spec := range file.Imports { if spec.Path.Pos() <= pos && pos < spec.Path.End() { imp = spec } @@ -294,12 +291,8 @@ func importSpec(ctx context.Context, view View, fAST *ast.File, pkgs []Package, return nil, errors.Errorf("import path not quoted: %s (%v)", imp.Path.Value, err) } uri := span.FileURI(view.Session().Cache().FileSet().Position(pos).Filename) - pkg, err := bestPackage(uri, pkgs) - if err != nil { - return nil, err - } var ph ParseGoHandle - for _, h := range pkg.GetHandles() { + for _, h := range pkg.Files() { if h.File().Identity().URI == uri { ph = h } @@ -308,7 +301,7 @@ func importSpec(ctx context.Context, view View, fAST *ast.File, pkgs []Package, View: view, File: ph, Name: importPath, - pkgs: pkgs, + pkg: pkg, } if result.mappedRange, err = posToMappedRange(ctx, view, pkg, imp.Path.Pos(), imp.Path.End()); err != nil { return nil, err diff --git a/internal/lsp/source/references.go b/internal/lsp/source/references.go index 1d1a993ee45..84c9d4d6507 100644 --- a/internal/lsp/source/references.go +++ b/internal/lsp/source/references.go @@ -28,64 +28,62 @@ type ReferenceInfo struct { func (i *IdentifierInfo) References(ctx context.Context) ([]*ReferenceInfo, error) { ctx, done := trace.StartSpan(ctx, "source.References") defer done() + var references []*ReferenceInfo // If the object declaration is nil, assume it is an import spec and do not look for references. if i.Declaration.obj == nil { return nil, errors.Errorf("no references for an import spec") } - for _, pkg := range i.pkgs { - info := pkg.GetTypesInfo() - if info == nil { - return nil, errors.Errorf("package %s has no types info", pkg.PkgPath()) + info := i.pkg.GetTypesInfo() + if info == nil { + return nil, errors.Errorf("package %s has no types info", i.pkg.PkgPath()) + } + if i.Declaration.wasImplicit { + // The definition is implicit, so we must add it separately. + // This occurs when the variable is declared in a type switch statement + // or is an implicit package name. Both implicits are local to a file. + references = append(references, &ReferenceInfo{ + Name: i.Declaration.obj.Name(), + mappedRange: i.Declaration.mappedRange, + obj: i.Declaration.obj, + pkg: i.pkg, + isDeclaration: true, + }) + } + for ident, obj := range info.Defs { + if obj == nil || !sameObj(obj, i.Declaration.obj) { + continue } - - if i.Declaration.wasImplicit { - // The definition is implicit, so we must add it separately. - // This occurs when the variable is declared in a type switch statement - // or is an implicit package name. Both implicits are local to a file. - references = append(references, &ReferenceInfo{ - Name: i.Declaration.obj.Name(), - mappedRange: i.Declaration.mappedRange, - obj: i.Declaration.obj, - pkg: pkg, - isDeclaration: true, - }) + rng, err := posToMappedRange(ctx, i.View, i.pkg, ident.Pos(), ident.End()) + if err != nil { + return nil, err } - for ident, obj := range info.Defs { - if obj == nil || !sameObj(obj, i.Declaration.obj) { - continue - } - rng, err := posToMappedRange(ctx, i.View, pkg, ident.Pos(), ident.End()) - if err != nil { - return nil, err - } - // Add the declarations at the beginning of the references list. - references = append([]*ReferenceInfo{{ - Name: ident.Name, - ident: ident, - obj: obj, - pkg: pkg, - isDeclaration: true, - mappedRange: rng, - }}, references...) + // Add the declarations at the beginning of the references list. + references = append([]*ReferenceInfo{{ + Name: ident.Name, + ident: ident, + obj: obj, + pkg: i.pkg, + isDeclaration: true, + mappedRange: rng, + }}, references...) + } + for ident, obj := range info.Uses { + if obj == nil || !sameObj(obj, i.Declaration.obj) { + continue } - for ident, obj := range info.Uses { - if obj == nil || !sameObj(obj, i.Declaration.obj) { - continue - } - rng, err := posToMappedRange(ctx, i.View, pkg, ident.Pos(), ident.End()) - if err != nil { - return nil, err - } - references = append(references, &ReferenceInfo{ - Name: ident.Name, - ident: ident, - pkg: pkg, - obj: obj, - mappedRange: rng, - }) + rng, err := posToMappedRange(ctx, i.View, i.pkg, ident.Pos(), ident.End()) + if err != nil { + return nil, err } + references = append(references, &ReferenceInfo{ + Name: ident.Name, + ident: ident, + pkg: i.pkg, + obj: obj, + mappedRange: rng, + }) } return references, nil } diff --git a/internal/lsp/source/rename.go b/internal/lsp/source/rename.go index 6f145479b40..0d5f3bb1317 100644 --- a/internal/lsp/source/rename.go +++ b/internal/lsp/source/rename.go @@ -102,15 +102,11 @@ func (i *IdentifierInfo) Rename(ctx context.Context, view View, newName string) if i.Declaration.obj.Parent() == types.Universe { return nil, errors.Errorf("cannot rename builtin %q", i.Name) } - pkg, err := bestPackage(i.File.File().Identity().URI, i.pkgs) - if err != nil { - return nil, err - } - if pkg == nil || pkg.IsIllTyped() { + if i.pkg == nil || i.pkg.IsIllTyped() { return nil, errors.Errorf("package for %s is ill typed", i.File.File().Identity().URI) } // Do not rename identifiers declared in another package. - if pkg.GetTypes() != i.Declaration.obj.Pkg() { + if i.pkg.GetTypes() != i.Declaration.obj.Pkg() { return nil, errors.Errorf("failed to rename because %q is declared in package %q", i.Name, i.Declaration.obj.Pkg().Name()) } @@ -184,11 +180,7 @@ func (i *IdentifierInfo) getPkgName(ctx context.Context) (*IdentifierInfo, error file *ast.File err error ) - pkg, err := bestPackage(i.File.File().Identity().URI, i.pkgs) - if err != nil { - return nil, err - } - for _, ph := range pkg.GetHandles() { + for _, ph := range i.pkg.Files() { if ph.File().Identity().URI == i.File.File().Identity().URI { file, _, err = ph.Cached(ctx) } @@ -208,13 +200,13 @@ func (i *IdentifierInfo) getPkgName(ctx context.Context) (*IdentifierInfo, error } // Look for the object defined at NamePos. - for _, obj := range pkg.GetTypesInfo().Defs { + for _, obj := range i.pkg.GetTypesInfo().Defs { pkgName, ok := obj.(*types.PkgName) if ok && pkgName.Pos() == namePos { return getPkgNameIdentifier(ctx, i, pkgName) } } - for _, obj := range pkg.GetTypesInfo().Implicits { + for _, obj := range i.pkg.GetTypesInfo().Implicits { pkgName, ok := obj.(*types.PkgName) if ok && pkgName.Pos() == namePos { return getPkgNameIdentifier(ctx, i, pkgName) @@ -230,14 +222,11 @@ func getPkgNameIdentifier(ctx context.Context, ident *IdentifierInfo, pkgName *t obj: pkgName, wasImplicit: true, } - pkg, err := bestPackage(ident.File.File().Identity().URI, ident.pkgs) - if err != nil { + var err error + if decl.mappedRange, err = objToMappedRange(ctx, ident.View, ident.pkg, decl.obj); err != nil { return nil, err } - if decl.mappedRange, err = objToMappedRange(ctx, ident.View, pkg, decl.obj); err != nil { - return nil, err - } - if decl.node, err = objToNode(ctx, ident.View, pkg, decl.obj); err != nil { + if decl.node, err = objToNode(ctx, ident.View, ident.pkg, decl.obj); err != nil { return nil, err } return &IdentifierInfo{ @@ -246,7 +235,7 @@ func getPkgNameIdentifier(ctx context.Context, ident *IdentifierInfo, pkgName *t mappedRange: decl.mappedRange, File: ident.File, Declaration: decl, - pkgs: ident.pkgs, + pkg: ident.pkg, wasEmbeddedField: false, qf: ident.qf, }, nil diff --git a/internal/lsp/source/signature_help.go b/internal/lsp/source/signature_help.go index 6260e50c518..4ede8cc7dde 100644 --- a/internal/lsp/source/signature_help.go +++ b/internal/lsp/source/signature_help.go @@ -31,16 +31,17 @@ func SignatureHelp(ctx context.Context, view View, f GoFile, pos protocol.Positi ctx, done := trace.StartSpan(ctx, "source.SignatureHelp") defer done() - pkgs, err := f.GetPackages(ctx) + cphs, err := f.CheckPackageHandles(ctx) if err != nil { return nil, err } - pkg, err := bestPackage(f.URI(), pkgs) + cph := NarrowestCheckPackageHandle(cphs) + pkg, err := cph.Check(ctx) if err != nil { return nil, err } var ph ParseGoHandle - for _, h := range pkg.GetHandles() { + for _, h := range pkg.Files() { if h.File().Identity().URI == f.URI() { ph = h break diff --git a/internal/lsp/source/symbols.go b/internal/lsp/source/symbols.go index 42bfaa396c6..05bb6f4387a 100644 --- a/internal/lsp/source/symbols.go +++ b/internal/lsp/source/symbols.go @@ -18,7 +18,12 @@ func DocumentSymbols(ctx context.Context, view View, f GoFile) ([]protocol.Docum ctx, done := trace.StartSpan(ctx, "source.DocumentSymbols") defer done() - pkg, err := f.GetPackage(ctx) + cphs, err := f.CheckPackageHandles(ctx) + if err != nil { + return nil, err + } + cph := NarrowestCheckPackageHandle(cphs) + pkg, err := cph.Check(ctx) if err != nil { return nil, err } @@ -26,7 +31,7 @@ func DocumentSymbols(ctx context.Context, view View, f GoFile) ([]protocol.Docum file *ast.File m *protocol.ColumnMapper ) - for _, ph := range pkg.GetHandles() { + for _, ph := range pkg.Files() { if ph.File().Identity().URI == f.URI() { file, m, err = ph.Cached(ctx) } diff --git a/internal/lsp/source/util.go b/internal/lsp/source/util.go index c992be0e074..5b563d67b38 100644 --- a/internal/lsp/source/util.go +++ b/internal/lsp/source/util.go @@ -51,22 +51,39 @@ func (s mappedRange) URI() span.URI { return s.m.URI } -// bestPackage picks the "narrowest" package for a given file. +// NarrowestCheckPackageHandle picks the "narrowest" package for a given file. // // By "narrowest" package, we mean the package with the fewest number of files // that includes the given file. This solves the problem of test variants, // as the test will have more files than the non-test package. -func bestPackage(uri span.URI, pkgs []Package) (Package, error) { - var result Package - for _, pkg := range pkgs { - if result == nil || len(pkg.GetHandles()) < len(result.GetHandles()) { - result = pkg +func NarrowestCheckPackageHandle(handles []CheckPackageHandle) CheckPackageHandle { + if len(handles) < 1 { + return nil + } + result := handles[0] + for _, handle := range handles[1:] { + if result == nil || len(handle.Files()) < len(result.Files()) { + result = handle } } - if result == nil { - return nil, errors.Errorf("no CheckPackageHandle for %s", uri) + return result +} + +// WidestCheckPackageHandle returns the CheckPackageHandle containing the most files. +// +// This is useful for something like diagnostics, where we'd prefer to offer diagnostics +// for as many files as possible. +func WidestCheckPackageHandle(handles []CheckPackageHandle) CheckPackageHandle { + if len(handles) < 1 { + return nil } - return result, nil + result := handles[0] + for _, handle := range handles[1:] { + if result == nil || len(handle.Files()) > len(result.Files()) { + result = handle + } + } + return result } func IsGenerated(ctx context.Context, view View, uri span.URI) bool { diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index 6920312cad2..0fbdf483fef 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -265,23 +265,9 @@ type File interface { type GoFile interface { File - // GetCachedPackage returns the cached package for the file, if any. - GetCachedPackage(ctx context.Context) (Package, error) - - // GetCachedPackage returns the cached package for the file, if any. - GetCachedPackages(ctx context.Context) ([]Package, error) - - // GetPackage returns the CheckPackageHandle for the package that this file belongs to. - GetCheckPackageHandle(ctx context.Context) (CheckPackageHandle, error) - - // GetPackages returns the CheckPackageHandles of the packages that this file belongs to. - GetCheckPackageHandles(ctx context.Context) ([]CheckPackageHandle, error) - - // GetPackage returns the Package that this file belongs to. - GetPackage(ctx context.Context) (Package, error) - - // GetPackages returns the Packages that this file belongs to. - GetPackages(ctx context.Context) ([]Package, error) + // GetCheckPackageHandles returns the CheckPackageHandles for the packages + // that this file belongs to. + CheckPackageHandles(ctx context.Context) ([]CheckPackageHandle, error) // GetActiveReverseDeps returns the active files belonging to the reverse // dependencies of this file's package. @@ -301,7 +287,7 @@ type SumFile interface { type Package interface { ID() string PkgPath() string - GetHandles() []ParseGoHandle + Files() []ParseGoHandle GetSyntax(context.Context) []*ast.File GetErrors() []packages.Error GetTypes() *types.Package diff --git a/internal/lsp/text_synchronization.go b/internal/lsp/text_synchronization.go index 76ba2e80491..96066eecda7 100644 --- a/internal/lsp/text_synchronization.go +++ b/internal/lsp/text_synchronization.go @@ -172,17 +172,21 @@ func (s *Server) didClose(ctx context.Context, params *protocol.DidCloseTextDocu log.Error(ctx, "closing a non-Go file, no diagnostics to clear", nil, telemetry.File) return nil } - pkg, err := gof.GetPackage(ctx) + cphs, err := gof.CheckPackageHandles(ctx) if err != nil { - return err + log.Error(ctx, "no CheckPackageHandles", err, telemetry.URI.Of(gof.URI())) + return nil } - for _, ph := range pkg.GetHandles() { - // If other files from this package are open, don't clear. - if s.session.IsOpen(ph.File().Identity().URI) { - clear = nil - return nil + for _, cph := range cphs { + for _, ph := range cph.Files() { + // If other files from this package are open, don't clear. + if s.session.IsOpen(ph.File().Identity().URI) { + clear = nil + return nil + } + clear = append(clear, ph.File().Identity().URI) } - clear = append(clear, ph.File().Identity().URI) } + return nil } diff --git a/internal/lsp/watched_files.go b/internal/lsp/watched_files.go index 98315e46f65..5740149ee97 100644 --- a/internal/lsp/watched_files.go +++ b/internal/lsp/watched_files.go @@ -6,6 +6,7 @@ package lsp import ( "context" + "sort" "golang.org/x/tools/internal/lsp/protocol" "golang.org/x/tools/internal/lsp/source" @@ -58,17 +59,22 @@ func (s *Server) didChangeWatchedFiles(ctx context.Context, params *protocol.Did case protocol.Deleted: log.Print(ctx, "watched file deleted", telemetry.File) - pkg, err := gof.GetPackage(ctx) + cphs, err := gof.CheckPackageHandles(ctx) if err != nil { log.Error(ctx, "didChangeWatchedFiles: GetPackage", err, telemetry.File) continue } - - // Find a different file in the same package we can use to - // trigger diagnostics. + // Find a different file in the same package we can use to trigger diagnostics. + // TODO(rstambler): Allow diagnostics to be called per-package to avoid this. var otherFile source.GoFile - for _, pgh := range pkg.GetHandles() { - ident := pgh.File().Identity() + sort.Slice(cphs, func(i, j int) bool { + return len(cphs[i].Files()) > len(cphs[j].Files()) + }) + for _, ph := range cphs[0].Files() { + if len(cphs) > 1 && contains(cphs[1], ph.File()) { + continue + } + ident := ph.File().Identity() if ident.URI == gof.URI() { continue } @@ -77,7 +83,6 @@ func (s *Server) didChangeWatchedFiles(ctx context.Context, params *protocol.Did break } } - s.session.DidChangeOutOfBand(ctx, gof, change.Type) if otherFile != nil { @@ -95,6 +100,14 @@ func (s *Server) didChangeWatchedFiles(ctx context.Context, params *protocol.Did } } } - return nil } + +func contains(cph source.CheckPackageHandle, fh source.FileHandle) bool { + for _, ph := range cph.Files() { + if ph.File().Identity().URI == fh.Identity().URI { + return true + } + } + return false +}