Skip to content

Commit

Permalink
text/template: permit eq and ne funcs to check against nil
Browse files Browse the repository at this point in the history 
The existing code errors out immediately if the argument is not
"comparable", making it impossible to test a slice, map, and so
on from being compared to nil.

Fix by delaying the "comparable" error check until we encounter
an actual check between two non-comparable, non-nil values.

Note for the future: reflect makes it unnecessarily clumsy
to deal with nil values in cases like this. For instance, it
should be possible to check if a value is nil without stepping
around a panic. See the new functions isNil and canCompare
for my (too expensive) workaround.

Fixes golang#51642

Change-Id: Ic4072698c4910130ea7e3d76e7a148d8a8b88162
Reviewed-on: https://go-review.googlesource.com/c/go/+/392274
Reviewed-by: Ian Lance Taylor <[email protected]>
Run-TryBot: Ian Lance Taylor <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Trust: Cherry Mui <[email protected]>
  • Loading branch information
@robpike @ianlancetaylor
robpike authored and ianlancetaylor committed Apr 4, 2022
1 parent ac31352 commit c58f1bb
Show file tree
Hide file tree
Showing 3 changed files with 71 additions and 37 deletions.
34 changes: 20 additions & 14 deletions src/html/template/exec_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1191,15 +1191,19 @@ var cmpTests = []cmpTest{
{"eq .Iface1 .Iface1", "true", true},
{"eq .Iface1 .Iface2", "false", true},
{"eq .Iface2 .Iface2", "true", true},
{"eq .Map .Map", "true", true}, // Uncomparable types but nil is OK.
{"eq .Map nil", "true", true}, // Uncomparable types but nil is OK.
{"eq nil .Map", "true", true}, // Uncomparable types but nil is OK.
{"eq .Map .NonNilMap", "false", true}, // Uncomparable types but nil is OK.
// Errors
{"eq `xy` 1", "", false}, // Different types.
{"eq 2 2.0", "", false}, // Different types.
{"lt true true", "", false}, // Unordered types.
{"lt 1+0i 1+0i", "", false}, // Unordered types.
{"eq .Ptr 1", "", false}, // Incompatible types.
{"eq .Ptr .NegOne", "", false}, // Incompatible types.
{"eq .Map .Map", "", false}, // Uncomparable types.
{"eq .Map .V1", "", false}, // Uncomparable types.
{"eq `xy` 1", "", false}, // Different types.
{"eq 2 2.0", "", false}, // Different types.
{"lt true true", "", false}, // Unordered types.
{"lt 1+0i 1+0i", "", false}, // Unordered types.
{"eq .Ptr 1", "", false}, // Incompatible types.
{"eq .Ptr .NegOne", "", false}, // Incompatible types.
{"eq .Map .V1", "", false}, // Uncomparable types.
{"eq .NonNilMap .NonNilMap", "", false}, // Uncomparable types.
}

func TestComparison(t *testing.T) {
Expand All @@ -1208,16 +1212,18 @@ func TestComparison(t *testing.T) {
Uthree, Ufour uint
NegOne, Three int
Ptr, NilPtr *int
NonNilMap map[int]int
Map map[int]int
V1, V2 V
Iface1, Iface2 fmt.Stringer
}{
Uthree: 3,
Ufour: 4,
NegOne: -1,
Three: 3,
Ptr: new(int),
Iface1: b,
Uthree: 3,
Ufour: 4,
NegOne: -1,
Three: 3,
Ptr: new(int),
NonNilMap: make(map[int]int),
Iface1: b,
}
for _, test := range cmpTests {
text := fmt.Sprintf("{{if %s}}true{{else}}false{{end}}", test.expr)
Expand Down
34 changes: 20 additions & 14 deletions src/text/template/exec_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1220,15 +1220,19 @@ var cmpTests = []cmpTest{
{"eq .NilIface .Iface1", "false", true},
{"eq .NilIface 0", "false", true},
{"eq 0 .NilIface", "false", true},
{"eq .Map .Map", "true", true}, // Uncomparable types but nil is OK.
{"eq .Map nil", "true", true}, // Uncomparable types but nil is OK.
{"eq nil .Map", "true", true}, // Uncomparable types but nil is OK.
{"eq .Map .NonNilMap", "false", true}, // Uncomparable types but nil is OK.
// Errors
{"eq `xy` 1", "", false}, // Different types.
{"eq 2 2.0", "", false}, // Different types.
{"lt true true", "", false}, // Unordered types.
{"lt 1+0i 1+0i", "", false}, // Unordered types.
{"eq .Ptr 1", "", false}, // Incompatible types.
{"eq .Ptr .NegOne", "", false}, // Incompatible types.
{"eq .Map .Map", "", false}, // Uncomparable types.
{"eq .Map .V1", "", false}, // Uncomparable types.
{"eq `xy` 1", "", false}, // Different types.
{"eq 2 2.0", "", false}, // Different types.
{"lt true true", "", false}, // Unordered types.
{"lt 1+0i 1+0i", "", false}, // Unordered types.
{"eq .Ptr 1", "", false}, // Incompatible types.
{"eq .Ptr .NegOne", "", false}, // Incompatible types.
{"eq .Map .V1", "", false}, // Uncomparable types.
{"eq .NonNilMap .NonNilMap", "", false}, // Uncomparable types.
}

func TestComparison(t *testing.T) {
Expand All @@ -1237,16 +1241,18 @@ func TestComparison(t *testing.T) {
Uthree, Ufour uint
NegOne, Three int
Ptr, NilPtr *int
NonNilMap map[int]int
Map map[int]int
V1, V2 V
Iface1, NilIface fmt.Stringer
}{
Uthree: 3,
Ufour: 4,
NegOne: -1,
Three: 3,
Ptr: new(int),
Iface1: b,
Uthree: 3,
Ufour: 4,
NegOne: -1,
Three: 3,
Ptr: new(int),
NonNilMap: make(map[int]int),
Iface1: b,
}
for _, test := range cmpTests {
text := fmt.Sprintf("{{if %s}}true{{else}}false{{end}}", test.expr)
Expand Down
40 changes: 31 additions & 9 deletions src/text/template/funcs.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -436,14 +436,33 @@ func basicKind(v reflect.Value) (kind, error) {
return invalidKind, errBadComparisonType
}

// isNil returns true if v is the zero reflect.Value, or nil of its type.
func isNil(v reflect.Value) bool {
if v == zero {
return true
}
switch v.Kind() {
case reflect.Chan, reflect.Func, reflect.Interface, reflect.Map, reflect.Pointer, reflect.Slice:
return v.IsNil()
}
return false
}

// canCompare reports whether v1 and v2 are both the same kind, or one is nil.
// Called only when dealing with nillable types, or there's about to be an error.
func canCompare(v1, v2 reflect.Value) bool {
k1 := v1.Kind()
k2 := v2.Kind()
if k1 == k2 {
return true
}
// We know the type can be compared to nil.
return k1 == reflect.Invalid || k2 == reflect.Invalid
}

// eq evaluates the comparison a == b || a == c || ...
func eq(arg1 reflect.Value, arg2 ...reflect.Value) (bool, error) {
arg1 = indirectInterface(arg1)
if arg1 != zero {
if t1 := arg1.Type(); !t1.Comparable() {
return false, fmt.Errorf("uncomparable type %s: %v", t1, arg1)
}
}
if len(arg2) == 0 {
return false, errNoComparison
}
Expand Down Expand Up @@ -479,11 +498,14 @@ func eq(arg1 reflect.Value, arg2 ...reflect.Value) (bool, error) {
case uintKind:
truth = arg1.Uint() == arg.Uint()
default:
if arg == zero || arg1 == zero {
truth = arg1 == arg
if !canCompare(arg1, arg) {
return false, fmt.Errorf("non-comparable types %s: %v, %s: %v", arg1, arg1.Type(), arg.Type(), arg)
}
if isNil(arg1) || isNil(arg) {
truth = isNil(arg) == isNil(arg1)
} else {
if t2 := arg.Type(); !t2.Comparable() {
return false, fmt.Errorf("uncomparable type %s: %v", t2, arg)
if !arg.Type().Comparable() {
return false, fmt.Errorf("non-comparable type %s: %v", arg, arg.Type())
}
truth = arg1.Interface() == arg.Interface()
}
Expand Down

0 comments on commit c58f1bb

Please sign in to comment.