go/tools: add vet check for direct comparion of reflect.Values
Comparing reflect.Values directly is almost certainly not what you want.
That compares reflect's internal representation, not the underlying
value it represents. One compares reflect.Values correctly by comparing
the results of .Interface() calls.
Fixes golang/go#43993
Update golang/go#18871
Change-Id: I6f441d920a5089bd346e5431032780403cefca76
Reviewed-on: https://go-review.googlesource.com/c/tools/+/308209
Trust: Keith Randall <khr@golang.org>
Trust: Joe Tsai <thebrokentoaster@gmail.com>
Run-TryBot: Keith Randall <khr@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
diff --git a/go/analysis/passes/reflectvaluecompare/reflectvaluecompare.go b/go/analysis/passes/reflectvaluecompare/reflectvaluecompare.go
new file mode 100644
index 0000000..ef21f0e
--- /dev/null
+++ b/go/analysis/passes/reflectvaluecompare/reflectvaluecompare.go
@@ -0,0 +1,99 @@
+// Copyright 2021 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// Package reflectvaluecompare defines an Analyzer that checks for accidentally
+// using == or reflect.DeepEqual to compare reflect.Value values.
+// See issues 43993 and 18871.
+package reflectvaluecompare
+
+import (
+ "go/ast"
+ "go/token"
+ "go/types"
+
+ "golang.org/x/tools/go/analysis"
+ "golang.org/x/tools/go/analysis/passes/inspect"
+ "golang.org/x/tools/go/ast/inspector"
+ "golang.org/x/tools/go/types/typeutil"
+)
+
+const Doc = `check for comparing reflect.Value values with == or reflect.DeepEqual
+
+The reflectvaluecompare checker looks for expressions of the form:
+
+ v1 == v2
+ v1 != v2
+ reflect.DeepEqual(v1, v2)
+
+where v1 or v2 are reflect.Values. Comparing reflect.Values directly
+is almost certainly not correct, as it compares the reflect package's
+internal representation, not the underlying value.
+Likely what is intended is:
+
+ v1.Interface() == v2.Interface()
+ v1.Interface() != v2.Interface()
+ reflect.DeepEqual(v1.Interface(), v2.Interface())
+`
+
+var Analyzer = &analysis.Analyzer{
+ Name: "reflectvaluecompare",
+ Doc: Doc,
+ Requires: []*analysis.Analyzer{inspect.Analyzer},
+ Run: run,
+}
+
+func run(pass *analysis.Pass) (interface{}, error) {
+ inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
+
+ nodeFilter := []ast.Node{
+ (*ast.BinaryExpr)(nil),
+ (*ast.CallExpr)(nil),
+ }
+ inspect.Preorder(nodeFilter, func(n ast.Node) {
+ switch n := n.(type) {
+ case *ast.BinaryExpr:
+ if n.Op != token.EQL && n.Op != token.NEQ {
+ return
+ }
+ if isReflectValue(pass, n.X) || isReflectValue(pass, n.Y) {
+ if n.Op == token.EQL {
+ pass.ReportRangef(n, "avoid using == with reflect.Value")
+ } else {
+ pass.ReportRangef(n, "avoid using != with reflect.Value")
+ }
+ }
+ case *ast.CallExpr:
+ fn, ok := typeutil.Callee(pass.TypesInfo, n).(*types.Func)
+ if !ok {
+ return
+ }
+ if fn.FullName() == "reflect.DeepEqual" && (isReflectValue(pass, n.Args[0]) || isReflectValue(pass, n.Args[1])) {
+ pass.ReportRangef(n, "avoid using reflect.DeepEqual with reflect.Value")
+ }
+ }
+ })
+ return nil, nil
+}
+
+// isReflectValue reports whether the type of e is reflect.Value.
+func isReflectValue(pass *analysis.Pass, e ast.Expr) bool {
+ tv, ok := pass.TypesInfo.Types[e]
+ if !ok { // no type info, something else is wrong
+ return false
+ }
+ // See if the type is reflect.Value
+ named, ok := tv.Type.(*types.Named)
+ if !ok {
+ return false
+ }
+ if obj := named.Obj(); obj == nil || obj.Pkg() == nil || obj.Pkg().Path() != "reflect" || obj.Name() != "Value" {
+ return false
+ }
+ if _, ok := e.(*ast.CompositeLit); ok {
+ // This is reflect.Value{}. Don't treat that as an error.
+ // Users should probably use x.IsValid() rather than x == reflect.Value{}, but the latter isn't wrong.
+ return false
+ }
+ return true
+}
diff --git a/go/analysis/passes/reflectvaluecompare/reflectvaluecompare_test.go b/go/analysis/passes/reflectvaluecompare/reflectvaluecompare_test.go
new file mode 100644
index 0000000..88fc8f2
--- /dev/null
+++ b/go/analysis/passes/reflectvaluecompare/reflectvaluecompare_test.go
@@ -0,0 +1,17 @@
+// Copyright 2021 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package reflectvaluecompare_test
+
+import (
+ "testing"
+
+ "golang.org/x/tools/go/analysis/analysistest"
+ "golang.org/x/tools/go/analysis/passes/reflectvaluecompare"
+)
+
+func TestReflectValueCompare(t *testing.T) {
+ testdata := analysistest.TestData()
+ analysistest.Run(t, testdata, reflectvaluecompare.Analyzer, "a")
+}
diff --git a/go/analysis/passes/reflectvaluecompare/testdata/src/a/a.go b/go/analysis/passes/reflectvaluecompare/testdata/src/a/a.go
new file mode 100644
index 0000000..e069c6d
--- /dev/null
+++ b/go/analysis/passes/reflectvaluecompare/testdata/src/a/a.go
@@ -0,0 +1,51 @@
+// Copyright 2021 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// This file contains tests for the reflectvaluecompare checker.
+
+package a
+
+import (
+ "reflect"
+)
+
+func f() {
+ var x, y reflect.Value
+ var a, b interface{}
+ _ = x == y // want `avoid using == with reflect.Value`
+ _ = x == a // want `avoid using == with reflect.Value`
+ _ = a == x // want `avoid using == with reflect.Value`
+ _ = a == b
+
+ // Comparing to reflect.Value{} is ok.
+ _ = a == reflect.Value{}
+ _ = reflect.Value{} == a
+ _ = reflect.Value{} == reflect.Value{}
+}
+func g() {
+ var x, y reflect.Value
+ var a, b interface{}
+ _ = x != y // want `avoid using != with reflect.Value`
+ _ = x != a // want `avoid using != with reflect.Value`
+ _ = a != x // want `avoid using != with reflect.Value`
+ _ = a != b
+
+ // Comparing to reflect.Value{} is ok.
+ _ = a != reflect.Value{}
+ _ = reflect.Value{} != a
+ _ = reflect.Value{} != reflect.Value{}
+}
+func h() {
+ var x, y reflect.Value
+ var a, b interface{}
+ reflect.DeepEqual(x, y) // want `avoid using reflect.DeepEqual with reflect.Value`
+ reflect.DeepEqual(x, a) // want `avoid using reflect.DeepEqual with reflect.Value`
+ reflect.DeepEqual(a, x) // want `avoid using reflect.DeepEqual with reflect.Value`
+ reflect.DeepEqual(a, b)
+
+ // Comparing to reflect.Value{} is ok.
+ reflect.DeepEqual(reflect.Value{}, a)
+ reflect.DeepEqual(a, reflect.Value{})
+ reflect.DeepEqual(reflect.Value{}, reflect.Value{})
+}
