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 <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
diff --git a/internal/lsp/cache/check.go b/internal/lsp/cache/check.go
index 2ccf62f..d724926 100644
--- a/internal/lsp/cache/check.go
+++ b/internal/lsp/cache/check.go
@@ -153,7 +153,7 @@
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 @@
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 c8ffc1f..7bf6fd5 100644
--- a/internal/lsp/cache/gofile.go
+++ b/internal/lsp/cache/gofile.go
@@ -11,8 +11,6 @@
"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 @@
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 @@
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 @@
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 14b5879..d153af2 100644
--- a/internal/lsp/cache/load.go
+++ b/internal/lsp/cache/load.go
@@ -59,7 +59,7 @@
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 @@
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 @@
// 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 8a0454f..ba7fed3 100644
--- a/internal/lsp/cache/pkg.go
+++ b/internal/lsp/cache/pkg.go
@@ -147,7 +147,7 @@
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 0aa294b..b16e34e 100644
--- a/internal/lsp/cache/view.go
+++ b/internal/lsp/cache/view.go
@@ -330,7 +330,7 @@
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 @@
// 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 @@
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 fe87a1a..a8c7e8c 100644
--- a/internal/lsp/code_action.go
+++ b/internal/lsp/code_action.go
@@ -201,7 +201,12 @@
// 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 e9281c0..17a8bc5 100644
--- a/internal/lsp/source/completion.go
+++ b/internal/lsp/source/completion.go
@@ -367,12 +367,17 @@
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 162ca2b..561e5d8 100644
--- a/internal/lsp/source/completion_format.go
+++ b/internal/lsp/source/completion_format.go
@@ -136,7 +136,7 @@
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 79c2fb5..009d892 100644
--- a/internal/lsp/source/diagnostics.go
+++ b/internal/lsp/source/diagnostics.go
@@ -69,22 +69,11 @@
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 @@
}
// 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 @@
// 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 @@
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 @@
}
// 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 3172224..b750b54 100644
--- a/internal/lsp/source/format.go
+++ b/internal/lsp/source/format.go
@@ -28,12 +28,17 @@
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 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 @@
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 @@
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 @@
}
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 7f15a99..9088e9a 100644
--- a/internal/lsp/source/identifier.go
+++ b/internal/lsp/source/identifier.go
@@ -32,7 +32,7 @@
Declaration Declaration
- pkgs []Package
+ pkg Package
ident *ast.Ident
wasEmbeddedField bool
qf types.Qualifier
@@ -48,16 +48,17 @@
// 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 @@
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 @@
}
// 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 @@
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 @@
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 @@
}
// 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 @@
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 @@
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 1d1a993..84c9d4d 100644
--- a/internal/lsp/source/references.go
+++ b/internal/lsp/source/references.go
@@ -28,64 +28,62 @@
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 6f14547..0d5f3bb 100644
--- a/internal/lsp/source/rename.go
+++ b/internal/lsp/source/rename.go
@@ -102,15 +102,11 @@
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 @@
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 @@
}
// 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 @@
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 @@
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 6260e50..4ede8cc 100644
--- a/internal/lsp/source/signature_help.go
+++ b/internal/lsp/source/signature_help.go
@@ -31,16 +31,17 @@
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 42bfaa3..05bb6f4 100644
--- a/internal/lsp/source/symbols.go
+++ b/internal/lsp/source/symbols.go
@@ -18,7 +18,12 @@
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 @@
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 c992be0..5b563d6 100644
--- a/internal/lsp/source/util.go
+++ b/internal/lsp/source/util.go
@@ -51,22 +51,39 @@
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 6920312..0fbdf48 100644
--- a/internal/lsp/source/view.go
+++ b/internal/lsp/source/view.go
@@ -265,23 +265,9 @@
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 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 76ba2e8..96066ee 100644
--- a/internal/lsp/text_synchronization.go
+++ b/internal/lsp/text_synchronization.go
@@ -172,17 +172,21 @@
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 98315e4..5740149 100644
--- a/internal/lsp/watched_files.go
+++ b/internal/lsp/watched_files.go
@@ -6,6 +6,7 @@
import (
"context"
+ "sort"
"golang.org/x/tools/internal/lsp/protocol"
"golang.org/x/tools/internal/lsp/source"
@@ -58,17 +59,22 @@
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 @@
break
}
}
-
s.session.DidChangeOutOfBand(ctx, gof, change.Type)
if otherFile != nil {
@@ -95,6 +100,14 @@
}
}
}
-
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
+}
