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

cmd/vet: warn about using reflect.DeepEqual and == on reflect.Value #43993

Open
randall77 opened this issue Jan 29, 2021 · 31 comments
Open

cmd/vet: warn about using reflect.DeepEqual and == on reflect.Value #43993

randall77 opened this issue Jan 29, 2021 · 31 comments

Comments

@randall77
Copy link
Contributor

@randall77 randall77 commented Jan 29, 2021

This code doesn't do what you think it does:

func f(x, y reflect.Value) bool {
    return reflect.DeepEqual(x, y)
}

It compares the internal details of the two reflect.Value structs, not their contents. The correct code would be reflect.DeepEqual(x.Interface(), y.Interface()).

I don't see any reason why you'd ever want to use DeepEqual on a reflect.Value. Hence no false positives.

If you really wanted to compare two reflect.Values (but you shouldn't), == is equivalent to DeepEqual.

Seems like an easy mistake to make (see #43986 ). Not sure how common it might be, but I suspect it might be common enough to warrant a check.

@gopherbot gopherbot added this to the Proposal milestone Jan 29, 2021
@randall77
Copy link
Contributor Author

@randall77 randall77 commented Jan 29, 2021

A few minutes of searching github turned up a few instances.

Both of these look like very confused code, where this vet check might at least point someone in the right direction:

https://github.com/hedzr/assert/blob/e76ed4c3b339a79c3b788bc3fdf4605511203e14/equal.go lines 106-110

https://github.com/cuirixin/phoenix_corelib/blob/fb4035076004c2eca44d4840d390191e052a4d7b/libs/gotransformer/gotransformer_test.go line 19, 37, ...

This one is probably someone who hit this bug, fixed their code, and left a comment about it:

https://github.com/coreos/vcontext/blob/ee043618d38dc1daf804fe352433b5ce2428ea32/validate/validate_test.go lines 212-221

Loading

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Jan 29, 2021
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jan 29, 2021

Another example (now fixed) at go-openapi/validate#137.

Loading

@dominikh
Copy link
Member

@dominikh dominikh commented Jan 30, 2021

Related: #18871

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Feb 10, 2021

Perhaps we should define that when you pass a reflect.Value to DeepEqual, DeepEqual uses that directly instead of calling reflect.ValueOf on it. Then instead of warning about broken code we just make it not broken.

Loading

@randall77
Copy link
Contributor Author

@randall77 randall77 commented Feb 10, 2021

That would work in my example case. Not sure what the right behavior is for, e.g., a struct with a reflect.Value-typed field.

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Feb 17, 2021

I agree about not applying that to reflect.Value-typed fields, but vet was not going to catch those either.

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Feb 17, 2021

We have some history here where fmt.Print interprets a reflect.Value by using what it contains.

case reflect.Value:
	// Handle extractable values with special methods
	// since printValue does not handle them at depth 0.
	if f.IsValid() && f.CanInterface() {
		p.arg = f.Interface()
		if p.handleMethods(verb) {
			return
		}
	}
	p.printValue(f, verb, 0)

@ianlancetaylor says he ran into this recently due to a Go 1.16 internal change and it was not at all clear given the call (like in Keith's example at the top) that there was even a problem. So we should do either the vet change or the "make it work" change.

I'm leaning toward "make it work".

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Feb 17, 2021

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

Loading

@rsc rsc moved this from Incoming to Active in Proposals Feb 17, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Feb 24, 2021

Does anyone object to just making DeepEqual handle reflect.Value arguments by interpreting what the Values represent?
(That is, it would avoid calling reflect.ValueOf on those arguments and then proceed as before.)

Loading

@go101
Copy link

@go101 go101 commented Feb 25, 2021

avoid calling reflect.ValueOf

typo? avoid calling .Interface()?

Loading

@go101
Copy link

@go101 go101 commented Feb 25, 2021

Does it apply to reflect.Value values with interfaces as underlying values?
And what about the underlying values of the underlying interface values are also reflect.Value values,
And ...

Loading

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Feb 25, 2021

avoid calling reflect.ValueOf

typo? avoid calling .Interface()?

It's not a typo. The current code is

    v1 := ValueOf(x)
    v2 := ValueOf(y)

The suggestion is to change that to something like

    var v1, v2 Value
    xv, xok := x.(Value)
    yv, yok := y.(Value)
    if xok && yok {
        v1 = xv
        v2 = xv
    } else {
        v1 = ValueOf(x)
        v2 = ValueOf(y)
    }

Loading

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Feb 25, 2021

Does it apply to reflect.Value values with interfaces as underlying values?

No.

And what about the underlying values of the underlying interface values are also reflect.Value values,

No.

And ...

No.

Loading

@go101
Copy link

@go101 go101 commented Feb 25, 2021

OK, I misinterpreted it affects user code. It affects std lib code actually.

But is it required both of the arguments are Value? Should it be like

    var v1, v2 Value
    if xv, xok := x.(Value); xok {
        v1 = xv
    } else {
        v1 = ValueOf(x)
    }
    if yv, yok := y.(Value); yok {
        v2 = yv
    } else {
        v2 = ValueOf(y)
    }

Loading

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Feb 25, 2021

Good question.

Loading

@bcmills
Copy link
Member

@bcmills bcmills commented Feb 25, 2021

IMO reflect.Value should be treated as analogous to a pointer type. We treat &x as semantically different from x,¹ so we should also treat reflect.ValueOf(x) as semantically different from x itself.

Loading

@liggitt
Copy link
Contributor

@liggitt liggitt commented Mar 4, 2021

@ianlancetaylor says he ran into this recently due to a Go 1.16 internal change

Yeah, we just hit this as well with the following code in a validation library that was attempting to check zero value:

reflect.DeepEqual(reflect.Zero(reflect.TypeOf(data)), reflect.ValueOf(data))

As it turns out, what it intended to do with a zero-value condition was incorrect, but the incorrect branch was never triggered before.

We'll deal with fixing that issue in that particular library, but is it acceptable that the following code has a different result in go1.16 than it did in all prior go versions?

func main() {
	data := ""
	fmt.Println(reflect.DeepEqual(reflect.Zero(reflect.TypeOf(data)), reflect.ValueOf(data)))
}

Looks like https://go-review.googlesource.com/c/go/+/192331 was likely the relevant change

Loading

@randall77
Copy link
Contributor Author

@randall77 randall77 commented Mar 4, 2021

@liggitt
Yes, it is unfortunate that it changed in 1.16 but the code is relying on an implementation detail of reflect.Value.
Kind of like &struct{} == &struct{}, it all depends on whether the implementation dedups two zero values into identical reflect.Values, or just two equivalent ones.

// To compare two Values, compare the results of the Interface method.
// Using == on two Values does not compare the underlying values
// they represent.

== is the same as DeepEqual in this situation (which is what this issue is proposing changing).

Loading

@liggitt
Copy link
Contributor

@liggitt liggitt commented Mar 5, 2021

Still probably worth a mention in the 1.16 release notes, since the externally visible behavior of reflect.Zero changed. Highlighting problematic comparisons of reflect.Values that would be affected would have prompted us to audit for that.

Loading

@randall77
Copy link
Contributor Author

@randall77 randall77 commented Mar 5, 2021

Still probably worth a mention in the 1.16 release notes, since the externally visible behavior of reflect.Zero changed.

It's only visible because of violations of reflect.Value's contract. Not sure what you'd put in the release notes about that. Have a suggestion?

Highlighting problematic comparisons of reflect.Values that would be affected would have prompted us to audit for that.

This issue (and #18871) was originally about highlighting problematic comparisons, hopefully more robustly than a release note would. i think we'll just end up fixing the behavior to do the right thing instead.

Loading

@liggitt
Copy link
Contributor

@liggitt liggitt commented Mar 5, 2021

It's only visible because of violations of reflect.Value's contract. Not sure what you'd put in the release notes about that. Have a suggestion?

Something like what was described in https://golang.org/doc/go1.10#reflect, describing the change made to the reflect package and describing potential impact for callers relying on incorrect or unspecified behavior. Perhaps something like:

The Zero function has been optimized to avoid allocations in many scenarios. Code which incorrectly compares the returned Value to another Value item using == or DeepEqual can now evaluate to true, where previous versions of go always evaluated to false. To compare two Values, compare the results of the Interface method. Using == or DeepEqual on two Values does not compare the underlying values they represent.

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 11, 2021

Change https://golang.org/cl/300992 mentions this issue: [release-branch.go1.16] doc: describe how Zero optimization might change results of incorrect reflect.Value comparison

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 16, 2021

Change https://golang.org/cl/302269 mentions this issue: doc: describe how Zero optimization might change results of incorrect reflect.Value comparison

Loading

gopherbot pushed a commit to golang/website that referenced this issue Mar 17, 2021
… reflect.Value comparison

Update golang/go#33136
Update golang/go#43993

Change-Id: I306a8fd60a4d58cfd338edea4f21690338bf9a0b
Reviewed-on: https://go-review.googlesource.com/c/website/+/302269
Trust: Keith Randall <khr@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@rsc
Copy link
Contributor

@rsc rsc commented Mar 24, 2021

Given that everyone on Go 1.16 is already broken and we're not hearing a big uproar, it seems like a vet check and keeping the ValueOf semantics simpler is a better path forward.

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Mar 24, 2021

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

Loading

@rsc rsc moved this from Active to Likely Accept in Proposals Mar 24, 2021
@randall77
Copy link
Contributor Author

@randall77 randall77 commented Mar 26, 2021

What about also flagging use of == on reflect.Values? That would fix #18871 but avoid the main objection there, which was use of reflect.Value as a map key for sets of reflect.Values.

We'd have to allow comparing to the zero Value (aka reflect.Value{}) as Dominik noted.

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Mar 31, 2021

It seems OK to put the == check in as well, with an exception for reflect.Value{}.
If it breaks too much, we can always back it out.
Will leave this in likely accept for another week.

Loading

@rsc rsc changed the title proposal: cmd/vet: warn about using reflect.DeepEqual on reflect.Value proposal: cmd/vet: warn about using reflect.DeepEqual and == on reflect.Value Apr 7, 2021
@rsc rsc moved this from Likely Accept to Accepted in Proposals Apr 7, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Apr 7, 2021

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

Loading

@rsc rsc changed the title proposal: cmd/vet: warn about using reflect.DeepEqual and == on reflect.Value cmd/vet: warn about using reflect.DeepEqual and == on reflect.Value Apr 7, 2021
@rsc rsc removed this from the Proposal milestone Apr 7, 2021
@rsc rsc added this to the Backlog milestone Apr 7, 2021
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 7, 2021

Change https://golang.org/cl/308209 mentions this issue: go/tools: add vet check for direct comparion of reflect.Values

Loading

@gopherbot gopherbot closed this in b261fe9 Apr 8, 2021
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 9, 2021

Change https://golang.org/cl/308769 mentions this issue: text/template: compare reflect.Value instances differently

Loading

@randall77
Copy link
Contributor Author

@randall77 randall77 commented Apr 9, 2021

Unfortunately, handling == runs up against some code in the stdlib. There are 3 kinds of false positives:

  1. zero := reflect.Value{}; x == zero
    We can handle x == reflect.Value{}, but with the above we'd have to detect that there are no other writes to the zero variable to squash the report. That's difficult. (in text/template)
  2. var singleton = reflect.ValueOf(...a value of some otherwise unused type...) ; if x == singleton
    Code builds a guaranteed-unique reflect.Value by using a unique type that's used nowhere else, and uses that Value as a special marker. (in text/template)
  3. v.Elem().Elem() == v
    When descending though a data structure using reflection, we sometimes want to know if we've reached a place where we've been before. (in encoding/json)

These false positives seem like they may be common enough to warrant rolling back the == part of this proposal.

I think maybe we do just DeepEqual. Cases 3 at least make no sense if done with DeepEqual. Case 2 also seems unlikely. Case 1 could come up using DeepEqual, I guess, but the fix is easy for case 1: just use IsValid instead.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Accepted
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
10 participants