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/
>