Skip to content

Commit 8185551

Browse files
heschiheschi
committed
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]>
1 parent dc03839 commit 8185551

File tree

6 files changed

+75
-56
lines changed

6 files changed

+75
-56
lines changed

‎internal/lsp/cache/pkg.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -147,5 +147,5 @@ func findFileInPackage(ctx context.Context, uri span.URI, pkg source.Package) (s
147147
}
148148
}
149149
}
150-
return nil, nil, errors.Errorf("no file for %s", uri)
150+
return nil, nil, errors.Errorf("no file for %s in package %s", uri, pkg.ID())
151151
}

‎internal/lsp/source/completion.go

Lines changed: 26 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@ import (
1414
"time"
1515

1616
"golang.org/x/tools/go/ast/astutil"
17-
"golang.org/x/tools/internal/imports"
1817
"golang.org/x/tools/internal/lsp/fuzzy"
1918
"golang.org/x/tools/internal/lsp/protocol"
2019
"golang.org/x/tools/internal/lsp/snippet"
@@ -221,6 +220,12 @@ type compLitInfo struct {
221220
maybeInFieldName bool
222221
}
223222

223+
type importInfo struct {
224+
importPath string
225+
name string
226+
pkg Package
227+
}
228+
224229
type methodSetKey struct {
225230
typ types.Type
226231
addressable bool
@@ -284,7 +289,7 @@ func (c *completer) getSurrounding() *Selection {
284289

285290
// found adds a candidate completion. We will also search through the object's
286291
// members for more candidates.
287-
func (c *completer) found(obj types.Object, score float64, imp *imports.ImportInfo) {
292+
func (c *completer) found(obj types.Object, score float64, imp *importInfo) {
288293
if obj.Pkg() != nil && obj.Pkg() != c.pkg.GetTypes() && !obj.Exported() {
289294
// obj is not accessible because it lives in another package and is not
290295
// exported. Don't treat it as a completion candidate.
@@ -370,7 +375,7 @@ type candidate struct {
370375

371376
// imp is the import that needs to be added to this package in order
372377
// for this candidate to be valid. nil if no import needed.
373-
imp *imports.ImportInfo
378+
imp *importInfo
374379
}
375380

376381
// ErrIsDefinition is an error that informs the user they got no
@@ -591,28 +596,35 @@ func (c *completer) selector(sel *ast.SelectorExpr) error {
591596
for _, pkgExport := range pkgExports {
592597
// If we've seen this import path, use the fully-typed version.
593598
if knownPkg, ok := known[pkgExport.Fix.StmtInfo.ImportPath]; ok {
594-
c.packageMembers(knownPkg.GetTypes(), &pkgExport.Fix.StmtInfo)
599+
c.packageMembers(knownPkg.GetTypes(), &importInfo{
600+
importPath: pkgExport.Fix.StmtInfo.ImportPath,
601+
name: pkgExport.Fix.StmtInfo.Name,
602+
pkg: knownPkg,
603+
})
595604
continue
596605
}
597606

598607
// Otherwise, continue with untyped proposals.
599608
pkg := types.NewPackage(pkgExport.Fix.StmtInfo.ImportPath, pkgExport.Fix.IdentName)
600609
for _, export := range pkgExport.Exports {
601-
c.found(types.NewVar(0, pkg, export, nil), 0.07, &pkgExport.Fix.StmtInfo)
610+
c.found(types.NewVar(0, pkg, export, nil), 0.07, &importInfo{
611+
importPath: pkgExport.Fix.StmtInfo.ImportPath,
612+
name: pkgExport.Fix.StmtInfo.Name,
613+
})
602614
}
603615
}
604616
}
605617
return nil
606618
}
607619

608-
func (c *completer) packageMembers(pkg *types.Package, imp *imports.ImportInfo) {
620+
func (c *completer) packageMembers(pkg *types.Package, imp *importInfo) {
609621
scope := pkg.Scope()
610622
for _, name := range scope.Names() {
611623
c.found(scope.Lookup(name), stdScore, imp)
612624
}
613625
}
614626

615-
func (c *completer) methodsAndFields(typ types.Type, addressable bool, imp *imports.ImportInfo) error {
627+
func (c *completer) methodsAndFields(typ types.Type, addressable bool, imp *importInfo) error {
616628
mset := c.methodSetCache[methodSetKey{typ, addressable}]
617629
if mset == nil {
618630
if addressable && !types.IsInterface(typ) && !isPointer(typ) {
@@ -716,8 +728,9 @@ func (c *completer) lexical() error {
716728
if _, ok := seen[pkg.Name()]; !ok && pkg != c.pkg.GetTypes() && !alreadyImports(c.file, pkg.Path()) {
717729
seen[pkg.Name()] = struct{}{}
718730
obj := types.NewPkgName(0, nil, pkg.Name(), pkg)
719-
c.found(obj, stdScore, &imports.ImportInfo{
720-
ImportPath: pkg.Path(),
731+
c.found(obj, stdScore, &importInfo{
732+
importPath: pkg.Path(),
733+
name: pkg.Name(),
721734
})
722735
}
723736
}
@@ -739,7 +752,10 @@ func (c *completer) lexical() error {
739752
// multiple packages of the same name as completion suggestions, since
740753
// only one will be chosen.
741754
obj := types.NewPkgName(0, nil, pkg.IdentName, types.NewPackage(pkg.StmtInfo.ImportPath, pkg.IdentName))
742-
c.found(obj, score, &pkg.StmtInfo)
755+
c.found(obj, score, &importInfo{
756+
importPath: pkg.StmtInfo.ImportPath,
757+
name: pkg.StmtInfo.Name,
758+
})
743759
}
744760
}
745761
}

‎internal/lsp/source/completion_format.go

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ import (
1313
"go/types"
1414
"strings"
1515

16-
"golang.org/x/tools/internal/imports"
1716
"golang.org/x/tools/internal/lsp/protocol"
1817
"golang.org/x/tools/internal/lsp/snippet"
1918
"golang.org/x/tools/internal/span"
@@ -108,7 +107,7 @@ func (c *completer) item(cand candidate) (CompletionItem, error) {
108107
if detail != "" {
109108
detail += " "
110109
}
111-
detail += fmt.Sprintf("(from %q)", cand.imp.ImportPath)
110+
detail += fmt.Sprintf("(from %q)", cand.imp.importPath)
112111
}
113112
}
114113

@@ -135,7 +134,14 @@ func (c *completer) item(cand candidate) (CompletionItem, error) {
135134
return item, nil
136135
}
137136
uri := span.FileURI(pos.Filename)
138-
ph, pkg, err := c.view.FindFileInPackage(c.ctx, uri, c.pkg)
137+
138+
// Find the source file of the candidate, starting from a package
139+
// that should have it in its dependencies.
140+
searchPkg := c.pkg
141+
if cand.imp != nil && cand.imp.pkg != nil {
142+
searchPkg = cand.imp.pkg
143+
}
144+
ph, pkg, err := c.view.FindFileInPackage(c.ctx, uri, searchPkg)
139145
if err != nil {
140146
return CompletionItem{}, err
141147
}
@@ -144,7 +150,7 @@ func (c *completer) item(cand candidate) (CompletionItem, error) {
144150
return CompletionItem{}, err
145151
}
146152
if !(file.Pos() <= obj.Pos() && obj.Pos() <= file.End()) {
147-
return CompletionItem{}, errors.Errorf("no file for %s", obj.Name())
153+
return CompletionItem{}, errors.Errorf("no file for completion object %s", obj.Name())
148154
}
149155
ident, err := findIdentifier(c.ctx, c.snapshot, pkg, file, obj.Pos())
150156
if err != nil {
@@ -162,12 +168,12 @@ func (c *completer) item(cand candidate) (CompletionItem, error) {
162168
}
163169

164170
// importEdits produces the text edits necessary to add the given import to the current file.
165-
func (c *completer) importEdits(imp *imports.ImportInfo) ([]protocol.TextEdit, error) {
171+
func (c *completer) importEdits(imp *importInfo) ([]protocol.TextEdit, error) {
166172
if imp == nil {
167173
return nil, nil
168174
}
169175

170-
edit, err := addNamedImport(c.view.Session().Cache().FileSet(), c.file, imp.Name, imp.ImportPath)
176+
edit, err := addNamedImport(c.view.Session().Cache().FileSet(), c.file, imp.name, imp.importPath)
171177
if err != nil {
172178
return nil, err
173179
}

‎internal/lsp/source/completion_literal.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@ import (
1111
"strings"
1212
"unicode"
1313

14-
"golang.org/x/tools/internal/imports"
1514
"golang.org/x/tools/internal/lsp/diff"
1615
"golang.org/x/tools/internal/lsp/protocol"
1716
"golang.org/x/tools/internal/lsp/snippet"
@@ -21,7 +20,7 @@ import (
2120

2221
// literal generates composite literal, function literal, and make()
2322
// completion items.
24-
func (c *completer) literal(literalType types.Type, imp *imports.ImportInfo) {
23+
func (c *completer) literal(literalType types.Type, imp *importInfo) {
2524
if c.expectedType.objType == nil {
2625
return
2726
}

‎internal/lsp/source/deep_completion.go

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,6 @@ import (
88
"go/types"
99
"strings"
1010
"time"
11-
12-
"golang.org/x/tools/internal/imports"
1311
)
1412

1513
// Limit deep completion results because in most cases there are too many
@@ -142,7 +140,7 @@ func (c *completer) shouldPrune() bool {
142140

143141
// deepSearch searches through obj's subordinate objects for more
144142
// completion items.
145-
func (c *completer) deepSearch(obj types.Object, imp *imports.ImportInfo) {
143+
func (c *completer) deepSearch(obj types.Object, imp *importInfo) {
146144
if c.deepState.maxDepth == 0 {
147145
return
148146
}

‎internal/lsp/source/imports_test.go

Lines changed: 34 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -35,11 +35,11 @@ type test struct {
3535
renamedPkg string
3636
pkg string
3737
in string
38-
want []importInfo
38+
want []imp
3939
unchanged bool // Expect added/deleted return value to be false.
4040
}
4141

42-
type importInfo struct {
42+
type imp struct {
4343
name string
4444
path string
4545
}
@@ -54,8 +54,8 @@ import (
5454
"os"
5555
)
5656
`,
57-
want: []importInfo{
58-
importInfo{
57+
want: []imp{
58+
{
5959
name: "",
6060
path: "os",
6161
},
@@ -67,8 +67,8 @@ import (
6767
pkg: "os",
6868
in: `package main
6969
`,
70-
want: []importInfo{
71-
importInfo{
70+
want: []imp{
71+
{
7272
name: "",
7373
path: "os",
7474
},
@@ -78,8 +78,8 @@ import (
7878
name: "package statement no new line",
7979
pkg: "os",
8080
in: `package main`,
81-
want: []importInfo{
82-
importInfo{
81+
want: []imp{
82+
{
8383
name: "",
8484
path: "os",
8585
},
@@ -92,8 +92,8 @@ import (
9292
in: `// Here is a comment before
9393
package main
9494
`,
95-
want: []importInfo{
96-
importInfo{
95+
want: []imp{
96+
{
9797
name: "",
9898
path: "os",
9999
},
@@ -104,8 +104,8 @@ package main
104104
pkg: "os",
105105
in: `package main // Here is a comment after
106106
`,
107-
want: []importInfo{
108-
importInfo{
107+
want: []imp{
108+
{
109109
name: "",
110110
path: "os",
111111
},
@@ -116,8 +116,8 @@ package main
116116
pkg: "os",
117117
in: `// Here is a comment before
118118
package main // Here is a comment after`,
119-
want: []importInfo{
120-
importInfo{
119+
want: []imp{
120+
{
121121
name: "",
122122
path: "os",
123123
},
@@ -129,8 +129,8 @@ package main // Here is a comment after`,
129129
in: `package main /* This is a multiline comment
130130
and it extends
131131
further down*/`,
132-
want: []importInfo{
133-
importInfo{
132+
want: []imp{
133+
{
134134
name: "",
135135
path: "os",
136136
},
@@ -143,12 +143,12 @@ further down*/`,
143143
144144
import "C"
145145
`,
146-
want: []importInfo{
147-
importInfo{
146+
want: []imp{
147+
{
148148
name: "",
149149
path: "os",
150150
},
151-
importInfo{
151+
{
152152
name: "",
153153
path: "C",
154154
},
@@ -161,12 +161,12 @@ import "C"
161161
162162
import "io"
163163
`,
164-
want: []importInfo{
165-
importInfo{
164+
want: []imp{
165+
{
166166
name: "",
167167
path: "os",
168168
},
169-
importInfo{
169+
{
170170
name: "",
171171
path: "io",
172172
},
@@ -179,12 +179,12 @@ import "io"
179179
180180
import "io" // A comment
181181
`,
182-
want: []importInfo{
183-
importInfo{
182+
want: []imp{
183+
{
184184
name: "",
185185
path: "os",
186186
},
187-
importInfo{
187+
{
188188
name: "",
189189
path: "io",
190190
},
@@ -199,12 +199,12 @@ import "io" /* A comment
199199
that
200200
extends */
201201
`,
202-
want: []importInfo{
203-
importInfo{
202+
want: []imp{
203+
{
204204
name: "",
205205
path: "os",
206206
},
207-
importInfo{
207+
{
208208
name: "",
209209
path: "io",
210210
},
@@ -216,8 +216,8 @@ extends */
216216
pkg: "os",
217217
in: `package main
218218
`,
219-
want: []importInfo{
220-
importInfo{
219+
want: []imp{
220+
{
221221
name: "o",
222222
path: "os",
223223
},
@@ -280,20 +280,20 @@ func TestDoubleAddNamedImport(t *testing.T) {
280280
got = applyEdits(got, edits)
281281
gotFile = parse(t, name, got)
282282

283-
want := []importInfo{
284-
importInfo{
283+
want := []imp{
284+
{
285285
name: "o",
286286
path: "os",
287287
},
288-
importInfo{
288+
{
289289
name: "i",
290290
path: "io",
291291
},
292292
}
293293
compareImports(t, "", gotFile.Imports, want)
294294
}
295295

296-
func compareImports(t *testing.T, prefix string, got []*ast.ImportSpec, want []importInfo) {
296+
func compareImports(t *testing.T, prefix string, got []*ast.ImportSpec, want []imp) {
297297
if len(got) != len(want) {
298298
t.Errorf("%s\ngot %d imports\nwant %d", prefix, len(got), len(want))
299299
return

0 commit comments

Comments
 (0)