code review 5440117: spec: allow comparison of structs, arrays containing co...

spec: allow comparison of structs, arrays containing comparable values

Also, clarify when interface comparison panics and
that comparison to nil is a special syntax rather than
a general comparison rule.

[rsc, 2011-12-06 15:37:59]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[r, 2011-12-06 16:36:54]

http://codereview.appspot.com/5440117/diff/1/doc/go_spec.html
File doc/go_spec.html (right):

http://codereview.appspot.com/5440117/diff/1/doc/go_spec.html#newcode2939
doc/go_spec.html:2939: apply to operands that are <i>orderable</i>.
s/orderable/ordered/
no need to coin a new term. (orderable to me sounds like we can get one from
amazon.com)

http://codereview.appspot.com/5440117/diff/1/doc/go_spec.html#newcode2972
doc/go_spec.html:2972: Two pointer values are equal if they point to the same
location or if both are <code>nil</code>.
s/are/have value/
and call out == nil but see below

http://codereview.appspot.com/5440117/diff/1/doc/go_spec.html#newcode2977
doc/go_spec.html:2977: Two channel values are equal if they were created by the
same call to <code>make</code>
this definition always bothered me because it depends strongly on the
distinction between value and variable in a way that's hard to understand for a
reference type, but i don't have a better suggestion.

http://codereview.appspot.com/5440117/diff/1/doc/go_spec.html#newcode2979
doc/go_spec.html:2979: or if both are <code>nil</code>.
s/are/have value/
also need to call out == nil but see below

http://codereview.appspot.com/5440117/diff/1/doc/go_spec.html#newcode2985
doc/go_spec.html:2985: and equal dynamic values or if both are <code>nil</code>.
s/are/have value/

http://codereview.appspot.com/5440117/diff/1/doc/go_spec.html#newcode2988
doc/go_spec.html:2988: of that type are not comparable.
call out == nil

but see below

http://codereview.appspot.com/5440117/diff/1/doc/go_spec.html#newcode2994
doc/go_spec.html:2994: The values are equal if their corresponding fields are
equal.
Struct values are comparable if all the fields are comparable and equal if and
only if all corresponding fields are equal.

http://codereview.appspot.com/5440117/diff/1/doc/go_spec.html#newcode2999
doc/go_spec.html:2999: Two array values are compared by comparing corresponding
elements.
similarly

http://codereview.appspot.com/5440117/diff/1/doc/go_spec.html#newcode3006
doc/go_spec.html:3006: However, as a special case, a slice, map, or function
value can
s/can/may/
but this is legalese.

Slice, map, and function values are comparable only to the ideal constant nil.
Also, along with the rules above, pointers, interface values and channels may be
compared with the ideal constant nil.

[rsc, 2011-12-06 16:58:08]

PTAL

http://codereview.appspot.com/5440117/diff/1/doc/go_spec.html
File doc/go_spec.html (right):

http://codereview.appspot.com/5440117/diff/1/doc/go_spec.html#newcode2994
doc/go_spec.html:2994: The values are equal if their corresponding fields are
equal.
On 2011/12/06 16:36:54, r wrote:
> Struct values are comparable if all the fields are comparable and equal if and
> only if all corresponding fields are equal.

It is important to say that the comparison compares corresponding
fields, because those comparisons may be interface comparisons,
which may panic.

http://codereview.appspot.com/5440117/diff/1/doc/go_spec.html#newcode3006
doc/go_spec.html:3006: However, as a special case, a slice, map, or function
value can
On 2011/12/06 16:36:54, r wrote:
> s/can/may/
> but this is legalese.
> 
> Slice, map, and function values are comparable only to the ideal constant nil.

I want to avoid the word comparable here, because it is used
above for things like the definition of whether a struct is comparable
or whether an interface comparison panics, and that does not
depend on whether one of the values involved is nil.

That is, I want to be clear that although you can write
    var x []int
    x == nil
you cannot do the same comparison through interfaces
    var i, j interface{}
    i = x
    j = []int(nil)
    i == j  // panics
or with structs
    var i, j struct { x []int }
    i.x = x
    j.x = nil
    i == j  // disallowed at compile time

Saying the word comparable here makes that less clear,
at least in my mind.

Reworded as:

---
Slice, map, and function values are not comparable.
However, as a special case, a slice, map, or function value may
be compared to the constant <code>nil</code>.
(The rules above already permit pointer, channel, and interface
values to be compared to <code>nil</code>.)
---

[r2, 2011-12-06 17:14:43]

On Dec 6, 2011, at 8:58 AM, rsc@golang.org wrote:

> On 2011/12/06 16:36:54, r wrote:
>> Struct values are comparable if all the fields are comparable and
> equal if and
>> only if all corresponding fields are equal.
> 
> It is important to say that the comparison compares corresponding
> fields, because those comparisons may be interface comparisons,
> which may panic.

what part of 'if and only if all corresponding fields are equal' doesn't say
that?

[r, 2011-12-06 17:16:28]

http://codereview.appspot.com/5440117/diff/4001/doc/go_spec.html
File doc/go_spec.html (right):

http://codereview.appspot.com/5440117/diff/4001/doc/go_spec.html#newcode2979
doc/go_spec.html:2979: or if both are <code>nil</code>.
i think that the phrase
  are nil
is ambiguous; moreover, the meaning of
  c1 == c2
where c1 is nil and
  c1 == nil
are different and should be spelled out. i stand by my previous comments.

[gri, 2011-12-06 17:33:11]

FYI

http://codereview.appspot.com/5440117/diff/4001/doc/go_spec.html
File doc/go_spec.html (right):

http://codereview.appspot.com/5440117/diff/4001/doc/go_spec.html#newcode2951
doc/go_spec.html:2951: Integer values are comparable and ordered, in the usual
way.
presumably runes are integer values

http://codereview.appspot.com/5440117/diff/4001/doc/go_spec.html#newcode3007
doc/go_spec.html:3007: be compared to the constant <code>nil</code>.
The current spec doesn't refer to nil as a constant elsewhere. Also, the
constant section doesn't mention nil as a constant.

[rsc, 2011-12-06 17:50:26]

On Tue, Dec 6, 2011 at 12:14, Rob 'Commander' Pike <r@google.com> wrote:
> On Dec 6, 2011, at 8:58 AM, rsc@golang.org wrote:
> > It is important to say that the comparison compares corresponding
> > fields, because those comparisons may be interface comparisons,
> > which may panic.
>
> what part of 'if and only if all corresponding fields are equal' doesn't say
> that?

The if and only if describes the result without mentioning
the comparison itself, and it is the comparison that has
the side effect of panicking.  I moved the discussion of
panic into its own paragraph, so that I can simplify the
sentences as suggested.

Russ

[rsc, 2011-12-06 17:51:11]

PTAL

http://codereview.appspot.com/5440117/diff/4001/doc/go_spec.html
File doc/go_spec.html (right):

http://codereview.appspot.com/5440117/diff/4001/doc/go_spec.html#newcode2951
doc/go_spec.html:2951: Integer values are comparable and ordered, in the usual
way.
On 2011/12/06 17:33:11, gri wrote:
> presumably runes are integer values

Yes.  They are listed under integer types above.

http://codereview.appspot.com/5440117/diff/4001/doc/go_spec.html#newcode2979
doc/go_spec.html:2979: or if both are <code>nil</code>.
On 2011/12/06 17:16:28, r wrote:
> i think that the phrase
>   are nil
> is ambiguous

Sorry, I just missed this one.  I did change all the others,
and now I've changed this one.

> moreover, the meaning of
>   c1 == c2
> where c1 is nil and
>   c1 == nil
> are different and should be spelled out. i stand by my previous comments.

They are only different for slice, map, and function value,
and I believe that is spelled out below.

http://codereview.appspot.com/5440117/diff/4001/doc/go_spec.html#newcode3007
doc/go_spec.html:3007: be compared to the constant <code>nil</code>.
On 2011/12/06 17:33:11, gri wrote:
> The current spec doesn't refer to nil as a constant elsewhere. Also, the
> constant section doesn't mention nil as a constant.

Changed to "the predeclared identifier nil", which is how
other places in the spec refer to it.

[r, 2011-12-06 18:03:11]

getting close

http://codereview.appspot.com/5440117/diff/4/doc/go_spec.html
File doc/go_spec.html (right):

http://codereview.appspot.com/5440117/diff/4/doc/go_spec.html#newcode2967
doc/go_spec.html:2967: String values are comparable and ordered, lexically
(byte-wise).
delete parens.

http://codereview.appspot.com/5440117/diff/4/doc/go_spec.html#newcode3004
doc/go_spec.html:3004: arrays of interface values or structs with
interface-valued fields.
the phrasing implies some other meaning of 'indirect' is possible, but i don't
think that's true. drop the term and just state the possibility:

This applies not only to direct value comparisons but also when comparing arrays
of interface values or structs with interface-valued fields.

http://codereview.appspot.com/5440117/diff/4/doc/go_spec.html#newcode3012
doc/go_spec.html:3012: is also allowed and follows from the general rules
above.)
parens unnecessary

[iant, 2011-12-06 18:03:19]

LGTM

http://codereview.appspot.com/5440117/diff/4/doc/go_spec.html
File doc/go_spec.html (right):

http://codereview.appspot.com/5440117/diff/4/doc/go_spec.html#newcode2960
doc/go_spec.html:2960: Complex values are comparable.
We could consider saying explicitly "Complex values are comparable but not
ordered."

[gri, 2011-12-06 18:06:11]

http://codereview.appspot.com/5440117/diff/4/doc/go_spec.html
File doc/go_spec.html (right):

http://codereview.appspot.com/5440117/diff/4/doc/go_spec.html#newcode2985
doc/go_spec.html:2985: and equal dynamic values or if both have value
<code>nil</code>.
Comparison of an interface value against a non-interface value is missing.

[cw, 2011-12-06 19:25:30]

I'm curious, other than nil why are function comparisons not allowed?

[gri, 2011-12-06 19:27:49]

On Tue, Dec 6, 2011 at 11:25 AM,  <cw@f00f.org> wrote:
> I'm curious, other than nil why are function comparisons not allowed?

For one: What does it mean to compare to closures? Are they the same
if they are the same code, or if they are the same code and enclosed
values?

- gri

[bradfitz, 2011-12-06 19:30:30]

On Tue, Dec 6, 2011 at 11:25 AM, <cw@f00f.org> wrote:

> I'm curious, other than nil why are function comparisons not allowed?
>

IIRC, there are also some optimizations that are possible without them
around function literals in bodies of functions that don't close over
anything.  I can't remember the example that came up before, though.
 Something about the compiler being able to promote such a non-closing
function literal to a normal function and not call runtime.newclosure on it
each time to make sure it has a unique address, as required by the old spec.

[rsc, 2011-12-06 19:40:02]

In addition to closures (which would be nice not to allocate),
function equality has a long history of being problematic
in C in the presence of dynamic linking and such.  It will
simplify future implementations if we don't require it.

Russ

[rsc, 2011-12-06 20:09:01]

On Tue, Dec 6, 2011 at 13:06,  <gri@golang.org> wrote:
> Comparison of an interface value against a non-interface value is
> missing.

I changed the lead-in paragraph to read:


In any comparison, the first operand
must be <a href="#Assignability">assignable</a>
to the type of the second operand, or vice versa.
When the operand types differ,
the comparison proceeds as if one operand
were first assigned to a temporary of the other type.

[rsc, 2011-12-06 20:10:18]

PTAL

[r, 2011-12-06 20:46:16]

http://codereview.appspot.com/5440117/diff/4006/doc/go_spec.html
File doc/go_spec.html (right):

http://codereview.appspot.com/5440117/diff/4006/doc/go_spec.html#newcode2936
doc/go_spec.html:2936: were first assigned to a temporary of the other type.
but which?

[gri, 2011-12-06 20:47:19]

http://codereview.appspot.com/5440117/diff/4006/doc/go_spec.html
File doc/go_spec.html (right):

http://codereview.appspot.com/5440117/diff/4006/doc/go_spec.html#newcode2936
doc/go_spec.html:2936: were first assigned to a temporary of the other type.
... and that temporary is compared instead. ?

(or something along those lines)

http://codereview.appspot.com/5440117/diff/4006/doc/go_spec.html#newcode3003
doc/go_spec.html:3003: A comparison of two interface values with identical
dynamic types
I would move this paragraph up to where interface comparison is defined.

http://codereview.appspot.com/5440117/diff/4006/doc/go_spec.html#newcode3005
doc/go_spec.html:3005: of that type are not comparable.  This applies not only
to direct interface
Once this paragraph is moved up, this 2nd sentence is not needed anymore - it's
implied.

http://codereview.appspot.com/5440117/diff/4006/doc/go_spec.html#newcode3014
doc/go_spec.html:3014: Comparison of pointer, channel, and interface values to
<code>nil</code>
I think this sentence is not needed. It's already stated above.

[rsc, 2011-12-06 20:52:43]

On Tue, Dec 6, 2011 at 15:46,  <r@golang.org> wrote:
> http://codereview.appspot.com/5440117/diff/4006/doc/go_spec.html#newcode2936
> doc/go_spec.html:2936: were first assigned to a temporary of the other
> type.
> but which?

If both are assignable to each other then the conversion
between them is a no-op, so you'd get the same result either way.
I'm not thrilled with the wording.  Suggestions?

Russ

[rsc, 2011-12-06 20:55:49]

On Tue, Dec 6, 2011 at 15:47,  <gri@golang.org> wrote:
> http://codereview.appspot.com/5440117/diff/4006/doc/go_spec.html#newcode3003
> doc/go_spec.html:3003: A comparison of two interface values with
> identical dynamic types
> I would move this paragraph up to where interface comparison is defined.

It started that way, but you can't move that paragraph up,
because at that point struct and array comparison are not
defined.  Also, struct, array, and interface comparability
are all circularly dependent on each other, so I wanted the
wording as short as possible in the bullet lists.

I think the separate paragraph is much clearer than
having it split among the three bullets (which was the
first revision that everyone reviewed).

> http://codereview.appspot.com/5440117/diff/4006/doc/go_spec.html#newcode3005
> doc/go_spec.html:3005: of that type are not comparable.  This applies
> not only to direct interface
> Once this paragraph is moved up, this 2nd sentence is not needed anymore
> - it's implied.

Explicit is clearer than implicit here.  I think it would be
very easy for someone to miss that struct comparison
might panic because of a field with interface type.
I agree that it is implied, but the extra sentence helps
people who don't realize that, which will be most people.

> http://codereview.appspot.com/5440117/diff/4006/doc/go_spec.html#newcode3014
> doc/go_spec.html:3014: Comparison of pointer, channel, and interface
> values to <code>nil</code>
> I think this sentence is not needed. It's already stated above.

Again, it's a tricky subject, and I agree that it is stated
above (and the sentence even says so).  Better to be
explicit.  Otherwise someone reading this might think
that you can't say ==nil except on those cases.

Russ

[r2, 2011-12-06 20:56:24]

On Dec 6, 2011, at 12:52 PM, Russ Cox wrote:

> On Tue, Dec 6, 2011 at 15:46,  <r@golang.org> wrote:
>> http://codereview.appspot.com/5440117/diff/4006/doc/go_spec.html#newcode2936
>> doc/go_spec.html:2936: were first assigned to a temporary of the other
>> type.
>> but which?
> 
> If both are assignable to each other then the conversion
> between them is a no-op, so you'd get the same result either way.
> I'm not thrilled with the wording.  Suggestions?

happy to make suggestions but i'm not sure exactly what you're trying to say.
assignability can be one-way, but the phrasing and your explanation here makes
it sound more symmetric.

-rob

[rsc, 2011-12-06 21:00:49]

On Tue, Dec 6, 2011 at 15:56, Rob 'Commander' Pike <r@google.com> wrote:
> happy to make suggestions but i'm not sure exactly what you're trying to say.
assignability can be one-way, but the phrasing and your explanation here makes
it sound more symmetric.

if you have x == y, then either:

1. x and y are same type
2. x is assignable to y, in which case the == behaves as if by
    var t typeof(y) = x
    t == y
3. y is assignable to x, in which case the == behaves as if by
    var t typeof(x) = y
    x == t

If 2 and 3 are both possible, then you get the same result
no matter which you choose.  That only happens in cases like

    type T struct{}
    var x struct{}
    var y T
    x == y

Russ

[rsc, 2011-12-06 21:09:03]

never mind.  this is what the compiler does but it is a mistake,
because it allows some comparisons (involving interface values)
that will obviously panic at runtime.  i will fix the spec and
the compiler.


On Tue, Dec 6, 2011 at 16:00, Russ Cox <rsc@golang.org> wrote:
> On Tue, Dec 6, 2011 at 15:56, Rob 'Commander' Pike <r@google.com> wrote:
>> happy to make suggestions but i'm not sure exactly what you're trying to say.
assignability can be one-way, but the phrasing and your explanation here makes
it sound more symmetric.
>
> if you have x == y, then either:
>
> 1. x and y are same type
> 2. x is assignable to y, in which case the == behaves as if by
>    var t typeof(y) = x
>    t == y
> 3. y is assignable to x, in which case the == behaves as if by
>    var t typeof(x) = y
>    x == t
>
> If 2 and 3 are both possible, then you get the same result
> no matter which you choose.  That only happens in cases like
>
>    type T struct{}
>    var x struct{}
>    var y T
>    x == y
>
> Russ

[rsc, 2011-12-06 21:11:27]

PTAL


 In any comparison, the first operand
 must be <a href="#Assignability">assignable</a>
 to the type of the second operand, or vice versa.
-When the operand types differ,
-the comparison proceeds as if one operand
-were first assigned to a temporary of the other type.
 </p>
 <p>
 The equality operators <code>==</code> and <code>!=</code> apply

...

 	Two interface values are equal if they have <a
href="#Type_identity">identical</a> dynamic types
 	and equal dynamic values or if both have value <code>nil</code>.
 	</li>
+	
+	<li>
+	A non-interface value of type T and an interface value are comparable
+	when values of type T are comparable and type T implements the interface.
+	</li>

 	<li>
 	Struct values are comparable if all the fields are comparable.

[gri, 2011-12-06 21:24:27]

http://codereview.appspot.com/5440117/diff/6004/doc/go_spec.html
File doc/go_spec.html (right):

http://codereview.appspot.com/5440117/diff/6004/doc/go_spec.html#newcode2990
doc/go_spec.html:2990: when values of type T are comparable and type T
implements the interface.
Much better. But I think we still need to say when they are equal, now that they
are comparable (like everywhere else).

[rsc, 2011-12-06 21:25:56]

updated:
	A non-interface value x of type T and an interface value are comparable
	when values of type T are comparable and type T implements the interface.
	They are equal if the interface value has dynamic type T and a dynamic
	value equal to the x.

[r, 2011-12-06 21:37:27]

LGTM

http://codereview.appspot.com/5440117/diff/6006/doc/go_spec.html
File doc/go_spec.html (right):

http://codereview.appspot.com/5440117/diff/6006/doc/go_spec.html#newcode2989
doc/go_spec.html:2989: A non-interface value x of type T and an interface value
are comparable
missing some <code> markers.

http://codereview.appspot.com/5440117/diff/6006/doc/go_spec.html#newcode2992
doc/go_spec.html:2992: value equal to x.
would help to have more names.

A non-interface value x of type X and an interface value of type T, with dynamic
value t, are comparable when values of type X are comparable and X implements T.
They are equal if t has type X and t is equal to x.

(feel free to choose different letters)

http://codereview.appspot.com/5440117/diff/6006/doc/go_spec.html#newcode3009
doc/go_spec.html:3009: of that type are not comparable.  This applies not only
to direct interface
s/This/This property/ or behavior

[rsc, 2011-12-06 21:43:08]

PTAL

I tweaked your wording ever so slightly, because the
comparability does not depend on the interface value
having a dynamic value at all.  (The interface value can
be nil, and the result will be false.)

	A non-interface value <code>x</code> of type <code>X</code> and
	a value <code>t</code> of interface type <code>T</code> are
comparable when values
	of type <code>X</code> are comparable and
	<code>X</code> implements <code>T</code>.
	They are equal if <code>t</code>'s dynamic type is identical to <code>X</code>
	and <code>t</code>'s dynamic value is equal to <code>x</code>.

Russ

[r2, 2011-12-06 21:45:20]

LGTM

[gri, 2011-12-06 22:01:07]

LGTM

http://codereview.appspot.com/5440117/diff/2013/doc/go_spec.html
File doc/go_spec.html (right):

http://codereview.appspot.com/5440117/diff/2013/doc/go_spec.html#newcode2989
doc/go_spec.html:2989: A non-interface value <code>x</code> of type
<code>X</code> and
A value <code>x</code> of non-interface type ...  ?

(to be symmetric - I had to read this a couple of times as it confused me)

http://codereview.appspot.com/5440117/diff/2013/doc/go_spec.html#newcode2997
doc/go_spec.html:2997: when values of type T are comparable and type T
implements the interface.
there seems to be some left-over stuff here (or is it Rietveld screwing up?)

[rsc, 2011-12-06 22:02:59]

thanks.  fixed both.

On Tue, Dec 6, 2011 at 17:01,  <gri@golang.org> wrote:
> LGTM
>
>
>
> http://codereview.appspot.com/5440117/diff/2013/doc/go_spec.html
> File doc/go_spec.html (right):
>
> http://codereview.appspot.com/5440117/diff/2013/doc/go_spec.html#newcode2989
> doc/go_spec.html:2989: A non-interface value <code>x</code> of type
> <code>X</code> and
> A value <code>x</code> of non-interface type ...  ?
>
> (to be symmetric - I had to read this a couple of times as it confused
> me)
>
> http://codereview.appspot.com/5440117/diff/2013/doc/go_spec.html#newcode2997
> doc/go_spec.html:2997: when values of type T are comparable and type T
> implements the interface.
> there seems to be some left-over stuff here (or is it Rietveld screwing
> up?)
>
> http://codereview.appspot.com/5440117/

[gri, 2011-12-06 22:28:38]

LGTM

[rsc, 2011-12-13 03:21:49]

*** Submitted as http://code.google.com/p/go/source/detail?r=5da1bc8899cf ***

spec: allow comparison of structs, arrays containing comparable values

Also, clarify when interface comparison panics and
that comparison to nil is a special syntax rather than
a general comparison rule.

R=r, gri, r, iant, cw, bradfitz
CC=golang-dev
http://codereview.appspot.com/5440117