code review 10271046: cmd/gc: fix computation of equality class of types.

cmd/gc: fix computation of equality class of types.

A struct with a single field was considered as equivalent to the
field type, which is incorrect is the field is blank.

Fields with padding could make the compiler think some
types are comparable when they are not.

Fixes issue 5698.

[remyoudompheng, 2013-06-13 21:20:28]

Hello rsc@golang.org, golang-dev@googlegroups.com (cc:
golang-dev@googlegroups.com),

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

[remyoudompheng, 2013-06-13 21:23:01]

On 2013/06/13 21:20:28, remyoudompheng wrote:
> Hello mailto:rsc@golang.org, mailto:golang-dev@googlegroups.com (cc:
> mailto:golang-dev@googlegroups.com),
> 
> I'd like you to review this change to
> https://go.googlecode.com/hg/

I have a debate within myself about whether struct{ _ []int }
is a comparable type or not.

The compiler and the test suite say it should be comparable.

But the spec says that: "Struct values are comparable if all their fields are
comparable." It doesn't say what happens if those fields are blank.

[remyoudompheng, 2013-06-18 06:57:50]

ping? or is it delayed until runtime is fixed?

[DMorsing, 2013-06-18 07:52:45]

LGTM.

I don't think this is blocked on the runtime freeze, but wait for Russ.

[bradfitz, 2013-06-21 16:17:24]

R=rsc

[rsc, 2013-06-27 03:17:50]

sorry, just overwhelmed by all the runtime stuff. will review tomorrow.

[rsc, 2013-06-27 15:45:43]

I'm not sure actually.

type T struct {
    _ []byte
    x int
}

var t1, t2
println(t1 == t2)

Is this valid? If the _ were a real name, the struct would not have an ==
because of the slice field. I don't remember deciding that skipping comparison
of _ meant ignoring the presence of uncomparable types.

Robert, what do you think?

My suspicion is that this program should be rejected too, in which case the
solution is to move the ANOEQ case up above the original two-clause if.

[gri, 2013-06-27 16:29:55]

What Rémy says ( http://tip.golang.org/ref/spec#Comparison_operators ):

"Struct values are comparable if *all* their fields are comparable. Two
struct values are equal if their corresponding non-blank fields are equal."

"All their fields" includes blank fields in my mind; and the 2nd sentence
makes this clear because it explicitly excludes blank fields for equality.
Thus if any blank field is not comparable, then the struct is not
comparable.

Maybe that's a mistake (in the spec), and blank fields should always be
ignored, but at least as the spec stands, I think it's clear.

- gri



On Thu, Jun 27, 2013 at 8:45 AM, <rsc@golang.org> wrote:

> I'm not sure actually.
>
> type T struct {
>     _ []byte
>     x int
> }
>
> var t1, t2
> println(t1 == t2)
>
> Is this valid? If the _ were a real name, the struct would not have an
> == because of the slice field. I don't remember deciding that skipping
> comparison of _ meant ignoring the presence of uncomparable types.
>
> Robert, what do you think?
>
> My suspicion is that this program should be rejected too, in which case
> the solution is to move the ANOEQ case up above the original two-clause
> if.
>
>
https://codereview.appspot.**com/10271046/<https://codereview.appspot.com/102...
>

[r, 2013-06-27 16:53:45]

The spec sounds right to me. It would feel wrong if changing the name
of an unused field from "foo" to "_" makes a program with an ==
suddenly compile, all other things being equal.  Such a change should
not affect type correctness.

-rob

[rsc, 2013-06-27 17:11:22]

Okay, we all agree that struct { _ []byte } is not comparable. Good.

[rsc, 2013-06-27 17:14:24]

Please add a test that struct { _ []byte } is not comparable.

https://codereview.appspot.com/10271046/diff/5001/src/cmd/gc/subr.c
File src/cmd/gc/subr.c (right):

https://codereview.appspot.com/10271046/diff/5001/src/cmd/gc/subr.c#newcode624
src/cmd/gc/subr.c:624: // Blank fields and padding must be ignored,
Given the new understanding I think this loop body should be (in its entirety):

a = algtype1(t1->type, bad);
if(a == ANOEQ)
    return ANOEQ;

// Blank fields, fields with padding, and fields with
// non-memory equality need special compare.
if(a != AMEM || isblanksym(t1->sym) || ispaddedfield(t1, t->width))
    ret = -1;

[remyoudompheng, 2013-06-28 15:35:27]

Hello rsc@golang.org, golang-dev@googlegroups.com, daniel.morsing@gmail.com,
bradfitz@golang.org, gri@golang.org, r@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[rsc, 2013-07-02 01:49:40]

LGTM

Thanks very much.

https://codereview.appspot.com/10271046/diff/18001/test/cmp.go
File test/cmp.go (right):

https://codereview.appspot.com/10271046/diff/18001/test/cmp.go#newcode299
test/cmp.go:299: _ string
Nice. We all agreed on what it should mean and there was a test to the contrary.
So it goes.

[remyoudompheng, 2013-07-02 07:09:16]

*** Submitted as https://code.google.com/p/go/source/detail?r=3a652b14dccc ***

cmd/gc: fix computation of equality class of types.

A struct with a single field was considered as equivalent to the
field type, which is incorrect is the field is blank.

Fields with padding could make the compiler think some
types are comparable when they are not.

Fixes issue 5698.

R=rsc, golang-dev, daniel.morsing, bradfitz, gri, r
CC=golang-dev
https://codereview.appspot.com/10271046