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

x/tools/cmd/godoc: use go/types, show embedded methods #6127

Open
bradfitz opened this issue Aug 13, 2013 · 20 comments
Open

x/tools/cmd/godoc: use go/types, show embedded methods #6127

bradfitz opened this issue Aug 13, 2013 · 20 comments
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@bradfitz
Copy link
Contributor

Fix the cmd/godoc side of issue 

https://code.google.com/p/go/source/detail?r=f5b37c93e4c5bb2962c added some methods to
go1.txt that were always there, but neither cmd/godoc nor cmd/api (both with their own
bad embedding rules) detected.

Robert's rewrite of cmd/api to use go/types fixed cmd/api, but cmd/godoc is still broken.

This bug is about making those methods (like issue #6125 -- xml.Encoder.Reset / Available
/ etc) show up, if they're actually there.  (But issue #6125 should remove them)
@rsc
Copy link
Contributor

rsc commented Sep 9, 2013

Comment 1:

Labels changed: added go1.3maybe, removed go1.2maybe.

@rsc
Copy link
Contributor

rsc commented Dec 4, 2013

Comment 2:

Labels changed: added release-none, removed go1.3maybe.

@rsc
Copy link
Contributor

rsc commented Dec 4, 2013

Comment 3:

Labels changed: added repo-tools.

@rsc rsc added this to the Unplanned milestone Apr 10, 2015
@rsc rsc changed the title cmd/godoc: use go/types, show embedded methods x/tools/cmd/godoc: use go/types, show embedded methods Apr 14, 2015
@rsc rsc removed the repo-tools label Apr 14, 2015
@agnivade
Copy link
Contributor

Took a look at this. There are various scenarios to be considered.

  • If the embedded type is not exported, the methods are shown.
  • If the embedded type is exported and in same package, the methods are shown by the "Mode.AllMethods" flag.
  • Only in the case where the embedded type is coming from another package, then the methods for that type is not extracted.

So in a case like this -

import "sync"

type T4 struct{}

// T4.M should appear as method of T5 only if AllMethods is set.
func (*T4) M() {}

type T5 struct {
	T4
	sync.Mutex
}

when AllMethods is set, T5 will show M(), but not Lock() and Unlock()

The relevant code is in (r *reader) recordAnonymousField() function in go/doc/reader.go. If the fieldType is imported, then it is ignored.

@griesemer - I am not very well-versed with the go/types package. Can you tell me how to get the methods for a type from another package ? Or is this a non-trivial task ?

@griesemer
Copy link
Contributor

@agnivade See https://github.com/golang/example/tree/master/gotypes . It's not hard to get to the methods, but the question is whether we want to run go/types every time somebody looks up some documentation. Or when we start the godoc server. Also, what happens when not all source code is available, etc.

Running go/types all the time may work just fine for small repos. But for large repos, there will be issues of memory consumption (if we keep it all in memory), runtime (if we always type-check everything), caching (if we want to keep things running fast), consistency and correctness (if we have caches), etc., to name a few things that come to mind.

I think making this work well requires a concentrated effort that goes beyond a "bug fix". This requires some design.

@agnivade
Copy link
Contributor

Thanks! I will spend some time on it and update this thread.

@agnivade
Copy link
Contributor

@griesemer - Sorry to bother you, but kinda got stuck here. I am trying to get the methods using NewMethodSet but it does not seem to give the embedded methods. Whereas, if I explicitly lookup the method by passing the name using LookupFieldOrMethod, it does give me the result.

The comment for NewMethodSet mentions

// WARNING: The code in this function is extremely subtle - do not modify casually!
// This function and lookupFieldOrMethod should be kept in sync.

But I don't see the behavior to be consistent. Most possibly, I am doing something wrong. Would appreciate if you can take a look -

// files is map[string]*ast.File from corpus.parseFiles
conf := types.Config{Importer: importer.Default()}
var ff []*ast.File
for _, de := range files {
	ff = append(ff, de)
}
tpkg, err := conf.Check("something", fset, ff, nil)
if err != nil {
	log.Fatal(err)
}

// Print the method sets of D
dType := tpkg.Scope().Lookup("D").Type()
mset := types.NewMethodSet(dType)
for i := 0; i < mset.Len(); i++ {
	sel := mset.At(i)
	log.Printf("%#v\n", sel.Obj())  // -- just shows C
}

obj, _, _ := types.LookupFieldOrMethod(dType, true, tpkg, "Lock") // -- gives proper info
log.Printf("%#v\n", obj)

source -

type a struct {
	B string
}

func (a) C() {}

type D struct {
	a
	sync.Mutex
}

Thank you for your time.

@griesemer
Copy link
Contributor

@agnivade The method set of D doesn't contain the Lock method (which requires a pointer receiver); only the method set of *D does. Which is why you don't get it reported. The compiler will complain, too, as you can see here: https://play.golang.org/p/5I7Evfl8WDY .

In your code, you need to provide the type *D to NewMethodSet; i.e., you need to change your dType computation to:

dType := types.NewPointer(tpkg.Scope().Lookup("D").Type())

Here's a complete stand-alone program that prints all the desired methods:

package main

import (
	"go/ast"
	"go/importer"
	"go/parser"
	"go/token"
	"go/types"
	"log"
)

const src = `
package p

import "sync"

type a struct {
	B string
}

func (a) C() {}

type D struct {
	a
	sync.Mutex
}
`

func main() {
	fset := token.NewFileSet()
	f, err := parser.ParseFile(fset, "p.go", src, 0)
	if err != nil {
		log.Fatal(err)
	}
	files := []*ast.File{f}

	conf := types.Config{Importer: importer.Default()}
	pkg, err := conf.Check("p", fset, files, nil)
	if err != nil {
		log.Fatal(err)
	}

	typ := types.NewPointer(pkg.Scope().Lookup("D").Type())
	mset := types.NewMethodSet(typ)
	for i := 0; i < mset.Len(); i++ {
		log.Printf("%d: %v\n", i, mset.At(i))
	}

}

Output:

2018/06/04 11:27:17 0: method (*p.D) C()
2018/06/04 11:27:17 1: method (*p.D) Lock()
2018/06/04 11:27:17 2: method (*p.D) Unlock()

@griesemer
Copy link
Contributor

PS: You may also want to look at golang.org/x/tools/go/types/typeutil/ui.go which contains a function to compute the "intuitive method set" which is probably what we would want to show in a UI.

@agnivade
Copy link
Contributor

agnivade commented Jun 4, 2018

Awesome, thank you ! I will look into it and prepare a prototype for 1.12.

@agnivade
Copy link
Contributor

@griesemer - It looks like getting the documentation for the method is slightly difficult. To maintain consistency with the other methods, we need to show the doc for embedded methods too.

The Obj() from the *Selection contains nearly everything except the documentation for the method. I tried passing types.Info to conf.Check too with the Types, Defs and Uses, but that didnt give anything useful that could lead me to the documentation of that embedded method.

Is there anything simple that can be done here ?

@agnivade
Copy link
Contributor

ping @griesemer

@griesemer
Copy link
Contributor

@agnivade go/types doesn't know anything about comments - there's no way to get the documentation for a method from go/types alone. You'll need to cross-reference the go/ast file with go/types (basically, find the objects for each method and then use the method matching the desired object). go/types doesn't point back to the AST so as to not hold on to a large data structure inadvertently.

@agnivade
Copy link
Contributor

But the go/ast file won't contain the methods of the required import package right ? I mean if my file is something like this -

package p

import "sync"

type a struct {
	B string
}

func (a) C() {}

type D struct {
	a
	sync.Mutex
}

Then the ast will have access to methods of a and D, not of sync.Mutex, right ? Or did you mean that I need to load the ast file given a package name ?

@griesemer
Copy link
Contributor

@agnivade Correct. An ast.File contains literally what's in the source - no more and no less. You'd have to actually get the ast.File(s) for the imported package. It's expensive.

@ghost
Copy link

ghost commented Jun 16, 2023

for anyone confused by this page, the above discussion is NOT what happens currently. For example this type:

https://godocs.io/bufio#ReadWriter

embeds this type:

https://godocs.io/bufio#Reader

which includes this method:

https://godocs.io/bufio#Reader.Buffered

but the wrapper type documents NO methods.

@go101
Copy link

go101 commented Jun 16, 2023

@4cq2 you can try Golds instead: https://docs.go101.org/std/pkg/bufio.html#name-ReadWriter

@go101
Copy link

go101 commented Jun 16, 2023

btw. although *bufio.Reader has a Buffered method, bufio.ReadWriter has not, because *bufio.Writer also has a Buffered method.

@ghost
Copy link

ghost commented Jun 16, 2023

@go101 no thank you. I actually prefer the embedded methods NOT be documented. as is done with https://godocs.io and https://pkg.go.dev. if I want to browse the embedded methods, I will visit the embedded types.

also your linked documentation is quite confusing, as it makes no distinction between an embedded method and a normal method, when those two items are quite different.

@dmitshur dmitshur added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Tools This label describes issues relating to any tools in the x/tools repository. labels Jun 16, 2023
@go101
Copy link

go101 commented Jun 16, 2023

also your linked documentation is quite confusing, as it makes no distinction between an embedded method and a normal method, when those two items are quite different.

For general using, there is almost no distinction. Now you can click a listed method to see which type this method is declared on.

Thanks for your suggestions, I will add a todo item to support an option to show full paths of methods (just like the current implementation for showing the paths of embedding fields).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

6 participants