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: ignore blank fields in IsZero #31450

Closed
josharian opened this issue Apr 13, 2019 · 19 comments
Closed

reflect: ignore blank fields in IsZero #31450

josharian opened this issue Apr 13, 2019 · 19 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. release-blocker
Milestone

Comments

@josharian
Copy link
Contributor

I believe that the newly added reflect.IsZero (https://go-review.googlesource.com/c/go/+/171337) might need to skip struct fields named “_”. It is not possible without unsafe (or maybe reflect?) to construct a struct value with a non-zero value for a _ field. Nevertheless, I think that IsZero should skip such fields; cmd/compile’s generated equality and hash functions do.

cc @elwinar @ianlancetaylor @bradfitz

@dsnet dsnet added this to the Go1.13 milestone Apr 13, 2019
@dsnet
Copy link
Member

dsnet commented Apr 13, 2019

It seems that reflect allows accessing an _ field for read-only purposes (i.e., can't call the Interface method, but can call Int, String, etc.). Thus, it seems that IsZero probably does not need any special handling for _ fields.

https://play.golang.org/p/6SQjEHzHyrI

@elwinar
Copy link
Contributor

elwinar commented Apr 13, 2019

I don't think we need do to anything about this. If the _ field has a zero value, then the struct will too, and if it doesn't have a zero value, then the struct doesn't either. And equality with a zero value doesn't guarantee being a zero value, as you can see for floats: a -0.0 == 0.0, but IsZero(-0.0) == false, so the fact that equality operations skip it isn't a concern.

@josharian
Copy link
Contributor Author

And equality with a zero value doesn't guarantee being a zero value, as you can see for floats

This is the thing that strikes me as uncomfortable. And floats are generally pretty weird, so that doesn't seem very exculpatory.

I guess I can see the argument either way. It is just unfortunate that being the zero value is not the same as being equal to the zero value. I guess the relevant question is what people are more likely to actually want, in practice.

At a minimum we should have a sentence of documentation about this, because the distinction is subtle. And if we do decide for the "equal to" interpretation of what this function should do, then we should change floats to match (i.e. remove the math.Float64bits from the implementation).

@josharian
Copy link
Contributor Author

cc @rsc since x/s/owners lists him as an owner for reflect

@elwinar
Copy link
Contributor

elwinar commented Apr 14, 2019

I would say that not skipping _ fields does follow to the letter the definition of a zero value, and this is the actual reason why we check the sign bit for float and complex kinds.

The main use case I had to write a function like this was for not overriding user-defined fields in unmarshal-like methods. In which case, both ways are fine.

I would say that not adding an exception like this is always better if not strictly necessary.

@josharian
Copy link
Contributor Author

I would say that not adding an exception like this is always better if not strictly necessary.

This is not about adding an exception. It is about asking whether the function should be about "is the zero value" or "is equal to the zero value", and then adjusting the implementation to match.

@elwinar
Copy link
Contributor

elwinar commented Apr 15, 2019

Well, the function is named IsZero, and being a strict equality rather than a loose one is a conscious choice: my first implementation was loose, and a pair of comments asked me to do a stricter check (namely checking the sign of floats and complexes).

Furthermore, checking for a loose equality is easy enough to do without this function, one can just take a real zero value and use to it to check equality.

@bradfitz bradfitz added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Apr 15, 2019
@dsnet
Copy link
Member

dsnet commented Apr 15, 2019

I looked at the equivalent usages of reflect.IsZero that I was aware of and they either didn't care about the exact definition, or did want IsZero to be the strict definition.

It could be argued that two reasonable definitions of IsZero might suggest that reflect shouldn't have such a function.

@dsnet dsnet changed the title reflect: handle blank fields in IsZero reflect: ignore blank fields in IsZero Apr 15, 2019
@elwinar
Copy link
Contributor

elwinar commented Apr 15, 2019

I think we should rather have more guidance in the doc than no method for that.

The fact that multiple people ended up writing such a function suggest that there isn't a simple and reliable way to do it and that there is a need for it.
I doubt that people able to write such a function wouldn't know if one existed. But I can very well imagine one forgetting about negative zero floats.

To be clear: I think it's better to add a comment along the lines IsZero checks for identity with the zero value of v's type. This is not equivalent to an equality., and maybe an example demonstrating the difference between the two, although there is very few.

@ianlancetaylor
Copy link
Contributor

I think we should continue to check _ fields in struct types, and we should document IsZero to make clear that we are testing exactly whether the value is the zero value of the type, not whether it is equal to the result of reflect.Zero. We can already test whether a reflect.Value is equal to the zero value of the type by writing v.Interface() == reflect.Zero(v.Type()).Interface().

Though perhaps the subtlety of this discussion would be a reason to not add IsZero in the first place.

@bradfitz
Copy link
Contributor

I was curious about the history here so dig some digging. A bit but not much here...

#3031

That's the bug that led to the current spec wording.

I also found that this (https://play.golang.org/p/L3vQvIhncwZ):

package main

import (
	"fmt"
)

type T struct{ a, _, c int }

func main() {
	t := T{1, 2, 3}
	fmt.Println(t)
}

... prints {1 0 3}, which was news to me at least.

@elwinar
Copy link
Contributor

elwinar commented Apr 17, 2019

I've tried setting the value of a blank field field, and only an unsafe.Pointer works (https://play.golang.org/p/sawqZxZz0SN).

I've also added tests in my branch to check IsZero's behavior:

diff --git a/src/reflect/all_test.go b/src/reflect/all_test.go
index 964d8c6e95..2e5d3e7519 100644
--- a/src/reflect/all_test.go
+++ b/src/reflect/all_test.go
@@ -1136,6 +1136,10 @@ func TestIsZero(t *testing.T) {
                // Structs
                {T{}, true},
                {T{123, 456.75, "hello", &_i}, false},
+               {struct{ _ int }{}, true},
+               {struct{ _ int }{0}, true},
+               {struct{ _ int }{2}, true},
+               {*(*struct{ _ int })(unsafe.Pointer(&struct{ i int }{2})), false},
                // UnsafePointer
                {(unsafe.Pointer)(nil), true},
                {(unsafe.Pointer)(new(int)), false},

Here, the case that don't seems obvious to me is the third one rather that the fourth. Ignoring the blank field would change the fourth behavior only, but the actual implementation conforms to IsZero

In light of this, I'm still in favor of checking the blank field and adding a comment. Or even a blog post detailing the issue and the choices made. (If you don't, I'll do it :P I've discovered plenty of things while doing this, and if this can encourage more people to start contributing, all the better.)

@elwinar
Copy link
Contributor

elwinar commented Apr 18, 2019

So, anyone to take a decision here? I can add the said documentation if we reach a consensus. @ianlancetaylor @josharian @dsnet @bradfitz

@bradfitz
Copy link
Contributor

Related: #31546

@josharian
Copy link
Contributor Author

@elwinar these kinds of decisions take time, research, and thought.

@elwinar
Copy link
Contributor

elwinar commented Apr 19, 2019

In light of #31546, I understand :P .

@rsc
Copy link
Contributor

rsc commented May 1, 2019

Discussed at proposal review.

Now that #31546 is fixed, nothing should be able to tell whether we look at blank fields or not: the blank fields are never written. The only way this comes up is if package unsafe is involved, whether from C or doing other shenanigans. If you explicitly go out of your way to scribble a non-zero value into a blank field, it does seem like reflect.IsZero should say "that's not the zero value". It isn't, just like float64 negative zero is not the zero value.

The compiler-generated hash/eq functions are implementing the == operation. IsZero is not "== zero value". It is "is the zero value" (cf float negative zero again).

Let's leave IsZero as is.

@rsc rsc closed this as completed May 1, 2019
@randall77
Copy link
Contributor

Not to object after closing, but consider this code:

type T struct {
	x int16
	_ int16
	y int32
}

func f() {
	t := T{x: 12, y: 7}
	g(&t)
}

//go:noescape
func g(t *T)

t is allocated on the stack. Does f need to clear the _ field? I don't think we ever had to before, but now we do, in case g calls reflect.IsZero.

It's kind of a moot point at the moment, because we currently do zero _ fields of objects allocated on the stack. So perhaps my objection is just "if we implement a compiler optimization removing zeroing _ fields, then we would also need to modify reflect.IsZero to ignore _ fields".

The situation described above already happens for padding. The compiler does not initialize alignment padding bytes in structs, and thus reflect.IsZero needs to not check that the padding bytes are zero. (Same example as above, but remove the _ field.)

@gopherbot
Copy link

Change https://golang.org/cl/182617 mentions this issue: reflect,doc: use "the" instead of "a" in IsZero docs

gopherbot pushed a commit that referenced this issue Jun 18, 2019
There is a subtle distinction between a value
*being* the zero value vs being *equal to* the zero value.
This was discussed at length in #31450.

Using "a zero value" in the docs suggests that there may
be more than zero value. That is possible on the "equal to
zero value" reading, but not the "is zero" reading that we
selected for the semantics of IsZero.

This change attempts to prevent any confusion on this front by
switching to "the zero value" in the documentation.

And while we're here, eliminate a double-space.
(Darn macbook keyboards.)

Change-Id: Iaa02ba297438793f5a90be9919a4d53baef92f8e
Reviewed-on: https://go-review.googlesource.com/c/go/+/182617
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@golang golang locked and limited conversation to collaborators Jun 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. release-blocker
Projects
None yet
Development

No branches or pull requests

8 participants