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

Jump to bottom

text/template: incorrect range syntax is accepted by the parser #28437

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

text/template: incorrect range syntax is accepted by the parser #28437

bep opened this issue Oct 27, 2018 · 8 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.12

Comments

@bep
Copy link
Contributor

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

mvdan commented Oct 27, 2018 •
edited

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 the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 27, 2018
@mvdan mvdan added this to the Go1.12 milestone Oct 27, 2018
@bep
Copy link
Contributor Author

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

mvdan 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.

@mvdan mvdan added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Oct 27, 2018
@bep
Copy link
Contributor Author

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

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

mvdan commented Oct 28, 2018 •
edited

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 The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. 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
Copy link

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

@mvdan
Copy link
Member

mvdan 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 :)

@golang golang locked and limited conversation to collaborators Oct 28, 2019
@rsc rsc unassigned mvdan Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.12
Development

No branches or pull requests
3 participants
@bep @mvdan @gopherbot