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/cmd/godoc: show exported fields promoted from unexported anonymous fields #6600

Open
ugorji opened this issue Oct 16, 2013 · 19 comments
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@ugorji
Copy link
Contributor

ugorji commented Oct 16, 2013

Within my code, I have the following structure.

type common struct {
    Option1 bool
}

func (c *common) Method1() {
}

type A struct {
    OptionA int
    common
}

type B struct {
    OptionB int
    common
}

I want godoc to show that type A and type B have field Option1 available, and Method1 in
their method sets.

However, godoc would not show Option1, because common is not exported. It however show
Method1 (the full method set).

The only current workaround is to export common (which really is an internal
implementation detail), or duplicate the functionality across all types that share it. 

TO fix, godoc should show these promoted fields got from unexported anonymous fields. 

For example, godoc output for A could look like:

type A struct {
    OptionA int
    // contains filtered or unexported fields
    // Available from unexported anonymous fields
    Option1 bool
}


Which version are you using?  (run 'go version')
go version devel +47b2b07a837f Fri Oct 11 16:39:40 2013 -0700 linux/amd64

Please provide any additional information below.
@rsc
Copy link
Contributor

rsc commented Oct 18, 2013

Comment 1:

Labels changed: added priority-later, removed priority-triage.

Status changed to Accepted.

@rsc
Copy link
Contributor

rsc commented Nov 27, 2013

Comment 2:

Labels changed: added go1.3maybe.

@rsc
Copy link
Contributor

rsc commented Dec 4, 2013

Comment 3:

Labels changed: added release-none, removed go1.3maybe.

@rsc
Copy link
Contributor

rsc commented Dec 4, 2013

Comment 4:

Labels changed: added repo-tools.

@rsc rsc added this to the Unplanned milestone Apr 10, 2015
@rsc rsc changed the title cmd/godoc: show exported fields promoted from unexported anonymous fields x/tools/cmd/godoc: show exported fields promoted from unexported anonymous fields Apr 14, 2015
@rsc rsc removed the repo-tools label Apr 14, 2015
@alecthegeek
Copy link

I have what I think is the same problem. Public receiver methods (on a hidden type) do not have their document strings displayed. Consider this code

//Package showme demonstrates that public receivers on a hidden type are not documented
package showme

type hiddenType struct {
  i int
  s string
}
type PublicType struct {
  i int
  s string
}

// PublicFA should be documented
func (hiddenType) PublicFA(){
  //do something boring
}

// PublicFB will be documented
func (PublicType) PublicFB(){
  //do something boring
}

Method PublicFA is not documented when I run godoc.

@agnivade
Copy link
Contributor

agnivade commented May 4, 2018

@alecthegeek - Your issue is different. PublicFA should not be documented because it is attached to hiddenType which is not exported. You need to expose hiddenType if you want PublicFA to be visible.

This issue is about displaying exported fields from embedded fields in a type which itself is exported.

@agnivade
Copy link
Contributor

agnivade commented May 4, 2018

Interesting thing to note is if common is exported, Method1() stops showing up. Probably related to #6127.

@agnivade agnivade self-assigned this Aug 23, 2018
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/131176 mentions this issue: go/doc: show exported fields of embedded structs

@griesemer griesemer added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Sep 10, 2018
@griesemer
Copy link
Contributor

Before we write any more code for this, we need to decide if and what we should do about this.

@agnivade
Copy link
Contributor

Copying over @dsnet's thoughts from the CL for completeness -

I don't think we should do this. I have following concerns:

  • Uplifting exported fields from embedded unexported structs to the parent struct lies to the user about the true nature of the type. Now godoc claims that the parent has a field of some given name, but instantiating the struct as a literal does not match what godoc says. For example (from your testdata), U1 claims to have a field F1, so I would expect to be able to write a literal of the form U1{F1: "string"}, but this is not valid and my code fails to build.
  • Sometimes people deliberately embed an unexported type precisely because they want to hide the exported fields.
  • This bakes in more understanding of type information into godoc that fundamentally belongs in go/types. When do we stop giving more understanding of types to godoc?

@zigo101
Copy link

zigo101 commented Sep 11, 2018

I'm some surprised that the following cmd doesn't work:

$ go doc *sync.Cond
doc: invalid identifier "*sync"
exit status 1

@dsnet
Copy link
Member

dsnet commented Sep 20, 2018

@go101, what you're describing is orthogonal to this issue. The go doc tool does not know about pointers. Instead, you should do:

$ go doc sync.Cond
type Cond struct {

	// L is held while observing or changing the condition
	L Locker

	// Has unexported fields.
}
    Cond implements a condition variable, a rendezvous point for goroutines
    waiting for or announcing the occurrence of an event.

    Each Cond has an associated Locker L (often a *Mutex or *RWMutex), which
    must be held when changing the condition and when calling the Wait method.

    A Cond must not be copied after first use.


func NewCond(l Locker) *Cond
func (c *Cond) Broadcast()
func (c *Cond) Signal()

@zigo101
Copy link

zigo101 commented Nov 9, 2018

There are more cases of exported methods/fields are not listed by doc cmd and godoc webpages.

  1. the exported methods and fields of exported variables of unexported types.
  2. the exported methods and fields of results of unexported types of exported functions.
  3. the exported methods and fields of the exported alias of unexported types

@ddrake17
Copy link

ddrake17 commented Dec 5, 2018

+1 for allowing documentation of embedded struct's exported fields. In response to @dsnet and @agnivade said:

  • It's true that there are some differences and so perhaps the fields should not be given the exact same doc status as regular exported fields, but I think there is still value in showing the fields since users can access them. The example in the issue report shows one way of documenting them differently.
  • One issue pointed out was forming a struct literal but a clear differentiation in comment would show the user they can't create it the normal way.
  • Also you can still access the embedded fields in other ways that are exactly the same as if they were fields of the exported struct itself (e.g. using the example code in the issue report, if B.Option == false ). Although you can access these fields and use them there isn't a good way to tell this from reading the docs since they aren't shown at all.
  • If package writers don't want people accessing exported fields then there are multiple ways to hide them like unexporting them, or embedding the struct with a named unexported field.

I think the proposed doc solution in the report was good and would differentiate things enough so users were not confused

@gonutz
Copy link

gonutz commented Oct 2, 2021

I am surprised that throughout this thread it seems people are unsure whether to do anything about it. Labels like priority-later and comments like we need to decide if and what we should do about this suggest that this is not an important issue.

For me it is. I want to go look at the package site and see what functions I can call on my objects but it does not show them to me. I am glad that there are people working on this and I am not entitled to tell you what to spend your time on, but I would like to bump this up in priority in people's minds since it is a major issue for me at the moment. I rely on the package site quite a lot since my IDE's auto-completion broke (when modules were introduced) and for the most part it works. But this is really a pain point for me and I would be very happy to see this issue fixed.

For reference, I was the one to open issue #48722

@ThisGuyCodes
Copy link

This issue creates situations where one reads code that uses exported fields that are not present in documentation.

Example, the usage section of the documentation for https://github.com/andreykaipov/goobs shows the following example code:
image

On line 16 you see this:

version, err := client.General.GetVersion()

but the documentation for *goobs.Client looks like this:
image

Despite .General being an available field on goobs.Client, it does not show in the documentation.

While I accept there are multiple possible solutions here, I have difficulty understanding how this can be seen as anything but obviously incorrect.

ThisGuyCodes added a commit to ThisGuyCodes/goobs that referenced this issue Jan 2, 2024
embedding a private struct creates the confusing effect of exporting the public fields of the sub-struct without having them show in documentation.

This has some discussion at the language / godoc level: golang/go#6600

This commit exports the subclients field of goobs.Client so the available fields show in documentation.
@zigo101
Copy link

zigo101 commented Jan 3, 2024

You can try Golds, which lists all promoted fields and methods.

andreykaipov pushed a commit to andreykaipov/goobs that referenced this issue Jan 3, 2024
* docs: export Subclients embedded struct

embedding a private struct creates the confusing effect of exporting the public fields of the sub-struct without having them show in documentation.

This has some discussion at the language / godoc level: golang/go#6600

This commit exports the subclients field of goobs.Client so the available fields show in documentation.

* client: rename subclients to categories
@betamos
Copy link

betamos commented May 7, 2024

I'm in favor. From a principled POV the primary goal of documentation should be to display everything that a user can access and do, i.e. how they can interact with the module. Anything accessible according to the visibility rules of the language should appear in the docs.

Now, from a direct use case, embeddings are particularly important because they affect which interfaces are implemented. Consider this use-case:

type netConn net.Conn

type Conn struct {
    netConn // We don't allow direct access to the inner net.Conn to prevent accidental misuse
    br *bufio.Reader
}

func (c *Conn) Read(p []byte) (int, error) {
    // Override to read from the bufio.Reader, common for wire protocols
}

// Forward the rest (7 methods) to the inner conn

Crucially, users need to know that *Conn implements net.Conn, but it's impossible to tell from the docs:

type Conn struct {
    // Has unexported fields.
}

func (c *Conn) Read(p []byte) (int, error)

@FiloSottile
Copy link
Contributor

This caught me by surprise, and can create compatibility issues: when preparing to publish a package, and especially when tagging v1.0.0, I always make sure I am happy with the exposed API. However, due to this, I exposed fields I did not meant to expose without noticing, and removing them would technically be a backwards compatibility issue.

At least, we should change the // contains filtered or unexported fields comment to flag the presence of exported embedded fields.

FiloSottile added a commit to FiloSottile/mlkem768 that referenced this issue May 18, 2024
@agnivade agnivade removed their assignment May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests