From ffc70b9ac150680da53ed53e8543fd0e938ce4f4 Mon Sep 17 00:00:00 2001 From: Muir Manders Date: Thu, 30 Jun 2022 19:44:46 -0700 Subject: [PATCH] lsp/completion: fix ranking of *types.PkgName candidates In Go 1.18 types.AssignableTo() started reporting that an invalid type is assignable to any interface. *types.PkgName (i.e. an import at the top of the file) has an invalid type for its Type(), so we started thinking all in scope imports were great candidates when the expected type was an interface. Fix by wrapping the AssignableTo (and AssertableTo) to explicitly return false if either operand is invalid. Updates golang/go#53595 Change-Id: Ie5a84b7f410ff5c73c6b7870e052bafaf3e21e99 Reviewed-on: https://go-review.googlesource.com/c/tools/+/415595 Reviewed-by: Hyang-Ah Hana Kim Reviewed-by: Robert Findley Run-TryBot: Robert Findley TryBot-Result: Gopher Robot gopls-CI: kokoro --- internal/lsp/source/completion/completion.go | 12 +++++------ internal/lsp/source/completion/util.go | 20 +++++++++++++++++++ internal/lsp/testdata/deep/deep.go | 7 +++++++ internal/lsp/testdata/summary.txt.golden | 2 +- .../lsp/testdata/summary_go1.18.txt.golden | 2 +- 5 files changed, 35 insertions(+), 8 deletions(-) diff --git a/internal/lsp/source/completion/completion.go b/internal/lsp/source/completion/completion.go index 0c1ff3f21b..a2dfae6984 100644 --- a/internal/lsp/source/completion/completion.go +++ b/internal/lsp/source/completion/completion.go @@ -2314,7 +2314,7 @@ func (ci candidateInference) applyTypeNameModifiers(typ types.Type) types.Type { // matchesVariadic returns true if we are completing a variadic // parameter and candType is a compatible slice type. func (ci candidateInference) matchesVariadic(candType types.Type) bool { - return ci.variadic && ci.objType != nil && types.AssignableTo(candType, types.NewSlice(ci.objType)) + return ci.variadic && ci.objType != nil && assignableTo(candType, types.NewSlice(ci.objType)) } // findSwitchStmt returns an *ast.CaseClause's corresponding *ast.SwitchStmt or @@ -2640,7 +2640,7 @@ func (ci *candidateInference) candTypeMatches(cand *candidate) bool { return false } - if ci.convertibleTo != nil && types.ConvertibleTo(candType, ci.convertibleTo) { + if ci.convertibleTo != nil && convertibleTo(candType, ci.convertibleTo) { return true } @@ -2728,7 +2728,7 @@ func considerTypeConversion(from, to types.Type, path []types.Object) bool { return false } - if !types.ConvertibleTo(from, to) { + if !convertibleTo(from, to) { return false } @@ -2777,7 +2777,7 @@ func (ci *candidateInference) typeMatches(expType, candType types.Type) bool { // AssignableTo covers the case where the types are equal, but also handles // cases like assigning a concrete type to an interface type. - return types.AssignableTo(candType, expType) + return assignableTo(candType, expType) } // kindMatches reports whether candType's kind matches our expected @@ -2840,7 +2840,7 @@ func (ci *candidateInference) assigneesMatch(cand *candidate, sig *types.Signatu assignee = ci.assignees[i] } - if assignee == nil { + if assignee == nil || assignee == types.Typ[types.Invalid] { continue } @@ -2894,7 +2894,7 @@ func (c *completer) matchingTypeName(cand *candidate) bool { // // Where our expected type is "[]int", and we expect a type name. if c.inference.objType != nil { - return types.AssignableTo(candType, c.inference.objType) + return assignableTo(candType, c.inference.objType) } // Default to saying any type name is a match. diff --git a/internal/lsp/source/completion/util.go b/internal/lsp/source/completion/util.go index cd7849af26..e6d3bfd745 100644 --- a/internal/lsp/source/completion/util.go +++ b/internal/lsp/source/completion/util.go @@ -321,3 +321,23 @@ func (c *completer) editText(from, to token.Pos, newText string) ([]protocol.Tex NewText: newText, }}) } + +// assignableTo is like types.AssignableTo, but returns false if +// either type is invalid. +func assignableTo(x, to types.Type) bool { + if x == types.Typ[types.Invalid] || to == types.Typ[types.Invalid] { + return false + } + + return types.AssignableTo(x, to) +} + +// convertibleTo is like types.ConvertibleTo, but returns false if +// either type is invalid. +func convertibleTo(x, to types.Type) bool { + if x == types.Typ[types.Invalid] || to == types.Typ[types.Invalid] { + return false + } + + return types.ConvertibleTo(x, to) +} diff --git a/internal/lsp/testdata/deep/deep.go b/internal/lsp/testdata/deep/deep.go index 6ed5ff8399..6908824f82 100644 --- a/internal/lsp/testdata/deep/deep.go +++ b/internal/lsp/testdata/deep/deep.go @@ -28,6 +28,13 @@ func _() { wantsContext(c) //@rank(")", ctxBackground),rank(")", ctxTODO) } +func _() { + var cork struct{ err error } + cork.err //@item(deepCorkErr, "cork.err", "error", "field") + context //@item(deepContextPkg, "context", "\"context\"", "package") + var _ error = co //@rank(" //", deepCorkErr, deepContextPkg) +} + func _() { // deepCircle is circular. type deepCircle struct { diff --git a/internal/lsp/testdata/summary.txt.golden b/internal/lsp/testdata/summary.txt.golden index 0247551f8b..b6c6c07b15 100644 --- a/internal/lsp/testdata/summary.txt.golden +++ b/internal/lsp/testdata/summary.txt.golden @@ -6,7 +6,7 @@ CompletionSnippetCount = 106 UnimportedCompletionsCount = 5 DeepCompletionsCount = 5 FuzzyCompletionsCount = 8 -RankedCompletionsCount = 163 +RankedCompletionsCount = 164 CaseSensitiveCompletionsCount = 4 DiagnosticsCount = 37 FoldingRangesCount = 2 diff --git a/internal/lsp/testdata/summary_go1.18.txt.golden b/internal/lsp/testdata/summary_go1.18.txt.golden index 7e8da12d76..9fadf63409 100644 --- a/internal/lsp/testdata/summary_go1.18.txt.golden +++ b/internal/lsp/testdata/summary_go1.18.txt.golden @@ -6,7 +6,7 @@ CompletionSnippetCount = 116 UnimportedCompletionsCount = 5 DeepCompletionsCount = 5 FuzzyCompletionsCount = 8 -RankedCompletionsCount = 173 +RankedCompletionsCount = 174 CaseSensitiveCompletionsCount = 4 DiagnosticsCount = 37 FoldingRangesCount = 2