code review 5339045: gc: Allow any type in switch on interface value. Static...

gc: Better typechecks and errors in switches.

Allow any type in switch on interface value.
Statically check typeswitch early.

Fixes issue 2423.
Fixes issue 2424.

[lvd, 2011-11-07 07:36:17]

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

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

[dsymonds, 2011-11-07 08:18:22]

Isn't this a language change? I understood (value) switches to be glorified
if chains with ==, but interface{} is not comparable to string.

[lvd, 2011-11-07 08:21:04]

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

Please take another look.

[lvd, 2011-11-07 08:22:40]

On 2011/11/07 08:18:22, dsymonds wrote:
> Isn't this a language change? I understood (value) switches to be glorified
> if chains with ==, but interface{} is not comparable to string.

if x == "hello" {... just compiles and passes here

[lvd, 2011-11-07 08:31:16]

(hm don't know why code. doesn't send my reply)

On Mon, Nov 7, 2011 at 09:18, David Symonds <dsymonds@golang.org> wrote:

> Isn't this a language change? I understood (value) switches to be
> glorified if chains with ==, but interface{} is not comparable to string.
>

if x == "hello" {... compiles and works fine here.

i added in a fix for the next issue, but that trips

 fixedbugs/bug270.go:19: impossible type switch case: I(T literal) (type I)
cannot have dynamic type interface {} (missing F method)

bug 270 reads

package main

type I interface { F() }

type T struct{}

func (T) F() {}

func main() {
switch I(T{}).(type) {
case interface{}:     // new error is here
}
}

i think the new behaviour is correct, since if i add

var x interface{}
var y I = x

i get
 cannot use x (type interface {}) as type I in assignment:
        interface {} does not implement I (missing F method)

[lvd, 2011-11-07 08:34:30]

On Mon, Nov 7, 2011 at 09:31, Luuk van Dijk <lvd@google.com> wrote:

> (hm don't know why code. doesn't send my reply)
>
>
> On Mon, Nov 7, 2011 at 09:18, David Symonds <dsymonds@golang.org> wrote:
>
>> Isn't this a language change? I understood (value) switches to be
>> glorified if chains with ==, but interface{} is not comparable to string.
>>
>
> if x == "hello" {... compiles and works fine here.
>
> i added in a fix for the next issue, but that trips
>
>  fixedbugs/bug270.go:19: impossible type switch case: I(T literal) (type
> I) cannot have dynamic type interface {} (missing F method)
>
> bug 270 reads
>
> package main
>
> type I interface { F() }
>
> type T struct{}
>
> func (T) F() {}
>
> func main() {
> switch I(T{}).(type) {
> case interface{}:     // new error is here
>  }
> }
>
> i think the new behaviour is correct, since if i add
>
> var x interface{}
> var y I = x
>
> i get
>  cannot use x (type interface {}) as type I in assignment:
>         interface {} does not implement I (missing F method)
>
>
>
I looked at the original issue
http://code.google.com/p/go/issues/detail?id=746   and i think replacing
interface{} with default keeps the gist of the test (for missing typecheck
on the switch test)

/L

[dsymonds, 2011-11-07 11:09:10]

On Mon, Nov 7, 2011 at 7:31 PM, Luuk van Dijk <lvd@google.com> wrote:

> On Mon, Nov 7, 2011 at 09:18, David Symonds <dsymonds@golang.org> wrote:
>>
>> Isn't this a language change? I understood (value) switches to be
>> glorified if chains with ==, but interface{} is not comparable to string.
>
> if x == "hello" {... compiles and works fine here.

Aah, duh. "hello" is assignable to x, so they are comparable.

Never mind me...

[lvd, 2011-11-07 14:45:00]

http://codereview.appspot.com/5339045/diff/1002/src/cmd/gc/swt.c
File src/cmd/gc/swt.c (right):

http://codereview.appspot.com/5339045/diff/1002/src/cmd/gc/swt.c#newcode863
src/cmd/gc/swt.c:863: ;
btw i just realized I could do an implements() check here too.

[rsc, 2011-11-07 17:36:58]

http://codereview.appspot.com/5339045/diff/1002/src/cmd/gc/swt.c
File src/cmd/gc/swt.c (right):

http://codereview.appspot.com/5339045/diff/1002/src/cmd/gc/swt.c#newcode863
src/cmd/gc/swt.c:863: ;
I think this new code is only necessary to work around the bug on line 864.
That bug is that it should be using assignability, not type equality,
to make the decision.  switch x { case y: } is allowed if x == y is allowed,
so the right test is whether assignop(x->type, y->type, nil) != 0 or
assignop(y->type, x->type, nil) != 0.

[lvd, 2011-11-07 19:00:44]

On Mon, Nov 7, 2011 at 18:36, <rsc@golang.org> wrote:

>
>
http://codereview.appspot.com/**5339045/diff/1002/src/cmd/gc/**swt.c<http://c...
> File src/cmd/gc/swt.c (right):
>
> http://codereview.appspot.com/**5339045/diff/1002/src/cmd/gc/**
>
swt.c#newcode863<http://codereview.appspot.com/5339045/diff/1002/src/cmd/gc/swt.c#newcode863>
> src/cmd/gc/swt.c:863: ;
> I think this new code is only necessary to work around the bug on line
> 864.
> That bug is that it should be using assignability, not type equality,
> to make the decision.  switch x { case y: } is allowed if x == y is
> allowed,
> so the right test is whether assignop(x->type, y->type, nil) != 0 or
> assignop(y->type, x->type, nil) != 0.
>
>
that makes perfect sense.  done.


>
http://codereview.appspot.com/**5339045/<http://codereview.appspot.com/5339045/>
>

[rsc, 2011-11-07 19:06:06]

http://codereview.appspot.com/5339045/diff/17002/test/fixedbugs/bug270.go
File test/fixedbugs/bug270.go (right):

http://codereview.appspot.com/5339045/diff/17002/test/fixedbugs/bug270.go#new...
test/fixedbugs/bug270.go:19: default:
I think you can remove this test.
It was specifically testing I2E conversion
but that's impossible in a type switch.

[lvd, 2011-11-07 19:10:25]

On Mon, Nov 7, 2011 at 20:06, <rsc@golang.org> wrote:

>
> http://codereview.appspot.com/**5339045/diff/17002/test/**
>
fixedbugs/bug270.go<http://codereview.appspot.com/5339045/diff/17002/test/fixedbugs/bug270.go>
> File test/fixedbugs/bug270.go (right):
>
> http://codereview.appspot.com/**5339045/diff/17002/test/**
>
fixedbugs/bug270.go#newcode19<http://codereview.appspot.com/5339045/diff/17002/test/fixedbugs/bug270.go#newcode19>
> test/fixedbugs/bug270.go:19: default:
> I think you can remove this test.
> It was specifically testing I2E conversion
> but that's impossible in a type switch.
>
>
done


>
http://codereview.appspot.com/**5339045/<http://codereview.appspot.com/5339045/>
>

[rsc, 2011-11-07 19:14:36]

http://codereview.appspot.com/5339045/diff/17002/src/cmd/gc/swt.c
File src/cmd/gc/swt.c (right):

http://codereview.appspot.com/5339045/diff/17002/src/cmd/gc/swt.c#newcode864
src/cmd/gc/swt.c:864: else if(ll->n->type != T && assignop(ll->n->type, t, &why)
== 0)
I believe you need both assignop checks, which means that
keeping the why is a little tricky.  I would settle for using the
same syntax as the error in this program:

package main
type I interface {m()}
func main() {
	var i I
	_ = i == "hello"
}

It prints

x.go:5: invalid operation: i == "hello" (mismatched types I and string)

Please add tests for both cases:

type I interface { M() }
var i I
var s string

switch i {
case s:  // ERROR "mismatched types I and string"
}
switch s {
case i:  // ERROR "mismatched types string and I"
}

but also add test cases that if you have

type I interface{}

then both those switches are valid.

http://codereview.appspot.com/5339045/diff/17002/src/cmd/gc/swt.c#newcode875
src/cmd/gc/swt.c:875: if(have && !missing->broke && !have->broke)
Presumably the !missing->broke && !have->broke is needed
in typecheck.c's copy of this code too.  Maybe lift out into a
bool checkimplements(Type*, Type*)
or whatever the signature needs to be to reproduce the errors.

[lvd, 2011-11-07 20:21:53]

On Mon, Nov 7, 2011 at 20:14, <rsc@golang.org> wrote:

>
>
http://codereview.appspot.com/**5339045/diff/17002/src/cmd/gc/**swt.c<http://...
> File src/cmd/gc/swt.c (right):
>
> http://codereview.appspot.com/**5339045/diff/17002/src/cmd/gc/**
>
swt.c#newcode864<http://codereview.appspot.com/5339045/diff/17002/src/cmd/gc/swt.c#newcode864>
> src/cmd/gc/swt.c:864: else if(ll->n->type != T && assignop(ll->n->type,
> t, &why) == 0)
> I believe you need both assignop checks, which means that
> keeping the why is a little tricky.  I would settle for using the
> same syntax as the error in this program:
>
> package main
> type I interface {m()}
> func main() {
>        var i I
>        _ = i == "hello"
> }
>
> It prints
>
> x.go:5: invalid operation: i == "hello" (mismatched types I and string)
>
> Please add tests for both cases:
>
> type I interface { M() }
> var i I
> var s string
>
> switch i {
> case s:  // ERROR "mismatched types I and string"
> }
> switch s {
> case i:  // ERROR "mismatched types string and I"
> }
>
> but also add test cases that if you have
>
> type I interface{}
>
> then both those switches are valid.
>

added those to typeswitch2.go, and surprise, they aren't valid, because of
the symmetric assign check. i think.  let me think that over.


>
> http://codereview.appspot.com/**5339045/diff/17002/src/cmd/gc/**
>
swt.c#newcode875<http://codereview.appspot.com/5339045/diff/17002/src/cmd/gc/swt.c#newcode875>
> src/cmd/gc/swt.c:875: if(have && !missing->broke && !have->broke)
> Presumably the !missing->broke && !have->broke is needed
> in typecheck.c's copy of this code too.  Maybe lift out into a
> bool checkimplements(Type*, Type*)
> or whatever the signature needs to be to reproduce the errors.
>
>
http://codereview.appspot.com/**5339045/<http://codereview.appspot.com/5339045/>
>

[lvd, 2011-11-07 20:40:53]

On Mon, Nov 7, 2011 at 21:21, Luuk van Dijk <lvd@google.com> wrote:

>
>
> On Mon, Nov 7, 2011 at 20:14, <rsc@golang.org> wrote:
>
>>
>>
http://codereview.appspot.com/**5339045/diff/17002/src/cmd/gc/**swt.c<http://...
>> File src/cmd/gc/swt.c (right):
>>
>> http://codereview.appspot.com/**5339045/diff/17002/src/cmd/gc/**
>>
swt.c#newcode864<http://codereview.appspot.com/5339045/diff/17002/src/cmd/gc/swt.c#newcode864>
>> src/cmd/gc/swt.c:864: else if(ll->n->type != T && assignop(ll->n->type,
>> t, &why) == 0)
>> I believe you need both assignop checks, which means that
>> keeping the why is a little tricky.  I would settle for using the
>> same syntax as the error in this program:
>>
>> package main
>> type I interface {m()}
>> func main() {
>>        var i I
>>        _ = i == "hello"
>> }
>>
>> It prints
>>
>> x.go:5: invalid operation: i == "hello" (mismatched types I and string)
>>
>> Please add tests for both cases:
>>
>> type I interface { M() }
>> var i I
>> var s string
>>
>> switch i {
>> case s:  // ERROR "mismatched types I and string"
>> }
>> switch s {
>> case i:  // ERROR "mismatched types string and I"
>> }
>>
>> but also add test cases that if you have
>>
>> type I interface{}
>>
>> then both those switches are valid.
>>
>
> added those to typeswitch2.go, and surprise, they aren't valid, because of
> the symmetric assign check. i think.  let me think that over.
>

it is because i have no brain at all.


>
>
>>
>> http://codereview.appspot.com/**5339045/diff/17002/src/cmd/gc/**
>>
swt.c#newcode875<http://codereview.appspot.com/5339045/diff/17002/src/cmd/gc/swt.c#newcode875>
>> src/cmd/gc/swt.c:875: if(have && !missing->broke && !have->broke)
>> Presumably the !missing->broke && !have->broke is needed
>> in typecheck.c's copy of this code too.  Maybe lift out into a
>> bool checkimplements(Type*, Type*)
>> or whatever the signature needs to be to reproduce the errors.
>>
>>
http://codereview.appspot.com/**5339045/<http://codereview.appspot.com/5339045/>
>>
>
>

[lvd, 2011-11-07 21:41:06]

On Mon, Nov 7, 2011 at 21:40, Luuk van Dijk <lvd@google.com> wrote:

>
>
> On Mon, Nov 7, 2011 at 21:21, Luuk van Dijk <lvd@google.com> wrote:
>
>>
>>
>> On Mon, Nov 7, 2011 at 20:14, <rsc@golang.org> wrote:
>>
>>>
>>>
http://codereview.appspot.com/**5339045/diff/17002/src/cmd/gc/**swt.c<http://...
>>> File src/cmd/gc/swt.c (right):
>>>
>>> http://codereview.appspot.com/**5339045/diff/17002/src/cmd/gc/**
>>>
swt.c#newcode864<http://codereview.appspot.com/5339045/diff/17002/src/cmd/gc/swt.c#newcode864>
>>> src/cmd/gc/swt.c:864: else if(ll->n->type != T && assignop(ll->n->type,
>>> t, &why) == 0)
>>> I believe you need both assignop checks, which means that
>>> keeping the why is a little tricky.  I would settle for using the
>>> same syntax as the error in this program:
>>>
>>> package main
>>> type I interface {m()}
>>> func main() {
>>>        var i I
>>>        _ = i == "hello"
>>> }
>>>
>>> It prints
>>>
>>> x.go:5: invalid operation: i == "hello" (mismatched types I and string)
>>>
>>> Please add tests for both cases:
>>>
>>> type I interface { M() }
>>> var i I
>>> var s string
>>>
>>> switch i {
>>> case s:  // ERROR "mismatched types I and string"
>>> }
>>> switch s {
>>> case i:  // ERROR "mismatched types string and I"
>>> }
>>>
>>> but also add test cases that if you have
>>>
>>> type I interface{}
>>>
>>> then both those switches are valid.
>>>
>>
>> added those to typeswitch2.go, and surprise, they aren't valid, because
>> of the symmetric assign check. i think.  let me think that over.
>>
>
> it is because i have no brain at all.
>

had to fiddle a bit with the linenumbers, and made it typecheck3.go,
because those errors will prevent walk from happening, which generates the
existing messages in typecheck2.go


>
>
>>
>>
>>>
>>> http://codereview.appspot.com/**5339045/diff/17002/src/cmd/gc/**
>>>
swt.c#newcode875<http://codereview.appspot.com/5339045/diff/17002/src/cmd/gc/swt.c#newcode875>
>>> src/cmd/gc/swt.c:875: if(have && !missing->broke && !have->broke)
>>> Presumably the !missing->broke && !have->broke is needed
>>> in typecheck.c's copy of this code too.  Maybe lift out into a
>>> bool checkimplements(Type*, Type*)
>>> or whatever the signature needs to be to reproduce the errors.
>>>
>>>
http://codereview.appspot.com/**5339045/<http://codereview.appspot.com/5339045/>
>>>
>>
>>
>

[lvd, 2011-11-07 21:51:59]

all tests pass.

will leave the checkimplements for another CL, where i can add more ->broke
inhibiting yyerrors too.

[rsc, 2011-11-09 04:37:35]

LGTM

http://codereview.appspot.com/5339045/diff/9/test/fixedbugs/bug375.go
File test/fixedbugs/bug375.go (right):

http://codereview.appspot.com/5339045/diff/9/test/fixedbugs/bug375.go#newcode12
test/fixedbugs/bug375.go:12: var x interface{} = "hello"
please gofmt. looks like some spaces crept in

http://codereview.appspot.com/5339045/diff/9/test/fixedbugs/bug376.go
File test/fixedbugs/bug376.go (right):

http://codereview.appspot.com/5339045/diff/9/test/fixedbugs/bug376.go#newcode17
test/fixedbugs/bug376.go:17: case string:	// ERROR "impossible"
please add this case to test/typeswitch2.go 
and drop this file.  this is an important enough
case to be in a named file.

http://codereview.appspot.com/5339045/diff/9/test/typeswitch3.go
File test/typeswitch3.go (right):

http://codereview.appspot.com/5339045/diff/9/test/typeswitch3.go#newcode15
test/typeswitch3.go:15: switch i {
This is in a file called typeswitch3.go but it is a value switch.
Please rename to switchN.go for some N.

[lvd, 2011-11-09 09:44:24]

http://codereview.appspot.com/5339045/diff/9/test/fixedbugs/bug375.go
File test/fixedbugs/bug375.go (right):

http://codereview.appspot.com/5339045/diff/9/test/fixedbugs/bug375.go#newcode12
test/fixedbugs/bug375.go:12: var x interface{} = "hello"
On 2011/11/09 04:37:35, rsc wrote:
> please gofmt. looks like some spaces crept in

Done.

http://codereview.appspot.com/5339045/diff/9/test/fixedbugs/bug376.go
File test/fixedbugs/bug376.go (right):

http://codereview.appspot.com/5339045/diff/9/test/fixedbugs/bug376.go#newcode17
test/fixedbugs/bug376.go:17: case string:	// ERROR "impossible"
On 2011/11/09 04:37:35, rsc wrote:
> please add this case to test/typeswitch2.go 
> and drop this file.  this is an important enough
> case to be in a named file.

this is a typecheck-time error, which will suppress the walk-time errors in
typeswitch2.go.  I'll add it to typeswitch3.go and move the value switch out of
there.

http://codereview.appspot.com/5339045/diff/9/test/typeswitch3.go
File test/typeswitch3.go (right):

http://codereview.appspot.com/5339045/diff/9/test/typeswitch3.go#newcode15
test/typeswitch3.go:15: switch i {
On 2011/11/09 04:37:35, rsc wrote:
> This is in a file called typeswitch3.go but it is a value switch.
> Please rename to switchN.go for some N.

Done.

[lvd, 2011-11-09 09:58:59]

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

gc: Better typechecks and errors in switches.

Allow any type in switch on interface value.
Statically check typeswitch early.

Fixes issue 2423.
Fixes issue 2424.

R=rsc, dsymonds
CC=golang-dev
http://codereview.appspot.com/5339045