text/template: calling a *T method on elements of []T: works with range, not with index

[bengalin]

1. What version of Go are you using (go version)?
   1.6
2. What operating system and processor architecture are you using (go env)?
   linux/amd64
3. What did you do?
   http://play.golang.org/p/5EinJM-QLi
   Consider a template where dot is set to a slice of (non-pointer) values.
   If I {{range}} on dot, I can call pointer-receiver methods on each element. However, if I {{index}} on dot, then pointer-receiver methods cause the template execution to fail with a "PointerReceiver is not a field of struct type main.someStruct" error.
4. What did you expect to see?
   Pointer-receiver method works on {{index}}ed elements just as it does on {{range}}d ones.
   It has been suggested to me that might be "a bug in the template package: for method calls on slice elements, it should probably be using v.Index(i).Addr().Call([...]) rather than v.Index(i).Call([...])"
5. What did you see instead?
   See error above.

[jessfraz]

This is text/template too just FYI.

[rsc]

@robpike:

Suppose dot is a []T with len==1 and there is a (*T) M() method (pointer receiver).
Today, {{range .}} {{.M}} {{end}} works but {{with index . 0}} {{.M}} {{end}} fails to find the method.

The problem is that the implementation of range ends up trying

x := reflect.ValueOf(slice).Index(0)
x.MethodByName("M")

while the use of index induces a conversion from reflect.Value to interface{} and back, so

x := reflect.ValueOf(slice).Index(0)
x = reflect.ValueOf(x.Interface())
x.MethodByName("M")

That round trip makes x not addressable, so MethodByName cannot do an implicit address-of, so it cannot get at the pointer-receiver method M.

The options I see are:

1. Leave everything alone and live with the inconsistency.
2. Define that registered template functions can take reflect.Values as arguments and return them as results, with the effect that it is possible to bypass the lossy round trip to interface{}. This is similar to how fmt.Printf treats printing reflect.ValueOf(x) the same as printing x. Then index could be changed to work on reflect.Values and bypass the round trip issue. Then {{with index . 0}} would work identically to {{range .}} (assuming slice of length 1). Templates that formerly failed could start working.
3. Change range to do the round trip as well, bringing it in line with index by breaking the method lookup. Templates that formerly worked could stop working.

Here is the diff for 2 (and then the obvious changes to func index to make it take and return reflect.Value instead of interface{}):

diff --git a/src/text/template/exec.go b/src/text/template/exec.go
index 8e5ad93..2af447f 100644
--- a/src/text/template/exec.go
+++ b/src/text/template/exec.go
@@ -661,7 +661,11 @@ func (s *state) evalCall(dot, fun reflect.Value, node parse.Node, name string, a
        s.at(node)
        s.errorf("error calling %s: %s", name, result[1].Interface().(error))
    }
-   return result[0]
+   v := result[0]
+   if v.Type() == reflect.TypeOf(reflect.Value{}) {
+       v = v.Interface().(reflect.Value)
+   }
+   return v
 }

 // canBeNil reports whether an untyped nil can be assigned to the type. See reflect.Zero.
@@ -682,6 +686,9 @@ func (s *state) validateType(value reflect.Value, typ reflect.Type) reflect.Valu
        }
        s.errorf("invalid value; expected %s", typ)
    }
+   if typ == reflect.TypeOf(reflect.Value{}) && value.Type() != typ {
+       return reflect.ValueOf(value)
+   }
    if typ != nil && !value.Type().AssignableTo(typ) {
        if value.Kind() == reflect.Interface && !value.IsNil() {
            value = value.Elem()
@@ -743,6 +750,10 @@ func (s *state) evalArg(dot reflect.Value, typ reflect.Type, n parse.Node) refle
        if typ.NumMethod() == 0 {
            return s.evalEmptyInterface(dot, n)
        }
+   case reflect.Struct:
+       if typ == reflect.TypeOf(reflect.Value{}) {
+           return reflect.ValueOf(s.evalEmptyInterface(dot, n))
+       }
    case reflect.String:
        return s.evalString(typ, n)
    case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr:

Here is the diff for 3:

diff --git a/src/text/template/exec.go b/src/text/template/exec.go
index 8e5ad93..2b306b9 100644
--- a/src/text/template/exec.go
+++ b/src/text/template/exec.go
@@ -327,7 +327,7 @@ func (s *state) walkRange(dot reflect.Value, r *parse.RangeNode) {
            break
        }
        for i := 0; i < val.Len(); i++ {
-           oneIteration(reflect.ValueOf(i), val.Index(i))
+           oneIteration(reflect.ValueOf(i), reflect.ValueOf(val.Index(i).Interface()))
        }
        return
    case reflect.Map:

Thoughts?

[robpike]

Makes sense to me. I have trouble reading diffs without context like this the but the theory behind option 2 sounds good and the excerpts seem OK. Please make a CL.

[gopherbot]

CL https://golang.org/cl/31462 mentions this issue.