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

html/template: behaviour of HTML templates changed between 1.7.5 and 1.8 #19294

Closed
seehuhn opened this issue Feb 26, 2017 · 17 comments
Closed

html/template: behaviour of HTML templates changed between 1.7.5 and 1.8 #19294

seehuhn opened this issue Feb 26, 2017 · 17 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@seehuhn
Copy link

seehuhn commented Feb 26, 2017

What did you do?

When I use the html/template package I get non-deterministic output with Go version 1.8. The same program, when run with Go version 1.7.5 always gives the same output.

A minimal "working" example is here:
https://play.golang.org/p/2vVYZkiTFx

The program parses and executes the same template 100 times and counts how often it gets which output. On Go 1.8 I get results like

89

--- 11 ---

What did you expect to see?

On Go 1.7.5 I always get the same output, namely the following:

100

---

What did you see instead?

see above

System details

go version go1.8 darwin/amd64
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/voss/go-external:/Users/voss/go"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="/usr/bin/gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/var/folders/2g/mfxxbx6d3yl_hqdh8b9j1wmm0000gn/T/go-build654454144=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="/usr/bin/clang++"
CGO_ENABLED="1"
PKG_CONFIG="pkg-config"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
GOROOT/bin/go version: go version go1.8 darwin/amd64
GOROOT/bin/go tool compile -V: compile version go1.8 X:framepointer
uname -v: Darwin Kernel Version 16.4.0: Thu Dec 22 22:53:21 PST 2016; root:xnu-3789.41.3~3/RELEASE_X86_64
ProductName:	Mac OS X
ProductVersion:	10.12.3
BuildVersion:	16D32
lldb --version: lldb-360.1.70
gdb --version: GNU gdb (GDB) 7.12.1
@seehuhn
Copy link
Author

seehuhn commented Feb 26, 2017

If I change {{block "stylesheets" .}}{{end -}} to {{template "stylesheets" . -}} in my example, the problem goes away. Thanks to Владимир Мулько on golang-nuts for this suggestion!

@bradfitz bradfitz changed the title behaviour of HTML templates changed between 1.7.5 and 1.8 html/template: behaviour of HTML templates changed between 1.7.5 and 1.8 Feb 26, 2017
@bradfitz bradfitz added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 26, 2017
@bradfitz
Copy link
Contributor

/cc @rsc @robpike ... who owns templates these days?

@robpike
Copy link
Contributor

robpike commented Feb 26, 2017

This is almost certainly a bug, and a mysterious one at that. Since it's a bug in text/template (the example uses html/template but the problem exists in the lower-level library) I can look at it.

@robpike robpike self-assigned this Feb 26, 2017
@bradfitz bradfitz added this to the Go1.8.1 milestone Feb 26, 2017
@robpike
Copy link
Contributor

robpike commented Feb 26, 2017

Reports on the mailing list say this one broke it:

commit 2b583a190eb14c69bffe5d488d2d6d3862fe76ea
Author: Jess Frazelle <me@jessfraz.com>
Date:   Wed Jun 22 21:57:52 2016 -0700

    text/template: fix Parse when called twice with empty text
    
    Fixes #16156
    
    Change-Id: I6989db4fd392583a2d490339cefc525b07c11b90
    Reviewed-on: https://go-review.googlesource.com/24380
    Reviewed-by: Andrew Gerrand <adg@golang.org>
    Run-TryBot: Andrew Gerrand <adg@golang.org>

@seehuhn
Copy link
Author

seehuhn commented Feb 27, 2017

I just run "git bisect" and it also came out with 2b583a1 as the first bad commit.

@rsc
Copy link
Contributor

rsc commented Feb 27, 2017

In the breaking CL, it looks to me like

if t.tmpl[new.name] != nil && parse.IsEmptyTree(tree.Root) && t.Tree != nil {

is checking the wrong thing tree. t.tmpl[new.name] is the interesting template, not t itself. Should probably be:

if old := t.tmpl[new.name]; old != nil && parse.IsEmptyTree(tree.Root) && old.Tree != nil {

The non-determinacy of the example is likely due to html/template walking a map and making calls a variety of orders (that should all behave the same).

@robpike
Copy link
Contributor

robpike commented Feb 27, 2017

Yes, I think that's the fix.

@robpike
Copy link
Contributor

robpike commented Feb 27, 2017

@rsc's change fixes the test here but breaks the TestEmptyTemplate test. Working on it.

@gopherbot
Copy link

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

@rsc
Copy link
Contributor

rsc commented Apr 5, 2017

Reopening for backport.

@rsc rsc reopened this Apr 5, 2017
@gopherbot
Copy link

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

@rsc
Copy link
Contributor

rsc commented Apr 5, 2017

Cherry-pick submitted.

@rsc rsc closed this as completed Apr 5, 2017
gopherbot pushed a commit that referenced this issue Apr 5, 2017
This was a subtle bug introduced in the previous release's fix for
issue 16156.

The definition of empty template was broken, causing the answer
to depend on the order of templates in the map.

Fixes #16156 (for real).
Fixes #19294.
Fixes #19204.

Change-Id: I1cd915c94534cad3116d83bd158cbc28700510b9
Reviewed-on: https://go-review.googlesource.com/38420
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-on: https://go-review.googlesource.com/39594
Reviewed-by: Rob Pike <r@golang.org>
siongui added a commit to siongui/wat-pah-photiyan that referenced this issue Apr 8, 2017
@ghost
Copy link

ghost commented Apr 8, 2017

This has not been fixed in 1.8.1

master.gohtml
...
{{block "DocumentFile" .}}
{{.FileFolder}}
{{end}}
...

acount.gohtml
{{define "DocumentFile"}}{{end}}

template: master.gohtml:187:63: executing "DocumentFile" at <.FileFolder>: can't evaluate field FileFolder in type *auth.User masterTmpl account map[page:0xc042671150 account:0xc0424c38c0 accountGroups:0xc0427d4620]

@robpike
Copy link
Contributor

robpike commented Apr 8, 2017

@minervadata Please post a complete program that reproduces the problem you are seeing.

@ghost
Copy link

ghost commented Apr 9, 2017

@robpike this is part of framework

tmpl.go
var masterTmpl = template.New("masterTmpl").Funcs(TmplFunc) func ParseSubTmpls(subTmplPath string) (t *template.Template) { t, err := template.Must(masterTmpl.Clone()).ParseGlob(filepath.Join(TmplPath, subTmplPath)) if err != nil { logger.Lgr.Println(err, subTmplPath, t) } return }

account.go
var accountTmpl = tmpl.ParseSubTmpls("account/templates/account/*")

master.gohtml
{{define "DocumentHeader"}} {{block "DocumentOption" .}} {{end}} {{block "DocumentFile" .}} {{.FileFolder}} {{end}} {{end}}
account.gohtml
{{define "DocumentFile"}}{{end}} {{define "account"}} {{with .account}}{{template "DocumentHeader" .}}{{end}} {{end}}

image

@robpike
Copy link
Contributor

robpike commented Apr 9, 2017

This is not a complete program, it's several incomplete snippets from several files. You should be able to create a single, complete file of Go source code that can be compiled in isolation and demonstrate the problem. The template text can be constants in the source.

Given your example I should be able to copy the text to file.go with no editing and run
go run file.go
and see the problem.

A playground link would be even better.

@rsc
Copy link
Contributor

rsc commented Apr 10, 2017

Please open a new issue (golang.org/issue/new) for other problems with html/template. The error message you are reporting is unrelated to the failure this issue was about.

lparth pushed a commit to lparth/go that referenced this issue Apr 13, 2017
This was a subtle bug introduced in the previous release's fix for
issue 16156.

The definition of empty template was broken, causing the answer
to depend on the order of templates in the map.

Fixes golang#16156 (for real).
Fixes golang#19294.
Fixes golang#19204.

Change-Id: I1cd915c94534cad3116d83bd158cbc28700510b9
Reviewed-on: https://go-review.googlesource.com/38420
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Apr 10, 2018
@rsc rsc unassigned robpike Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants