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: incorrect range syntax is accepted by the parser #28437

Closed
bep opened this issue Oct 27, 2018 · 8 comments

Comments

Projects
None yet
3 participants
@bep
Copy link
Contributor

commented Oct 27, 2018

Note: This isn't me suggesting that you should fix this. I have had some people reporting broken Hugo sites because of this, and I was "I don't see how that can have worked" until the third person arrived with the same issue.

The program below works on Go 1.10, fails in Go 1.11:

go/bep/temp  master ✗                                                                                                                                                                                                                  9m ⚑
▶ go version && go run main.go
go version go1.11.1 darwin/amd64
2018/10/27 14:02:36 template: :2:13: executing "" at <.abc>: undefined variable: $v
exit status 1

go/bep/temp  master ✗                                                                                                                                                                                                                10m ⚑  ⍉
▶ go1.10.4 version && go1.10.4 run main.go
go version go1.10.4 darwin/amd64
abc
package main

import (
	"bytes"
	"fmt"
	"log"
	"strings"
	"text/template"
)

func main() {
	data := map[string]interface{}{
		"abc": []string{"a", "b", "c"},
	}
	tpl := `
{{ range $v, .abc }}{{ $v }}{{ end }}
`

	var buf bytes.Buffer
	tmpl, err := template.New("").Parse(tpl)
	if err != nil {
		log.Fatal(err)
	}

	if err := tmpl.Execute(&buf, data); err != nil {
		log.Fatal(err)
	}

	result := strings.TrimSpace(buf.String())

	fmt.Println(result)

}

https://play.golang.org/p/5xrifxQFHQd

@mvdan

This comment has been minimized.

Copy link
Member

commented Oct 27, 2018

Minified repro: https://play.golang.org/p/266xq0CRTIY

Without digging into this, it seems like broken syntax to me. I presume that's why you don't necessarily think that this is a bug in 1.11.

Do you have another reason to call this a regression, besides it having worked on 1.10? For example, did it work on many previous Go versions, or does the syntax make sense following any previous text/template docs?

@mvdan mvdan added this to the Go1.12 milestone Oct 27, 2018

@bep

This comment has been minimized.

Copy link
Contributor Author

commented Oct 27, 2018

Without digging into this, it seems like broken syntax to me. I presume that's why you don't necessarily think that this is a bug in 1.11.

Yes, but it depends on the definition of "a bug". It has broken some apps/sites that worked before Go 1.11, but the error message is loud and clear, which is good.

@mvdan

This comment has been minimized.

Copy link
Member

commented Oct 27, 2018

Yes - agreed that this isn't a clear case. Given that the template doesn't make sense, appears to be undocumented behavior, and errors out in an obvious way, I'm leaning towards leaving things the way they are.

It would be a different scenario if templates were breaking in silent or hard to debug ways, or if the template made some sense syntactically.

If you think we should do something, do you have anything in mind besides reverting to 1.10's behavior? If we did that, we'd likely have to document this syntax and add regression tests for it, which I presume is not something we want to do.

/cc @robpike for a second opinion.

@bep

This comment has been minimized.

Copy link
Contributor Author

commented Oct 27, 2018

If you think we should do something, do you have anything in mind besides reverting to 1.10's behavior?

I think you should just leave it as is. This issue can serve as documentation. Some people have found an "accidental feature" that is for most people (and according to documentation) obvious wrong syntax. I created this issue out of curiousness more than anything.

@mvdan

This comment has been minimized.

Copy link
Member

commented Oct 28, 2018

I think you should just leave it as is. This issue can serve as documentation.

Agreed. I'm curious as to exactly what CL fixed this weird behavior, and I'd like to add a regression test, so I'll leave this issue open until I have a look at that.

@mvdan mvdan self-assigned this Oct 28, 2018

@mvdan mvdan changed the title text/template: Regression from 1.10 to 1.11 in range variable assignment text/template: weird range syntax broken from 1.10 to 1.11 Oct 28, 2018

@mvdan

This comment has been minimized.

Copy link
Member

commented Oct 28, 2018

This actually is a bug. text/template/parse should be rejecting that syntax, but isn't. The only reason why 1.11 is erroring when executing the template is because the new assignment logic gets confused by the lack of :=, and tries to assign to an existing variable $v, which has not been declared.

This is a subtle syntax parser bug that has existed for a while. Both 1.10 and 1.11 parse the template with no errors, which is incorrect. Will send a fix shortly, which should mean that the error happens sooner and has a clearer message.

@mvdan mvdan added NeedsFix and removed NeedsDecision labels Oct 28, 2018

@mvdan mvdan changed the title text/template: weird range syntax broken from 1.10 to 1.11 text/template: incorrect range syntax is accepted by the parser Oct 28, 2018

@gopherbot

This comment has been minimized.

Copy link

commented Oct 28, 2018

Change https://golang.org/cl/145282 mentions this issue: text/template/parse: error on bad range variables

@mvdan

This comment has been minimized.

Copy link
Member

commented Oct 28, 2018

@bep please take a look at the CL above, and feel free to give it a try. With my minified program using template.Must, now I get:

 $ go run f.go
panic: template: :1: range can only initialize variables

goroutine 1 [running]:
text/template.Must(...)
        /home/mvdan/tip/src/text/template/helper.go:23
main.main()
        /home/mvdan/f.go:12 +0x27a
exit status 2

I don't know if you're on Gerrit, but reviews are very much welcome :)

@gopherbot gopherbot closed this in 1399b52 Oct 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.