text/template: improve nil errors in evalField

If we're accessing a field on a nil struct pointer, and that field is
present in the type, we should print a "nil pointer evaluating X.Y" error
instead of the broader "can't evaluate field Y in X". The latter error
should still be used for the cases where the field is simply missing.

While at it, remove the isNil checks in the struct and map cases. The
indirect func will only return a true isNil when returning a pointer or
interface reflect.Value, so it's impossible for either of these checks
to be useful.

Finally, extend the test suite to test a handful of these edge cases,
including the one shown in the original issue.

Fixes #29137.

Change-Id: I53408ced8a7b53807a0a8461b6baef1cd01d25ae
Reviewed-on: https://go-review.googlesource.com/c/153341
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rob Pike <r@golang.org>

diff --git a/src/text/template/exec.go b/src/text/template/exec.go
index d34d2484411edeba3e3ddf40c40b3af919665a1c..964bb87cdac11d5756a17b1561b84421ea67adce 100644 (file)
--- a/src/text/template/exec.go
+++ b/src/text/template/exec.go
@@ -591,9 +591,6 @@ func (s *state) evalField(dot reflect.Value, fieldName string, node parse.Node,
        case reflect.Struct:
                tField, ok := receiver.Type().FieldByName(fieldName)
                if ok {
-                       if isNil {
-                               s.errorf("nil pointer evaluating %s.%s", typ, fieldName)
-                       }
                        field := receiver.FieldByIndex(tField.Index)
                        if tField.PkgPath != "" { // field is unexported
                                s.errorf("%s is an unexported field of struct type %s", fieldName, typ)
@@ -605,9 +602,6 @@ func (s *state) evalField(dot reflect.Value, fieldName string, node parse.Node,
                        return field
                }
        case reflect.Map:
-               if isNil {
-                       s.errorf("nil pointer evaluating %s.%s", typ, fieldName)
-               }
                // If it's a map, attempt to use the field name as a key.
                nameVal := reflect.ValueOf(fieldName)
                if nameVal.Type().AssignableTo(receiver.Type().Key()) {
@@ -627,6 +621,18 @@ func (s *state) evalField(dot reflect.Value, fieldName string, node parse.Node,
                        }
                        return result
                }
+       case reflect.Ptr:
+               etyp := receiver.Type().Elem()
+               if etyp.Kind() == reflect.Struct {
+                       if _, ok := etyp.FieldByName(fieldName); !ok {
+                               // If there's no such field, say "can't evaluate"
+                               // instead of "nil pointer evaluating".
+                               break
+                       }
+               }
+               if isNil {
+                       s.errorf("nil pointer evaluating %s.%s", typ, fieldName)
+               }
        }
        s.errorf("can't evaluate field %s in type %s", fieldName, typ)
        panic("not reached")
@@ -899,7 +905,9 @@ func (s *state) evalEmptyInterface(dot reflect.Value, n parse.Node) reflect.Valu
        panic("not reached")
 }
 
-// indirect returns the item at the end of indirection, and a bool to indicate if it's nil.
+// indirect returns the item at the end of indirection, and a bool to indicate
+// if it's nil. If the returned bool is true, the returned value's kind will be
+// either a pointer or interface.
 func indirect(v reflect.Value) (rv reflect.Value, isNil bool) {
        for ; v.Kind() == reflect.Ptr || v.Kind() == reflect.Interface; v = v.Elem() {
                if v.IsNil() {

diff --git a/src/text/template/exec_test.go b/src/text/template/exec_test.go
index 6cdb285bd8e51242fa23dd7ad7a1e0375c6779ef..5947e3ec63c00be4a74449710b2c3db32b0f3667 100644 (file)
--- a/src/text/template/exec_test.go
+++ b/src/text/template/exec_test.go
@@ -1349,20 +1349,64 @@ func TestBlock(t *testing.T) {
        }
 }
 
-// Check that calling an invalid field on nil pointer prints
-// a field error instead of a distracting nil pointer error.
-// https://golang.org/issue/15125
-func TestMissingFieldOnNil(t *testing.T) {
-       tmpl := Must(New("tmpl").Parse("{{.MissingField}}"))
-       var d *T
-       err := tmpl.Execute(ioutil.Discard, d)
-       got := "<nil>"
-       if err != nil {
-               got = err.Error()
+func TestEvalFieldErrors(t *testing.T) {
+       tests := []struct {
+               name, src string
+               value     interface{}
+               want      string
+       }{
+               {
+                       // Check that calling an invalid field on nil pointer
+                       // prints a field error instead of a distracting nil
+                       // pointer error. https://golang.org/issue/15125
+                       "MissingFieldOnNil",
+                       "{{.MissingField}}",
+                       (*T)(nil),
+                       "can't evaluate field MissingField in type *template.T",
+               },
+               {
+                       "MissingFieldOnNonNil",
+                       "{{.MissingField}}",
+                       &T{},
+                       "can't evaluate field MissingField in type *template.T",
+               },
+               {
+                       "ExistingFieldOnNil",
+                       "{{.X}}",
+                       (*T)(nil),
+                       "nil pointer evaluating *template.T.X",
+               },
+               {
+                       "MissingKeyOnNilMap",
+                       "{{.MissingKey}}",
+                       (*map[string]string)(nil),
+                       "nil pointer evaluating *map[string]string.MissingKey",
+               },
+               {
+                       "MissingKeyOnNilMapPtr",
+                       "{{.MissingKey}}",
+                       (*map[string]string)(nil),
+                       "nil pointer evaluating *map[string]string.MissingKey",
+               },
+               {
+                       "MissingKeyOnMapPtrToNil",
+                       "{{.MissingKey}}",
+                       &map[string]string{},
+                       "<nil>",
+               },
        }
-       want := "can't evaluate field MissingField in type *template.T"
-       if !strings.HasSuffix(got, want) {
-               t.Errorf("got error %q, want %q", got, want)
+       for _, tc := range tests {
+               t.Run(tc.name, func(t *testing.T) {
+                       tmpl := Must(New("tmpl").Parse(tc.src))
+                       err := tmpl.Execute(ioutil.Discard, tc.value)
+                       got := "<nil>"
+                       if err != nil {
+                               got = err.Error()
+                       }
+                       if !strings.HasSuffix(got, tc.want) {
+                               t.Fatalf("got error %q, want %q", got, tc.want)
+                       }
+               })
        }
 }