go.tools/go/types: export Comparable and Assertable, si...

go.tools/go/types: export Comparable and Assertable, simplify Implements

The "static bool" parameter to Implements was confusing; typically we think about interface implementation and type assertion as separate but related concepts, but here they were merged.

[gri, 2014-01-31 23:06:01]

LGTM

[gri, 2014-01-31 23:17:46]

FYI.

https://codereview.appspot.com/58540044/diff/20001/go/types/api.go
File go/types/api.go (right):

https://codereview.appspot.com/58540044/diff/20001/go/types/api.go#newcode238
go/types/api.go:238: // Comparable reports whether values of type T are
comparable.
I'm fine with exporting this, but let's just leave in where it is instead of
moving it for now.

https://codereview.appspot.com/58540044/diff/20001/go/types/predicates.go
File go/types/predicates.go (left):

https://codereview.appspot.com/58540044/diff/20001/go/types/predicates.go#old...
go/types/predicates.go:79: func isComparable(typ Type) bool {
Leave this function here. Just rename it.

[gmk, 2014-02-01 21:09:33]

I've signed the CLA, now.

MissingMethod is still a little confusing.  Some options:
1. At the very least, clean up the documentation.  Maybe redefine and invert
"static" into "typeAssertion".
2. Make Implements and Assertable return method and wrongType; delete
MissingMethod.
Let me know if you like either of these.

https://codereview.appspot.com/58540044/diff/20001/go/types/api.go
File go/types/api.go (right):

https://codereview.appspot.com/58540044/diff/20001/go/types/api.go#newcode238
go/types/api.go:238: // Comparable reports whether values of type T are
comparable.
On 2014/01/31 23:17:46, gri wrote:
> I'm fine with exporting this, but let's just leave in where it is instead of
> moving it for now.

Done

[gri, 2014-02-03 19:07:01]

LGTM

[gri, 2014-02-03 19:21:03]

*** Submitted as
https://code.google.com/p/go/source/detail?r=eaffe09e3f17&repo=tools ***

go.tools/go/types: export Comparable and Assertable, simplify Implements

The "static bool" parameter to Implements was confusing; typically we think
about interface implementation and type assertion as separate but related
concepts, but here they were merged.

LGTM=gri
R=gri
CC=golang-codereviews
https://codereview.appspot.com/58540044

Committer: Robert Griesemer <gri@golang.org>

[adonovan, 2014-02-03 20:22:08]

https://codereview.appspot.com/58540044/diff/40001/go/types/api.go
File go/types/api.go (right):

https://codereview.appspot.com/58540044/diff/40001/go/types/api.go#newcode227
go/types/api.go:227: func Assertable(V *Interface, T Type) bool {
Can we call these AssertableTo and ComparableTo?

It's very easy to mistake the parameter order of non-commutative binary
relations of similar argument types.  Bugs can be latent for because the most
common inputs by far are (x, x) for which the result is still true, and (y, z)
for unrelated types, for which the result is still false.  (Also, it might
suggest to the naive reader that the relation is in fact commutative.)

I do like that you have eliminated the (very subtle) boolean parameter, though.

[gmk, 2014-02-03 20:43:35]

https://codereview.appspot.com/58540044/diff/40001/go/types/api.go
File go/types/api.go (right):

https://codereview.appspot.com/58540044/diff/40001/go/types/api.go#newcode227
go/types/api.go:227: func Assertable(V *Interface, T Type) bool {
On 2014/02/03 20:22:09, adonovan wrote:
> Can we call these AssertableTo and ComparableTo?
> 
> It's very easy to mistake the parameter order of non-commutative binary
> relations of similar argument types.  Bugs can be latent for because the most
> common inputs by far are (x, x) for which the result is still true, and (y, z)
> for unrelated types, for which the result is still false.  (Also, it might
> suggest to the naive reader that the relation is in fact commutative.)
> 
> I do like that you have eliminated the (very subtle) boolean parameter,
though.

Comparable is unary.

I don't think the "To" suffixes add much or any value; one can still interpret
the argument order in either way.  (Consider that the builtin copy function
could be sensibly named copyTo(dst, src).)  Ultimately, one has to rely on the
docs; parameters can also be named helpfully.  In the case of Assertable, we
also have that the parameter types are different.

I think the clearest possible way is to make AssignableTo and ConvertibleTo
methods instead of functions (in this case, the "To" seems necessary).  Although
it makes the library code a bit ugly (a la Type.Underlying).

[adonovan, 2014-02-03 21:27:26]

On 2014/02/03 20:43:35, gmk wrote:
> https://codereview.appspot.com/58540044/diff/40001/go/types/api.go
> File go/types/api.go (right):
> 
> https://codereview.appspot.com/58540044/diff/40001/go/types/api.go#newcode227
> go/types/api.go:227: func Assertable(V *Interface, T Type) bool {
> On 2014/02/03 20:22:09, adonovan wrote:
> > Can we call these AssertableTo and ComparableTo?
> > 
> > It's very easy to mistake the parameter order of non-commutative binary
> > relations of similar argument types.  Bugs can be latent for because the
most
> > common inputs by far are (x, x) for which the result is still true, and (y,
z)
> > for unrelated types, for which the result is still false.  (Also, it might
> > suggest to the naive reader that the relation is in fact commutative.)
> > 
> > I do like that you have eliminated the (very subtle) boolean parameter,
> though.
> 
> Comparable is unary.

Quite right; never mind.


> I don't think the "To" suffixes add much or any value; one can still interpret
> the argument order in either way.  (Consider that the builtin copy function
> could be sensibly named copyTo(dst, src).)

There seems to be a fairly consistent convention of reading the method suffix To
(and From) on a 2-ary function as if it stands between the two operands, so
copyTo(x, y) would be read as "copy x to y", as would x.copyTo(y).

> Ultimately, one has to rely on the
> docs; parameters can also be named helpfully.  In the case of Assertable, we
> also have that the parameter types are different.

True, but they're still both instances of Type, and if you have two *Interfaces,
you can easily mix them up.


> I think the clearest possible way is to make AssignableTo and ConvertibleTo
> methods instead of functions (in this case, the "To" seems necessary). 
Although
> it makes the library code a bit ugly (a la Type.Underlying).

Binary relations are a poor fit for single-dispatch methods and are most
naturally defined as a single, recursive standalone function, so why not expose
them that way?  Also I'd rather not bloat the interface.

[gmk, 2014-02-03 22:17:37]

On Mon, Feb 3, 2014 at 10:27 PM, <adonovan@google.com> wrote:

> There seems to be a fairly consistent convention of reading the method
> suffix To (and From) on a 2-ary function as if it stands between the two
> operands, so copyTo(x, y) would be read as "copy x to y", as would
> x.copyTo(y).
>
> I used to think there was a fairly consistent convention of putting source
before destination in the parameter list, but then along came builtin copy
and io.Copy.  My point is that the To/From conventions may not be as
consistent as one might hope.  Even if they are "fairly" consistent, that's
still not enough -- one still has to consult the docs to be sure, and thus
suffixes are superfluous.

But I'll not argue it any further, it's a very minor detail.

>
>  Ultimately, one has to rely on the
>> docs; parameters can also be named helpfully.  In the case of
>>
> Assertable, we
>
>> also have that the parameter types are different.
>>
>
> True, but they're still both instances of Type, and if you have two
> *Interfaces, you can easily mix them up.
>
> I see what you mean.

>
>
>  I think the clearest possible way is to make AssignableTo and
>>
> ConvertibleTo
>
>> methods instead of functions (in this case, the "To" seems necessary).
>>
>  Although
>
>> it makes the library code a bit ugly (a la Type.Underlying).
>>
>
> Binary relations are a poor fit for single-dispatch methods and are most
> naturally defined as a single, recursive standalone function, so why not
> expose them that way?  Also I'd rather not bloat the interface.
>
> I don't see what you mean.  I was only thinking about the clarity of code:
 x.AssignableTo(y) is unambiguous.  It would be the most "natural" syntax.

I've had similar feelings about not bloating interfaces, but on reflection
I don't think they're grounded in this case.  The Type interface isn't
meant to be implemented externally, so adding methods to it doesn't place
extra burden on clients.  (However, I find Type.Underlying to be unwelcome
as it essentially wraps a type assertion that can be implemented in a
4-line function in client code.  Also, it collides with the field
Named.Underlying when that one is exported.)

>
> https://codereview.appspot.com/58540044/
>

[adonovan, 2014-02-03 22:26:00]

On 2014/02/03 22:17:37, gmk wrote:
> On Mon, Feb 3, 2014 at 10:27 PM, <mailto:adonovan@google.com> wrote:
> > Binary relations are a poor fit for single-dispatch methods and are most
> > naturally defined as a single, recursive standalone function, so why not
> > expose them that way?  Also I'd rather not bloat the interface.
>
> I don't see what you mean.  I was only thinking about the clarity of code:
> x.AssignableTo(y) is unambiguous.  It would be the most "natural" syntax.
>
> I've had similar feelings about not bloating interfaces, but on reflection
> I don't think they're grounded in this case.  The Type interface isn't
> meant to be implemented externally, so adding methods to it doesn't place
> extra burden on clients.

Fair point.


> (However, I find Type.Underlying to be unwelcome
> as it essentially wraps a type assertion that can be implemented in a
> 4-line function in client code.  Also, it collides with the field
> Named.Underlying when that one is exported.)

It's not just a type assertion, it's a condition too, so it means you can no
longer write T.Underlying() as a single expression, which I must have used at
least a hundred times in {go/{ssa,pointer},oracle}.

Agree about the collision.