Golang x/tools/gopls: completing argument to %p verb adds unwanted & for some types

gopls version

f4fa7a75e44868caa1b37947e8b811b1bdb144e6

go env

-

What did you do?

Trigger completion at the end of

package pkg

import "fmt"

func foo() {
    var longword any
    fmt.Printf("%p", lon

What did you see happen?

The line completes to

fmt.Printf("%p", &longword

What did you expect to see?

fmt.Printf("%p", longword

The variable has an interface type. I intended to print the pointer stored in the interface value, not the address of the local variable.

The same happens with maps, channels, unsafe.Pointer, and functions, all of which are accepted by %p. It does not happen with true pointers, or slices.

Editor and settings

No response

Logs

No response

Comment From: findleyr

Thanks! I imagine this is straightforward to fix.

High level notes: - Add a new kindInterface here: https://cs.opensource.google/go/x/tools/+/master:gopls/internal/golang/completion/printf.go;l=155;drc=6823da4bc3f39c895ac7f0a46d3994d146397193 - Update completion.candKind to recognize interfaces - Write tests, e.g. at https://cs.opensource.google/go/x/tools/+/master:gopls/internal/test/marker/testdata/completion/type_mods.txt

Adding to the next milestone.

Comment From: naeemaei

I will work on this issue

Comment From: findleyr

Thanks @naeemaei! Let us know if you need any additional guidance.

Comment From: dominikh

* Update `completion.candKind` to recognize interfaces

Maps, channels, unsafe.Pointer, and functions as well. Anything pointer-esque, really. Also, interfaces are probably a special case separate from that and not really limited to %p: if the static type is an interface, then the dynamic type might be valid for any verb.

Comment From: naeemaei

I added the kindInterface to the parsePrintfVerb function and also handled the kindInterface in completion.candKind then the line completes without & when using local self-build gopls.

But when I print an interface variable, the print output is as below:

structInterface = person{FirstName: "name"}
fmt.Printf("structInterface: %p\n", structInterface) // structInterface: %!p(main.person={name})
fmt.Printf("structInterface pointer: %p\n", &structInterface) // structInterface pointer: 0xc000014070

When the interface underlying type is a pointer output is the underlying variable address:

strct := &person{FirstName: "name"}
structInterface = strct
fmt.Printf("struct pointer: %p\n", strict) // 0xc000014090
fmt.Printf("structInterface: %p\n", structInterface) // structInterface: 0xc000014090
fmt.Printf("structInterface pointer: %p\n", &structInterface) // structInterface pointer: 0xc000014070

if the static type is an interface, then the dynamic type might be valid for any verb.

This part is a bit vague, I need your guidance regarding the specific logic that should be handled after recognizing interfaces in completion.candKind @findleyr @dominikh

Comment From: findleyr

@naeemaei I think the point is that when the argument is an interface, we don't generally know whether or not it holds a pointer (or any other kind of type). In the absence of certainty, it is better not to apply the &. We should only apply type modifiers when they are strictly required to have a valid type.

So it sounds like gopls is working as intended in your example.

Comment From: muirdm

Another way to fix this might be to make derivableTypes not try "deriving" a pointer from interface, chan, etc. types that already have pointer semantics. In the example, derivableTypes says you can derive a pointer type *interface{} from longword by doing &longword, which isn't useful in general.

As for what completion candidates the "%p" verb prefers, we should also include "chan", "map" and "func". I don't think we should make it prefer interfaces since those aren't necessarily printable with "%p".

Comment From: gopherbot

Change https://go.dev/cl/593575 mentions this issue: gopls/completion: don't take address of interfaces for "%p" values