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

go/types: LookupFieldOrMethod(T, "f") may return (*T).f #8584

Closed
adonovan opened this issue Aug 25, 2014 · 10 comments
Closed

go/types: LookupFieldOrMethod(T, "f") may return (*T).f #8584

adonovan opened this issue Aug 25, 2014 · 10 comments
Assignees
Milestone

Comments

@adonovan
Copy link
Contributor

@adonovan adonovan commented Aug 25, 2014

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()
}
@griesemer
Copy link
Contributor

@griesemer griesemer commented Aug 25, 2014

Comment 1:

Labels changed: added release-go1.4, repo-tools.

Status changed to Accepted.

Loading

@gordonklaus
Copy link
Contributor

@gordonklaus gordonklaus commented Aug 26, 2014

Comment 2:

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/

Loading

@griesemer
Copy link
Contributor

@griesemer griesemer commented Aug 26, 2014

Comment 3:

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.

Loading

@griesemer
Copy link
Contributor

@griesemer griesemer commented Aug 26, 2014

Comment 4:

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.

Loading

@gordonklaus
Copy link
Contributor

@gordonklaus gordonklaus commented Aug 26, 2014

Comment 5:

> 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.)

Loading

@gordonklaus
Copy link
Contributor

@gordonklaus gordonklaus commented Aug 26, 2014

Comment 6:

> This functionality could be moved into LookupFieldOrMethod (actually the internal
version lookupFieldOrMethod).
Yes, this is exactly what the issue is about.  I tried to do this refactoring but
obviously missed some important detail because plenty of tests were failing.

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 27, 2014

Comment 7:

CL https://golang.org/cl/131440043 mentions this issue.

Loading

@griesemer
Copy link
Contributor

@griesemer griesemer commented Aug 27, 2014

Comment 8:

Pending CL (in progress): https://golang.org/cl/132260043

Status changed to Started.

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 28, 2014

Comment 9:

CL https://golang.org/cl/132260043 mentions this issue.

Loading

@griesemer
Copy link
Contributor

@griesemer griesemer commented Aug 28, 2014

Comment 10:

This issue was closed by revision golang/tools@5dca7d8.

Status changed to Fixed.

Loading

@rsc rsc added this to the Go1.4 milestone Apr 14, 2015
@rsc rsc removed the release-go1.4 label Apr 14, 2015
@golang golang locked and limited conversation to collaborators Jun 25, 2016
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants