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: SetMapIndex does not permit adding nil values of interface type #34898

Open
thallgren opened this issue Oct 14, 2019 · 7 comments
Open

reflect: SetMapIndex does not permit adding nil values of interface type #34898

thallgren opened this issue Oct 14, 2019 · 7 comments
Labels
Documentation NeedsInvestigation
Milestone

Comments

@thallgren
Copy link

@thallgren thallgren commented Oct 14, 2019

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

I'm executing this on the Go playground (currently at 1.13.1). Sample provided.

What did you do?

I tried to add an entry to a map of type map[string]interface{} using reflection. The behavior differs depending on if the actual value is an interface or a concrete value. There seem to be no way to actually add a nil value based on an interface type alone. Example can be executed here: https://play.golang.org/p/yfQo-wyP0B9

package main

import (
	"fmt"
	"reflect"
	"regexp"
)

func addAsHello(y interface{}) {
	m := map[string]interface{}{}

	// Add an entry for `hello` with the value nil value using reflection
	mr := reflect.ValueOf(m)
	mr.SetMapIndex(reflect.ValueOf(`hello`), reflect.ValueOf(y))
	fmt.Println(m)

	// Do the same without reflection
	m[`hello`] = y
	fmt.Println(m)
	fmt.Println()
}

func main() {
	addAsHello(nil)
	addAsHello(fmt.Stringer(nil))
	addAsHello((*regexp.Regexp)(nil))
}

What did you expect to see?

map[hello:<nil>]
map[hello:<nil>]

map[hello:<nil>]
map[hello:<nil>]

map[hello:<nil>]
map[hello:<nil>]

What did you see instead?

map[]
map[hello:<nil>]

map[]
map[hello:<nil>]

map[hello:<nil>]
map[hello:<nil>]
@go101
Copy link

@go101 go101 commented Oct 14, 2019

The docs says, if the second argument of a SetMapIndex call is a zero reflect.Value, then the call is equivalent to map deletion operation.

@go101
Copy link

@go101 go101 commented Oct 14, 2019

If you do want to add a nil element entry, you can do it like:

func addAsHello(y interface{}) {
	m := map[string]interface{}{}

	// Add an entry for `hello` with the value nil value using reflection
	mr := reflect.ValueOf(m)
	var element reflect.Value
	if y == nil {
		element = reflect.ValueOf(&y).Elem()
	} else {
		element = reflect.ValueOf(y)
	}
	mr.SetMapIndex(reflect.ValueOf(`hello`), element)
	fmt.Println(m)

	// Do the same without reflection
	m[`hello`] = y
	fmt.Println(m)
	fmt.Println()
}

@thallgren
Copy link
Author

@thallgren thallgren commented Oct 14, 2019

I've read the docs. I'm passing a nil value in all three calls and the behavior is inconsistent. Different nil's yield different results. Also, passing a zero value doesn't consistently delete. The zero value of a string is the empty string. But if I pass that to my sample function, it is added, not deleted.

Perhaps the documentation needs some clarification?

@randall77
Copy link
Contributor

@randall77 randall77 commented Oct 14, 2019

The docs of SetMapIndex don't mention nil. They mention the zero Value, which is a very specific thing. Read the docs of the Value type to see what that means.
See also what ValueOf says about nil and the zero Value.

I'm happy to entertain doc clarifications if you have suggestions. But any changes need to be consistent with how reflect is currently documented.

@go101
Copy link

@go101 go101 commented Oct 14, 2019

@thallgren
Only interface nils corresponds to zero/invalid reflect.Values. Non-interface nils, such as (*regexp.Regexp)(nil), are not nil interface values. For example print(interface{}(nil) == (*int)(nil)) print false.

@bradfitz bradfitz changed the title SetMapIndex does not permit adding nil values of interface type reflect: SetMapIndex does not permit adding nil values of interface type Oct 14, 2019
@thallgren
Copy link
Author

@thallgren thallgren commented Oct 14, 2019

Ok. I understand what I interpreted wrong in the docs. From the docs on reflect.Zero(typ Type) Value:

Zero returns a Value representing the zero value for the specified type. The result is different from the zero value of the Value struct, which represents no value at all.

I immediately understood this. The zero value of the Value struct will of course have a nil typ field whereas the zero value of any given type has a non-nil typ field. The documentation here is very clear and concise. The reflect.SetMapIndex(key, elem Value) docs however, is less so:

If elem is the zero Value, SetMapIndex deletes the key from the map

Admittedly, there's a capital V in Value here which should have pointed me in the right direction, but it would be far clearer if it actually stated the zero value of the Value struct here too. Especially since the concept of "zero value" is used throughout the documentation.

@randall77
Copy link
Contributor

@randall77 randall77 commented Oct 14, 2019

The reflect docs should use zero Value vs. zero value consistently (see Value.TryRecv for an example). Please point out errors if you see something wrong.

If we're going to fix any wording, we would want to fix it everywhere in the docs.

Instead of and/or in addition to "x is the zero Value" we could say "!x.IsValid()" or something (see the doc for Value.Kind). But it's not clear to me that it is worth the additional wordiness.

@julieqiu julieqiu added the NeedsInvestigation label Oct 15, 2019
@julieqiu julieqiu added this to the Unreleased milestone Oct 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation NeedsInvestigation
Projects
None yet
Development

No branches or pull requests

5 participants