godoc should show Exported methods for unexported types

[mark-summerfield]

8a09ce0cefc6 weekly/weekly.2011-09-16

A common scenario:

type unexported struct { ... }
type Exported interface { Public(); ... }

// Public() does things
func (u unexported) Public() { ... }

The struct is unexported because we don't want its internals accessed except via
exported methods.

The interface is exported so that users can pass pointers to the struct as items of type
Exported.

But godoc *ignores* the docs for the unexported.Public() method, which it seems to me it
shouldn't do.

[rsc]

Comment 1:

I don't believe this is common.  It never comes up in the
Go standard library.  In what code is this common?
This scenario is incomplete.  It does not show any way for a caller
to create an unexported.  Without that, there is no point to showing
it in godoc.
If the scenario did have a way, like:
func NewUnexported() unexported
then I would suggest making it instead
func NewUnexported() Exported
If you are not willing to let others use the name ("unexported"), then
you shouldn't force it on them in the public API.  Among other
problems, you can't declare a variable that will process the result.
You can write
x := p.NewUnexported()
but not
var x p.unexported = p.NewUnexported()
That's not good API design.
The various 32-bit hash functions are a good
example of this in practice: hash/adler32's New returns
a hash.Hash, not a *digest.  There's no need to expose
the internal name in the external API.
Whether it's internals can be accessed depends on
whether the field names are upper or lower case.
It does not depend on the struct name.
Russ

[mark-summerfield]

Comment 2:

My original note was too terse and assumed a New() function but didn't put one in.
And the pattern is clearly common since it is exactly what is done in hash/adler32: an
unexported struct (digest) and and exported API (hash.Hash).
In adler32 the unexported digest struct has many public methods, Reset, Size, Write,
Sum, and Sum32. But none of these show up in the documentation, and this is what I'm
suggesting is a bug.
The situation I have is essentially this:
type x struct {
    i int
    f func(int) bool
}
func NewX(i int, f func(int) bool) *x {
    return &x{i, f}
}
func (x *x) Check(y int) bool {
    return x.f(y)
}
The x type must not be created using var a x or a := &x{} because that would leave f at
nil which is not acceptable for this type. So my solution is to force the user to use a
New function and to make the x type unexported. This way only valid x's can be created.
If I need to be able to pass an x I can create an interface:
type Xer interface { Check(int) bool }
and now I can pass Xer's around and the internals of x are not exposed anywhere.
However, the *documentation* for x's methods do not appear in godoc. (Just as the
documentation for adler32's digest's exported method's docs don't appear.)
Or are you saying that this pattern is wrong and that it should be done differently?
(And if so, why doesn't hash/adler32 do it differently?)

[rsc]

Comment 3:

This is different from what hash/adler32 does.
Here you have used an unexported name in
an exported API.  The answer is not to do that,
precisely because it puts the caller in a weird
straightjacket where the type cannot be mentioned.
You are fighting the language.  Zero values happen.
Good solutions include:
 (1) define Xer somewhere and make NewX return
     Xer instead of *x (this is what hash/adler32 does).
 (2) document that the zero value is not valid
     and maybe provide an Init method
     (this is what tabwriter.Writer does).
Taking away the caller's ability to speak about
the return type - which is what returning *x does -
is not a good solution.
Russ

[mark-summerfield]

Comment 4:

Sorry, I'd intended to write 
func NewX(i int, f func(int) bool) Xer {
    return &x{i, f}
}
which really is what hash/adler32 does.
But this still leaves the problem that, there are no docs for x.Check() or for
adler32.digest.Write() etc.

[rsc]

Comment 5:

Yes and no.  adler32.New returns a hash.Hash32,
and that interface is documented
(godoc hash Hash; godoc hash Hash32).
There should be a cross-link in the HTML straight
to it, but we're not there yet (we'll get there).
It is true that if there are things specific to this
implementation that need to be mentioned, they
cannot be documented on the digest.Write
method, because that method doesn't show up in docs.
But a prospective user of the package doesn't
know what digest is anyway, since it is unexported,
so showing method docs on it wouldn't be a good
user experience.  Instead the usual approach is to
document the function that is returning the interface
value.  So the adler32.New doc says not just that
it returns a Hash32 but that it returns a Hash32
computing the Adler-32 checksum.
I just noticed that compress/zlib demonstrates
both choices: NewReader returns an io.ReadCloser
and describes that the ReadCloser is a decompressing
reader.  NewWriter returns a concrete *Writer, because
it needed to add the non-standard Flush method and
associated documentation.  If you were to declare
    var w zlib.Writer
then using w would crash.  That's okay.  We trust
programmers either not to do that or to accept the
consequences.
Russ

[rsc]

Comment 6:

Glad to have this discussion here in an issue to point at.
I can't find the one from the last time it came up.

Owner changed to @rsc.

Status changed to WorkingAsIntended.

[peterGo]

Comment 7:

I think this was the last time this issue came up.
Godoc output is inconsistent (compare md5 and blowfish packages) - golang-nuts
http://groups.google.com/group/golang-nuts/browse_thread/thread/5e1709381889e3d5/030f051e33ee769d

[rsc]

Comment 8:

Thanks for tracking down that link.

[rsc]

Comment 9:

Issue #3808 has been merged into this issue.

[rsc]

Comment 10:

Issue #4213 has been merged into this issue.

[adonovan]

Comment 11:

Another datapoint: this arises in the Go protobuf package, w.r.t the type
extendableProto.
Perhaps this is a reasonable thing for govet linting to complain about ("private type in
public function")?  Or for godoc to highlight as design error.

[rsc]

Comment 12:

I think that's bad design in goprotobuf, fwiw.