From 818555187f961fc0f6b11cdb7163349e41ba75f5 Mon Sep 17 00:00:00 2001 From: Heschi Kreinick Date: Tue, 5 Nov 2019 14:53:55 -0500 Subject: [PATCH] internal/lsp/source: attach Package to completions when available Unimported completions now try to pull Packages from everywhere, not just the transitive dependencies of the current package. That confused the import formatting code, which only looked at deps. Pass the Package along with the import suggestion, and use it when it's present. Also change some error messages to be different for diagnostic purposes. Fixes golang/go#35359. Change-Id: Ia8ca923e46723e855ddd2da7611e6eb13c02bb4f Reviewed-on: https://go-review.googlesource.com/c/tools/+/205501 Run-TryBot: Heschi Kreinick TryBot-Result: Gobot Gobot Reviewed-by: Rebecca Stambler --- internal/lsp/cache/pkg.go | 2 +- internal/lsp/source/completion.go | 36 ++++++++---- internal/lsp/source/completion_format.go | 18 ++++-- internal/lsp/source/completion_literal.go | 3 +- internal/lsp/source/deep_completion.go | 4 +- internal/lsp/source/imports_test.go | 68 +++++++++++------------ 6 files changed, 75 insertions(+), 56 deletions(-) diff --git a/internal/lsp/cache/pkg.go b/internal/lsp/cache/pkg.go index 613f8097f9..bf398a2da6 100644 --- a/internal/lsp/cache/pkg.go +++ b/internal/lsp/cache/pkg.go @@ -147,5 +147,5 @@ func findFileInPackage(ctx context.Context, uri span.URI, pkg source.Package) (s } } } - return nil, nil, errors.Errorf("no file for %s", uri) + return nil, nil, errors.Errorf("no file for %s in package %s", uri, pkg.ID()) } diff --git a/internal/lsp/source/completion.go b/internal/lsp/source/completion.go index 70be745229..2b79350dc7 100644 --- a/internal/lsp/source/completion.go +++ b/internal/lsp/source/completion.go @@ -14,7 +14,6 @@ import ( "time" "golang.org/x/tools/go/ast/astutil" - "golang.org/x/tools/internal/imports" "golang.org/x/tools/internal/lsp/fuzzy" "golang.org/x/tools/internal/lsp/protocol" "golang.org/x/tools/internal/lsp/snippet" @@ -221,6 +220,12 @@ type compLitInfo struct { maybeInFieldName bool } +type importInfo struct { + importPath string + name string + pkg Package +} + type methodSetKey struct { typ types.Type addressable bool @@ -284,7 +289,7 @@ func (c *completer) getSurrounding() *Selection { // found adds a candidate completion. We will also search through the object's // members for more candidates. -func (c *completer) found(obj types.Object, score float64, imp *imports.ImportInfo) { +func (c *completer) found(obj types.Object, score float64, imp *importInfo) { if obj.Pkg() != nil && obj.Pkg() != c.pkg.GetTypes() && !obj.Exported() { // obj is not accessible because it lives in another package and is not // exported. Don't treat it as a completion candidate. @@ -370,7 +375,7 @@ type candidate struct { // imp is the import that needs to be added to this package in order // for this candidate to be valid. nil if no import needed. - imp *imports.ImportInfo + imp *importInfo } // ErrIsDefinition is an error that informs the user they got no @@ -591,28 +596,35 @@ func (c *completer) selector(sel *ast.SelectorExpr) error { for _, pkgExport := range pkgExports { // If we've seen this import path, use the fully-typed version. if knownPkg, ok := known[pkgExport.Fix.StmtInfo.ImportPath]; ok { - c.packageMembers(knownPkg.GetTypes(), &pkgExport.Fix.StmtInfo) + c.packageMembers(knownPkg.GetTypes(), &importInfo{ + importPath: pkgExport.Fix.StmtInfo.ImportPath, + name: pkgExport.Fix.StmtInfo.Name, + pkg: knownPkg, + }) continue } // Otherwise, continue with untyped proposals. pkg := types.NewPackage(pkgExport.Fix.StmtInfo.ImportPath, pkgExport.Fix.IdentName) for _, export := range pkgExport.Exports { - c.found(types.NewVar(0, pkg, export, nil), 0.07, &pkgExport.Fix.StmtInfo) + c.found(types.NewVar(0, pkg, export, nil), 0.07, &importInfo{ + importPath: pkgExport.Fix.StmtInfo.ImportPath, + name: pkgExport.Fix.StmtInfo.Name, + }) } } } return nil } -func (c *completer) packageMembers(pkg *types.Package, imp *imports.ImportInfo) { +func (c *completer) packageMembers(pkg *types.Package, imp *importInfo) { scope := pkg.Scope() for _, name := range scope.Names() { c.found(scope.Lookup(name), stdScore, imp) } } -func (c *completer) methodsAndFields(typ types.Type, addressable bool, imp *imports.ImportInfo) error { +func (c *completer) methodsAndFields(typ types.Type, addressable bool, imp *importInfo) error { mset := c.methodSetCache[methodSetKey{typ, addressable}] if mset == nil { if addressable && !types.IsInterface(typ) && !isPointer(typ) { @@ -716,8 +728,9 @@ func (c *completer) lexical() error { if _, ok := seen[pkg.Name()]; !ok && pkg != c.pkg.GetTypes() && !alreadyImports(c.file, pkg.Path()) { seen[pkg.Name()] = struct{}{} obj := types.NewPkgName(0, nil, pkg.Name(), pkg) - c.found(obj, stdScore, &imports.ImportInfo{ - ImportPath: pkg.Path(), + c.found(obj, stdScore, &importInfo{ + importPath: pkg.Path(), + name: pkg.Name(), }) } } @@ -739,7 +752,10 @@ func (c *completer) lexical() error { // multiple packages of the same name as completion suggestions, since // only one will be chosen. obj := types.NewPkgName(0, nil, pkg.IdentName, types.NewPackage(pkg.StmtInfo.ImportPath, pkg.IdentName)) - c.found(obj, score, &pkg.StmtInfo) + c.found(obj, score, &importInfo{ + importPath: pkg.StmtInfo.ImportPath, + name: pkg.StmtInfo.Name, + }) } } } diff --git a/internal/lsp/source/completion_format.go b/internal/lsp/source/completion_format.go index 000e664e2c..05b36b9fc9 100644 --- a/internal/lsp/source/completion_format.go +++ b/internal/lsp/source/completion_format.go @@ -13,7 +13,6 @@ import ( "go/types" "strings" - "golang.org/x/tools/internal/imports" "golang.org/x/tools/internal/lsp/protocol" "golang.org/x/tools/internal/lsp/snippet" "golang.org/x/tools/internal/span" @@ -108,7 +107,7 @@ func (c *completer) item(cand candidate) (CompletionItem, error) { if detail != "" { detail += " " } - detail += fmt.Sprintf("(from %q)", cand.imp.ImportPath) + detail += fmt.Sprintf("(from %q)", cand.imp.importPath) } } @@ -135,7 +134,14 @@ func (c *completer) item(cand candidate) (CompletionItem, error) { return item, nil } uri := span.FileURI(pos.Filename) - ph, pkg, err := c.view.FindFileInPackage(c.ctx, uri, c.pkg) + + // Find the source file of the candidate, starting from a package + // that should have it in its dependencies. + searchPkg := c.pkg + if cand.imp != nil && cand.imp.pkg != nil { + searchPkg = cand.imp.pkg + } + ph, pkg, err := c.view.FindFileInPackage(c.ctx, uri, searchPkg) if err != nil { return CompletionItem{}, err } @@ -144,7 +150,7 @@ func (c *completer) item(cand candidate) (CompletionItem, error) { return CompletionItem{}, err } if !(file.Pos() <= obj.Pos() && obj.Pos() <= file.End()) { - return CompletionItem{}, errors.Errorf("no file for %s", obj.Name()) + return CompletionItem{}, errors.Errorf("no file for completion object %s", obj.Name()) } ident, err := findIdentifier(c.ctx, c.snapshot, pkg, file, obj.Pos()) if err != nil { @@ -162,12 +168,12 @@ func (c *completer) item(cand candidate) (CompletionItem, error) { } // importEdits produces the text edits necessary to add the given import to the current file. -func (c *completer) importEdits(imp *imports.ImportInfo) ([]protocol.TextEdit, error) { +func (c *completer) importEdits(imp *importInfo) ([]protocol.TextEdit, error) { if imp == nil { return nil, nil } - edit, err := addNamedImport(c.view.Session().Cache().FileSet(), c.file, imp.Name, imp.ImportPath) + edit, err := addNamedImport(c.view.Session().Cache().FileSet(), c.file, imp.name, imp.importPath) if err != nil { return nil, err } diff --git a/internal/lsp/source/completion_literal.go b/internal/lsp/source/completion_literal.go index 110f9c695c..b87f1eceef 100644 --- a/internal/lsp/source/completion_literal.go +++ b/internal/lsp/source/completion_literal.go @@ -11,7 +11,6 @@ import ( "strings" "unicode" - "golang.org/x/tools/internal/imports" "golang.org/x/tools/internal/lsp/diff" "golang.org/x/tools/internal/lsp/protocol" "golang.org/x/tools/internal/lsp/snippet" @@ -21,7 +20,7 @@ import ( // literal generates composite literal, function literal, and make() // completion items. -func (c *completer) literal(literalType types.Type, imp *imports.ImportInfo) { +func (c *completer) literal(literalType types.Type, imp *importInfo) { if c.expectedType.objType == nil { return } diff --git a/internal/lsp/source/deep_completion.go b/internal/lsp/source/deep_completion.go index eddacac752..8f6995e46b 100644 --- a/internal/lsp/source/deep_completion.go +++ b/internal/lsp/source/deep_completion.go @@ -8,8 +8,6 @@ import ( "go/types" "strings" "time" - - "golang.org/x/tools/internal/imports" ) // Limit deep completion results because in most cases there are too many @@ -142,7 +140,7 @@ func (c *completer) shouldPrune() bool { // deepSearch searches through obj's subordinate objects for more // completion items. -func (c *completer) deepSearch(obj types.Object, imp *imports.ImportInfo) { +func (c *completer) deepSearch(obj types.Object, imp *importInfo) { if c.deepState.maxDepth == 0 { return } diff --git a/internal/lsp/source/imports_test.go b/internal/lsp/source/imports_test.go index e10c082c6f..0927c59948 100644 --- a/internal/lsp/source/imports_test.go +++ b/internal/lsp/source/imports_test.go @@ -35,11 +35,11 @@ type test struct { renamedPkg string pkg string in string - want []importInfo + want []imp unchanged bool // Expect added/deleted return value to be false. } -type importInfo struct { +type imp struct { name string path string } @@ -54,8 +54,8 @@ import ( "os" ) `, - want: []importInfo{ - importInfo{ + want: []imp{ + { name: "", path: "os", }, @@ -67,8 +67,8 @@ import ( pkg: "os", in: `package main `, - want: []importInfo{ - importInfo{ + want: []imp{ + { name: "", path: "os", }, @@ -78,8 +78,8 @@ import ( name: "package statement no new line", pkg: "os", in: `package main`, - want: []importInfo{ - importInfo{ + want: []imp{ + { name: "", path: "os", }, @@ -92,8 +92,8 @@ import ( in: `// Here is a comment before package main `, - want: []importInfo{ - importInfo{ + want: []imp{ + { name: "", path: "os", }, @@ -104,8 +104,8 @@ package main pkg: "os", in: `package main // Here is a comment after `, - want: []importInfo{ - importInfo{ + want: []imp{ + { name: "", path: "os", }, @@ -116,8 +116,8 @@ package main pkg: "os", in: `// Here is a comment before package main // Here is a comment after`, - want: []importInfo{ - importInfo{ + want: []imp{ + { name: "", path: "os", }, @@ -129,8 +129,8 @@ package main // Here is a comment after`, in: `package main /* This is a multiline comment and it extends further down*/`, - want: []importInfo{ - importInfo{ + want: []imp{ + { name: "", path: "os", }, @@ -143,12 +143,12 @@ further down*/`, import "C" `, - want: []importInfo{ - importInfo{ + want: []imp{ + { name: "", path: "os", }, - importInfo{ + { name: "", path: "C", }, @@ -161,12 +161,12 @@ import "C" import "io" `, - want: []importInfo{ - importInfo{ + want: []imp{ + { name: "", path: "os", }, - importInfo{ + { name: "", path: "io", }, @@ -179,12 +179,12 @@ import "io" import "io" // A comment `, - want: []importInfo{ - importInfo{ + want: []imp{ + { name: "", path: "os", }, - importInfo{ + { name: "", path: "io", }, @@ -199,12 +199,12 @@ import "io" /* A comment that extends */ `, - want: []importInfo{ - importInfo{ + want: []imp{ + { name: "", path: "os", }, - importInfo{ + { name: "", path: "io", }, @@ -216,8 +216,8 @@ extends */ pkg: "os", in: `package main `, - want: []importInfo{ - importInfo{ + want: []imp{ + { name: "o", path: "os", }, @@ -280,12 +280,12 @@ func TestDoubleAddNamedImport(t *testing.T) { got = applyEdits(got, edits) gotFile = parse(t, name, got) - want := []importInfo{ - importInfo{ + want := []imp{ + { name: "o", path: "os", }, - importInfo{ + { name: "i", path: "io", }, @@ -293,7 +293,7 @@ func TestDoubleAddNamedImport(t *testing.T) { compareImports(t, "", gotFile.Imports, want) } -func compareImports(t *testing.T, prefix string, got []*ast.ImportSpec, want []importInfo) { +func compareImports(t *testing.T, prefix string, got []*ast.ImportSpec, want []imp) { if len(got) != len(want) { t.Errorf("%s\ngot %d imports\nwant %d", prefix, len(got), len(want)) return