go/types: LookupFieldOrMethod(T, "f") may return (*T).f #8584
Labels
Milestone
Comments
I took a quick crack at fixing this a couple days ago but didn't have much luck. I guess you (Alan) or Robert will have an easier time. An alternative implementation that I had in mind (which would also fix this bug) is simply to construct a selectionSet and look up the field or method therein. It's slower, but I don't think that's important: Clients that want speed should use a SelectionSetCache, which I expect gives better performance when there are multiple queries on the same type (which I think is the case for performance-critical applications). The main benefit is removing lookupFieldOrMethod, which is a near duplicate of newSelectionSet/NewMethodSet. Of course, this fix depends on https://golang.org/cl/66730052/ |
The long-term fix may well be the removal of LookupFieldOrMethod in favor of selection sets (decision pending, but there are higher-priority items to be dealt with at the moment). That said, I looked into changing the semantics of this function to not automatically indirect an incoming pointer type *T when looking for methods. There are multiple issues with such a change: 1) Per the spec, methods are attached to the base type of a receiver type. In case of a pointer receiver, the base type is the pointer's base (or element) type. That is, to find methods on (unnamed) pointer types *T, one has to indirect to T. That indirect could me hidden, but it makes using the function more complicated (see below). 2) LookupFieldOrMethod actually doesn't know what it is going to find (and the callers of this function may not know what they are looking for, either). In other words, the selector x.f may denote a field or a method. Only _after_ we have found a field or method do we know whether we should or should not have indirected in the first place. (As a related matter, if we don't find anything, and we have a named pointer, we will need to indirect that named pointer and try again, but throw away the result if it's a method, because Go automatically indirects pointers to fields, but _not_ pointers to methods). 3) Let's assume we are looking up a method x.m, where x is a non-pointer type T, and m is a method with receiver *T. If LookupField were to lookup m based on T, if would have to return nil as a result, because m is not in the method set of T. However, per the spec: "If x is addressable and &x's method set contains m, x.m() is shorthand for (&x).m()"; that is, in this case we would have to look up m again, this time with *T as the incoming type. We could avoid that by always starting with *T if x is a addressable, and it would work even if m is a field (because of the automatic indirection for fields). But such an approach would require different calls to LookupFieldOrMethod depending on whether we have an addressable or non-addressable x. That is fine, but it is more complicated than the current implementation where LookupFieldOrMethod is factored out (see calls.go:305) and the result values are sufficient to make the various case distinctions. The best fix might be to document this function better. |
Actually, there's some better factoring that could be done, and which might go into the
right direction: Right now all callers of LookupFieldOrMethod need to check if a method
is actually in the type's method set. That is, they all do something along the lines of:
obj, index, indirect = LookupFieldOrMethod(T, ...)
...
if obj != nil {
if f, _ := obj.(*Func); f != nil && !indirect && ptrRecv(f) {
// error: not f in method set of T
}
}
This functionality could be moved into LookupFieldOrMethod (actually the internal
version lookupFieldOrMethod). The main drawback of factoring this out is that right now
a caller can give a more specific error message if a method is found (but is not in the
methodset) then if the result of the lookup is just a nil object. The solution might be
a more information returned by LookupFieldOrMethod regarding the actual failure.
To be considered.
Finally, fixing this issue should also address issue #8353 which is closely related.
|
> I looked into changing the semantics of this function to not automatically indirect an incoming pointer type *T when looking for methods. You have it backwards here. Automatic indirection of *T is fine and necessary. The problem is that this function automatically _addresses_ the incoming type when looking for methods. (Actually, technically, it doesn't do any "addressing"; it just fails to discard methods with *T receivers when T was queried.) |
CL https://golang.org/cl/131440043 mentions this issue. |
Pending CL (in progress): https://golang.org/cl/132260043 Status changed to Started. |
CL https://golang.org/cl/132260043 mentions this issue. |
|
This issue was closed by revision golang/tools@5dca7d8. Status changed to Fixed. |
This issue was closed.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Append this code to go/types/api_test.go and run it. LookupFieldOrMethod, contra its spec, returns pointer-receiver methods even when invoked with a non-pointer named type. (This is something like the "implicit &" rule, but this is not the right place for that logic since that rule only applies to addressable values of type T.) func TestLookupFieldOrMethod(t *testing.T) { const src = `package P; type T int; func (*T) f() {}` pkg, err := pkgFor("test", src, nil) if err != nil { t.Fatal(err) } T := pkg.Scope().Lookup("T").Type() obj, index, indirect := LookupFieldOrMethod(T, pkg, "f") fmt.Println(T, obj, index, indirect) // P.T func (*P.T).f() [0] false t.Fail() }The text was updated successfully, but these errors were encountered: