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 <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Rebecca Stambler <[email protected]>
(cherry picked from commit 8185551)
Reviewed-on: https://go-review.googlesource.com/c/tools/+/205658
Run-TryBot: Rebecca Stambler <[email protected]>
Reviewed-by: Heschi Kreinick <[email protected]>

Author: heschi

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())
 }

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,
+				})
 			}
 		}
 	}

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
 	}

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
 	}

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
 	}

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,20 +280,20 @@ 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",
 		},
 	}
 	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