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: exclude unexported fields from hover information for structs #42654

Closed
myitcv opened this issue Nov 17, 2020 · 15 comments
Closed

x/tools/gopls: exclude unexported fields from hover information for structs #42654

myitcv opened this issue Nov 17, 2020 · 15 comments

Comments

@myitcv
Copy link
Member

@myitcv myitcv commented Nov 17, 2020

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

$ go version
go version devel +869e2957b9 Mon Nov 16 22:24:14 2020 +0000 linux/amd64
$ go list -m golang.org/x/tools
golang.org/x/tools v0.0.0-20201111213328-5794f8bd7a57
$ go list -m golang.org/x/tools/gopls
golang.org/x/tools/gopls v0.0.0-20201111213328-5794f8bd7a57

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=""
GOMODCACHE="/home/myitcv/gostuff/pkg/mod"
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"
GOVCS=""
GOVERSION="devel +869e2957b9 Mon Nov 16 22:24:14 2020 +0000"
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-build164265260=/tmp/go-build -gno-record-gcc-switches"

What did you do?

-- go.mod --
module other

go 1.16

require cuelang.org/go v0.3.0-alpha4.0.20201116194914-7463d11dea50
-- main.go --
package main

import "cuelang.org/go/cue"

func main() {
	var _ cue.Path
}

Hover over Path

What did you expect to see?

Only the exported fields of cue.Path

What did you see instead?

type Path struct {
        path []Selector
}
A Path is series of selectors to query a CUE value

Log file: bad.log


cc @stamblerre

FYI @leitzler

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Nov 17, 2020

The hover information for structs is actually just the AST node of the declaration. I guess we could filter out the unexported fields, but that would require cloning the AST node.

@stamblerre stamblerre changed the title x/tools/gopls: hover information for struct includes unexported fields x/tools/gopls: exclude unexported fields from hover information for structs Nov 17, 2020
@danishprakash
Copy link

@danishprakash danishprakash commented Jan 6, 2021

@stamblerre I'd like to work on this, would that be fine?

@muirdm
Copy link

@muirdm muirdm commented Jan 6, 2021

Will we still show unexported fields if the struct is defined in the current package?

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Jan 6, 2021

Will we still show unexported fields if the struct is defined in the current package?

Yes, I think it would make sense to still show unexported fields in such cases.

@stamblerre I'd like to work on this, would that be fine?

Sure, though this may be a little bit complicated because of the cloning of AST nodes required.

@danishprakash
Copy link

@danishprakash danishprakash commented Jan 7, 2021

@stamblerre thanks, I'll start with this, will try to get some help from #gopls-dev

@danishprakash
Copy link

@danishprakash danishprakash commented May 30, 2021

@stamblerre Sorry for the multi-month delay. I've a very crude implementation https://github.com/golang/tools/compare/master...danishprakash:hover-struct-exported?expand=1 if you could take a look whenever you've time. I'm not very confident on using the filtering logic from https://golang.org/src/go/ast/filter.go here albeit with some changes.

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Jun 1, 2021

@danishprakash: Thank you for trying that out. As I mentioned above, we cannot modify the AST nodes directly, since the same ASTs are also used for other types of requests. I think you will want some logic like the cloneExpr function here--but it will be simpler since you only need to clone structs I believe. Then you can filter the cloned nodes. Does that make sense?

@danishprakash
Copy link

@danishprakash danishprakash commented Jun 6, 2021

@stamblerre my bad, I was just trying to get a draft implementation working. But that made complete sense. I've made a few changes here. But I still don't feel right about copying over the cloning logic albeit a little stripped down for this use case. Could you suggest something here? Maybe we could have a generic cloneAST method in utils/ or maybe propose something like this to astutil for that matter? What do you think?

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Jun 10, 2021

Thank you for spending time looking at this, @danishprakash! Seeing how involved and complicated this change would be is making me wonder if this is even worth having. I think in some cases it might be useful to see the unexported fields, even if you can't access them from the current package. What do you think?

@danishprakash
Copy link

@danishprakash danishprakash commented Jun 14, 2021

Now that you've mentioned, I realize I'd much rather have all the members of the struct shown on hover while I'm working on something than have them filtered.

On the other hand, and I think is what @myitcv(correct me If I'm wrong) also had in mind when he opened this issue, is the fact that If I'm hovering over a struct from a package that I haven't authored, I would like to see the fields that are exported for me to use. In this case, it's currently implied that the individual should figure out that there are unexported fields in there as well along with the exported ones. But all said and done, I feel that showing all the fields on hover makes more sense.

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Jun 15, 2021

Yes, I agree with your assessment--it really varies by your usage and intentions. I don't think it makes sense to offer configuration for this, so I think it's probably best to close this issue and leave it as is. Thank you for your work on this, @danishprakash--I'm sorry we weren't able to merge your changes.

@stamblerre stamblerre closed this Jun 15, 2021
@myitcv
Copy link
Member Author

@myitcv myitcv commented Jun 21, 2021

But all said and done, I feel that showing all the fields on hover makes more sense.

Unless I am missing something, if I am looking at a struct defined in a package outside of the main module, I can have no interest in the non-exported fields (because unless I resort to reflection I can't get/set their values). The ambiguity comes for structs defined by packages inside by the main module: in that case I agree it does make sense to show all fields. This issue was created for the former category; as such I don't agree with the conclusion.

@stamblerre stamblerre removed this from the gopls/unplanned milestone Jun 21, 2021
@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Jun 22, 2021

Unless I am missing something, if I am looking at a struct defined in a package outside of the main module, I can have no interest in the non-exported fields (because unless I resort to reflection I can't get/set their values).

In our discussion, we thought that there can still be value in seeing these fields, though it's fairly rare. But more importantly, the logic required to get this right is nontrivial, but it's not totally clear to me what the benefits would be. It also could be confusing to users to see no fields if all of the fields of a struct are unexported.

@myitcv
Copy link
Member Author

@myitcv myitcv commented Jun 22, 2021

In our discussion, we thought that there can still be value in seeing these fields

Please can you share more details?

it's not totally clear to me what the benefits would be

The main benefits to my mind are that as the user of a struct defined outside of the main module you are not confronted with lots of detail that is irrelevant. Indeed during completion we filter out non-exported fields for exactly this reason.

It also could be confusing to users to see no fields if all of the fields of a struct are unexported.

In this case I suggest we simply do what go doc does:

type Attribute struct {
	// contains filtered or unexported fields
}

more importantly, the logic required to get this right is nontrivial

I can appreciate this point, and it might be that the costs outweigh the benefits. I'm just keen to understand if I'm missing the point regarding the reasons against doing this (ignoring the complexity for one second), given the obvious consistency it would seem to offer with respect to go doc and completion.

@danishprakash
Copy link

@danishprakash danishprakash commented Jun 22, 2021

In this case I suggest we simply do what go doc does:

Didn't think of this earlier, it sounds much more intuitive, better than not showing any fields or showing all the fields.

(ignoring the complexity for one second)

I think this was part of the reason for not implementing it but I'll be glad to look into a more reasonable implementation if there's one given that we go ahead with how go doc does it currently.

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
5 participants