New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reflect: inconsistent results between reflection and non-reflection #29469

Open
go101 opened this Issue Dec 30, 2018 · 12 comments

Comments

Projects
None yet
3 participants
@go101
Copy link

go101 commented Dec 30, 2018

What version of Go are you using (go version)?

go version go1.11.3 linux/amd64

Does this issue reproduce with the latest release?

yes

What did you do?

package main

import "reflect"

var _ = reflect.ValueOf

func main() {
	type C chan int
	type C1 chan<- int
	type C2 <-chan int
	var c C
	_ = chan<- int(c)   // ok
	_ = (<-chan int)(c) // ok
	//_ = C1(c)         // error: 
	//_ = C2(c)         // error: 
	var c1a chan<- int
	var c2a <-chan int
	var c1b C1
	var c2b C2
	
	var tc1a = reflect.TypeOf(c1a)
	var tc2a = reflect.TypeOf(c2a)
	var tc1b = reflect.TypeOf(c1b)
	var tc2b = reflect.TypeOf(c2b)
	
	var t = reflect.TypeOf(c)
	println(t.ConvertibleTo(tc1a)) // true
	println(t.ConvertibleTo(tc2a)) // true
	println(t.ConvertibleTo(tc1b)) // true, but the above corresponding line compiles error
	println(t.ConvertibleTo(tc2b)) // true, but the above corresponding line compiles error
}

Another bug is mentioned a below comment: #29469 (comment)

@go101

This comment has been minimized.

Copy link

go101 commented Dec 30, 2018

@ianlancetaylor same problem for gccgo.

@go101

This comment has been minimized.

Copy link

go101 commented Dec 30, 2018

It looks, Go spec really doesn't cover the error conversion cases.

@go101

This comment has been minimized.

Copy link

go101 commented Dec 30, 2018

I think the restrictions are unnecessary. After all, the restricted conversions can be achieved in indirect ways.

package main

func main() {
	type C chan int
	type C1 chan<- int
	type C2 <-chan int
	var c C
	//_ = C1(c)  // error: 
	//_ = C2(c)  // error: 
	_ = C1(chan<- int(c))   // ok
	_ = C2((<-chan int)(c)) // ok
}
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

ianlancetaylor commented Dec 30, 2018

I'm not sure I understand why the conversions C1(c) and C2(c) are reported as errors. CC @griesemer

@ianlancetaylor ianlancetaylor added this to the Go1.13 milestone Dec 30, 2018

@ALTree ALTree changed the title reflect: inconsistent results between refection and non-refelction reflect: inconsistent results between reflection and non-reflection Dec 30, 2018

@griesemer

This comment has been minimized.

Copy link
Contributor

griesemer commented Dec 30, 2018

The conversions C1(c) and C2(c) are simply not allowed by the existing conversion rules (a c cannot be assigned to a C1 because they are both defined types which are different; and there's no other special rule about channel conversions). I think this just falls out from a straight-forward implementation of the existing rules.

The conversions via chan<- int and <-chan int work because there assignability permits it.

I don't know why reflect's ConvertibleTo doesn't follow the spec rules.

As is, there are no restrictions here; it's just that there are no special cases for channel conversions (maybe there should be; or maybe the rules can be simplified/streamlined for a more consistent experience).

@go101

This comment has been minimized.

Copy link

go101 commented Jan 13, 2019

I tend to think this is a bug of the reflect package now, for such conversion needs in practice are rare, and there is a similar case in which values of two types can't be converted to each other directly but can indirectly.

package main

func main() {
	type MyInt int
	type IntPtr *int
	type MyIntPtr *MyInt

	var pi = new(int)
	var ip IntPtr = pi

	// var _ = MyIntPtr(ip) // can't convert directly
	var _ = MyIntPtr((*MyInt)((*int)(ip))) // can indirectly
}
@griesemer

This comment has been minimized.

Copy link
Contributor

griesemer commented Jan 14, 2019

@go101 We know that indirect conversions are allowed in the language. But the issue remains as to why direct conversions which are invalid in the language do work via reflection.

@go101

This comment has been minimized.

Copy link

go101 commented Jan 14, 2019

It looks the haveIdenticalUnderlyingType function is not implemented correctly.
A whether or not one is non-defined condition is missing.

func haveIdenticalUnderlyingType(T, V *rtype, cmpTags bool) bool {
...

	// Composite types.
	switch kind {
...

	case Chan:
		// Special case:
		// x is a bidirectional channel value, T is a channel type,
		// and x's type V and T have identical element types.
		if V.ChanDir() == BothDir && haveIdenticalType(T.Elem(), V.Elem(), cmpTags) {
			return true // !!! here is wrong. Need check whether or not one is non-defined.
		}

		// Otherwise continue test for identical underlying type.
		return V.ChanDir() == T.ChanDir() && haveIdenticalType(T.Elem(), V.Elem(), cmpTags)
@go101

This comment has been minimized.

Copy link

go101 commented Jan 14, 2019

And it looks the code forgets to check the case by exchanging the roles of the two input types. (sorry, this is not true, my mistake.)

package main

import "reflect"

var _ = reflect.ValueOf

func main() {
	type C chan int
	type C1 chan<- int
	type C2 <-chan int

	var c C
	var c1b C1
	var c2b C2

	var t = reflect.TypeOf(c)
	var tc1b = reflect.TypeOf(c1b)
	var tc2b = reflect.TypeOf(c2b)

	println(t.ConvertibleTo(tc1b)) // true
	println(t.ConvertibleTo(tc2b)) // true
	println(tc1b.ConvertibleTo(t)) // false
	println(tc2b.ConvertibleTo(t)) // false
}
@go101

This comment has been minimized.

Copy link

go101 commented Jan 14, 2019

sorry, looks my last comment is a shit. :)

@go101

This comment has been minimized.

Copy link

go101 commented Jan 15, 2019

Another bug:

package main

import "reflect"

func main() {
	type C chan int
	
	var x *C
	var y *chan<- int
	
	//y = x                 // error
	//x = y                 // error
	//y = (*chan<- int)(x)  // error
	//x = (*C)(y)           // error
	
	println(reflect.TypeOf(x).AssignableTo(reflect.TypeOf(y)))  // false
	println(reflect.TypeOf(y).AssignableTo(reflect.TypeOf(x)))  // false
	println(reflect.TypeOf(x).ConvertibleTo(reflect.TypeOf(y))) // true (bug)
	println(reflect.TypeOf(y).ConvertibleTo(reflect.TypeOf(x))) // false
}
@go101

This comment has been minimized.

Copy link

go101 commented Jan 15, 2019

The cause of the new bug is the functionality of the haveIdenticalUnderlyingType function in reflect package is not exact as its name hints.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment