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/go/types/typeutil: wrong computed method set #37081

Open
dotaheor opened this issue Feb 6, 2020 · 8 comments
Open

x/tools/go/types/typeutil: wrong computed method set #37081

dotaheor opened this issue Feb 6, 2020 · 8 comments
Assignees
Labels
Milestone

Comments

@dotaheor
Copy link

@dotaheor dotaheor commented Feb 6, 2020

What version of Go are you using (go version)?

$ go version
go version go1.13.7 linux/amd64

What did you do?

package main

import (
	"fmt"
	"go/ast"
	"go/importer"
	"go/parser"
	"go/token"
	"go/types"
	"log"
	"reflect"
	
	"golang.org/x/tools/go/types/typeutil"
)

const input = `
package abc

type Conn struct {
	frameReader
	PayloadType int
}

type frameReader interface {
	PayloadType() byte
}
`

var fset = token.NewFileSet()

func main() {
	f, err := parser.ParseFile(fset, "hello.go", input, 0)
	if err != nil {
		log.Fatal(err)
	}

	conf := types.Config{Importer: importer.Default()}
	info := &types.Info{Types: make(map[ast.Expr]types.TypeAndValue)}
	pkg, err := conf.Check("cmd/hello", fset, []*ast.File{f}, info)
	if err != nil {
		log.Fatal(err)
	}
	
	ct := pkg.Scope().Lookup("Conn").Type()
	
	var cache typeutil.MethodSetCache
	var methetset = cache.MethodSet(ct)
	fmt.Println(methetset)
		// MethodSet {
		// 	method (cmd/hello.Conn) PayloadType() byte
		// }
	
	fmt.Println(reflect.TypeOf(Conn{}).NumMethod())
		// 0
}

type Conn struct {
	frameReader
	PayloadType int
}

type frameReader interface {
	PayloadType() byte
}

What did you expect to see?

Type Conn has no methods.

What did you see instead?

Type Conn has one method.

@gopherbot gopherbot added this to the Unreleased milestone Feb 6, 2020
@gopherbot gopherbot added the Tools label Feb 6, 2020
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Feb 6, 2020

But Conn does have a method, from the embedded frameReader type. Why should it have no methods?

@dotaheor

This comment has been minimized.

Copy link
Author

@dotaheor dotaheor commented Feb 7, 2020

But reflect says it hasn't.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Feb 7, 2020

Did you reverse "what did you expect to see" and "what did you see instead"?

Hmmm, looks like you did. When I run the program, it prints 0 for NumMethod.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Feb 7, 2020

Oh, sorry, I get it now. The method should be hidden by the field of the same name. The reflect package sees that, but golang.org/x/tools/go/types/typeutil does not.

We appreciate the bug reports, but it will save time if you can say more clearly what the issue is, rather than leaving us to work it out. Thanks.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Feb 7, 2020

@griesemer

This comment has been minimized.

Copy link
Contributor

@griesemer griesemer commented Feb 7, 2020

Smaller reproducer: https://play.golang.org/p/TRM-fN9nvVd

The primary function types.LookupFieldOrMethod correctly reports a field, but types.NewMethodSet returns the method. Looks like a bug in the method set computation.

(As an aside, types.NewMethodSet is not directly used by go/types, so there's no danger of go/typesdirectly being compromised by this - it was requested forx/tools/go/types/typeutil`. I should have said no at that time... :-)

@griesemer griesemer added NeedsFix and removed Tools labels Feb 7, 2020
@griesemer griesemer self-assigned this Feb 7, 2020
@gopherbot gopherbot added the Tools label Feb 7, 2020
@griesemer griesemer modified the milestones: Unreleased, Go1.15 Feb 7, 2020
@griesemer griesemer removed the Tools label Feb 7, 2020
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Feb 7, 2020

Change https://golang.org/cl/218617 mentions this issue: go/types: fix method set computation

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Feb 8, 2020

Change https://golang.org/cl/218618 mentions this issue: go/types: simplify method set computation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.