go.tools/go/types: Enable enumeration of the fields of a type.

go.tools/go/types: Enable enumeration of the fields of a type - its field set, analogous to method set. SelectionSet replaces MethodSet and is used for both field sets and method sets. The field set and method set for a type are tightly bound -- computing one necessarily involves computing the other. MethodSetCache therefore becomes SelectionSetCache and stores both field and method sets. These changes were tested by plugging NewMethodSet and NewFieldSet into MissingMethod and LookupFieldOrMethod, but I don't submit those changes because they slowed things down; but that shouldn't be a problem once caching is figured out. All affected clients in go.tools are updated.

[gri, 2014-02-24 18:12:19]

Thanks. This will need to wait for a bit since we are about to freeze the main repo (not go.tools) for 1.3 by the end of this week and we have higher priority items. - Robert On Sun, Feb 23, 2014 at 1:30 PM, <gordon.klaus@gmail.com> wrote: > Reviewers: gri, adonovan, > > Description: > go.tools/go/types: Add FieldSet for enumerating the fields of a type. > > NewMethodSet was already collecting fields, but ultimately discarded > them. The non-exported types methodSet and fieldSet are now subsumed by > selectionSet; perhaps the same ought to happen to MethodSet and > FieldSet. > > I indirectly tested NewMethodSet and NewFieldSet by plugging them into > MissingMethod and LookupFieldOrMethod, but I don't submit those changes > because they slowed things down; but that shouldn't be a problem once > caching is figured out. > > Please review this at https://codereview.appspot.com/66730052/ > > Affected files (+143, -78 lines): > M go/types/methodset.go > > > 

[gmk, 2014-06-12 07:03:13]

Ping. On Mon, Feb 24, 2014 at 7:12 PM, Robert Griesemer <gri@golang.org> wrote: > Thanks. This will need to wait for a bit since we are about to freeze the > main repo (not go.tools) for 1.3 by the end of this week and we have > higher priority items. > > - Robert > > > On Sun, Feb 23, 2014 at 1:30 PM, <gordon.klaus@gmail.com> wrote: > >> Reviewers: gri, adonovan, >> >> Description: >> go.tools/go/types: Add FieldSet for enumerating the fields of a type. >> >> NewMethodSet was already collecting fields, but ultimately discarded >> them. The non-exported types methodSet and fieldSet are now subsumed by >> selectionSet; perhaps the same ought to happen to MethodSet and >> FieldSet. >> >> I indirectly tested NewMethodSet and NewFieldSet by plugging them into >> MissingMethod and LookupFieldOrMethod, but I don't submit those changes >> because they slowed things down; but that shouldn't be a problem once >> caching is figured out. >> >> Please review this at https://codereview.appspot.com/66730052/ >> >> Affected files (+143, -78 lines): >> M go/types/methodset.go >> >> >> > 

[gri, 2014-07-10 18:07:12]

Apologies for the delay. I'd be more open to this if it would simplify the code to a SelectionSet only. Also, perhaps some renaming would be in order (SelectionSet -> MemberSet). I may take a stab at this myself if I get around to it, or you can try another round if you like. https://codereview.appspot.com/66730052/diff/20001/go/types/methodset.go File go/types/methodset.go (right): https://codereview.appspot.com/66730052/diff/20001/go/types/methodset.go#newc... go/types/methodset.go:105: type FieldSet struct { As far as I can tell, the only difference between FieldSet and MethodSet is the name, and some minor differences in String printing, etc. This makes not much sense. If we really want to go this route, we should make this a SelectionSet, or at the very least factor out all the shared code in a selectionSet, which we may embed in a FieldSet and a MethodSet (though I'm not convinced yet that the latter is better than the former).

[gmk, 2014-07-11 07:04:54]

On 2014/07/10 18:07:12, gri wrote: > Apologies for the delay. No problem. > I'd be more open to this if it would simplify the code to a SelectionSet only. I wholeheartedly agree. > Also, perhaps some renaming would be in order (SelectionSet -> MemberSet). I also thought there might be a better name, but I couldn't come up with anything. MemberSet doesn't seem right: "member" doesn't appear in the spec; but "selection" certainly does. > I may take a stab at this myself if I get around to it, or you can try another > round if you like. I'll give it a go. > https://codereview.appspot.com/66730052/diff/20001/go/types/methodset.go > File go/types/methodset.go (right): > > https://codereview.appspot.com/66730052/diff/20001/go/types/methodset.go#newc... > go/types/methodset.go:105: type FieldSet struct { > As far as I can tell, the only difference between FieldSet and MethodSet is the > name, and some minor differences in String printing, etc. This makes not much > sense. > > If we really want to go this route, we should make this a SelectionSet, or at > the very least factor out all the shared code in a selectionSet, which we may > embed in a FieldSet and a MethodSet (though I'm not convinced yet that the > latter is better than the former).

[gmk, 2014-07-12 18:04:27]

I expanded MethodSet into SelectionSet, and updated the affected clients in go.tools. Upon reflection, I don't think this is quite the right solution. In particular, it's distracting to deal with a SelectionSet, filtering by Selection.Kind, when you're only interested in methods (or only fields). It's clearer to have separate types. The motivation for subsuming MethodSet and FieldSet into SelectionSet arose from the fact that the two are necessarily computed together. To avoid duplicate computation, they need to be cached together, but they can still be presented separately. My plan now is to reintroduce MethodSet, introduce FieldSet, and change SelectionSet to type SelectionSet struct { mset *MethodSet fset *FieldSet } with accessor methods for the separate subsets. SelectionSetCache will remain as the only caching mechanism. It may even be desirable to completely hide SelectionSet as an implementation detail of the cache. The casualties, SelectionSet.{Lookup, Len, At} are trivial to emulate when (rarely?) needed.

[gmk, 2014-07-12 20:25:12]

I went the route of caching field and method sets together in an unexported type "selectionSet": type selectionSet { fset, mset *SelectionSet } Maybe it should have a different name, to distinguish it from the exported type "SelectionSet"; how about selectionSetCacheEntry or fieldsAndMethods or just selSet? There was no need to introduce different types for field and method sets. Please take a look.

[gmk, 2014-07-13 12:58:33]

On 2014/07/12 20:25:12, gmk wrote: > I went the route of caching field and method sets together in an unexported type > "selectionSet": > > type selectionSet { > fset, mset *SelectionSet > } > > Maybe it should have a different name, to distinguish it from the exported type > "SelectionSet"; how about selectionSetCacheEntry or fieldsAndMethods or just > selSet? > > There was no need to introduce different types for field and method sets. > > Please take a look. I opted for the name "selectionSets" to distinguish the unexported type from the exported "SelectionSet".

[gri, 2014-07-14 18:40:10]

This looks pretty good to me so far. Handing over to adonovan for some more feedback. https://codereview.appspot.com/66730052/diff/100001/go/ssa/ssa.go File go/ssa/ssa.go (right): https://codereview.appspot.com/66730052/diff/100001/go/ssa/ssa.go#newcode29 go/ssa/ssa.go:29: Cache types.SelectionSetCache // cache of type-checker's selection sets Can we chose a better name here? Cache is pretty nondescript and generic. LookupCache? https://codereview.appspot.com/66730052/diff/100001/go/types/selectionset.go File go/types/selectionset.go (right): https://codereview.appspot.com/66730052/diff/100001/go/types/selectionset.go#... go/types/selectionset.go:63: // always returns a non-nil set, even if it is empty. We should change that 2nd sentence, it's always a bit confusing. Please make it: // It always returns a (possibly empty) non-nil set. https://codereview.appspot.com/66730052/diff/100001/go/types/selectionset.go#... go/types/selectionset.go:71: // always returns a non-nil set, even if it is empty. ditto https://codereview.appspot.com/66730052/diff/100001/go/types/selectionset.go#... go/types/selectionset.go:79: type selectionSets struct { s/selectionSets/selectionSetPair/ or perhaps fullSelectionSet (or even just leave it at: selectionSet) (e.g., it's still called emptySelectionSet, and not emptySelectionSets) https://codereview.appspot.com/66730052/diff/100001/go/types/selectionset.go#... go/types/selectionset.go:84: var emptySelectionSet = &selectionSets{&SelectionSet{}, &SelectionSet{}} I prefer using new(SelectionSet) in situations like this. https://codereview.appspot.com/66730052/diff/100001/go/types/selectionset.go#... go/types/selectionset.go:109: for name, s := range sel { add a comment: // remove methods from sel https://codereview.appspot.com/66730052/diff/100001/go/types/selectionset.go#... go/types/selectionset.go:117: // TODO: clear nil selections (collisions) from sel before checking for emptiness please do https://codereview.appspot.com/66730052/diff/100001/go/types/selectionset.go#... go/types/selectionset.go:127: if s.kind == FieldVal { switch s.kind { case FieldVal: case MethodVal: default: panic("unreachable") } is more symmetric

[gmk, 2014-07-14 20:38:36]

Thanks for the feedback. https://codereview.appspot.com/66730052/diff/100001/go/ssa/ssa.go File go/ssa/ssa.go (right): https://codereview.appspot.com/66730052/diff/100001/go/ssa/ssa.go#newcode29 go/ssa/ssa.go:29: Cache types.SelectionSetCache // cache of type-checker's selection sets On 2014/07/14 18:40:09, gri wrote: > Can we chose a better name here? Cache is pretty nondescript and generic. > > LookupCache? I think Cache is clear. There is no other cache to distinguish it from, and its usage - prog.Cache.MethodSet(t) - makes its functionality apparent. Let's see what Alan thinks. https://codereview.appspot.com/66730052/diff/100001/go/types/selectionset.go File go/types/selectionset.go (right): https://codereview.appspot.com/66730052/diff/100001/go/types/selectionset.go#... go/types/selectionset.go:63: // always returns a non-nil set, even if it is empty. On 2014/07/14 18:40:09, gri wrote: > We should change that 2nd sentence, it's always a bit confusing. Please make it: > > // It always returns a (possibly empty) non-nil set. Done. https://codereview.appspot.com/66730052/diff/100001/go/types/selectionset.go#... go/types/selectionset.go:71: // always returns a non-nil set, even if it is empty. On 2014/07/14 18:40:09, gri wrote: > ditto Done. https://codereview.appspot.com/66730052/diff/100001/go/types/selectionset.go#... go/types/selectionset.go:79: type selectionSets struct { On 2014/07/14 18:40:09, gri wrote: > s/selectionSets/selectionSetPair/ > > or perhaps fullSelectionSet > > (or even just leave it at: selectionSet) > > (e.g., it's still called emptySelectionSet, and not emptySelectionSets) Naming can be hard :) Maybe Alan has some inspiration, or can roll a die. Renaming emptySelectionSet to emptySelectionSets for now. https://codereview.appspot.com/66730052/diff/100001/go/types/selectionset.go#... go/types/selectionset.go:84: var emptySelectionSet = &selectionSets{&SelectionSet{}, &SelectionSet{}} On 2014/07/14 18:40:09, gri wrote: > I prefer using new(SelectionSet) in situations like this. But then you need a func init() to set the fields (they need to be non-nil). https://codereview.appspot.com/66730052/diff/100001/go/types/selectionset.go#... go/types/selectionset.go:109: for name, s := range sel { On 2014/07/14 18:40:10, gri wrote: > add a comment: > > // remove methods from sel Done. https://codereview.appspot.com/66730052/diff/100001/go/types/selectionset.go#... go/types/selectionset.go:117: // TODO: clear nil selections (collisions) from sel before checking for emptiness On 2014/07/14 18:40:09, gri wrote: > please do Done. https://codereview.appspot.com/66730052/diff/100001/go/types/selectionset.go#... go/types/selectionset.go:127: if s.kind == FieldVal { On 2014/07/14 18:40:09, gri wrote: > switch s.kind { > case FieldVal: > case MethodVal: > default: > panic("unreachable") > } > > is more symmetric Done.

[gri, 2014-07-14 20:56:43]

leaving for adonovan https://codereview.appspot.com/66730052/diff/100001/go/types/selectionset.go File go/types/selectionset.go (right): https://codereview.appspot.com/66730052/diff/100001/go/types/selectionset.go#... go/types/selectionset.go:79: type selectionSets struct { On 2014/07/14 20:38:36, gmk wrote: > On 2014/07/14 18:40:09, gri wrote: > > s/selectionSets/selectionSetPair/ > > > > or perhaps fullSelectionSet > > > > (or even just leave it at: selectionSet) > > > > (e.g., it's still called emptySelectionSet, and not emptySelectionSets) > > Naming can be hard :) Maybe Alan has some inspiration, or can roll a die. > > Renaming emptySelectionSet to emptySelectionSets for now. That's definitively worse. An -s ending is fine in other contexts, but it implies multiples, not just 2. -Pair implies 2. singular implies a set, it's just not specialized to Fields of values. https://codereview.appspot.com/66730052/diff/100001/go/types/selectionset.go#... go/types/selectionset.go:84: var emptySelectionSet = &selectionSets{&SelectionSet{}, &SelectionSet{}} On 2014/07/14 20:38:36, gmk wrote: > On 2014/07/14 18:40:09, gri wrote: > > I prefer using new(SelectionSet) in situations like this. > > But then you need a func init() to set the fields (they need to be non-nil). what I meant is: var emptySelectionSet = &selectionSet{new(SelectionSet), new(SelectionSet)} see capitalization! Another reason for calling it selectionSetPair, or fullSelectionSet.

[gmk, 2014-07-15 07:23:38]

https://codereview.appspot.com/66730052/diff/100001/go/types/selectionset.go File go/types/selectionset.go (right): https://codereview.appspot.com/66730052/diff/100001/go/types/selectionset.go#... go/types/selectionset.go:79: type selectionSets struct { On 2014/07/14 20:56:43, gri wrote: > On 2014/07/14 20:38:36, gmk wrote: > > On 2014/07/14 18:40:09, gri wrote: > > > s/selectionSets/selectionSetPair/ > > > > > > or perhaps fullSelectionSet > > > > > > (or even just leave it at: selectionSet) > > > > > > (e.g., it's still called emptySelectionSet, and not emptySelectionSets) > > > > Naming can be hard :) Maybe Alan has some inspiration, or can roll a die. > > > > Renaming emptySelectionSet to emptySelectionSets for now. > > That's definitively worse. An -s ending is fine in other contexts, but it > implies multiples, not just 2. -Pair implies 2. singular implies a set, it's > just not specialized to Fields of values. I wouldn't say selectionSets is worse than selectionSet; multiples doesn't necessarily imply "however many you want", and a little vagueness is fine if you only need to distinguish two types. That said, it doesn't hurt to be more specific, if it's not too wordy. fullSelectionSet would lead to the unfortunate emptyFullSelectionSet, so I'll go with selectionSetPair (which I foresee becoming selectionSetTrio when collisions are included). https://codereview.appspot.com/66730052/diff/100001/go/types/selectionset.go#... go/types/selectionset.go:84: var emptySelectionSet = &selectionSets{&SelectionSet{}, &SelectionSet{}} On 2014/07/14 20:56:43, gri wrote: > On 2014/07/14 20:38:36, gmk wrote: > > On 2014/07/14 18:40:09, gri wrote: > > > I prefer using new(SelectionSet) in situations like this. > > > > But then you need a func init() to set the fields (they need to be non-nil). > > what I meant is: > > var emptySelectionSet = &selectionSet{new(SelectionSet), new(SelectionSet)} > > see capitalization! Another reason for calling it selectionSetPair, or > fullSelectionSet. I realized after I went to bed that I must have misread. Yes, evidence for a better name. Why do you prefer new(T)? &T{} is shorter. I've rarely had a need for new(T). That said, I am always open to new things! Giving it a go...

[adonovan, 2014-07-15 10:00:17]

LGTM https://codereview.appspot.com/66730052/diff/180001/go/types/call.go File go/types/call.go (right): https://codereview.appspot.com/66730052/diff/180001/go/types/call.go#newcode395 go/types/call.go:395: // lookup. // ... (requires locking). https://codereview.appspot.com/66730052/diff/180001/go/types/selectionset.go File go/types/selectionset.go (right): https://codereview.appspot.com/66730052/diff/180001/go/types/selectionset.go#... go/types/selectionset.go:63: // It always returns a (possibly empty) non-nil set. NewFieldSet and NewMethodSet are misnomers. Firstly, the returned value isn't always new: it could be the shared empty one. Secondly, NewT is more often used to create an empty(ish) initialized T instance that the client is expected to mutate, but in this case, it's a pure accessor. FieldSet(T) and MethodSet(T) are better names, I think. They also match the methods of SelectionSetCache. https://codereview.appspot.com/66730052/diff/180001/go/types/selectionset.go#... go/types/selectionset.go:71: // It always returns a (possibly empty) non-nil set. "returns a new..." https://codereview.appspot.com/66730052/diff/180001/go/types/selectionset.go#... go/types/selectionset.go:79: type selectionSetPair struct { fieldsAndMethods ? https://codereview.appspot.com/66730052/diff/180001/go/types/selectionset.go#... go/types/selectionset.go:80: fset, mset *SelectionSet call them: fields, methods?

[gri, 2014-07-15 16:47:58]

https://codereview.appspot.com/66730052/diff/180001/go/types/selectionset.go File go/types/selectionset.go (right): https://codereview.appspot.com/66730052/diff/180001/go/types/selectionset.go#... go/types/selectionset.go:63: // It always returns a (possibly empty) non-nil set. On 2014/07/15 10:00:17, adonovan wrote: > NewFieldSet and NewMethodSet are misnomers. > > Firstly, the returned value isn't always new: it could be the shared empty one. > > Secondly, NewT is more often used to create an empty(ish) initialized T instance > that the client is expected to mutate, but in this case, it's a pure accessor. > > FieldSet(T) and MethodSet(T) are better names, I think. They also match the > methods of SelectionSetCache. Agreed. They are filter functions. https://codereview.appspot.com/66730052/diff/180001/go/types/selectionset.go#... go/types/selectionset.go:71: // It always returns a (possibly empty) non-nil set. On 2014/07/15 10:00:17, adonovan wrote: > "returns a new..." well, if it's the empty set it's not new, it's always the same - i'd leave it alone https://codereview.appspot.com/66730052/diff/180001/go/types/selectionset.go#... go/types/selectionset.go:79: type selectionSetPair struct { On 2014/07/15 10:00:17, adonovan wrote: > fieldsAndMethods ? fieldAndMethodSet and FieldSet, MethodSet would work https://codereview.appspot.com/66730052/diff/180001/go/types/selectionset.go#... go/types/selectionset.go:80: fset, mset *SelectionSet On 2014/07/15 10:00:17, adonovan wrote: > call them: fields, methods? fset, mset seems fine

[gri, 2014-07-15 16:50:18]

https://codereview.appspot.com/66730052/diff/100001/go/types/selectionset.go File go/types/selectionset.go (right): https://codereview.appspot.com/66730052/diff/100001/go/types/selectionset.go#... go/types/selectionset.go:84: var emptySelectionSet = &selectionSets{&SelectionSet{}, &SelectionSet{}} On 2014/07/15 07:23:37, gmk wrote: > On 2014/07/14 20:56:43, gri wrote: > > On 2014/07/14 20:38:36, gmk wrote: > > > On 2014/07/14 18:40:09, gri wrote: > > > > I prefer using new(SelectionSet) in situations like this. > > > > > > But then you need a func init() to set the fields (they need to be non-nil). > > > > what I meant is: > > > > var emptySelectionSet = &selectionSet{new(SelectionSet), new(SelectionSet)} > > > > see capitalization! Another reason for calling it selectionSetPair, or > > fullSelectionSet. > > I realized after I went to bed that I must have misread. Yes, evidence for a > better name. > > Why do you prefer new(T)? &T{} is shorter. I've rarely had a need for new(T). > That said, I am always open to new things! Giving it a go... I prefer to use T{} and &T{} when I create a non-empty composite literal. I like to use new(T) when I want to emphasize that I am just allocating a new empty T. I understand that different people feel quite differently about this.

[gmk, 2014-07-15 21:01:04]

https://codereview.appspot.com/66730052/diff/180001/go/types/call.go File go/types/call.go (right): https://codereview.appspot.com/66730052/diff/180001/go/types/call.go#newcode395 go/types/call.go:395: // lookup. On 2014/07/15 10:00:17, adonovan wrote: > // ... (requires locking). Done. https://codereview.appspot.com/66730052/diff/180001/go/types/selectionset.go File go/types/selectionset.go (right): https://codereview.appspot.com/66730052/diff/180001/go/types/selectionset.go#... go/types/selectionset.go:63: // It always returns a (possibly empty) non-nil set. On 2014/07/15 16:47:57, gri wrote: > On 2014/07/15 10:00:17, adonovan wrote: > > NewFieldSet and NewMethodSet are misnomers. > > > > Firstly, the returned value isn't always new: it could be the shared empty > one. > > > > Secondly, NewT is more often used to create an empty(ish) initialized T > instance > > that the client is expected to mutate, but in this case, it's a pure accessor. > > > > FieldSet(T) and MethodSet(T) are better names, I think. They also match the > > methods of SelectionSetCache. > > Agreed. They are filter functions. Done. https://codereview.appspot.com/66730052/diff/180001/go/types/selectionset.go#... go/types/selectionset.go:71: // It always returns a (possibly empty) non-nil set. On 2014/07/15 16:47:57, gri wrote: > On 2014/07/15 10:00:17, adonovan wrote: > > "returns a new..." > > well, if it's the empty set it's not new, it's always the same - i'd leave it > alone leaving it alone https://codereview.appspot.com/66730052/diff/180001/go/types/selectionset.go#... go/types/selectionset.go:79: type selectionSetPair struct { On 2014/07/15 16:47:58, gri wrote: > On 2014/07/15 10:00:17, adonovan wrote: > > fieldsAndMethods ? > > fieldAndMethodSet > > and > > FieldSet, MethodSet > > would work Done. https://codereview.appspot.com/66730052/diff/180001/go/types/selectionset.go#... go/types/selectionset.go:80: fset, mset *SelectionSet On 2014/07/15 16:47:57, gri wrote: > On 2014/07/15 10:00:17, adonovan wrote: > > call them: fields, methods? > > fset, mset seems fine Now that you mention it, I like fields, methods. In the same spirit I think we should rename FieldSet and MethodSet (both the funcs and the cache methods) to Fields and Methods. We could even rename SelectionSet to Selections. Yes, it is very close to the singular Selection, but how could that actually manifest itself as a problem? In the rare case that someone actually has to type one of these names and forgets/appends an "s", they get a compiler error. Also, SelectionSetCache -> SelectionCache. All of the "sets" are getting a bit stuttery. The only downside I see is a slight divergence from the spec term "method set" (which we can still use in the docs). Whaddya say?

[adonovan, 2014-07-15 22:52:27]

On 15 July 2014 22:01, <gordon.klaus@gmail.com> wrote: > Now that you mention it, I like fields, methods. In the same spirit I > think we should rename FieldSet and MethodSet (both the funcs and the > cache methods) to Fields and Methods. I think I prefer "MethodSet" for two reasons: (a) MethodSet is a spec term, and (b) Methods is a funny name for a singular data type. The other names should be consistent with whatever we do for Methods. 

[gmk, 2014-07-16 13:33:53]

On Wed, Jul 16, 2014 at 12:52 AM, Alan Donovan <adonovan@google.com> wrote: > On 15 July 2014 22:01, <gordon.klaus@gmail.com> wrote: > > Now that you mention it, I like fields, methods. In the same spirit I > > think we should rename FieldSet and MethodSet (both the funcs and the > > cache methods) to Fields and Methods. > > I think I prefer "MethodSet" for two reasons: > (a) MethodSet is a spec term, and > (b) Methods is a funny name for a singular data type. > It's not a data type. It used to be, but now it's func and a method. And the return type *Selections would be plural. > > The other names should be consistent with whatever we do for Methods. > Agreed. Another argument against this possible renaming: FieldSet and MethodSet might better distinguish themselves from Struct.Fields and Named.Methods (or the equivalent accessors) than Fields and Methods do. I guess we should stick with FieldSet and MethodSet. 

[gri, 2014-07-16 17:28:57]

This still doesn't quite feel right. The only two files in question are selectionset.go and selectionsetcache.go, of course. The FieldSet and Method set functions simply project a field. Perhaps these fields should just be exposed. The type is always a SelectionSet anyway. They are not really filter functions anymore also. And in fact they do compute a new selection set (but for the empty case). Let me mull this over for a couple of days.

[gmk, 2014-07-17 16:41:42]

Some spices for your mulling, Robert: The fact that the FieldSet and MethodSet functions project a field is only a result of the factoring of this implementation. See Patch Set 1 in this CL for another implementation that doesn't have this property. The fieldsAndMethods struct only exists so that it can be stored in SelectionSetCache -- without the cache there would be no need for it. That said, there still might be a reason to export fieldsAndMethods and its fields fset and mset, but I don't see it. As a container, this struct doesn't provide much value. And in terms of behavior I can only imagine it providing a Lookup method that checked both SelectionSets; but this is just the equivalent of what LookupFieldOrMethod does (and what I expect it to actually do, eventually). I think FieldSet and MethodSet might still be filter functions. Yes, they compute a new SelectionSet with new Selections each time, but those Selections are identical over subsequent calls. But I still wouldn't be averse to calling them NewFieldSet and NewMethodSet (the funcs, not the cache methods). After all, we use NewT for all the Type factories, but they also create identical immutable instances for some given parameters; or maybe you would like to rename them to NamedOf, StructOf, PointerTo, SliceOf, etc? On Wed, Jul 16, 2014 at 7:28 PM, <gri@golang.org> wrote: > This still doesn't quite feel right. The only two files in question are > selectionset.go and selectionsetcache.go, of course. > > The FieldSet and Method set functions simply project a field. Perhaps > these fields should just be exposed. The type is always a SelectionSet > anyway. They are not really filter functions anymore also. And in fact > they do compute a new selection set (but for the empty case). > > Let me mull this over for a couple of days. > > https://codereview.appspot.com/66730052/ >