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.

Sign up for GitHub

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

Jump to bottom

go/doc: doc.New returns wrong type for constant declaration #16153

Closed
adonovan opened this issue Jun 22, 2016 · 7 comments
Closed

go/doc: doc.New returns wrong type for constant declaration #16153

adonovan opened this issue Jun 22, 2016 · 7 comments
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.10

Comments

@adonovan
Copy link
Member

doc.New returns incorrect type information about the constant Y in this program:

package hello
const (
    x uint8 = 255
    Y = 256
)

Test case: https://play.golang.org/p/-2ZQcrQgCM

The cause is this heuristic code in (*reader).readValue:

        case decl.Tok == token.CONST:
            // no type is present but we have a constant declaration;
            // use the previous type name (w/o more type information
            // we cannot handle the case of unnamed variables with
            // initializer expressions except for some trivial cases)
            name = prev

Enabling the doc.AllDecls mode bit works around the problem in this particular example because x is unexported.

This bug is the underlying cause of godoc.org issue 409.
golang/gddo#409
@adonovan adonovan mentioned this issue Jun 22, 2016
Open
@dmitris
Copy link
Contributor

dmitris commented Jun 22, 2016 •
edited
Loading

I digged a bit into the code and it seems the problem is (also?) in go/doc/exports.go and the way type propagation is done in filterSpecList.

filterSpecList is called here:
https://github.com/golang/go/blob/master/src/go/doc/exports.go#L229

and defined in
https://github.com/golang/go/blob/master/src/go/doc/exports.go#L196

in filterSpecList, if specType = nil && prevType != nil, spec.Type is set from prevType with a copyConstType call just returns the previous Type's Name:
https://github.com/golang/go/blob/master/src/go/doc/exports.go#L203
https://github.codm/golang/go/blob/master/src/go/doc/exports.go#L176-183

and I believe this is how we end up with incorrect type information.

Do we need that type propagation at all in the doc package for consts? Or could we simply delete this block of code:
https://github.com/golang/go/blob/master/src/go/doc/exports.go#L197-215

I modified my test package to highlight the fact that the type propagation is not related to integers:
https://godoc.org/github.com/dmitris/mytest shows now:

const (
    ConstBad string = 256 // max uint8 + 1

    Bar uint8 = "bar"
)

@josharian
Copy link
Contributor

Related: #5397.

The right fix for this may be to rewrite the entire package to use go/types, now that go/types is in the stdlib. That's a fair amount of work, though.

@dmitshur
Copy link
Contributor

dmitshur commented Jun 23, 2016 •
edited
Loading

The right fix for this may be to rewrite the entire package to use go/types, now that go/types is in the stdlib.

Right now, go/doc has a very distinguishable property that it can work on a single package without needing to look at any of its dependencies (in fact, they could be missing completely). Would that property be preserved if it's built on top of go/types (as far as I can tell, it would not)? Would we want to preserve it?

There are certain things that go/doc cannot currently do because it requires knowing about dependencies (for example, listing all methods of a type, when some of the methods are coming from an embedded type which is defined in a dependency).

Another point, the package is currently described as Package doc extracts source code documentation from a Go AST., that would no longer be very accurate if it's re-written on top of go/types.

@adonovan
Copy link
Member Author

I agree with Dmitri: this package should not use go/types. It's not clear to me that go/doc really belongs in the standard library at all. It is primarily a collection of utility functions common to the increasing number of tools whose name is some variation of "godoc": golang.org/x/tools/cmd/godoc, go doc, http://godoc.org, and at least two more tools within Google.

@quentinmit quentinmit added this to the Go1.8 milestone Jun 23, 2016
@quentinmit quentinmit added the NeedsFix The path to resolution is known, but the work has not been done. label Jun 23, 2016
@rsc rsc modified the milestones: Go1.9, Go1.8 Nov 11, 2016
@bradfitz bradfitz modified the milestones: Go1.10, Go1.9 Jun 14, 2017
@bradfitz
Copy link
Contributor

Punting to Go 1.10. Sorry. Any help wanted, though.

/cc @griesemer (in case you hadn't seen this one)

@griesemer griesemer self-assigned this Jun 14, 2017
@griesemer griesemer added the early-in-cycle A change that should be done early in the 3 month dev cycle. label Jun 30, 2017
@griesemer
Copy link
Contributor

The bug is fairly simple: A previous constant type should only be used if there's no type and no value in a constant declaration. The existing code ignores values.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/68730 mentions this issue: go/doc: fix constant type propagation

@golang golang locked and limited conversation to collaborators Oct 9, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.10
Development

No branches or pull requests
9 participants
@bradfitz @dmitris @josharian @rsc @quentinmit @dmitshur @adonovan @griesemer @gopherbot