gopls/internal/lsp/source completion: don't suggest untyped conversions
During candidate inference, it is possible that the implicit type of the
completion is untyped (as would be the case for an operand in an untyped
constant expression). In this case, when we've determined that a
candidate must be converted, use the default type rather than suggesting
a conversion to e.g. "untyped float". Notably, untyped consts of
different kinds are considered compatible, so this conversion logic does
not apply to them.
While at it, express the test in the new marker framework, by adding an
'acceptcompletion' marker.
Fixes golang/go#62141
Change-Id: I81fa7b61f54d141084ac02ee863076355e4effc4
Reviewed-on: https://go-review.googlesource.com/c/tools/+/520876
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
diff --git a/gopls/internal/lsp/regtest/marker.go b/gopls/internal/lsp/regtest/marker.go
index 6ae306b..1c364bd 100644
--- a/gopls/internal/lsp/regtest/marker.go
+++ b/gopls/internal/lsp/regtest/marker.go
@@ -132,6 +132,10 @@
//
// The following markers are supported within marker tests:
//
+// - acceptcompletion(location, label, golden): specifies that accepting the
+// completion candidate produced at the given location with provided label
+// results in the given golden state.
+//
// - codeaction(kind, start, end, golden): specifies a codeaction to request
// for the given range. To support multi-line ranges, the range is defined
// to be between start.Start and end.End. The golden directory contains
@@ -546,21 +550,22 @@
// Marker funcs should not mutate the test environment (e.g. via opening files
// or applying edits in the editor).
var markerFuncs = map[string]markerFunc{
- "codeaction": makeMarkerFunc(codeActionMarker),
- "codeactionerr": makeMarkerFunc(codeActionErrMarker),
- "complete": makeMarkerFunc(completeMarker),
- "def": makeMarkerFunc(defMarker),
- "diag": makeMarkerFunc(diagMarker),
- "hover": makeMarkerFunc(hoverMarker),
- "format": makeMarkerFunc(formatMarker),
- "implementation": makeMarkerFunc(implementationMarker),
- "loc": makeMarkerFunc(locMarker),
- "rename": makeMarkerFunc(renameMarker),
- "renameerr": makeMarkerFunc(renameErrMarker),
- "suggestedfix": makeMarkerFunc(suggestedfixMarker),
- "symbol": makeMarkerFunc(symbolMarker),
- "refs": makeMarkerFunc(refsMarker),
- "workspacesymbol": makeMarkerFunc(workspaceSymbolMarker),
+ "acceptcompletion": makeMarkerFunc(acceptCompletionMarker),
+ "codeaction": makeMarkerFunc(codeActionMarker),
+ "codeactionerr": makeMarkerFunc(codeActionErrMarker),
+ "complete": makeMarkerFunc(completeMarker),
+ "def": makeMarkerFunc(defMarker),
+ "diag": makeMarkerFunc(diagMarker),
+ "hover": makeMarkerFunc(hoverMarker),
+ "format": makeMarkerFunc(formatMarker),
+ "implementation": makeMarkerFunc(implementationMarker),
+ "loc": makeMarkerFunc(locMarker),
+ "rename": makeMarkerFunc(renameMarker),
+ "renameerr": makeMarkerFunc(renameErrMarker),
+ "suggestedfix": makeMarkerFunc(suggestedfixMarker),
+ "symbol": makeMarkerFunc(symbolMarker),
+ "refs": makeMarkerFunc(refsMarker),
+ "workspacesymbol": makeMarkerFunc(workspaceSymbolMarker),
}
// markerTest holds all the test data extracted from a test txtar archive.
@@ -1294,6 +1299,43 @@
}
}
+// acceptCompletionMarker implements the @acceptCompletion marker, running
+// textDocument/completion at the given src location and accepting the
+// candidate with the given label. The resulting source must match the provided
+// golden content.
+func acceptCompletionMarker(mark marker, src protocol.Location, label string, golden *Golden) {
+ list := mark.run.env.Completion(src)
+ var selected *protocol.CompletionItem
+ for _, item := range list.Items {
+ if item.Label == label {
+ selected = &item
+ break
+ }
+ }
+ if selected == nil {
+ mark.errorf("Completion(...) did not return an item labeled %q", label)
+ return
+ }
+ filename := mark.run.env.Sandbox.Workdir.URIToPath(mark.uri())
+ mapper, err := mark.run.env.Editor.Mapper(filename)
+ if err != nil {
+ mark.errorf("Editor.Mapper(%s) failed: %v", filename, err)
+ return
+ }
+
+ patched, _, err := source.ApplyProtocolEdits(mapper, append([]protocol.TextEdit{
+ *selected.TextEdit,
+ }, selected.AdditionalTextEdits...))
+
+ if err != nil {
+ mark.errorf("ApplyProtocolEdits failed: %v", err)
+ return
+ }
+ changes := map[string][]byte{filename: patched}
+ // Check the file state.
+ checkChangedFiles(mark, changes, golden)
+}
+
// defMarker implements the @def marker, running textDocument/definition at
// the given src location and asserting that there is exactly one resulting
// location, matching dst.
diff --git a/gopls/internal/lsp/source/completion/format.go b/gopls/internal/lsp/source/completion/format.go
index c2d2c0b..848f52d 100644
--- a/gopls/internal/lsp/source/completion/format.go
+++ b/gopls/internal/lsp/source/completion/format.go
@@ -183,11 +183,18 @@
if cand.convertTo != nil {
typeName := types.TypeString(cand.convertTo, c.qf)
- switch cand.convertTo.(type) {
+ switch t := cand.convertTo.(type) {
// We need extra parens when casting to these types. For example,
// we need "(*int)(foo)", not "*int(foo)".
case *types.Pointer, *types.Signature:
typeName = "(" + typeName + ")"
+ case *types.Basic:
+ // If the types are incompatible (as determined by typeMatches), then we
+ // must need a conversion here. However, if the target type is untyped,
+ // don't suggest converting to e.g. "untyped float" (golang/go#62141).
+ if t.Info()&types.IsUntyped != 0 {
+ typeName = types.TypeString(types.Default(cand.convertTo), c.qf)
+ }
}
prefix = typeName + "(" + prefix
diff --git a/gopls/internal/regtest/marker/testdata/completion/issue62141.txt b/gopls/internal/regtest/marker/testdata/completion/issue62141.txt
new file mode 100644
index 0000000..877e59d
--- /dev/null
+++ b/gopls/internal/regtest/marker/testdata/completion/issue62141.txt
@@ -0,0 +1,39 @@
+This test checks that we don't suggest completion to an untyped conversion such
+as "untyped float(abcdef)".
+
+-- main.go --
+package main
+
+func main() {
+ abcdef := 32 //@diag("abcdef", re"not used")
+ x := 1.0 / abcd //@acceptcompletion(re"abcd()", "abcdef", int), diag("x", re"not used"), diag("abcd", re"(undefined|undeclared)")
+
+ // Verify that we don't suggest converting compatible untyped constants.
+ const untypedConst = 42
+ y := 1.1 / untypedC //@acceptcompletion(re"untypedC()", "untypedConst", untyped), diag("y", re"not used"), diag("untypedC", re"(undefined|undeclared)")
+}
+
+-- @int/main.go --
+package main
+
+func main() {
+ abcdef := 32 //@diag("abcdef", re"not used")
+ x := 1.0 / float64(abcdef) //@acceptcompletion(re"abcd()", "abcdef", int), diag("x", re"not used"), diag("abcd", re"(undefined|undeclared)")
+
+ // Verify that we don't suggest converting compatible untyped constants.
+ const untypedConst = 42
+ y := 1.1 / untypedC //@acceptcompletion(re"untypedC()", "untypedConst", untyped), diag("y", re"not used"), diag("untypedC", re"(undefined|undeclared)")
+}
+
+-- @untyped/main.go --
+package main
+
+func main() {
+ abcdef := 32 //@diag("abcdef", re"not used")
+ x := 1.0 / abcd //@acceptcompletion(re"abcd()", "abcdef", int), diag("x", re"not used"), diag("abcd", re"(undefined|undeclared)")
+
+ // Verify that we don't suggest converting compatible untyped constants.
+ const untypedConst = 42
+ y := 1.1 / untypedConst //@acceptcompletion(re"untypedC()", "untypedConst", untyped), diag("y", re"not used"), diag("untypedC", re"(undefined|undeclared)")
+}
+
