Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/tools/gopls: signature help does not properly qualify types #38591

Closed
myitcv opened this issue Apr 22, 2020 · 10 comments
Closed

x/tools/gopls: signature help does not properly qualify types #38591

myitcv opened this issue Apr 22, 2020 · 10 comments

Comments

@myitcv
Copy link
Member

@myitcv myitcv commented Apr 22, 2020

What version of Go are you using (go version)?

$ go version
go version devel +801cd7c84d Thu Apr 2 09:00:44 2020 +0000 linux/amd64
$ go list -m golang.org/x/tools
golang.org/x/tools v0.0.0-20200408132156-9ee5ef7a2c0d
$ go list -m golang.org/x/tools/gopls
golang.org/x/tools/gopls v0.0.0-20200408132156-9ee5ef7a2c0d

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/myitcv/.cache/go-build"
GOENV="/home/myitcv/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/myitcv/gostuff"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/myitcv/gos"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/myitcv/gos/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/myitcv/.vim/plugged/govim/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build731423971=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Consider the following example:

-- go.mod --
module mod.com

go 1.12

require cuelang.org/go v0.1.1
-- main.go --
package main

import "cuelang.org/go/cue/load"

func main() {
	bps := load.Instances()
}

If we request signature help between the parentheses of the call expression load.Instances() then we get the following back:

&protocol.SignatureHelp{
    Signatures: {
        {
            Label:         "Instances(args []string, c *Config) []*build.Instance",
            Documentation: "Instances returns the instances named by the command line arguments 'args'.",
            Parameters:    {
                {Label:"args []string", Documentation:""},
                {Label:"c *Config", Documentation:""},
            },
        },
    },
    ActiveSignature: 0,
    ActiveParameter: 0,
}

To my mind the c parameter's type should be qualified as *load.Config.

And strictly speaking I think the return type should be fully qualified because we do not have cuelang.org/go/build imported in the current file, so that type is potentially ambiguous.

What did you expect to see?

A label of:

Instances(args []string, c *load.Config) []*cuelang.org/go/build.Instance

What did you see instead?

As above.


cc @stamblerre

FYI @leitzler

@gopherbot gopherbot added this to the Unreleased milestone Apr 22, 2020
@gopherbot gopherbot added the Tools label Apr 22, 2020
@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Apr 22, 2020

Duplicate of #38230

@stamblerre stamblerre marked this as a duplicate of #38230 Apr 22, 2020
@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Apr 22, 2020

This has the same root cause as the above issue, and it will be fixed by https://golang.org/cl/229319/.

@stamblerre stamblerre closed this Apr 22, 2020
@myitcv
Copy link
Member Author

@myitcv myitcv commented Apr 22, 2020

Duplicate of #38230

Apologies. Thanks for linking.

@myitcv
Copy link
Member Author

@myitcv myitcv commented Apr 24, 2020

I see that #38230 has now been closed, which fixes part of the problem raised in this issue. Specifically it now correctly qualifies the parameter so the label appears as:

Instances(args []string, c *load.Config) []*build.Instance

But this still leaves the question about the return value not being qualified.

@myitcv myitcv reopened this Apr 24, 2020
@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Apr 24, 2020

This is doable, but I wonder if it may be bad UX. Of course the test case I came up with is pathological, but it was basically this:

-- testy/testy.go --
package testy

import (
	"golang.org/x/tools/internal/lsp/snippets"
)

func _() {
	snippets.X()
}
-- snippets/snippets.go --
package snippets

import (
	"golang.org/x/tools/internal/lsp/signature"
	"golang.org/x/tools/internal/lsp/types"
)

func X(_ map[signature.Alias]types.CoolAlias) map[signatures.Alias]types.CoolAlias) {
	return nil
}

Then, the signature help for X is X(_ map[golang.org/x/tools/internal/lsp/signature.Alias]golang.org/x/tools/internal/lsp/types.CoolAlias) map[golang.org/x/tools/internal/lsp/signatures.Alias]golang.org/x/tools/internal/lsp/types.CoolAlias).

I'd be curious to get @heschik's thoughts on this.

A note on how to do implement this: We'd have to check package names for any *ast.SelectorExprs we come across in qualifyExpr.

@myitcv
Copy link
Member Author

@myitcv myitcv commented Apr 24, 2020

I wonder if it may be bad UX

I definitely share this concern. But whenever you come across a situation like this it's always jarring if the package is not imported by the current file.

@heschik
Copy link
Contributor

@heschik heschik commented Apr 24, 2020

IMO, if there's a problem here fully qualifying the path is not the right solution. If you're familiar with the API, the full path is just going to be an impediment to readability. If you're not, then the full path is not going to be enough to teach you how to use it.

What is the actual use case?

@stamblerre stamblerre modified the milestones: Unreleased, gopls/v0.5.0 Apr 24, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 25, 2020

Change https://golang.org/cl/229979 mentions this issue: internal/lsp: handle different package names in signature help

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Apr 25, 2020

Just mailed a CL that handles a few edge cases, but I agree that the use case is not clearly defined. I think that we should leave this as-is for now, and if this is something that many users bring up, we can address it then. I'm not sure that the benefits of being more precise outweigh the readability costs here.

gopherbot pushed a commit to golang/tools that referenced this issue Apr 27, 2020
There were a few cases where we were not properly qualifying package
names, particularly if the original package had a named import. Now,
we map between these names correctly - handling the case of multiple
packages that need to be qualified. This requires applying edits to
*ast.SelectorExprs, as well as *ast.Idents.

We still do not fully qualify unimported packages, and likely won't,
unless that's an issue for many users.

Updates golang/go#38591

Change-Id: I966a4d1f936f37ede89362d03da3ff98d8952a06
Reviewed-on: https://go-review.googlesource.com/c/tools/+/229979
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented May 7, 2020

Closing this since the edge cases have been handled, and we're planning to leave unimported package paths as-is.

@stamblerre stamblerre closed this May 7, 2020
@stamblerre stamblerre modified the milestones: gopls/v0.5.0, gopls/v0.4.1 May 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.