Skip to content
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: Value docs and Value.Set panic message are not perfect #29701

Closed
go101 opened this issue Jan 12, 2019 · 8 comments
Closed

reflect: Value docs and Value.Set panic message are not perfect #29701

go101 opened this issue Jan 12, 2019 · 8 comments

Comments

@go101
Copy link

go101 commented Jan 12, 2019

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

go version go1.12beta2 linux/amd64

What did you do?

package main

import "reflect"
import "fmt"

func main() {
	var z reflect.Value
	fmt.Println(z) // <invalid reflect.Value>
}

What did you expect to see?

<invalid value> is better than <invalid reflect.Value>.

All reflect.Value values are valid. It is just that the values represented by some reflect.Value values are invalid.

[update]: the intention of this issue has changed. Now it requests:

  1. correct the docs of reflect.Value to

The zero Value represents no value. Its IsValid method returns false, its Kind method returns Invalid, its String method returns "<invalid reflect.Value>"

2. use a better panic message for invalid reflect.Value.Set argument (Not right)

see #29701 (comment) for details.

=======

The design for the string representation of a valid reflect.Value which represents another reflect.Value value is like the form [the-type-of-the-value-represented-by-the-Value-represented-by-the-Value Value] is a little weird, but it is not a big problem I think. It is not weird when you get it.

@ianlancetaylor
Copy link
Contributor

Making this change at this point is sure to break some existing tests, and the benefit seems quite small.

@go101
Copy link
Author

go101 commented Jan 12, 2019

Agree.

Another interesting find is the string representation of a zero reflect.Value may be different:

package main

import "reflect"
import "fmt"

func main() {
	var z reflect.Value // a zero Value value
	var v = reflect.ValueOf(&z).Elem()
	fmt.Println(v) // <invalid Value>
	fmt.Println(z) // <invalid reflect.Value>
}

@go101
Copy link
Author

go101 commented Jan 12, 2019

It looks the docs of reflect.Value clearly states

The zero Value represents no value. Its IsValid method returns false, its Kind method returns Invalid, its String method returns "<invalid Value>"

@go101
Copy link
Author

go101 commented Jan 12, 2019

Ah, in the last code example, the v and z really represent different values. The v is reflect.Value value which represents another reflect.Value value, whereas z represents no value.

However, I think the docs may really need to be corrected.

@go101
Copy link
Author

go101 commented Jan 12, 2019

[edit]: sorry, my old understanding for the string representations was wrong.
It looks the string representation of a valid reflect.Value is like
the form [the-type-of-the-value-represented-by-the-Value-represented-by-the-Value Value] if the value represented by a reflect.Value value is also a reflect.Value value.

I also think the string representation of v is wrong. It should be <reflect.Value Value>, same as the string representation of y in the following example:

package main

import "reflect"
import "fmt"

func main() {
	var z reflect.Value // a zero Value value
	var v = reflect.ValueOf(&z).Elem()
	fmt.Println(v) // <invalid Value>
	fmt.Println(z) // <invalid reflect.Value>
	fmt.Println(v.IsValid()) // true
	
	var y = reflect.ValueOf(&v).Elem()
	fmt.Println(y) // <reflect.Value Value>
}

@go101
Copy link
Author

go101 commented Jan 12, 2019

More:

package main

import "reflect"
import "fmt"

func main() {
	var z reflect.Value // a zero Value value
	var w = reflect.ValueOf(&z)
	
	var y = reflect.ValueOf(&w).Elem()
	fmt.Println(y.Kind()) // struct
	
	var v = w.Elem()
	fmt.Println(v.Kind()) // struct
	fmt.Println(v) // <invalid Value>
	fmt.Println(v.IsValid()) // true
	
	// The two lines should be both panic or both not.
	v.Set(v) // ok
	v.Set(z) // panic: reflect: call of reflect.Value.Set on zero Value
}

[edit]: sorry, the behavior of the program is okay. But the error message is misleading. argument of reflect.Value.Set should be valid would be better, for on zero Value makes people think the receiver argument is zero.

@go101
Copy link
Author

go101 commented Jan 12, 2019

Sorry, updated my last two comments.

@go101 go101 changed the title reflect: the string representation of zero reflect.Value value is not good reflect: Value docs and Value.Set panic message are not perfect Jan 12, 2019
@go101
Copy link
Author

go101 commented Mar 4, 2019

closed for some misunderstanding on printing reflect.Value values.

@go101 go101 closed this as completed Mar 4, 2019
@golang golang locked and limited conversation to collaborators Mar 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants