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

go/doc: comments dropped from interior of interface definition #10858

Open
rsc opened this issue May 14, 2015 · 13 comments
Open

go/doc: comments dropped from interior of interface definition #10858

rsc opened this issue May 14, 2015 · 13 comments
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@rsc
Copy link
Contributor

rsc commented May 14, 2015

The go doc output is missing some important comments from the definition of reflect.Type (so is godoc, but maybe go doc can do better). I've added the missing comments, bracketed by **, below. It's dropping anything that doesn't immediately precede a method definition.

$ go doc reflect.Type
type Type interface {

**
    // Methods applicable to all types.
**

    // Align returns the alignment in bytes of a value of
    // this type when allocated in memory.
    Align() int

    // FieldAlign returns the alignment in bytes of a value of
    // this type when used as a field in a struct.
    FieldAlign() int

    // Method returns the i'th method in the type's method set.
    // It panics if i is not in the range [0, NumMethod()).
    //
    // For a non-interface type T or *T, the returned Method's Type and Func
    // fields describe a function whose first argument is the receiver.
    //
    // For an interface type, the returned Method's Type field gives the
    // method signature, without a receiver, and the Func field is nil.
    Method(int) Method

    // MethodByName returns the method with that name in the type's
    // method set and a boolean indicating if the method was found.
    //
    // For a non-interface type T or *T, the returned Method's Type and Func
    // fields describe a function whose first argument is the receiver.
    //
    // For an interface type, the returned Method's Type field gives the
    // method signature, without a receiver, and the Func field is nil.
    MethodByName(string) (Method, bool)

    // NumMethod returns the number of methods in the type's method set.
    NumMethod() int

    // Name returns the type's name within its package.
    // It returns an empty string for unnamed types.
    Name() string

    // PkgPath returns a named type's package path, that is, the import path
    // that uniquely identifies the package, such as "encoding/base64".
    // If the type was predeclared (string, error) or unnamed (*T, struct{}, []int),
    // the package path will be the empty string.
    PkgPath() string

    // Size returns the number of bytes needed to store
    // a value of the given type; it is analogous to unsafe.Sizeof.
    Size() uintptr

    // String returns a string representation of the type.
    // The string representation may use shortened package names
    // (e.g., base64 instead of "encoding/base64") and is not
    // guaranteed to be unique among types.  To test for equality,
    // compare the Types directly.
    String() string

    // Kind returns the specific kind of this type.
    Kind() Kind

    // Implements reports whether the type implements the interface type u.
    Implements(u Type) bool

    // AssignableTo reports whether a value of the type is assignable to type u.
    AssignableTo(u Type) bool

    // ConvertibleTo reports whether a value of the type is convertible to type u.
    ConvertibleTo(u Type) bool

    // Comparable reports whether values of this type are comparable.
    Comparable() bool

**
    // Methods applicable only to some types, depending on Kind.
    // The methods allowed for each kind are:
    //
    //  Int*, Uint*, Float*, Complex*: Bits
    //  Array: Elem, Len
    //  Chan: ChanDir, Elem
    //  Func: In, NumIn, Out, NumOut, IsVariadic.
    //  Map: Key, Elem
    //  Ptr: Elem
    //  Slice: Elem
    //  Struct: Field, FieldByIndex, FieldByName, FieldByNameFunc, NumField
**

    // Bits returns the size of the type in bits.
    // It panics if the type's Kind is not one of the
    // sized or unsized Int, Uint, Float, or Complex kinds.
    Bits() int

    // ChanDir returns a channel type's direction.
    // It panics if the type's Kind is not Chan.
    ChanDir() ChanDir

    // IsVariadic reports whether a function type's final input parameter
    // is a "..." parameter.  If so, t.In(t.NumIn() - 1) returns the parameter's
    // implicit actual type []T.
    //
    // For concreteness, if t represents func(x int, y ... float64), then
    //
    //  t.NumIn() == 2
    //  t.In(0) is the reflect.Type for "int"
    //  t.In(1) is the reflect.Type for "[]float64"
    //  t.IsVariadic() == true
    //
    // IsVariadic panics if the type's Kind is not Func.
    IsVariadic() bool

    // Elem returns a type's element type.
    // It panics if the type's Kind is not Array, Chan, Map, Ptr, or Slice.
    Elem() Type

    // Field returns a struct type's i'th field.
    // It panics if the type's Kind is not Struct.
    // It panics if i is not in the range [0, NumField()).
    Field(i int) StructField

    // FieldByIndex returns the nested field corresponding
    // to the index sequence.  It is equivalent to calling Field
    // successively for each index i.
    // It panics if the type's Kind is not Struct.
    FieldByIndex(index []int) StructField

    // FieldByName returns the struct field with the given name
    // and a boolean indicating if the field was found.
    FieldByName(name string) (StructField, bool)

    // FieldByNameFunc returns the first struct field with a name
    // that satisfies the match function and a boolean indicating if
    // the field was found.
    FieldByNameFunc(match func(string) bool) (StructField, bool)

    // In returns the type of a function type's i'th input parameter.
    // It panics if the type's Kind is not Func.
    // It panics if i is not in the range [0, NumIn()).
    In(i int) Type

    // Key returns a map type's key type.
    // It panics if the type's Kind is not Map.
    Key() Type

    // Len returns an array type's length.
    // It panics if the type's Kind is not Array.
    Len() int

    // NumField returns a struct type's field count.
    // It panics if the type's Kind is not Struct.
    NumField() int

    // NumIn returns a function type's input parameter count.
    // It panics if the type's Kind is not Func.
    NumIn() int

    // NumOut returns a function type's output parameter count.
    // It panics if the type's Kind is not Func.
    NumOut() int

    // Out returns the type of a function type's i'th output parameter.
    // It panics if the type's Kind is not Func.
    // It panics if i is not in the range [0, NumOut()).
    Out(i int) Type

    common() *rtype
    uncommon() *uncommonType
}
@rsc rsc added this to the Go1.5 milestone May 14, 2015
@robpike robpike assigned griesemer and unassigned robpike May 15, 2015
@robpike robpike changed the title cmd/doc: comments dropped from interior of interface definition go/doc: comments dropped from interior of interface definition May 15, 2015
@robpike robpike modified the milestones: Go1.6, Go1.5 May 15, 2015
@robpike
Copy link
Contributor

robpike commented May 15, 2015

go/doc doesn't capture this information, so it's not easy to present. Should be easier when the new go/* packages appear.

Assigning to gri and deferring.

@rsc rsc modified the milestones: Go1.7Early, Go1.6 Jan 6, 2016
@griesemer griesemer modified the milestones: Go1.7, Go1.7Early May 5, 2016
@rsc rsc modified the milestones: Go1.8, Go1.7 May 18, 2016
@quentinmit quentinmit added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 7, 2016
@rsc rsc modified the milestones: Go1.9Early, Go1.8 Oct 26, 2016
@griesemer
Copy link
Contributor

This is an issue of heuristics: How do we know that such interior comments should be collected in the first place? There's probably cases of structs/interfaces that contain such comments which are for internal use only. Should they always be shown?

@rsc
Copy link
Contributor Author

rsc commented Feb 24, 2017

If it's an exported type and they precede an exported field/method, then I think yes it is reasonable to collect them. People will adjust.

@bradfitz bradfitz modified the milestones: Go1.9Maybe, Go1.9Early May 3, 2017
@griesemer
Copy link
Contributor

There's currently no way to represent/attach blocks of non-contiguous comments (w/o empty lines between them) to fields (which is what go doc, godoc, etc. are relying on for this). At cheap solution would be to make such comments contiguous by "connecting them" via an empty line comment, but that would change the original source in undesirable ways (it would require some nasty context-sensitive hackery in the go/parser). This could be improved upon by allowing ast.Comments to represent empty lines. However, a lot of code makes the assumption that ast.Comment.Text is never empty, so that's probably not an easy change to make.

Overall, too late for Go 1.9.

Should make this higher priority for Go 1.10.

@griesemer griesemer modified the milestones: Go1.10, Go1.9Maybe Jun 23, 2017
@rsc rsc modified the milestones: Go1.10, Go1.11 Nov 22, 2017
@griesemer griesemer modified the milestones: Go1.11, Go1.12 Jun 27, 2018
@griesemer griesemer modified the milestones: Go1.12, Go1.13 Dec 6, 2018
@griesemer
Copy link
Contributor

Too late for 1.12.

To try for 1.13: Collect all comments (even if there's empty lines) preceeding a field/method if inside a struct/interface. That should be easy to do; but we should do it early in the cycle to see what the consequences are.

@griesemer griesemer added the early-in-cycle A change that should be done early in the 3 month dev cycle. label Dec 6, 2018
@navytux
Copy link
Contributor

navytux commented Dec 18, 2018

Please also consider showing exported interface comments when they are not preceeding any exported method. For example (** highlights what should be exported):

// IStorageDriver is the raw interface provided by ZODB storage drivers.
type IStorageDriver interface {
        // URL returns URL of how the storage was opened
        URL() string

        // Close closes storage
        Close() error

        // LastTid returns the id of the last committed transaction.
        //
        // If no transactions have been committed yet, LastTid returns 0.
        LastTid(ctx context.Context) (Tid, error)

        Loader
        Iterator

**        
        // A storage driver also delivers database change events to watchq
        // channel, which is passed to it when the driver is created.
**
}

Thanks beforehand,
Kirill

@agnivade
Copy link
Contributor

agnivade commented Feb 3, 2019

Looking into this. This seems to work if I change param to consumeCommentGroup from 1 to 2 here in go/parser/parser.go (*parser) next()-

// consume successor comments, if any
endline = -1
for p.tok == token.COMMENT {
	comment, endline = p.consumeCommentGroup(2)
}

This changes the no. of lines to skip while looking for the next comment line.

But to add additional logic to check if the parser is currently inside a struct/interface, we need to add additional state inside the parser. Or do we want to pass this through params (may be cumbersome ?).

@griesemer ?

@agnivade
Copy link
Contributor

agnivade commented Feb 5, 2019

I have a CL which takes the approach of an additional field inside the parser struct. Let's discuss further on the CL.

@gopherbot
Copy link

Change https://golang.org/cl/161177 mentions this issue: go/parser: include more comments in a struct or interface

@gopherbot
Copy link

Change https://golang.org/cl/185040 mentions this issue: Revert "go/parser: include more comments in a struct or interface"

@griesemer griesemer reopened this Jul 8, 2019
gopherbot pushed a commit that referenced this issue Jul 8, 2019
This reverts commit https://golang.org/cl/161177/.

Reason for revert: this led to non-contiguous comments spaced
by an empty line to be grouped into a single CommentGroup

Fixes #32944
Updates #10858

Change-Id: I5e16663b308c3b560496da8e66c33befdf9ed9dd
Reviewed-on: https://go-review.googlesource.com/c/go/+/185040
Reviewed-by: Robert Griesemer <gri@golang.org>
@griesemer
Copy link
Contributor

Re-opened since we decided to revert https://golang.org/cl/161177.

@andybons andybons removed this from the Go1.13 milestone Jul 8, 2019
@dmitshur dmitshur self-assigned this Jul 8, 2019
@gertcuykens

This comment has been minimized.

@agnivade
Copy link
Contributor

It is different. I think this is happening because r is an anonymous interface, and gddo shows its comments in the index itself.

@seankhliao seankhliao added this to the Unplanned milestone Aug 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests