code review 132260043: go.tools/go/types: LookupFieldOrMethod checks method set

go.tools/go/types: LookupFieldOrMethod checks method set

LookupFieldOrMethod now also decides whether a found
method is actually in the method set. Simplifies call
sites. Added corresponding API tests.

TODO (separate CL): Decide what the correct value for
the indirect result should be (as required for code
generation). For now, the result value for indirect
is unchanged from before if a field/method is found.

Fixes issue 8584.

[gri, 2014-08-27 22:21:11]

Hello adonovan@google.com (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go.tools

[adonovan, 2014-08-28 19:22:47]

LGTM

w.r.t. the status quo.

https://codereview.appspot.com/132260043/diff/100001/go/types/api_test.go
File go/types/api_test.go (right):

https://codereview.appspot.com/132260043/diff/100001/go/types/api_test.go#new...
go/types/api_test.go:821: {"var x T; type T struct{}; func (*T) f() {}", false,
nil, true},
Add a comment describing the expected error message, e.g. 
// "(*T).f method exists, but value of type T is not addressable"

https://codereview.appspot.com/132260043/diff/100001/go/types/call.go
File go/types/call.go (right):

https://codereview.appspot.com/132260043/diff/100001/go/types/call.go#newcode308
go/types/call.go:308: case index != nil:
The benefit of switch with implicit tag 'true' is often questionable, I find.

In particular, it requires an extra line for the 'switch {', and you can't add
cases like this:
  case x, ok := x.(T); ok:

https://codereview.appspot.com/132260043/diff/100001/go/types/lookup.go
File go/types/lookup.go (right):

https://codereview.appspot.com/132260043/diff/100001/go/types/lookup.go#newco...
go/types/lookup.go:31: //	- If indirect is set, a method with a pointer receiver
type was found
I thought indirect recorded all pointer traversals, even if the found method was
attached to the value type?

e.g. 

type T int
func (T) f()

type S struct { *T }
S{}.f()  // indirect, because S.T is a pointer we had to load.

https://codereview.appspot.com/132260043/diff/100001/go/types/lookup.go#newco...
go/types/lookup.go:35: func LookupFieldOrMethod(T Type, addressable bool, pkg
*Package, name string) (obj Object, index []int, indirect bool) {
Don't forget to update cmd/vet/types.go and refactor/eg/match.go.

[gri, 2014-08-28 19:49:53]

https://codereview.appspot.com/132260043/diff/100001/go/types/api_test.go
File go/types/api_test.go (right):

https://codereview.appspot.com/132260043/diff/100001/go/types/api_test.go#new...
go/types/api_test.go:821: {"var x T; type T struct{}; func (*T) f() {}", false,
nil, true},
On 2014/08/28 19:22:47, adonovan wrote:
> Add a comment describing the expected error message, e.g. 
> // "(*T).f method exists, but value of type T is not addressable"

Done.

https://codereview.appspot.com/132260043/diff/100001/go/types/call.go
File go/types/call.go (right):

https://codereview.appspot.com/132260043/diff/100001/go/types/call.go#newcode308
go/types/call.go:308: case index != nil:
On 2014/08/28 19:22:47, adonovan wrote:
> The benefit of switch with implicit tag 'true' is often questionable, I find.
> 
> In particular, it requires an extra line for the 'switch {', and you can't add
> cases like this:
>   case x, ok := x.(T); ok:

Acknowledged.

https://codereview.appspot.com/132260043/diff/100001/go/types/lookup.go
File go/types/lookup.go (right):

https://codereview.appspot.com/132260043/diff/100001/go/types/lookup.go#newco...
go/types/lookup.go:31: //	- If indirect is set, a method with a pointer receiver
type was found
On 2014/08/28 19:22:47, adonovan wrote:
> I thought indirect recorded all pointer traversals, even if the found method
was
> attached to the value type?
> 
> e.g. 
> 
> type T int
> func (T) f()
> 
> type S struct { *T }
> S{}.f()  // indirect, because S.T is a pointer we had to load.

yes, but this is the value of indirect in the failure case, not in the success
case. The value of indirect in the failure case says that we _should_ have had a
pointer.

https://codereview.appspot.com/132260043/diff/100001/go/types/lookup.go#newco...
go/types/lookup.go:35: func LookupFieldOrMethod(T Type, addressable bool, pkg
*Package, name string) (obj Object, index []int, indirect bool) {
On 2014/08/28 19:22:47, adonovan wrote:
> Don't forget to update cmd/vet/types.go and refactor/eg/match.go.

Done.

[gri, 2014-08-28 19:50:06]

Hello adonovan@google.com (cc: golang-codereviews@googlegroups.com),

Please take another look.

[gri, 2014-08-28 20:03:16]

*** Submitted as
https://code.google.com/p/go/source/detail?r=e964465ea698&repo=tools ***

go.tools/go/types: LookupFieldOrMethod checks method set

LookupFieldOrMethod now also decides whether a found
method is actually in the method set. Simplifies call
sites. Added corresponding API tests.

TODO (separate CL): Decide what the correct value for
the indirect result should be (as required for code
generation). For now, the result value for indirect
is unchanged from before if a field/method is found.

Fixes issue 8584.

LGTM=adonovan
R=adonovan
CC=golang-codereviews
https://codereview.appspot.com/132260043