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

Open
josharian opened this Issue Apr 13, 2019 · 16 comments

Comments

Projects
None yet
5 participants
@josharian
Copy link
Contributor

commented Apr 13, 2019

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2019

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

@elwinar

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2019

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

This comment has been minimized.

Copy link
Contributor

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.

@dsnet

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2019

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

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

Related: #31546

@josharian

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

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

@elwinar

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2019

In light of #31546, I understand :P .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.