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: index should return nil instead of index out of range error #14751

Closed
bep opened this issue Mar 10, 2016 · 8 comments
Closed

text/template: index should return nil instead of index out of range error #14751

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

Comments

@bep
Copy link
Contributor

bep commented Mar 10, 2016

Please answer these questions before submitting your issue. Thanks!

  1. What version of Go are you using (go version)?
go version go1.6 darwin/amd64
  1. What operating system and processor architecture are you using (go env)?
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/bep/go"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GO15VENDOREXPERIMENT="1"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fno-common"
CXX="clang++"
CGO_ENABLED="1"
  1. What did you do?
package main

import (
    "bytes"
    "fmt"
    "html/template"
)

func main() {
    var data struct {
        A map[string]interface{}
        B []interface{}
    }

    t, err := template.New("a").Parse(`{{ with index .A "a" }}{{ else }}Not Found{{ end }}`)

    if err != nil {
        fmt.Println(err)
        return
    }
    var b bytes.Buffer
    err = t.Execute(&b, data)

    if err != nil {
        fmt.Println(err)
        return
    }

    fmt.Println("A:", b.String())
    b.Reset()

    t, err = template.New("b").Parse(`{{ with index .B 0}}{{ else }}Not Found{{ end }}`)

    if err != nil {
        fmt.Println(err)
        return
    }

    err = t.Execute(&b, data)

    if err != nil {
        fmt.Println(err)
        return
    }

    fmt.Println("B:", b.String())

}
  1. What did you expect to see?
A: Not Found
B: Not Found
  1. What did you see instead?
A: Not Found
template: b:1:8: executing "b" at <index .B 0>: error calling index: index out of range: 0

I understand why I get this error and I expect arguments ala "you can fix this by doing proper length checking of the slice. Or you can provide your own implementation of the index template func".

Sure. But it isn't always obvious in a template if I interact with a map or a slice, and with DRY in mind, I would say that range checking should be done in one place: Inside the index template func.

And the answer to the question "is this really a problem?":

I'm one of the maintainers in Hugo, probably the project with the largest amount of end-user-provided Go templates, and the number one most frequent support question by a large margin, is: "How do I access that parameter/argument/data in a safe way?"

gohugoio/hugo#1949

@ianlancetaylor ianlancetaylor added this to the Go1.7 milestone Mar 10, 2016
@louwers
Copy link

louwers commented Apr 22, 2016

This is a huge pain indeed!
Is this supposed to work to check the length?

<li><a class="{{if (and (ge (len $.Path) 1) (eq (index $.Path 1) .))}}current{{end}}" href="courses/{{.}}/">{{.}}</a></li>

Edit: one off error, just further proves the point.
If you or anyone wrote a custom index function can be used in the meantime, it would be great if you could share it.

@bep
Copy link
Contributor Author

bep commented Apr 22, 2016

https://github.com/spf13/hugo/blob/master/tpl/template_funcs.go#L1460

@mdempsky mdempsky modified the milestones: Go1.8, Go1.7, Go1.8Maybe May 20, 2016
@quentinmit quentinmit added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Oct 10, 2016
@rsc
Copy link
Contributor

rsc commented Oct 17, 2016

The current behavior matches Go itself: a map lookup for a missing key returns a zero value; a slice lookup for an invalid index panics. I don't see a strong argument for diverging from the language here. Does the template package replace panics with nils in other cases?

Also, the test is not that hard. Instead of:

{{with index .B 0}}{{else}}Not Found{{end}}

you write:

{{if lt 0 (len .B)}}{{index .B 0}}{{else}}Not Found{{end}}

Eliminating the panic will no doubt break other templates. This seems OK as is.

/cc @robpike in case he disagrees.

@rsc rsc closed this as completed Oct 17, 2016
@bep
Copy link
Contributor Author

bep commented Oct 17, 2016

@rsc I don't buy your argument that "the current behavior matches Go itself".

The signature for index is:

index(item interface{}, indices ...interface{}) (interface{}, error)

It is a helper func full of reflection to make it usable in many situations (slices of anything, maps of anything). And when you live in a template world where everything is an interface{} I think it is up to the helper funcs to help avoiding a mess of nested conditionals.

You cannot write:

{{if lt 0 (len .B)}}{{index .B .C}}{{else}}Not Found{{end}}

Without knowing that .B is a slice and .C is an int.

index knows this and should be able to take advantage of that to help the end user.

@rsc
Copy link
Contributor

rsc commented Oct 17, 2016

What I mean is that {{index x y}} is behaving exactly like Go's x[y], and similarly {{index x y z}} is behaving like x[y][z] and so on.

You are arguing that index should diverge from Go's definition of those operations. As I said above:

I don't see a strong argument for diverging from the language here. Does the template package replace panics with nils in other cases?

@bep
Copy link
Contributor Author

bep commented Oct 17, 2016

Does the template package replace panics with nils in other cases?

There are plenty of cases where the template package avoids panics, which is a more relevant question:

if idx < 0 || idx + 1 > len(s)  {
   return nil // avoid panic!
}

But it is up to you. The workaround for people in the wild is to maintain forks of the index func.

@rsc
Copy link
Contributor

rsc commented Oct 17, 2016

@bep, please point to specific instances. I can't find the code snippet you posted in the text/template package nor the html/template package. In fact I can't find the string 'idx' in either of those at all.

@bep
Copy link
Contributor Author

bep commented Oct 18, 2016

@rsc it was just a general remark -- as I assume that html/template and text/template were full of slice bounds checking (I would even consider the for i := 0; i < len(mySlice) loop to fall into that category) to prevent panics, but if that is not the case I'm impressed.

@golang golang locked and limited conversation to collaborators Oct 18, 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

7 participants