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 · 15 comments

Comments

Projects
None yet
4 participants
@bradfitz
Member

bradfitz commented Aug 13, 2013

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

This comment has been minimized.

Contributor

rsc commented Sep 9, 2013

Comment 1:

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

@rsc

This comment has been minimized.

Contributor

rsc commented Dec 4, 2013

Comment 2:

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

@rsc

This comment has been minimized.

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 removed release-none labels Apr 10, 2015

@rsc rsc changed the title from cmd/godoc: use go/types, show embedded methods to x/tools/cmd/godoc: use go/types, show embedded methods Apr 14, 2015

@rsc rsc removed the repo-tools label Apr 14, 2015

@agnivade

This comment has been minimized.

Member

agnivade commented May 19, 2018

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

This comment has been minimized.

Contributor

griesemer commented May 21, 2018

@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

This comment has been minimized.

Member

agnivade commented May 21, 2018

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

@agnivade

This comment has been minimized.

Member

agnivade commented May 31, 2018

@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

This comment has been minimized.

Contributor

griesemer commented Jun 4, 2018

@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

This comment has been minimized.

Contributor

griesemer commented Jun 4, 2018

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

This comment has been minimized.

Member

agnivade commented Jun 4, 2018

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

@agnivade

This comment has been minimized.

Member

agnivade commented Jun 29, 2018

@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

This comment has been minimized.

Member

agnivade commented Sep 17, 2018

ping @griesemer

@griesemer

This comment has been minimized.

Contributor

griesemer commented Sep 24, 2018

@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

This comment has been minimized.

Member

agnivade commented Sep 24, 2018

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

This comment has been minimized.

Contributor

griesemer commented Sep 24, 2018

@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.

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