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 I'd like to work on this, would that be fine?

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

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.

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

@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.

@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?

@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?

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?

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.

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.

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.

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.

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.

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.

Please can you share more details?

I don't have a concrete example of a case where you would benefit from checking an exported field, but at the very least, it might save you a "jump to definition" if you do want to look at a struct's fields or implementation. I'll try to think of a more concrete case.

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.

There might be a way to do it by adjusting the way that we write out AST nodes (currently we simply call format.Node in https://pkg.go.dev/golang.org/x/tools/internal/lsp/source#HoverIdentifier). It might be tricky to get right, and a really hacky approach would be to check the case of the first letter of every line in the output...