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

proposal: reflect: reflect.ValueOf(nil).IsNil() panics; let's make it return true #51649

Open
robpike opened this issue Mar 14, 2022 · 18 comments
Open

Comments

@robpike
Copy link
Contributor

@robpike robpike commented Mar 14, 2022

The IsNil method of reflect.Value applies only to typed nils, so this program panics:

package main

import	"reflect"

func main() {
	x := reflect.ValueOf(nil)
	x.IsNil()
}

panic: reflect: call of reflect.Value.IsNil on zero Value

On the face of it, this is nuts: it says that nil is not nil. (What is this, a NaN? Just joking.) In fact, even asking if nil is nil causes the program to crash!

Every time I dig into uses of reflect, which would be rare except that I "own" several core packages that depend on reflection, I bounce off this. It just sits wrong with me that reflect.ValueOf(nil) gives the zero reflect.Value, but that is "invalid" not nil.

I propose, without thinking through the consequences in nearly enough detail, that either "invalid" becomes different from "value created from literal nil", or that we just change "invalid" to "nil" and allow it to satisfy IsNil.

I'm not sure this can be changed without causing a major ruckus, but I thought I'd at least ask. It might be easy and could clean up a fair bit of code in some places.

@gopherbot gopherbot added this to the Proposal milestone Mar 14, 2022
@mvdan
Copy link
Member

@mvdan mvdan commented Mar 14, 2022

change "invalid" to "nil" and allow it to satisfy IsNil.

I think this would be more likely to cause breakage, as I've seen the uses of the zero and invalid reflect.Value to signal "there is no value at all here - not even untyped nil". Here are some examples: https://cs.github.com/?scopeName=All+repos&scope=&q=%2Fvar+%5Cw%2B+reflect.Value%2F+language%3Ago

"invalid" becomes different from "value created from literal nil"

I think that's reasonable. If someone does want the invalid value, they can always do var invalid reflect.Value, and that's what I've seen in existing code so far. Unfortunately, it does seem like many pieces of code use reflect.ValueOf(nil): https://cs.github.com/?scopeName=All+repos&scope=&q=reflect.ValueOf%28nil%29+language%3Ago

@mvdan
Copy link
Member

@mvdan mvdan commented Mar 14, 2022

Here's a slightly better search for changing the zero value: Go files which do both var name reflect.Value and name.IsValid: https://cs.github.com/?scopeName=All+repos&scope=&q=%2Fvar+%5Cw%2B+reflect.Value%2F+%22.IsValid%22+language%3Ago

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Mar 14, 2022
@mvdan
Copy link
Member

@mvdan mvdan commented Mar 15, 2022

Thinking outloud: can this be one of those instances where we change the behavior of a Go API based on the go 1.X version that the module containing the source code declares? For example, with go 1.17, reflect.ValueOf(nil).IsValid() == false as today, but with a future go 1.19, reflect.ValueOf(nil).IsValid() == true. Presumably that gets us the best of both worlds: make the API behave in an obvious way for new code, but avoid breaking existing code.

That kind of backwards compatibility might get a bit complex when reflect.Values cross module boundaries, but it would still be better than changing the API behavior for all existing code at once.

@rsc rsc changed the title proposal: reflect: reflect.ValueOf(nil).IsNil() is false; let's make it true proposal: reflect: reflect.ValueOf(nil).IsNil() panics; let's make it return true Mar 16, 2022
@rsc
Copy link
Contributor

@rsc rsc commented Mar 16, 2022

FWIW I am skeptical that changing this would be helpful enough to mitigate the breakage it will cause. The behavior is clearly documented:

% go doc reflect.IsNil
package reflect // import "reflect"

func (v Value) IsNil() bool
    IsNil reports whether its argument v is nil. The argument must be a chan,
    func, interface, map, pointer, or slice value; if it is not, IsNil panics.
    Note that IsNil is not always equivalent to a regular comparison with nil in
    Go. For example, if v was created by calling ValueOf with an uninitialized
    interface variable i, i==nil will be true but v.IsNil will panic as v will
    be the zero Value.

% 

@rsc
Copy link
Contributor

@rsc rsc commented Mar 16, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Active in Proposals Mar 16, 2022
@robpike
Copy link
Contributor Author

@robpike robpike commented Mar 16, 2022

I understand that it is documented; my point is that what is documented is far from intuitive. It was perhaps a mistake to make the zero value represent invalid and nil both. But I also understand it may be hard to change.

I wonder, though, what would actually break if reflect.ValueOf(nil).IsNil() did not panic.

@go101
Copy link

@go101 go101 commented Mar 17, 2022

ValueOf returns a representation of the value boxed in the argument interface,
instead of the argument interface itself.
A nil interface boxes nothing. Maybe nothing should not be nil?

package main

import "reflect"

func main() {
	var v any
	y := reflect.ValueOf(&v).Elem()
	println(y.IsNil()) // true
}

@rogpeppe
Copy link
Contributor

@rogpeppe rogpeppe commented Mar 17, 2022

I wonder, though, what would actually break if reflect.ValueOf(nil).IsNil() did not panic.

That was my thought too.

I think it would probably be fine if:

  • reflect.ValueOf(nil) returns the zero value of Value (as it does now)
  • for the zero value of Value, IsNil returns true
  • for the zero value of Value, Type returns nil.

In general a nil Type represents "no type" in the same way that the zero Value represents "no value", and nil is the nearest we've got in Go to a generic way to represent "nothing", so IsNil returning true seems reasonable to me.

I wonder if there's any code at all that would break if this behaviour were to change.

@rsc
Copy link
Contributor

@rsc rsc commented Mar 23, 2022

The big problem here is that reflect is about typed values yet in many use cases people want some representation for untyped nil. And reflect.ValueOf(nil) returns the zero reflect.Value, which people use as that representation.

So suppose we make the following changes to support that (assume v is the zero reflect.Value):

  1. v.Type() == nil // untyped
  2. v.IsNil() == true // untyped nil
  3. v.IsZero() == true // any nil is a zero value, so probably the untyped nil is too

It's a little inconsistent but it will fix some code and probably won't break much, since all these panic today and code is written to avoid those panics.

Does anyone object to this?

@rogpeppe
Copy link
Contributor

@rogpeppe rogpeppe commented Mar 23, 2022

SGTM.

Note that Kind already works on the zero value, so there's definitely precedent for methods that don't panic other than IsValid.

@randall77
Copy link
Contributor

@randall77 randall77 commented Mar 23, 2022

I'm worried about the fact that if people start using reflect.Value{} for an untyped nil, then they are going to want to start using it more places, e.g. as an argument to reflect.Call. I think this would just push the confusion point elsewhere instead of solving it.

@robpike
Copy link
Contributor Author

@robpike robpike commented Mar 23, 2022

Maybe untyped nil could be a new Kind?

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 23, 2022

I guess the next question would whether it's OK to write

    reflect.Value{}.Convert(reflect.TypeOf(make(map[int]string)))

to get a Value whose value is nil and whose type is map[int]string. And then the one after that would be whether it's OK to write

    v.Set(reflect.Value{})

if v has a type that can be set to nil.

I also think it's worth noting that there is no way to describe an untyped integer in a reflect.Value. Should nil be different?

@rsc
Copy link
Contributor

@rsc rsc commented Apr 6, 2022

Untyped nil can't really be a new Kind because lots of code assumes what reflect.NewValue(nil) will return - the zero Value.

I agree that special-casing Convert and Set for zero Value representing nil would be the next logical extensions. It does give me some pause about heading down this road, especially since untyped integers don't have this kind of support.

@dsnet
Copy link
Member

@dsnet dsnet commented Apr 13, 2022

If we treat a zero Value as roughly equivalent to an untyped nil, then I propose the following comprehensive changes.

Changes for cases where v is the zero Value:

  • func (v Value) IsNil() bool

    Reports true. This address the original proposal.

  • func (v Value) IsValid() bool

    No change; returns false. Documentation will be updated to say that it reports whether v is a valid typed value.

  • func (v Value) Kind() Kind

    No change; returns 0 (which is the Invalid kind). We could create an alias renaming Invalid as UntypedNil.

  • func (v Value) IsZero() bool

    Reports true. Rationale: reflect.ValueOf([1]T{nil}).Index(0).IsZero reports true
    when T is a chan, func, interface, map, pointer, or slice kind.

  • func (v Value) Convert(t Type) Value

    Returns a typed nil value if t.Kind is a chan, func, interface, map, pointer, or slice.
    Rationale: T(nil) is valid Go where T is one of the above kinds.

  • func (v Value) CanConvert(t Type) bool

    Reports true if t.Kind is a chan, func, interface, map, pointer, or slice.

  • func (v Value) Interface() (i any)

    Returns nil. This is to preserve round-trip behavior such that ValueOf(nil).Interface() == nil

  • func (v Value) CanInterface() bool

    Reports true. This is to preserve round-trip behavior such that ValueOf(nil).Interface() == nil

  • func (v Value) UnsafePointer() unsafe.Pointer

    Returns nil. This change is debatable.

  • func (v Value) Pointer() uintptr

    Returns 0. This change is debatable.

  • func (v Value) String() string

    No change; this continues to return <invalid reflect.Value>.
    The reasonable alternative is to return nil or <nil>,
    but that will almost certainly break many tests that assume this never changes.

  • func (v Value) Type() Type

    No change; this continues to panic. The reasonable alternative is to return nil,
    but then we will need to figure out how Make and New functions interact with a nil Type.

  • func Indirect(v Value) Value

    No change; this already accepts a zero Value.

Changes for cases where v is not a zero Value:

  • func (v Value) Set(x Value)

    If v.CanSet and v.Kind is a chan, func, interface, map, pointer, or slice,
    and x is a zero Value, then this is equivalent to v.Set(Zero(v.Type())).
    Rationale: var x T = nil is valid Go where T is one of the above kinds.

    Note that v cannot be zero for the same reason that nil = x is not valid Go.

    This addresses #52310.

  • func Append(v Value, x ...Value) Value

    Assuming v is a valid slice and v.Elem.Kind is a chan, func, interface, map, pointer, or slice value,
    then elements of x may be a zero Value, in which case they are appended as typed nil values.
    Rationale: v = append(v, nil) is valid Go.

    Note that v cannot be zero for the same reason that v = append(nil) is not valid Go.

  • func AppendSlice(v, t Value) Value

    Assuming v is a valid slice, then t may be a zero Value, in which case v is returned as is.
    Rationale: v = append(v, nil...) is valid Go.

    Note that v cannot be zero for the same reason that v = append(nil) is not valid Go.

@rsc
Copy link
Contributor

@rsc rsc commented May 4, 2022

Thanks for that list @dsnet. It still seems a bit odd to me that we would put all this work into untyped nil when we don't have a similar answer for untyped 1. But untyped nil does seem to bite people a lot more than nil.

Does anyone want to prototype this? We could run it through our internal tests at Google to see how much might break.

@rsc
Copy link
Contributor

@rsc rsc commented May 18, 2022

Let's put this on hold for a prototype that we can evaluate against real programs.

@rsc rsc moved this from Active to Hold in Proposals May 18, 2022
@rsc
Copy link
Contributor

@rsc rsc commented May 18, 2022

Placed on hold.
— rsc for the proposal review group

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

No branches or pull requests

9 participants