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: list types that satisfy an interface within its package #20131

Open
mvdan opened this Issue Apr 26, 2017 · 17 comments

Comments

Projects
None yet
4 participants
@mvdan
Member

mvdan commented Apr 26, 2017

Forked from #19412 (comment), to continue discussion with @jimmyfrasche and any others without taking over that proposal thread.

The pattern of having an interface with an unexported method and types that implement it is a fairly common pattern in Go as we don't have sum types. A good example is in go/ast: https://golang.org/pkg/go/ast/#Spec

For a user reading the godoc, it's very useful to them to know what are the types that can go in that interface. The actual information is hidden in the code, so most packages like go/ast above list them manually: The Spec type stands for any of *ImportSpec, *ValueSpec, and *TypeSpec.

This works OK, but the issue is that these names aren't linked to the types in the same page. I propose that this happen automatically, much like #4953 for packages. Linking to types (and potentially other exported names like funcs and consts) could be useful for other purposes.

This is the conservative solution. A more aggressive solution would be to automatically generate the list of types that can fit into the interface. If we had sum types that could be limited to those kinds of types and be trivial to implement.

As long as we don't have sum types, @jimmyfrasche was suggesting to just list all the types that satisfy the interface in its package. I'm not sure if that's a good idea for all interfaces, but would certainly be useful for those that follow the pattern of an unexported method to mimic sum types. Not sure how safe/desired such a change would be.

Manually listing the types in the godoc isn't a huge pain, but not having links is a problem.

CC @griesemer who wrote the go/ast godoc and might have some input

@mvdan mvdan added this to the Unreleased milestone Apr 26, 2017

@mvdan

This comment has been minimized.

Show comment
Hide comment
@mvdan

mvdan Apr 26, 2017

Member

For context, this is what users currently see on the package I'm working on: https://godoc.org/github.com/mvdan/sh/syntax#Command

The types are there, and you can select and do a search within the page, but that's just inconvenient for no reason.

Member

mvdan commented Apr 26, 2017

For context, this is what users currently see on the package I'm working on: https://godoc.org/github.com/mvdan/sh/syntax#Command

The types are there, and you can select and do a search within the page, but that's just inconvenient for no reason.

@jimmyfrasche

This comment has been minimized.

Show comment
Hide comment
@jimmyfrasche

jimmyfrasche Apr 26, 2017

Member

In #18342, starting here, automatic linking is discussed and accepted.

That will be very useful in general but I don't think it's optimal for this case.

Especially in AST code, there's going to be many types involved and the links could be automatically generated without any user intervention automatically improving docs that do not list the types manually avoiding manual lists going out of sync.

The questions are how to link the types, where, and when.

In any case, it should only be limited to the types in a given package or godoc would have to check everything against everything and the lists generated could be quite long, even if it were practical to compute them in a reasonable amount of time.

Empty interfaces should always be skipped because there's no information gained and much noise produced.

Unexported interfaces would only make sense when ?m=all (or if #7823 is implemented and the unexported interface matches the definition of "accessible" given in that thread).

In the go/ast example essentially every type in that package implements ast.Node so listing the implementing types on ast.Node could be quite noisy, though it would be very useful when constructing type switches as you could see all the possible cases at a glance. But it also raises the question of whether it should list compatible interfaces such as ast.Spec. Maybe it could be in a collapsed-by-default fieldset, as examples are, if it exceeds more than a handful of entries.

Listing the local, non-empty interfaces a non-interface type implements would be brief and useful. A line below the doc string for, say, *ast.TypeSpec that says "implements: ast.Node, ast.Spec" (properly linked) is short and easy to follow.

This could be further limited to only interface types with unexported methods as those are the major pain point, but it seems generally useful.

Member

jimmyfrasche commented Apr 26, 2017

In #18342, starting here, automatic linking is discussed and accepted.

That will be very useful in general but I don't think it's optimal for this case.

Especially in AST code, there's going to be many types involved and the links could be automatically generated without any user intervention automatically improving docs that do not list the types manually avoiding manual lists going out of sync.

The questions are how to link the types, where, and when.

In any case, it should only be limited to the types in a given package or godoc would have to check everything against everything and the lists generated could be quite long, even if it were practical to compute them in a reasonable amount of time.

Empty interfaces should always be skipped because there's no information gained and much noise produced.

Unexported interfaces would only make sense when ?m=all (or if #7823 is implemented and the unexported interface matches the definition of "accessible" given in that thread).

In the go/ast example essentially every type in that package implements ast.Node so listing the implementing types on ast.Node could be quite noisy, though it would be very useful when constructing type switches as you could see all the possible cases at a glance. But it also raises the question of whether it should list compatible interfaces such as ast.Spec. Maybe it could be in a collapsed-by-default fieldset, as examples are, if it exceeds more than a handful of entries.

Listing the local, non-empty interfaces a non-interface type implements would be brief and useful. A line below the doc string for, say, *ast.TypeSpec that says "implements: ast.Node, ast.Spec" (properly linked) is short and easy to follow.

This could be further limited to only interface types with unexported methods as those are the major pain point, but it seems generally useful.

@mvdan

This comment has been minimized.

Show comment
Hide comment
@mvdan

mvdan Apr 26, 2017

Member

I wasn't aware that automatic linking had already been proposed and accepted, thanks. I didn't find it because that issue doesn't mention any of this in the title.

That means that the first part of my proposal has already been proposed and acepted, so I'll retitle the issue to be more clear.

What you suggest - doing it for any interface within a package - seems OK, but I'm worried about it having unwanted effects or being too verbose. Like you say, some interface types may be very broad and you might end up with very long lists. Or some interfaces might not be of this "sum type" kind, and the list would not be appropriate.

Limiting it to interfaces with unexported methods would perhaps improve the situation, but I wonder if that would make the logic too convoluted. And perhaps there are other ways to represent sum types in current Go that I'm not aware of.

Member

mvdan commented Apr 26, 2017

I wasn't aware that automatic linking had already been proposed and accepted, thanks. I didn't find it because that issue doesn't mention any of this in the title.

That means that the first part of my proposal has already been proposed and acepted, so I'll retitle the issue to be more clear.

What you suggest - doing it for any interface within a package - seems OK, but I'm worried about it having unwanted effects or being too verbose. Like you say, some interface types may be very broad and you might end up with very long lists. Or some interfaces might not be of this "sum type" kind, and the list would not be appropriate.

Limiting it to interfaces with unexported methods would perhaps improve the situation, but I wonder if that would make the logic too convoluted. And perhaps there are other ways to represent sum types in current Go that I'm not aware of.

@mvdan mvdan changed the title from x/tools/cmd/godoc: link to types that fit a "sum type" interface within its package to x/tools/cmd/godoc: list types that fit a "sum type" interface within its package Apr 26, 2017

@jimmyfrasche

This comment has been minimized.

Show comment
Hide comment
@jimmyfrasche

jimmyfrasche Apr 26, 2017

Member

yeah, I had a vague recollection that automatic linking had come up at some point, but it took me forever to find it since it didn't have its own issue.

To be clear, since re-reading it seems somewhat ambiguous, I don't mean grouping anything in the table of contents, like listing implementing types that implement an interface under the interface the way constructors are listed under a type—that would get quite messy very quickly. I mean adding a note under https://golang.org/pkg/go/ast/#TypeSpec saying which interfaces TypeSpec implements that are declared in the same package and a collapsed-by-default list under https://golang.org/pkg/go/ast/#Node so it doesn't take up too much room but's there if you need it.

I think every interface should be considered because it's a natural expansion—same logic minus one proposition—and I can't think of a reason it would be harmful or unhelpful to make note of that information. Maybe there's some extant package out there that makes it obvious why doing so would be a bad idea, but I can't think of any.

(re sum types: *T is the sum 1 + T and const/iota can be 1 + 1 + ⋯ + 1 sum types, technically)

Member

jimmyfrasche commented Apr 26, 2017

yeah, I had a vague recollection that automatic linking had come up at some point, but it took me forever to find it since it didn't have its own issue.

To be clear, since re-reading it seems somewhat ambiguous, I don't mean grouping anything in the table of contents, like listing implementing types that implement an interface under the interface the way constructors are listed under a type—that would get quite messy very quickly. I mean adding a note under https://golang.org/pkg/go/ast/#TypeSpec saying which interfaces TypeSpec implements that are declared in the same package and a collapsed-by-default list under https://golang.org/pkg/go/ast/#Node so it doesn't take up too much room but's there if you need it.

I think every interface should be considered because it's a natural expansion—same logic minus one proposition—and I can't think of a reason it would be harmful or unhelpful to make note of that information. Maybe there's some extant package out there that makes it obvious why doing so would be a bad idea, but I can't think of any.

(re sum types: *T is the sum 1 + T and const/iota can be 1 + 1 + ⋯ + 1 sum types, technically)

@mvdan

This comment has been minimized.

Show comment
Hide comment
@mvdan

mvdan Apr 27, 2017

Member

@jimmyfrasche perhaps you're right and doing it for all interfaces would be a good idea. This could be similar to how types list their constructors. You could make the same argument that a constructor could be a false positive, but it's still OK to list it there.

Let's see if there are any counter-arguments to doing it for all interfaces.

Member

mvdan commented Apr 27, 2017

@jimmyfrasche perhaps you're right and doing it for all interfaces would be a good idea. This could be similar to how types list their constructors. You could make the same argument that a constructor could be a false positive, but it's still OK to list it there.

Let's see if there are any counter-arguments to doing it for all interfaces.

@jimmyfrasche

This comment has been minimized.

Show comment
Hide comment
@jimmyfrasche

jimmyfrasche Apr 27, 2017

Member

Here's a fancy mock up of the presentation I imagine:

mockup

Member

jimmyfrasche commented Apr 27, 2017

Here's a fancy mock up of the presentation I imagine:

mockup

@mvdan

This comment has been minimized.

Show comment
Hide comment
@mvdan

mvdan Apr 28, 2017

Member

@jimmyfrasche that looks good - we should have that implemented and ready in a CL by the time the 1.10 tree opens in August. For now I'd wait a few more weeks to see if there are any objections.

I also just realised I didn't write this as a proposal. Perhaps I should re-open as a formal proposal so that the weekly proposal review meeting can catch it.

Member

mvdan commented Apr 28, 2017

@jimmyfrasche that looks good - we should have that implemented and ready in a CL by the time the 1.10 tree opens in August. For now I'd wait a few more weeks to see if there are any objections.

I also just realised I didn't write this as a proposal. Perhaps I should re-open as a formal proposal so that the weekly proposal review meeting can catch it.

@mvdan mvdan changed the title from x/tools/cmd/godoc: list types that fit a "sum type" interface within its package to Proposal: x/tools/cmd/godoc: list types that fit a "sum type" interface within its package May 24, 2017

@mvdan mvdan added the Proposal label May 24, 2017

@mvdan mvdan modified the milestones: Proposal, Unreleased May 24, 2017

@mvdan

This comment has been minimized.

Show comment
Hide comment
@mvdan

mvdan May 24, 2017

Member

For now, I've made it a proposal. Hopefully the proposal review meetings will catch it at some point soon.

Member

mvdan commented May 24, 2017

For now, I've made it a proposal. Hopefully the proposal review meetings will catch it at some point soon.

@rsc rsc changed the title from Proposal: x/tools/cmd/godoc: list types that fit a "sum type" interface within its package to proposal: x/tools/cmd/godoc: list types that fit a "sum type" interface within its package Jun 5, 2017

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Jun 5, 2017

Contributor

This seems OK to surface, and not just for pseudo-sum types but any interface. Please do send a CL for Go 1.10.

Contributor

rsc commented Jun 5, 2017

This seems OK to surface, and not just for pseudo-sum types but any interface. Please do send a CL for Go 1.10.

@rsc rsc changed the title from proposal: x/tools/cmd/godoc: list types that fit a "sum type" interface within its package to x/tools/cmd/godoc: list types that fit a "sum type" interface within its package Jun 5, 2017

@mvdan mvdan changed the title from x/tools/cmd/godoc: list types that fit a "sum type" interface within its package to x/tools/cmd/godoc: list types that satisfy an interface within its package Jun 16, 2017

@mvdan

This comment has been minimized.

Show comment
Hide comment
@mvdan

mvdan Jun 16, 2017

Member

Thank you, Russ. @jimmyfrasche would you like to collaborate on this?

Member

mvdan commented Jun 16, 2017

Thank you, Russ. @jimmyfrasche would you like to collaborate on this?

@jimmyfrasche

This comment has been minimized.

Show comment
Hide comment
@jimmyfrasche

jimmyfrasche Jun 16, 2017

Member

@mvdan I'd be happy to

Member

jimmyfrasche commented Jun 16, 2017

@mvdan I'd be happy to

@jimmyfrasche

This comment has been minimized.

Show comment
Hide comment
@jimmyfrasche

jimmyfrasche Sep 29, 2017

Member

@mvdan I played with this a little tonight.

I couldn't figure out a way to get the source importer to interface with godoc's vfs. I rigged a fake importer that just always returned an incomplete and empty types.Package. That works surprisingly well, but it means it only works for interfaces that don't refer to types in other packages. So go/ast's out 👎

I looked at the source for the source importer. It's not that big, about 200 lines (and another 150 of tests), but it would still be a bit of a hassle to bolt it onto godoc and if looking at package A can load packages B through Z there might have to be caching and cache invalidation baked into it. No matter what the two copies would be somewhat divergent. That might be fine, but it has a lot of TODOs and bugs already.
. . .

golang.org/x/tools/go/gcexportdata looks like it could work but that requires things be built
Another option would be to rig an entirely source based mini-typechecker to figure it out. The big issues with that would be dealing with all the edge cases of different import names in different files.

At that point it would probably be easier to take the mostly filled in type info from the first approach and do some extra tests for interfaces that don't have full type info. That might be pretty simple, but I haven't had a chance to try it out yet and I've only been using the types package for 5 hours now so I am winging all of this

Member

jimmyfrasche commented Sep 29, 2017

@mvdan I played with this a little tonight.

I couldn't figure out a way to get the source importer to interface with godoc's vfs. I rigged a fake importer that just always returned an incomplete and empty types.Package. That works surprisingly well, but it means it only works for interfaces that don't refer to types in other packages. So go/ast's out 👎

I looked at the source for the source importer. It's not that big, about 200 lines (and another 150 of tests), but it would still be a bit of a hassle to bolt it onto godoc and if looking at package A can load packages B through Z there might have to be caching and cache invalidation baked into it. No matter what the two copies would be somewhat divergent. That might be fine, but it has a lot of TODOs and bugs already.
. . .

golang.org/x/tools/go/gcexportdata looks like it could work but that requires things be built
Another option would be to rig an entirely source based mini-typechecker to figure it out. The big issues with that would be dealing with all the edge cases of different import names in different files.

At that point it would probably be easier to take the mostly filled in type info from the first approach and do some extra tests for interfaces that don't have full type info. That might be pretty simple, but I haven't had a chance to try it out yet and I've only been using the types package for 5 hours now so I am winging all of this

@mvdan

This comment has been minimized.

Show comment
Hide comment
@mvdan

mvdan Sep 29, 2017

Member

I am not familiar with godoc's vfs, nor with how it currently loads the packages from disk. I was not aware it had this layer of abstraction.

I do have some experience with go/types and package loading, but almost every single time I've ended up just using go/loader. I would imagine that's not an option here. It always loads from source too, working out the type info from scratch, so it would also make godoc noticeably slower, which I understand would be considered a regression.

I believe that the state of Go importers is a bit confusing at the moment. Some only load from cached build artifacts, and some only load from source. I do not know if a hybrid would be possible, and if so if one exists at the moment. But I think that is what we would want here.

Perhaps we should get some input from @rsc and @griesemer before we try to go down any of these routes.

Member

mvdan commented Sep 29, 2017

I am not familiar with godoc's vfs, nor with how it currently loads the packages from disk. I was not aware it had this layer of abstraction.

I do have some experience with go/types and package loading, but almost every single time I've ended up just using go/loader. I would imagine that's not an option here. It always loads from source too, working out the type info from scratch, so it would also make godoc noticeably slower, which I understand would be considered a regression.

I believe that the state of Go importers is a bit confusing at the moment. Some only load from cached build artifacts, and some only load from source. I do not know if a hybrid would be possible, and if so if one exists at the moment. But I think that is what we would want here.

Perhaps we should get some input from @rsc and @griesemer before we try to go down any of these routes.

@jimmyfrasche

This comment has been minimized.

Show comment
Hide comment
@jimmyfrasche

jimmyfrasche Sep 29, 2017

Member

yeah, go/loader looks like it's the only one with an option for a source importer that I could find but it always hits the real file system and there's not a mechanism to change that currently. But even if it did there'd be the issue with it having to recheck all deps all the time.

After sleeping on it though, I think the best way to do this would be to write a custom version of the code to see whether a concrete type satisfies an interface using the type info I've pulled out so far. It shouldn't be too complicated since it only needs to answer a very specific question in a very specific case.

There would be some false negatives in some edge cases (type aliases defined in external packages and interfaces that embed interfaces from other packages (unless the concrete type is a struct which also embeds that same interface)), but it looks like even with only type checking the package in isolation there's enough information to say "well I don't know what a token.Pos is but you both agree that you return whatever that is and the rest of it lines up . . .".

While that would have false negatives, like I said, I can't think of that having any false positives, which would be the unforgivable thing.

Worth a shot, at any rate.

Hopefully I'll have some time to play with that today, but will definitely have time to do so tomorrow. I'll try to throw what I have in a DO NOT REVIEW CL later, regardless of whether I make any progress, so you can take a look.

Member

jimmyfrasche commented Sep 29, 2017

yeah, go/loader looks like it's the only one with an option for a source importer that I could find but it always hits the real file system and there's not a mechanism to change that currently. But even if it did there'd be the issue with it having to recheck all deps all the time.

After sleeping on it though, I think the best way to do this would be to write a custom version of the code to see whether a concrete type satisfies an interface using the type info I've pulled out so far. It shouldn't be too complicated since it only needs to answer a very specific question in a very specific case.

There would be some false negatives in some edge cases (type aliases defined in external packages and interfaces that embed interfaces from other packages (unless the concrete type is a struct which also embeds that same interface)), but it looks like even with only type checking the package in isolation there's enough information to say "well I don't know what a token.Pos is but you both agree that you return whatever that is and the rest of it lines up . . .".

While that would have false negatives, like I said, I can't think of that having any false positives, which would be the unforgivable thing.

Worth a shot, at any rate.

Hopefully I'll have some time to play with that today, but will definitely have time to do so tomorrow. I'll try to throw what I have in a DO NOT REVIEW CL later, regardless of whether I make any progress, so you can take a look.

@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot Sep 30, 2017

Change https://golang.org/cl/67192 mentions this issue: x/tools/cmd/godoc: List types that satisfy an interface within package

gopherbot commented Sep 30, 2017

Change https://golang.org/cl/67192 mentions this issue: x/tools/cmd/godoc: List types that satisfy an interface within package

@jimmyfrasche

This comment has been minimized.

Show comment
Hide comment
@jimmyfrasche

jimmyfrasche Sep 30, 2017

Member

go/types sets all types that don't resolve to the same invalid type sentinel. We can get back to the original ast definition from the token.Pos and find the nth param/ret and see the type. As long as it's not from a dot import we can look up what package it came from in that ast.File and treat all (package, type) pairs as distinct and use that information to compute most of the interface satisfaction relation for the package, but at that point we might as well write our own purpose-built typechecker.

It seems like it would be less effort to have a custom source importer that works with godoc's vfs but it's not clear to me that that's desirable. That would also involve factoring out how godoc generates go/build contexts into its own package since that logic would be needed multiple places.

Member

jimmyfrasche commented Sep 30, 2017

go/types sets all types that don't resolve to the same invalid type sentinel. We can get back to the original ast definition from the token.Pos and find the nth param/ret and see the type. As long as it's not from a dot import we can look up what package it came from in that ast.File and treat all (package, type) pairs as distinct and use that information to compute most of the interface satisfaction relation for the package, but at that point we might as well write our own purpose-built typechecker.

It seems like it would be less effort to have a custom source importer that works with godoc's vfs but it's not clear to me that that's desirable. That would also involve factoring out how godoc generates go/build contexts into its own package since that logic would be needed multiple places.

@jimmyfrasche

This comment has been minimized.

Show comment
Hide comment
@jimmyfrasche

jimmyfrasche Oct 1, 2017

Member

@mvdan I've updated the CL to use go/loader. It probably does too much work but it has the benefit that it works, or at least appears to work so far.

In typecheck.go there's a commented out config param. That's needed to integrate into the godoc vfs. Not exactly sure what needs to go into it yet, though.

Member

jimmyfrasche commented Oct 1, 2017

@mvdan I've updated the CL to use go/loader. It probably does too much work but it has the benefit that it works, or at least appears to work so far.

In typecheck.go there's a commented out config param. That's needed to integrate into the godoc vfs. Not exactly sure what needs to go into it yet, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment