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

proposal: go/doc: consts/vars should be grouped with types by their computed type #22918

Closed
willfaught opened this issue Nov 29, 2017 · 30 comments

Comments

@willfaught
Copy link
Contributor

commented Nov 29, 2017

Currently, constant and variable declarations must have an explicit type to be grouped with that type. For example, go/build.Context is declared like var Default Context = defaultContext(), with an explicit Context type, because otherwise Default wouldn't be listed under the Context type.

If you add an explicit type, and the type can be inferred from the right-hand side of the assignment, then golint complains about the redundant explicit type.

If the Context type was missing from the Default declaration, it would be impossible to know for sure what type it is, since defaultContext isn't exported. (Okay, maybe you could guess in this situation, but what if it was called getDefault instead?)

We should instead compute a const/var declaration's type with the type checker and group them in doc accordingly.

@gopherbot gopherbot added this to the Proposal milestone Nov 29, 2017

@gopherbot gopherbot added the Proposal label Nov 29, 2017

@mvdan

This comment has been minimized.

Copy link
Member

commented Nov 29, 2017

This makes sense, but I wonder if the restriction was made on purpose so that the doc family of programs didn't need to run the type-checker.

Perhaps this is more attainable now that better caching is in place. vet now requires type information at all times, for example.

@dsnet

This comment has been minimized.

Copy link
Member

commented Nov 29, 2017

The grouping is performed by the go/doc package. As @mvdan mentioned, I'm pretty sure the reason is because doc.New which takes in an ast.Package lacks the type information needed to infer what the type of the variable is. It's not clear to me how to resolve this. You could derive a limited amount of type information from the ast.Package alone, but I'm not sure the complexity of that logic is worth it.

\cc @griesemer

@dsnet dsnet changed the title proposal: cmd/doc: consts/vars should be grouped with types by their computed type proposal: go/doc: consts/vars should be grouped with types by their computed type Nov 29, 2017

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2017

What @mvdan and @dsnet said. go/doc doesn't have exact type information and uses heuristics. For variables and constants that don't have an explicit (or named) type in the declaration, the type would have to be inferred from the initialization expressions which is non-trivial.

With respect to any "decisions being made", note that go/doc is one of the earliest Go packages around (just after go/ast), years before go/types came into being. It was always meant to be working on source code alone (so that it would work with partial packages or missing import information).

Now, with go/types being available, one could do a much better job when all information is present, and gracefully degrade to heuristics when information is missing.

The suggestion is reasonable, not sure it needs to be a proposal. If somebody wants to pick this up and do the actual work, I'd be ok with it. Note that changes to go/doc's API need to be backward compatible to satisfy the Go 1 guarantee. Also, all consumers of go/doc need to be adjusted accordingly. It's a lot of work.

Finally, and perhaps most importantly, there need to be decisions as to what to group where exactly, and what to do when type information is missing.

@willfaught

This comment has been minimized.

Copy link
Contributor Author

commented Nov 29, 2017

@griesemer When should an issue be a proposal? I'd assumed it was for non-bug/enhancement issues.

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2017

@willfaught I just re-read the proposal requirements and you're correct, this suggestion meets the requirements for a proposal.

@jimmyfrasche

This comment has been minimized.

Copy link
Member

commented Nov 29, 2017

There would need to be two changes to the go/doc api.

A new Package constructor:

package doc
//use typs instead of heuristics for grouping
func NewTyped(pkg *ast.Package, typs *types.Package, importPath string, mode Mode) *Package

And a means to handle type aliases. Maybe an Aliases []*ast.GenDecl on doc.Type?

However, since this is to support the godoc/go doc commands the real hard part is getting the type information into them. The godoc vfs caused some issues when I tried it ( #20131 (comment) ) and when run locally you'd want access to the go tools's cache, but godoc isn't part of the go tool so it's unclear to me how that would work (though I haven't really looked at any of the caching code).

@rsc

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2017

As @griesemer said above, this would require type information inside go/doc (and therefore available to cmd/doc, etc). That has not been an assumption before, and it would significantly complicate cmd/doc's file system scans.

@rsc rsc closed this Dec 4, 2017

@rsc rsc reopened this Dec 4, 2017

@rsc

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2017

I didn't mean to close this - wrong button. But probably we can't do this - leaving open for more discussion.

@willfaught

This comment has been minimized.

Copy link
Contributor Author

commented Dec 12, 2017

Wouldn't doc.New be able to construct a types.Package from its pkg parameter?

doc.New:

func New(pkg *ast.Package, importPath string, mode Mode) *Package

types.Config.Check:

func (conf *Config) Check(path string, fset *token.FileSet, files []*ast.File, info *Info) (*Package, error)

For example:

// doc.New
func New(pkg *ast.Package, importPath string, mode Mode) *Package {
    var c = &types.Config{
        DisableUnusedImportCheck: true,
        IgnoreFuncBodies: true,
        Importer: importer.Default(),
    }

    var files []*ast.File

    for _, f := range pkg.Files {
        files = append(files, f)
    }

    var tp *types.Package

    if p, err := c.Check(importPath, token.NewFileSet(), files); err == nil && p.Complete() {
        tp = p
    }

    // ...
}

@jimmyfrasche What's godoc vfs?

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Dec 12, 2017

@willfaught There's no question that go/doc could type-check packages. But it is also supposed to work with incomplete packages (at least that's one of the original intents). The devil is in the details.

@jimmyfrasche

This comment has been minimized.

Copy link
Member

commented Dec 12, 2017

@willfaught godoc (the server) has a Virtual File System.

When you run godoc locally it's just a proxy to the local file system, but when it's being used to run golang.org all the Go source files are bundled into a zip file and the vfs grabs files from that instead of the real file system.

None of that affects go/doc the package but it could mean that godoc the program would not be able to take advantage of this new feature in go/doc the package; at least not without a good deal of effort.

@willfaught

This comment has been minimized.

Copy link
Contributor Author

commented Dec 12, 2017

@griesemer Couldn't it fall back to the original syntax-based approach if type checking fails, as you suggested earlier? This wouldn't require any new declarations or modifying any of the doc callers.

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Dec 12, 2017

@willfaught This has already been all discussed. Let's not continue this here, as this issue is closed. Instead if you like to discuss this and various options further I suggest one of the usual mailing lists. Thanks.

Redacted: Sorry; I misread. The proposal is still open.

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Dec 12, 2017

@willfaught Going back to your question: Yes, one could fail back to the syntax-based approach (I think this was discussed above). But then we may have some constants/variables associated with types and some free-standing. Is that the behavior we want? Wouldn't that be confusing and only invite more questions?

@jimmyfrasche

This comment has been minimized.

Copy link
Member

commented Dec 12, 2017

@griesemer godoc the program could have a little note at the top warning that typechecking failed and that the results may not be as accurate as possible.

@willfaught

This comment has been minimized.

Copy link
Contributor Author

commented Dec 12, 2017

@griesemer I think that's the behavior we currently have; it's the original motivation for this issue. In the worst case, we would have the same problem, and in the best case, everything would be organized properly.

@jimmyfrasche

This comment has been minimized.

Copy link
Member

commented Dec 12, 2017

@willfaught I believe the issue is you look at godoc then make a change (which breaks typechecking) and look at godoc again and now the package looks completely different because it couldn't use the typechecking to order everything with as much precision

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Dec 12, 2017

@jimmyfrasche Exactly. What I have been saying in #22918 (comment) a while back, though not spelled out in detail.

This is not about "can it be done?", but about "should it be done?" and if so "how should it be done?".

@jimmyfrasche

This comment has been minimized.

Copy link
Member

commented Dec 12, 2017

@griesemer I think a notification should be sufficient, but there are actually two related things being discussed:

  1. allow go/doc to optionally use type info to provide better results
  2. have official tools that use go/doc attempt to use that option

If 1 is opt-in then 2 can continue to opt-out until a decision is made—or opt-in with an experimental switch to see if it's worth the bother.

There are plenty of non-official tools using go/doc, so a reasonable question is whether 1 would provide value to them. I've written several tools that use go/doc and none of mine would benefit from opting-in, but there could be other tools that would. Does anyone know of any?

@dsnet

This comment has been minimized.

Copy link
Member

commented Dec 12, 2017

I'm not sure a warning is such a good idea.

The reality is that today depending on how the ast.Package was generated, the output of go/doc may differ because there may be partial information in the ast.Package. Even worse, it's not obvious when information is missing or not.

People have to be willing to accept that godoc is fundamentally a best-effort at providing sensible docs depending on a number of heuristics. Personally, I'm not opposed to the idea of using more type information to make godoc better, but I'm still concerned about the increase in complexity. The code for godoc is already complicated enough.

@jimmyfrasche

This comment has been minimized.

Copy link
Member

commented Dec 12, 2017

A warning would make it obvious that information is missing, but having poked godoc several times I certainly agree that it's complicated enough already: it could stand some refactoring

@willfaught

This comment has been minimized.

Copy link
Contributor Author

commented Dec 13, 2017

I would think if you change your own code, or update others' code, and it doesn't compile, and the doc changes to something not quite as accurate as it was before, then that's to be expected, and it's easily explained. In my experience, I most often use godoc for others' code, which I'm not changing and compiles, so I would guess such a scenario is rare.

Note that changing the organization at all isn't out of the question, since 1.10 changes slice results to be constructors, and array results as constructors have also been approved in #22856. Both changes would move functions under a type, where they weren't before. This issue proposes a similar move under types for consts and vars.

@jimmyfrasche

This comment has been minimized.

Copy link
Member

commented Dec 13, 2017

Also without type information you can hide stuff that shouldn't be hidden:

type Exported struct{}
type unexported = Exported
func (unexported) String() string { return "surprise, I'm a fmt.Stringer!" }

Though that particular case could probably be handled by go/doc with just the AST using a "group aliases" pass

@rsc

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2018

For example, go/build.Context is declared like var Default Context = defaultContext(), with an explicit Context type, because otherwise Default wouldn't be listed under the Context type.

The reason it says Context there is so that when that line shows up in the documentation or when editing the source file, people can see what the type is. The fact that go/doc can see it too is nice, but not the point.

Trying to type check on the file system scan in go doc foo.Bar (at least resolving all imports) really seems prohibitive. Especially since there are more reasons to make the type explicit. To the extent that the go/doc conventions and limitations force people to write more readable code by making types explicit, that seems like a good thing to me.

@willfaught

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2018

The reason it says Context there is so that when that line shows up in the documentation or when editing the source file, people can see what the type is. The fact that go/doc can see it too is nice, but not the point. [...] To the extent that the go/doc conventions and limitations force people to write more readable code by making types explicit, that seems like a good thing to me.

The golint rule contradicts this. Do you think we should remove the rule, then?

If we compute the type, then the declaration is grouped with the type in the doc, so this only improves readability when you're looking at it in code and can't easily find/see the function's declaration, but that's also the case in function bodies, where I rarely see explicit types like var foo T = bar(), which would similarly be more readable.

If the RHS expression is a literal like

var X Foo = Foo{...}
var Y Foo = Foo{...]
var Z Foo = Foo{...}

then the explicit type doesn't improve readability, and the code stutters.

Trying to type check on the file system scan in go doc foo.Bar (at least resolving all imports) really seems prohibitive.

Do we know how much slower it would be? Maybe it wouldn't be noticeable.

If the package has already been built, then the type info is cached in the package object files, if I understand correctly, so this would only apply to unbuilt or modified packages.

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2018

go/doc was written to work w/o exact type information; it's using heuristics and so far that has worked quite well. A user also has the choice to write down types where they are currently inferred to improve documentation. While type-checking might be possible in some cases, it's likely too expensive in general and w/o caching. And when only partial information is available, it won't work 100% either; thus still requiring a heuristic approach. Finally, adjusting go/doc to use go/types may require API changes in go/doc (to support the go/types setup, e.g. for caching, etc.). A future support library, designed around go/types, might do a better job, but we're not going to retrofit go/doc at this point.

Closing, on behalf of the proposal review team.

@willfaught

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2018

A user also has the choice to write down types where they are currently inferred to improve documentation.

Should we then remove the golint rule?

@dsnet

This comment has been minimized.

Copy link
Member

commented Mar 19, 2018

/cc @andybons

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2018

@willfaught Please discuss with the golint maintainers. My (personal) opinion is that a tool or style guide shouldn't prevent me from writing down an explicit type if I feel it's improving documentation or readability.

@andybons

This comment has been minimized.

Copy link
Member

commented Mar 20, 2018

golint's scope states:

Golint is meant to carry out the stylistic conventions put forth in Effective Go and CodeReviewComments. Changes that are not aligned with those documents will not be considered.

If the golint rule does not appear within either of these documents, then it can be removed. I have not combed those documents to see whether this is the case, though.

Feel free to file a bug at github.com/golang/lint and send along a change.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
9 participants
You can’t perform that action at this time.