gopls/internal/lsp/source: Package clarifications
Some preparatory simplifications:
- Group the fields of Package into metadata, parse info, type info.
- Derive TypesSizes from metadata; eliminate field.
- Remove MissingDependencies method by specializing it for its sole use,
avoiding the need to invert the go/types heuristic.
Also, clarify sort in codelens.
Change-Id: Iecd5b49046207034c64acd5b0240a9fed7761538
Reviewed-on: https://go-review.googlesource.com/c/tools/+/452835
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
diff --git a/gopls/internal/lsp/cache/check.go b/gopls/internal/lsp/cache/check.go
index a22e285..663cf25 100644
--- a/gopls/internal/lsp/cache/check.go
+++ b/gopls/internal/lsp/cache/check.go
@@ -457,7 +457,6 @@
Selections: make(map[*ast.SelectorExpr]*types.Selection),
Scopes: make(map[ast.Node]*types.Scope),
},
- typesSizes: m.TypesSizes,
}
typeparams.InitInstanceInfo(pkg.typesInfo)
diff --git a/gopls/internal/lsp/cache/pkg.go b/gopls/internal/lsp/cache/pkg.go
index 5c43a82..6466b8e 100644
--- a/gopls/internal/lsp/cache/pkg.go
+++ b/gopls/internal/lsp/cache/pkg.go
@@ -10,7 +10,6 @@
"go/scanner"
"go/token"
"go/types"
- "sort"
"golang.org/x/mod/module"
"golang.org/x/tools/gopls/internal/lsp/source"
@@ -40,7 +39,6 @@
typeErrors []types.Error
types *types.Package
typesInfo *types.Info
- typesSizes types.Sizes
hasFixedFiles bool // if true, AST was sufficiently mangled that we should hide type errors
analyses memoize.Store // maps analyzer.Name to Promise[actionResult]
@@ -113,7 +111,7 @@
}
func (p *pkg) GetTypesSizes() types.Sizes {
- return p.typesSizes
+ return p.m.TypesSizes
}
func (p *pkg) ForTest() string {
@@ -144,41 +142,6 @@
return nil, fmt.Errorf("package does not import %s", importPath)
}
-func (p *pkg) MissingDependencies() []ImportPath {
- // We don't invalidate metadata for import deletions,
- // so check the package imports via the *types.Package.
- //
- // rfindley says: it looks like this is intending to implement
- // a heuristic "if go list couldn't resolve import paths to
- // packages, then probably you're not in GOPATH or a module".
- // This is used to determine if we need to show a warning diagnostic.
- // It looks like this logic is implementing the heuristic that
- // "even if the metadata has a MissingDep, if the types.Package
- // doesn't need that dep anymore we shouldn't show the warning".
- // But either we're outside of GOPATH/Module, or we're not...
- //
- // adonovan says: I think this effectively reverses the
- // heuristic used by the type checker when Importer.Import
- // returns an error---go/types synthesizes a package whose
- // Path is the import path (sans "vendor/")---hence the
- // dubious ImportPath() conversion. A blank DepsByImpPath
- // entry means a missing import.
- //
- // If we invalidate the metadata for import deletions (which
- // should be fast) then we can simply return the blank entries
- // in DepsByImpPath. (They are PackageIDs not PackagePaths,
- // but the caller only cares whether the set is empty!)
- var missing []ImportPath
- for _, pkg := range p.types.Imports() {
- importPath := ImportPath(pkg.Path())
- if id, ok := p.m.DepsByImpPath[importPath]; ok && id == "" {
- missing = append(missing, importPath)
- }
- }
- sort.Slice(missing, func(i, j int) bool { return missing[i] < missing[j] })
- return missing
-}
-
func (p *pkg) Imports() []source.Package {
var result []source.Package // unordered
for _, dep := range p.deps {
diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go
index 9a4c5b3..369235f 100644
--- a/gopls/internal/lsp/cache/snapshot.go
+++ b/gopls/internal/lsp/cache/snapshot.go
@@ -1430,13 +1430,43 @@
return ""
}
for _, pkg := range pkgs {
- if len(pkg.MissingDependencies()) > 0 {
+ if hasMissingDependencies(pkg) {
return adHocPackagesWarning
}
}
return ""
}
+func hasMissingDependencies(pkg source.Package) bool {
+ // We don't invalidate metadata for import deletions,
+ // so check the package imports via the syntax tree
+ // (not via types.Package.Imports, since it contains packages
+ // synthesized under the vendoring-hostile assumption that
+ // ImportPath equals PackagePath).
+ //
+ // rfindley says: it looks like this is intending to implement
+ // a heuristic "if go list couldn't resolve import paths to
+ // packages, then probably you're not in GOPATH or a module".
+ // This is used to determine if we need to show a warning diagnostic.
+ // It looks like this logic is implementing the heuristic that
+ // "even if the metadata has a MissingDep, if the types.Package
+ // doesn't need that dep anymore we shouldn't show the warning".
+ // But either we're outside of GOPATH/Module, or we're not...
+ //
+ // If we invalidate the metadata for import deletions (which
+ // should be fast) then we can simply return the blank entries
+ // in DepsByImpPath.
+ for _, f := range pkg.GetSyntax() {
+ for _, imp := range f.Imports {
+ importPath := source.UnquoteImportPath(imp)
+ if _, err := pkg.ResolveImportPath(importPath); err != nil {
+ return true
+ }
+ }
+ }
+ return false
+}
+
func containsCommandLineArguments(pkgs []source.Package) bool {
for _, pkg := range pkgs {
if source.IsCommandLineArguments(pkg.ID()) {
diff --git a/gopls/internal/lsp/code_lens.go b/gopls/internal/lsp/code_lens.go
index 4bbe2bb..f554e79 100644
--- a/gopls/internal/lsp/code_lens.go
+++ b/gopls/internal/lsp/code_lens.go
@@ -9,11 +9,11 @@
"fmt"
"sort"
- "golang.org/x/tools/internal/event"
"golang.org/x/tools/gopls/internal/lsp/command"
"golang.org/x/tools/gopls/internal/lsp/mod"
"golang.org/x/tools/gopls/internal/lsp/protocol"
"golang.org/x/tools/gopls/internal/lsp/source"
+ "golang.org/x/tools/internal/event"
)
func (s *Server) codeLens(ctx context.Context, params *protocol.CodeLensParams) ([]protocol.CodeLens, error) {
@@ -48,10 +48,10 @@
}
sort.Slice(result, func(i, j int) bool {
a, b := result[i], result[j]
- if protocol.CompareRange(a.Range, b.Range) == 0 {
- return a.Command.Command < b.Command.Command
+ if cmp := protocol.CompareRange(a.Range, b.Range); cmp != 0 {
+ return cmp < 0
}
- return protocol.CompareRange(a.Range, b.Range) < 0
+ return a.Command.Command < b.Command.Command
})
return result, nil
}
diff --git a/gopls/internal/lsp/source/completion/completion.go b/gopls/internal/lsp/source/completion/completion.go
index 144d658..1eeabc9 100644
--- a/gopls/internal/lsp/source/completion/completion.go
+++ b/gopls/internal/lsp/source/completion/completion.go
@@ -1083,12 +1083,7 @@
// Is sel a qualified identifier?
if id, ok := sel.X.(*ast.Ident); ok {
if pkgName, ok := c.pkg.GetTypesInfo().Uses[id].(*types.PkgName); ok {
- var pkg source.Package
- for _, imp := range c.pkg.Imports() {
- if imp.PkgPath() == source.PackagePath(pkgName.Imported().Path()) {
- pkg = imp
- }
- }
+ pkg, _ := c.pkg.DirectDep(source.PackagePath(pkgName.Imported().Path()))
// If the package is not imported, try searching for unimported
// completions.
if pkg == nil && c.opts.unimported {
diff --git a/gopls/internal/lsp/source/view.go b/gopls/internal/lsp/source/view.go
index aed3f54..ad10129 100644
--- a/gopls/internal/lsp/source/view.go
+++ b/gopls/internal/lsp/source/view.go
@@ -676,28 +676,32 @@
ImportPath string // path that appears in an import declaration (e.g. "example.com/foo")
)
-// Package represents a Go package that has been type-checked. It maintains
-// only the relevant fields of a *go/packages.Package.
+// Package represents a Go package that has been parsed and type-checked.
+// It maintains only the relevant fields of a *go/packages.Package.
type Package interface {
+ // Metadata:
ID() PackageID
Name() PackageName
PkgPath() PackagePath
- CompiledGoFiles() []*ParsedGoFile
- File(uri span.URI) (*ParsedGoFile, error)
- FileSet() *token.FileSet
- GetSyntax() []*ast.File
- GetTypes() *types.Package
- GetTypesInfo() *types.Info
GetTypesSizes() types.Sizes
ForTest() string
+ Version() *module.Version
+
+ // Results of parsing:
+ FileSet() *token.FileSet
+ ParseMode() ParseMode
+ CompiledGoFiles() []*ParsedGoFile // (borrowed)
+ File(uri span.URI) (*ParsedGoFile, error)
+ GetSyntax() []*ast.File // (borrowed)
+ HasListOrParseErrors() bool
+
+ // Results of type checking:
+ GetTypes() *types.Package
+ GetTypesInfo() *types.Info
DirectDep(path PackagePath) (Package, error)
ResolveImportPath(path ImportPath) (Package, error)
- MissingDependencies() []ImportPath // unordered
- Imports() []Package
- Version() *module.Version
- HasListOrParseErrors() bool
+ Imports() []Package // new slice of all direct dependencies, unordered
HasTypeErrors() bool
- ParseMode() ParseMode
}
// A CriticalError is a workspace-wide error that generally prevents gopls from
