Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
bengalin opened this issue Mar 22, 2016 · 4 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@bengalin
Copy link

  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.
@ianlancetaylor ianlancetaylor added this to the Go1.7 milestone Mar 22, 2016
@mdempsky mdempsky modified the milestones: Go1.8, Go1.7 May 20, 2016
@jessfraz
Copy link
Contributor

This is text/template too just FYI.

@quentinmit quentinmit added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 7, 2016
@rsc
Copy link
Contributor

rsc commented Oct 19, 2016

@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?

@rsc rsc changed the title html/template: calling a *T method on elements of []T: works with range, not with index text/template: calling a *T method on elements of []T: works with range, not with index Oct 19, 2016
@rsc rsc added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed NeedsFix The path to resolution is known, but the work has not been done. labels Oct 19, 2016
@robpike
Copy link
Contributor

robpike commented Oct 19, 2016

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
Copy link

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

@golang golang locked and limited conversation to collaborators Oct 19, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

8 participants