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

Jump to bottom

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

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

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

dotaheor opened this issue Feb 6, 2020 · 8 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.15

Comments

@dotaheor
Copy link

dotaheor commented Feb 6, 2020 •
edited

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 This label describes issues relating to any tools in the x/tools repository. label Feb 6, 2020
@ianlancetaylor
Copy link
Contributor

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

@dotaheor
Copy link
Author

dotaheor commented Feb 7, 2020

But reflect says it hasn't.

@ianlancetaylor
Copy link
Contributor

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
Copy link
Contributor

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 ianlancetaylor added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 7, 2020
@ianlancetaylor
Copy link
Contributor

CC @griesemer

@griesemer
Copy link
Contributor

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 The path to resolution is known, but the work has not been done. and removed Tools This label describes issues relating to any tools in the x/tools repository. labels Feb 7, 2020
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 7, 2020
@griesemer griesemer self-assigned this Feb 7, 2020
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Feb 7, 2020
@griesemer griesemer modified the milestones: Unreleased, Go1.15 Feb 7, 2020
@griesemer griesemer removed the Tools This label describes issues relating to any tools in the x/tools repository. label Feb 7, 2020
@gopherbot
Copy link

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

@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue Mar 2, 2020
@griesemer
go/types: simplify method set computation
117297c 
After fixing #37081 we don't need to explicitly keep track of
field collisions in the method set computation anymore; we only
need to know which field (names) exists at each embedding level.
Simplify the code by removing the dedicated fieldSet data type
in favor of a simple string set.

Follow-up on https://golang.org/cl/218617; separate CL to make it
easier to identify a problem with these two changes, should there
be one.

Updates #37081.

Change-Id: I5c259c63c75a148a42d5c3e1e4860e1ffe5631bd
Reviewed-on: https://go-review.googlesource.com/c/go/+/218618
Reviewed-by: Alan Donovan <adonovan@google.com>
@golang golang locked and limited conversation to collaborators Mar 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.15
Development

No branches or pull requests
4 participants
@ianlancetaylor @griesemer @gopherbot @dotaheor