code review 129260043: go.tools/refactor/eg: Support promoted fields and methods.

go.tools/refactor/eg: Support promoted fields and methods. As per http://golang.org/ref/spec#Struct_types enable eg tool to wildcard match against promoted fields and methods.

[pnasrat, 2014-08-18 13:57:12]

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-20 02:30:14]

Thanks, Paul. https://codereview.appspot.com/129260043/diff/40001/refactor/eg/match.go File refactor/eg/match.go (right): https://codereview.appspot.com/129260043/diff/40001/refactor/eg/match.go#newc... refactor/eg/match.go:167: field := x.Sel.Name sel := (it might be a method, not a field) https://codereview.appspot.com/129260043/diff/40001/refactor/eg/match.go#newc... refactor/eg/match.go:169: o, _, _ := types.LookupFieldOrMethod(yt, tr.currentPkg, field) if o ... := ...; o != nil { https://codereview.appspot.com/129260043/diff/40001/refactor/eg/match.go#newc... refactor/eg/match.go:170: if o != nil { If o==nil you should also check whether types.NewPointer(TypeOf(y.X)) has that field or method. (Strictly speaking you should only do this when y.X is addressable.) See my comments below. https://codereview.appspot.com/129260043/diff/40001/refactor/eg/testdata/F1.go File refactor/eg/testdata/F1.go (right): https://codereview.appspot.com/129260043/diff/40001/refactor/eg/testdata/F1.g... refactor/eg/testdata/F1.go:32: y.Lock() I'm baffled as to how this case works. I would expect it not to match: tr.wildcardObj(x.X) is true for the pattern here, so your new logic is used, but TypeOf(y.X) here refers to struct{RWMutex}, which does not have a Lock() method---only *struct{RWMutex} has the method, and the Go compiler treats this statement as equivalent to (&y).Lock(), but there is no similar logic in your new code.

[pnasrat, 2014-08-20 03:34:29]

On Tue, Aug 19, 2014 at 10:30 PM, <adonovan@google.com> wrote: > Thanks, Paul. > > > > https://codereview.appspot.com/129260043/diff/40001/refactor/eg/match.go > File refactor/eg/match.go (right): > > https://codereview.appspot.com/129260043/diff/40001/refactor/eg/match.go# > newcode167 > refactor/eg/match.go:167: field := x.Sel.Name > sel := > > (it might be a method, not a field) > > https://codereview.appspot.com/129260043/diff/40001/refactor/eg/match.go# > newcode169 > refactor/eg/match.go:169: o, _, _ := types.LookupFieldOrMethod(yt, > tr.currentPkg, field) > if o ... := ...; o != nil { > > https://codereview.appspot.com/129260043/diff/40001/refactor/eg/match.go# > newcode170 > refactor/eg/match.go:170: if o != nil { > If o==nil you should also check whether types.NewPointer(TypeOf(y.X)) > has that field or method. > (Strictly speaking you should only do this when y.X is addressable.) > See my comments below. > > https://codereview.appspot.com/129260043/diff/40001/ > refactor/eg/testdata/F1.go > File refactor/eg/testdata/F1.go (right): > > https://codereview.appspot.com/129260043/diff/40001/ > refactor/eg/testdata/F1.go#newcode32 > refactor/eg/testdata/F1.go:32: y.Lock() > I'm baffled as to how this case works. I would expect it not to match: > tr.wildcardObj(x.X) is true for the pattern here, so your new logic is > used, but TypeOf(y.X) here refers to struct{RWMutex}, which does not > have a Lock() method---only *struct{RWMutex} has the method, and the Go > compiler treats this statement as equivalent to (&y).Lock(), but there > is no similar logic in your new code. > > https://codereview.appspot.com/129260043/ Yeah I think there is some complexity here. I'd like to write some more tests which I'd hope which would break. I think I need to add another type that has a Lock() which isn't RWMutex and assert that does not match (thinking about it I think this case is broken currently). As I understand it the match is working like: Given a wildcard x.Lock() where s is a RWMutex When we find foo.Lock() in the AST First we find Lock() as a Call and compare the calls (and args), then we find the Selector and look into that. The new matchSelector is not comparing the caller of the selector which I think is a bug in my code. Previously we matched on the selector, then went to the wildcard match to compare the left hand side off the '.' which then checks if x and foo are assignable. I'm going to be tied up with some interrupt/oncall work this week but I'll try carve out some time to iterate on the tests and fix the issues above this week, else it may slip to next. Paul -- Paul Nasrat Colossus/D SRE New York 

[gmk, 2014-08-20 08:13:26]

https://codereview.appspot.com/129260043/diff/40001/refactor/eg/testdata/F1.go File refactor/eg/testdata/F1.go (right): https://codereview.appspot.com/129260043/diff/40001/refactor/eg/testdata/F1.g... refactor/eg/testdata/F1.go:32: y.Lock() On 2014/08/20 02:30:14, adonovan wrote: > I'm baffled as to how this case works. I would expect it not to match: > tr.wildcardObj(x.X) is true for the pattern here, so your new logic is used, but > TypeOf(y.X) here refers to struct{RWMutex}, which does not have a Lock() > method---only *struct{RWMutex} has the method, and the Go compiler treats this > statement as equivalent to (&y).Lock(), but there is no similar logic in your > new code. If I recall correctly, LookupFieldOrMethod returns a pointer-receiver method even when a non-pointer type is supplied (which would explain how your above case works when it shouldn't). I planned to fix this by reimplementing LookupFieldOrMethod to use SelectionSets (but the SelectionSet CL seems to be stalled).

[pnasrat, 2014-08-21 00:47:10]

On 2014/08/20 08:13:26, gmk wrote: > https://codereview.appspot.com/129260043/diff/40001/refactor/eg/testdata/F1.go > File refactor/eg/testdata/F1.go (right): > > https://codereview.appspot.com/129260043/diff/40001/refactor/eg/testdata/F1.g... > refactor/eg/testdata/F1.go:32: y.Lock() > On 2014/08/20 02:30:14, adonovan wrote: > > I'm baffled as to how this case works. I would expect it not to match: > > tr.wildcardObj(x.X) is true for the pattern here, so your new logic is used, > but > > TypeOf(y.X) here refers to struct{RWMutex}, which does not have a Lock() > > method---only *struct{RWMutex} has the method, and the Go compiler treats this > > statement as equivalent to (&y).Lock(), but there is no similar logic in your > > new code. > > If I recall correctly, LookupFieldOrMethod returns a pointer-receiver method > even when a non-pointer type is supplied (which would explain how your above > case works when it shouldn't). I planned to fix this by reimplementing > LookupFieldOrMethod to use SelectionSets (but the SelectionSet CL seems to be > stalled). So should I try and fix this here and add a comment/TODO around SelectionSets? I think so long as I fix the type comparison, then the test is stating user intent with wildcard matches and selectors with promoted fields/methods. If my assumption there is incorrect and you'd rather richer ways of expressing constraints for the refactor let me know and I'll try tackle that. Paul

[gmk, 2014-08-21 07:05:18]

On 2014/08/21 00:47:10, pnasrat wrote: > On 2014/08/20 08:13:26, gmk wrote: > > https://codereview.appspot.com/129260043/diff/40001/refactor/eg/testdata/F1.go > > File refactor/eg/testdata/F1.go (right): > > > > > https://codereview.appspot.com/129260043/diff/40001/refactor/eg/testdata/F1.g... > > refactor/eg/testdata/F1.go:32: y.Lock() > > On 2014/08/20 02:30:14, adonovan wrote: > > > I'm baffled as to how this case works. I would expect it not to match: > > > tr.wildcardObj(x.X) is true for the pattern here, so your new logic is used, > > but > > > TypeOf(y.X) here refers to struct{RWMutex}, which does not have a Lock() > > > method---only *struct{RWMutex} has the method, and the Go compiler treats > this > > > statement as equivalent to (&y).Lock(), but there is no similar logic in > your > > > new code. > > > > If I recall correctly, LookupFieldOrMethod returns a pointer-receiver method > > even when a non-pointer type is supplied (which would explain how your above > > case works when it shouldn't). I planned to fix this by reimplementing > > LookupFieldOrMethod to use SelectionSets (but the SelectionSet CL seems to be > > stalled). > > So should I try and fix this here and add a comment/TODO around SelectionSets? > > I think so long as I fix the type comparison, then the test is stating user > intent with wildcard matches and selectors with promoted fields/methods. > > If my assumption there is incorrect and you'd rather richer ways of expressing > constraints for the refactor let me know and I'll try tackle that. > > Paul Don't worry about SelectionSets -- that's a distraction; LookupFieldOrMethod should be fixed regardless. In the meantime, if you want your test to fail as Alan expects it to, then you have to use LookupFieldOrMethod the way it is used here: https://code.google.com/p/go/source/browse/go/types/call.go?repo=tools#305. That is, don't ignore the 'indirect' result; and reject the selection result if it is a method with a pointer receiver while indirect is false, as in: https://code.google.com/p/go/source/browse/go/types/call.go?repo=tools#325.

[pnasrat, 2014-08-25 17:51:38]

On 2014/08/21 07:05:18, gmk wrote: > On 2014/08/21 00:47:10, pnasrat wrote: > > On 2014/08/20 08:13:26, gmk wrote: > > > > https://codereview.appspot.com/129260043/diff/40001/refactor/eg/testdata/F1.go > > > File refactor/eg/testdata/F1.go (right): > > > > > > > > > https://codereview.appspot.com/129260043/diff/40001/refactor/eg/testdata/F1.g... > > > refactor/eg/testdata/F1.go:32: y.Lock() > > > On 2014/08/20 02:30:14, adonovan wrote: > > > > I'm baffled as to how this case works. I would expect it not to match: > > > > tr.wildcardObj(x.X) is true for the pattern here, so your new logic is > used, > > > but > > > > TypeOf(y.X) here refers to struct{RWMutex}, which does not have a Lock() > > > > method---only *struct{RWMutex} has the method, and the Go compiler treats > > this > > > > statement as equivalent to (&y).Lock(), but there is no similar logic in > > your > > > > new code. > > > > > > If I recall correctly, LookupFieldOrMethod returns a pointer-receiver method > > > even when a non-pointer type is supplied (which would explain how your above > > > case works when it shouldn't). I planned to fix this by reimplementing > > > LookupFieldOrMethod to use SelectionSets (but the SelectionSet CL seems to > be > > > stalled). > > > > So should I try and fix this here and add a comment/TODO around SelectionSets? > > > > I think so long as I fix the type comparison, then the test is stating user > > intent with wildcard matches and selectors with promoted fields/methods. > > > > If my assumption there is incorrect and you'd rather richer ways of expressing > > constraints for the refactor let me know and I'll try tackle that. > > > > Paul > > Don't worry about SelectionSets -- that's a distraction; LookupFieldOrMethod > should be fixed regardless. > > In the meantime, if you want your test to fail as Alan expects it to, then you > have to use LookupFieldOrMethod the way it is used here: > https://code.google.com/p/go/source/browse/go/types/call.go?repo=tools#305. > That is, don't ignore the 'indirect' result; and reject the selection result if > it is a method with a pointer receiver while indirect is false, as in: > https://code.google.com/p/go/source/browse/go/types/call.go?repo=tools#325. I've added one more test and have changed to check indirect. I'm not sure if this is sufficient as is or I need to do as you suggest and reject if it is a method with a pointer receiver while indirect is false. In any case I'd probably need a new test case for that also. Paul

[adonovan, 2014-08-25 19:14:57]

On 20 August 2014 04:13, <gordon.klaus@gmail.com> wrote: > If I recall correctly, LookupFieldOrMethod returns a pointer-receiver > method even when a non-pointer type is supplied (which would explain how > your above case works when it shouldn't). Indeed it does. That's a bug. Now it's bug https://code.google.com/p/go/issues/detail?id=8584. 

[adonovan, 2014-08-25 20:04:09]

On 2014/08/25 19:14:57, adonovan wrote: > On 20 August 2014 04:13, <mailto:gordon.klaus@gmail.com> wrote: > > > If I recall correctly, LookupFieldOrMethod returns a pointer-receiver > > method even when a non-pointer type is supplied (which would explain how > > your above case works when it shouldn't). > > > Indeed it does. That's a bug. Now it's bug > https://code.google.com/p/go/issues/detail?id=8584. Paul, I think it's fine to ignore the underlying bug in LookupFieldOrMethod, but document the status quo in a comment in the test so that it fails perspicuously when the underlying bug is fixed. Otherwise LGTM. cheers alan

[pnasrat, 2014-08-25 20:23:00]

On 2014/08/25 20:04:09, adonovan wrote: > On 2014/08/25 19:14:57, adonovan wrote: > > On 20 August 2014 04:13, <mailto:gordon.klaus@gmail.com> wrote: > > > > > If I recall correctly, LookupFieldOrMethod returns a pointer-receiver > > > method even when a non-pointer type is supplied (which would explain how > > > your above case works when it shouldn't). > > > > > > Indeed it does. That's a bug. Now it's bug > > https://code.google.com/p/go/issues/detail?id=8584. > > Paul, I think it's fine to ignore the underlying bug in LookupFieldOrMethod, but > document the status quo in a comment in the test so that it fails perspicuously > when the underlying bug is fixed. > > Otherwise LGTM. > > cheers > alan Done

[adonovan, 2014-08-25 20:25:03]

On 2014/08/25 20:23:00, pnasrat wrote: > On 2014/08/25 20:04:09, adonovan wrote: > > On 2014/08/25 19:14:57, adonovan wrote: > > > On 20 August 2014 04:13, <mailto:gordon.klaus@gmail.com> wrote: > > > > > > > If I recall correctly, LookupFieldOrMethod returns a pointer-receiver > > > > method even when a non-pointer type is supplied (which would explain how > > > > your above case works when it shouldn't). > > > > > > > > > Indeed it does. That's a bug. Now it's bug > > > https://code.google.com/p/go/issues/detail?id=8584. > > > > Paul, I think it's fine to ignore the underlying bug in LookupFieldOrMethod, > but > > document the status quo in a comment in the test so that it fails > perspicuously > > when the underlying bug is fixed. > > > > Otherwise LGTM. > > > > cheers > > alan > > Done LGTM

[adonovan, 2014-08-25 20:26:57]

*** Submitted as https://code.google.com/p/go/source/detail?r=0926067da7b4&repo=tools *** go.tools/refactor/eg: Support promoted fields and methods. As per http://golang.org/ref/spec#Struct_types enable eg tool to wildcard match against promoted fields and methods. LGTM=adonovan R=adonovan, gordon.klaus CC=golang-codereviews https://codereview.appspot.com/129260043 Committer: Alan Donovan <adonovan@google.com>