[go] text/template: permit eq and ne funcs to check against nil
324 views
Skip to first unread message
Rob Pike (Gerrit)
Mar 13, 2022, 10:33:49 PM3/13/22
to Russ Cox, goph...@pubsubhelper.golang.org, Rob Pike, golang-co...@googlegroups.com
Attention is currently required from: Russ Cox.
Rob Pike would like Russ Cox to review this change.
text/template: permit eq and ne funcs to check against nil
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 #51642
Change-Id: Ic4072698c4910130ea7e3d76e7a148d8a8b88162
---
M src/html/template/exec_test.go
M src/text/template/exec_test.go
M src/text/template/funcs.go
3 files changed, 95 insertions(+), 37 deletions(-)
diff --git a/src/html/template/exec_test.go b/src/html/template/exec_test.go
index 6cf710e..f042cf5 100644
--- a/src/html/template/exec_test.go
+++ b/src/html/template/exec_test.go
@@ -1191,15 +1191,19 @@
{"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) {
@@ -1208,16 +1212,18 @@
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)
diff --git a/src/text/template/exec_test.go b/src/text/template/exec_test.go
index 8c81433..56566b9 100644
--- a/src/text/template/exec_test.go
+++ b/src/text/template/exec_test.go
@@ -1220,15 +1220,19 @@
{"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) {
@@ -1237,16 +1241,18 @@
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)
diff --git a/src/text/template/funcs.go b/src/text/template/funcs.go
index dca5ed2..4c53f47 100644
--- a/src/text/template/funcs.go
+++ b/src/text/template/funcs.go
@@ -436,14 +436,33 @@
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
}
@@ -479,11 +498,14 @@
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("cannot compare %s: %v to nil", arg1, arg1.Type())
}
truth = arg1.Interface() == arg.Interface()
}
To view, visit change 392274. To unsubscribe, or for help writing mail filters, visit settings.
Rob Pike (Gerrit)
Mar 13, 2022, 10:41:50 PM3/13/22
to Rob Pike, goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention is currently required from: Russ Cox.
Rob Pike uploaded patch set #2 to this change.
text/template: permit eq and ne funcs to check against nil
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 #51642
Change-Id: Ic4072698c4910130ea7e3d76e7a148d8a8b88162
---
M src/html/template/exec_test.go
M src/text/template/exec_test.go
M src/text/template/funcs.go
3 files changed, 95 insertions(+), 37 deletions(-)
To view, visit change 392274. To unsubscribe, or for help writing mail filters, visit settings.
mzh (Gerrit)
Mar 14, 2022, 2:27:14 AM3/14/22
to Rob Pike, goph...@pubsubhelper.golang.org, mzh, Russ Cox, golang-co...@googlegroups.com
Attention is currently required from: Rob Pike, Russ Cox.
1 comment:
File src/text/template/funcs.go:
Patch Set #2, Line 445:
case reflect.Chan, reflect.Func, reflect.Interface, reflect.Map, reflect.Pointer, reflect.Slice:Could we test slice, channel, interface, pointer and func too?
To view, visit change 392274. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Mar 20, 2022, 12:25:04 PM3/20/22
to Rob Pike, goph...@pubsubhelper.golang.org, Ian Lance Taylor, mzh, Russ Cox, golang-co...@googlegroups.com
Attention is currently required from: mzh, Rob Pike, Russ Cox.
2 comments:
File src/text/template/funcs.go:
Patch Set #2, Line 441:
if v == zero {You are being careful to handle the zero Value here, but in fact this function will never be called with the Zero value. The only call follows a call to canCompare, which will panic if called with the zero Value.
Patch Set #2, Line 445:
case reflect.Chan, reflect.Func, reflect.Interface, reflect.Map, reflect.Pointer, reflect.Slice:Could we test slice, channel, interface, pointer and func too?
Done
To view, visit change 392274. To unsubscribe, or for help writing mail filters, visit settings.
Rob Pike (Gerrit)
Mar 20, 2022, 5:17:58 PM3/20/22
to Rob Pike, goph...@pubsubhelper.golang.org, Ian Lance Taylor, mzh, Russ Cox, golang-co...@googlegroups.com
Attention is currently required from: mzh, Ian Lance Taylor, Russ Cox.
1 comment:
File src/text/template/funcs.go:
Patch Set #2, Line 441:
if v == zero {
You are being careful to handle the zero Value here, but in fact this function will never be called […]
future-proofing? i suspect a function called isNil might end up being called somewhere where that invariant is not guaranteed.
To view, visit change 392274. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Mar 20, 2022, 7:09:56 PM3/20/22
to Rob Pike, goph...@pubsubhelper.golang.org, Ian Lance Taylor, mzh, Russ Cox, golang-co...@googlegroups.com
Attention is currently required from: mzh, Rob Pike, Russ Cox.
Patch set 2:Run-TryBot +1Code-Review +2
Cherry Mui (Gerrit)
Apr 4, 2022, 12:54:28 PM4/4/22
to Rob Pike, goph...@pubsubhelper.golang.org, Gopher Robot, Ian Lance Taylor, mzh, Russ Cox, golang-co...@googlegroups.com
Attention is currently required from: mzh, Rob Pike, Russ Cox.
Patch set 2:Trust +1
Ian Lance Taylor (Gerrit)
Apr 4, 2022, 1:28:35 PM4/4/22
to Ian Lance Taylor, Rob Pike, goph...@pubsubhelper.golang.org, golang-...@googlegroups.com, Cherry Mui, Gopher Robot, mzh, Russ Cox, golang-co...@googlegroups.com
Ian Lance Taylor submitted this change.
Approvals:
Ian Lance Taylor: Looks good to me, approved; Run TryBots
Cherry Mui: Trusted
Gopher Robot: TryBots succeeded
text/template: permit eq and ne funcs to check against nil
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 #51642
Change-Id: Ic4072698c4910130ea7e3d76e7a148d8a8b88162
Reviewed-on: https://go-review.googlesource.com/c/go/+/392274
Reviewed-by: Ian Lance Taylor <ia...@golang.org>
Run-TryBot: Ian Lance Taylor <ia...@golang.org>
TryBot-Result: Gopher Robot <go...@golang.org>
Trust: Cherry Mui <cher...@google.com>
---
M src/html/template/exec_test.go
M src/text/template/exec_test.go
M src/text/template/funcs.go
3 files changed, 100 insertions(+), 37 deletions(-)
index dca5ed2..1f63b36 100644+ return false, fmt.Errorf("non-comparable type %s: %v", arg, arg.Type())
}
truth = arg1.Interface() == arg.Interface()
}
To view, visit change 392274. To unsubscribe, or for help writing mail filters, visit settings.
Reply all
Reply to author
Forward
0 new messages