code review 131440043: go.tools/go/types: LookupFieldOrMethod should not retu...

go.tools/go/types:  LookupFieldOrMethod should not return methods with receiver *T when T is queried.

As a nice side-effect, some error strings were improved:  T.m now reports "not in method set", not "no field or method"; and T{}.m reports "no field or method" instead of "not in method set".

Fixes issue 8584

[gmk, 2014-08-27 16:26:31]

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

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

[gri, 2014-08-27 16:36:20]

Please abandon.

https://codereview.appspot.com/131440043/diff/40001/go/types/call.go
File go/types/call.go (right):

https://codereview.appspot.com/131440043/diff/40001/go/types/call.go#newcode308
go/types/call.go:308: obj, index, indirect =
LookupFieldOrMethod(NewPointer(x.typ), check.pkg, sel)
Now you require two lookup calls for this (common) case.

https://codereview.appspot.com/131440043/diff/40001/go/types/lookup.go
File go/types/lookup.go (right):

https://codereview.appspot.com/131440043/diff/40001/go/types/lookup.go#newcod...
go/types/lookup.go:127: if i, m := lookupMethod(e.typ.methods, pkg, name); m !=
nil && (e.indirect || !ptrRecv(m)) {
I don't believe this is correct. Now a method that exists at the right level
(with the right name) may not be considered because the receiver type is not
correct. This method may cause a shadow which is now missing, possibly exposing
lower-level methods or fields instead.

https://codereview.appspot.com/131440043/diff/40001/go/types/testdata/methods...
File go/types/testdata/methodsets.src (right):

https://codereview.appspot.com/131440043/diff/40001/go/types/testdata/methods...
go/types/testdata/methodsets.src:138: _ func() = T0 /* ERROR "no field or
method" */ {}.p0
This is less information than what we had before.

[gri, 2014-08-27 16:56:20]

PS: The correct fix is to make this check (of !indirect && ptrRecv(f)) upon
return from lookupFieldOrMethod. That way, method/field shadowing is not
affected, and MissingMethod can also benefit from the factoring. To avoid a
2nd lookup in case of addressable variables, one needs to pass in an extra
parameter ("addressable") or some such. Furthermore, we don't want to lose
information as to why lookup failed (because we want good error messages),
thus requiring some more information upon return. And finally, the
Info.Selections information should only have the indirect bit set if we
really needed to indirect.

I've been working on a CL, but it's not quite ready yet. Just doing the
factoring of the test alone is not worth the change.
Thanks.
- gri


On Wed, Aug 27, 2014 at 9:36 AM, <gri@golang.org> wrote:

> Please abandon.
>
>
> https://codereview.appspot.com/131440043/diff/40001/go/types/call.go
> File go/types/call.go (right):
>
> https://codereview.appspot.com/131440043/diff/40001/go/
> types/call.go#newcode308
> go/types/call.go:308: obj, index, indirect =
> LookupFieldOrMethod(NewPointer(x.typ), check.pkg, sel)
> Now you require two lookup calls for this (common) case.
>
> https://codereview.appspot.com/131440043/diff/40001/go/types/lookup.go
> File go/types/lookup.go (right):
>
> https://codereview.appspot.com/131440043/diff/40001/go/
> types/lookup.go#newcode127
> go/types/lookup.go:127: if i, m := lookupMethod(e.typ.methods, pkg,
>
> name); m != nil && (e.indirect || !ptrRecv(m)) {
> I don't believe this is correct. Now a method that exists at the right
> level (with the right name) may not be considered because the receiver
> type is not correct. This method may cause a shadow which is now
> missing, possibly exposing lower-level methods or fields instead.
>
> https://codereview.appspot.com/131440043/diff/40001/go/
> types/testdata/methodsets.src
> File go/types/testdata/methodsets.src (right):
>
> https://codereview.appspot.com/131440043/diff/40001/go/
> types/testdata/methodsets.src#newcode138
> go/types/testdata/methodsets.src:138: _ func() = T0 /* ERROR "no field
> or method" */ {}.p0
> This is less information than what we had before.
>
> https://codereview.appspot.com/131440043/
>

[gmk, 2014-08-27 17:18:49]

Will abandon.

https://codereview.appspot.com/131440043/diff/40001/go/types/call.go
File go/types/call.go (right):

https://codereview.appspot.com/131440043/diff/40001/go/types/call.go#newcode308
go/types/call.go:308: obj, index, indirect =
LookupFieldOrMethod(NewPointer(x.typ), check.pkg, sel)
On 2014/08/27 16:36:19, gri wrote:
> Now you require two lookup calls for this (common) case.

I ran the benchmark.  There was no performance difference.  If you want to do
only one lookup then you have to check x.mode == variable and whether x.typ is a
*Pointer or *Interface; the code is much less readable.

https://codereview.appspot.com/131440043/diff/40001/go/types/lookup.go
File go/types/lookup.go (right):

https://codereview.appspot.com/131440043/diff/40001/go/types/lookup.go#newcod...
go/types/lookup.go:127: if i, m := lookupMethod(e.typ.methods, pkg, name); m !=
nil && (e.indirect || !ptrRecv(m)) {
On 2014/08/27 16:36:19, gri wrote:
> I don't believe this is correct. Now a method that exists at the right level
> (with the right name) may not be considered because the receiver type is not
> correct. This method may cause a shadow which is now missing, possibly
exposing
> lower-level methods or fields instead.

Good point.  I think NewMethodSet has this same problem (I borrowed this
solution from it).

https://codereview.appspot.com/131440043/diff/40001/go/types/testdata/methods...
File go/types/testdata/methodsets.src (right):

https://codereview.appspot.com/131440043/diff/40001/go/types/testdata/methods...
go/types/testdata/methodsets.src:138: _ func() = T0 /* ERROR "no field or
method" */ {}.p0
On 2014/08/27 16:36:19, gri wrote:
> This is less information than what we had before.

Oops, you're right.

[gmk, 2014-08-27 17:20:26]

*** Abandoned ***

[gri, 2014-08-27 17:29:22]

Thanks. See also my pending CL below. Not quite ready yet, but passes all
tests.
- gri

https://codereview.appspot.com/132260043


On Wed, Aug 27, 2014 at 10:18 AM, <gordon.klaus@gmail.com> wrote:

> Will abandon.
>
>
>
> https://codereview.appspot.com/131440043/diff/40001/go/types/call.go
> File go/types/call.go (right):
>
> https://codereview.appspot.com/131440043/diff/40001/go/
> types/call.go#newcode308
> go/types/call.go:308: obj, index, indirect =
> LookupFieldOrMethod(NewPointer(x.typ), check.pkg, sel)
> On 2014/08/27 16:36:19, gri wrote:
>
>> Now you require two lookup calls for this (common) case.
>>
>
> I ran the benchmark.  There was no performance difference.  If you want
> to do only one lookup then you have to check x.mode == variable and
> whether x.typ is a *Pointer or *Interface; the code is much less
> readable.
>
>
> https://codereview.appspot.com/131440043/diff/40001/go/types/lookup.go
> File go/types/lookup.go (right):
>
> https://codereview.appspot.com/131440043/diff/40001/go/
> types/lookup.go#newcode127
> go/types/lookup.go:127: if i, m := lookupMethod(e.typ.methods, pkg,
> name); m != nil && (e.indirect || !ptrRecv(m)) {
> On 2014/08/27 16:36:19, gri wrote:
>
>> I don't believe this is correct. Now a method that exists at the right
>>
> level
>
>> (with the right name) may not be considered because the receiver type
>>
> is not
>
>> correct. This method may cause a shadow which is now missing, possibly
>>
> exposing
>
>> lower-level methods or fields instead.
>>
>
> Good point.  I think NewMethodSet has this same problem (I borrowed this
> solution from it).
>
>
> https://codereview.appspot.com/131440043/diff/40001/go/
> types/testdata/methodsets.src
> File go/types/testdata/methodsets.src (right):
>
> https://codereview.appspot.com/131440043/diff/40001/go/
> types/testdata/methodsets.src#newcode138
> go/types/testdata/methodsets.src:138: _ func() = T0 /* ERROR "no field
> or method" */ {}.p0
> On 2014/08/27 16:36:19, gri wrote:
>
>> This is less information than what we had before.
>>
>
> Oops, you're right.
>
> https://codereview.appspot.com/131440043/
>

[gmk, 2014-08-27 17:46:18]

On 2014/08/27 16:56:20, gri wrote:
> PS: The correct fix is to make this check (of !indirect && ptrRecv(f)) upon
> return from lookupFieldOrMethod. That way, method/field shadowing is not
> affected, and MissingMethod can also benefit from the factoring. To avoid a
> 2nd lookup in case of addressable variables, one needs to pass in an extra
> parameter ("addressable") or some such. Furthermore, we don't want to lose
> information as to why lookup failed (because we want good error messages),
> thus requiring some more information upon return. And finally, the
> Info.Selections information should only have the indirect bit set if we
> really needed to indirect.

An alternative is to do the "implicit &" of addressable values outside of
LookupFieldOrMethod.  As in:

typ := x.typ
if x.mode == variable {
    switch typ.(type) {
    case *Pointer, *Interface:
    default:
        typ = NewPointer(typ)
    }
}
obj, index, indirect = LookupFieldOrMethod(typ, check.pkg, sel)

I think you can still get good error messages, you just have to do another
lookup in the error handling code.

The benefit being avoiding adding another parameter to LookupFieldOrMethod.

[gri, 2014-08-27 22:29:51]

We may need to do this (implicit& outside the lookup). There are various
open questions:

- What actual receiver type should be reported via Info.Selections for x.f
where we used the fact that x is addressable? Should it be type(x) or
&type(x)?
- What should the value of indirect be?

After some back-and-forth, the current CL is only a step into that
direction. Will need more work. Waiting for feedback from adonovan.
- gri


On Wed, Aug 27, 2014 at 10:46 AM, <gordon.klaus@gmail.com> wrote:

> On 2014/08/27 16:56:20, gri wrote:
>
>> PS: The correct fix is to make this check (of !indirect && ptrRecv(f))
>>
> upon
>
>> return from lookupFieldOrMethod. That way, method/field shadowing is
>>
> not
>
>> affected, and MissingMethod can also benefit from the factoring. To
>>
> avoid a
>
>> 2nd lookup in case of addressable variables, one needs to pass in an
>>
> extra
>
>> parameter ("addressable") or some such. Furthermore, we don't want to
>>
> lose
>
>> information as to why lookup failed (because we want good error
>>
> messages),
>
>> thus requiring some more information upon return. And finally, the
>> Info.Selections information should only have the indirect bit set if
>>
> we
>
>> really needed to indirect.
>>
>
> An alternative is to do the "implicit &" of addressable values outside
> of LookupFieldOrMethod.  As in:
>
> typ := x.typ
> if x.mode == variable {
>     switch typ.(type) {
>     case *Pointer, *Interface:
>     default:
>         typ = NewPointer(typ)
>     }
> }
> obj, index, indirect = LookupFieldOrMethod(typ, check.pkg, sel)
>
> I think you can still get good error messages, you just have to do
> another lookup in the error handling code.
>
> The benefit being avoiding adding another parameter to
> LookupFieldOrMethod.
>
> https://codereview.appspot.com/131440043/
>

[gobot, 2014-12-19 05:12:09]

R=close

To the author of this CL:

The Go project has moved to Gerrit Code Review.

If this CL should be continued, please see the latest version of
https://golang.org/doc/contribute.html for instructions on
how to set up Git and the Go project's Gerrit codereview plugin,
and then create a new change with your current code.

If there has been discussion on this CL, please give a link to it
(golang.org/cl/131440043 is best) in the description in your
new CL.

Thanks very much.