html/template, text/template: docs and fixes for template redefinition

All prior versions of Go have allowed redefining empty templates
to become non-empty. Unfortunately, that has never consistently
taken effect in html/template after the first execution:

	// define and execute
	t := template.New("root")
	t.Parse(`{{define "T"}}{{end}}<a href="{{template "T"}}">`)
	t.Execute(w, nil) // <a href="">

	// redefine
	t.Parse(`{{define "T"}}my.url{{end}}`) // succeeds, but ignored
	t.Execute(w, nil) // <a href="">

When Go 1.6 added {{block...}} to text/template, that loosened the
redefinition rules to allow redefinition at any time. The loosening was
undone a bit in html/template, although inconsistently:

	// define and execute
	t := template.New("root")
	t.Parse(`{{define "T"}}body{{end}}`)
	t.Lookup("T").Execute(ioutil.Discard, nil)

	// attempt to redefine
	t.Parse(`{{define "T"}}body{{end}}`) // rejected in all Go versions
	t.Lookup("T").Parse("body") // OK as of Go 1.6, likely unintentionally

Like in the empty->non-empty case, whether future execution takes
notice of a redefinition basically can't be explained without going into
the details of the template escape analysis.

Address both the original inconsistencies in whether a redefinition
would have any effect and the new inconsistencies about whether a
redefinition is allowed by adopting a new rule: no parsing or modifying
any templates after the first execution of any template in the same set.
Template analysis begins at first execution, and once template analysis
has begun, we simply don't have the right logic to update the analysis
for incremental modifications (and never have).

If this new rule breaks existing uses of templates that we decide need
to be supported, we can try to invalidate all escape analysis for the
entire set after any modifications. But let's wait on that until we know
we need to and why.

Also fix documentation of text/template redefinition policy
(redefinition is always OK).

Fixes #15761.

Change-Id: I7d58d7c08a7d9df2440ee0d651a5b2ecaff3006c
Reviewed-on: https://go-review.googlesource.com/31464
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Andrew Gerrand <adg@golang.org>

src/html/template/template.go

@@ -33,8 +33,9 @@ var escapeOK = fmt.Errorf("template escaped correctly")
 
 // nameSpace is the data structure shared by all templates in an association.
 type nameSpace struct {
-	mu  sync.Mutex
-	set map[string]*Template
+	mu      sync.Mutex
+	set     map[string]*Template
+	escaped bool
 }
 
 // Templates returns a slice of the templates associated with t, including t
@@ -74,10 +75,25 @@ func (t *Template) Option(opt ...string) *Template {
 	return t
 }
 
+// checkCanParse checks whether it is OK to parse templates.
+// If not, it returns an error.
+func (t *Template) checkCanParse() error {
+	if t == nil {
+		return nil
+	}
+	t.nameSpace.mu.Lock()
+	defer t.nameSpace.mu.Unlock()
+	if t.nameSpace.escaped {
+		return fmt.Errorf("html/template: cannot Parse after Execute")
+	}
+	return nil
+}
+
 // escape escapes all associated templates.
 func (t *Template) escape() error {
 	t.nameSpace.mu.Lock()
 	defer t.nameSpace.mu.Unlock()
+	t.nameSpace.escaped = true
 	if t.escapeErr == nil {
 		if t.Tree == nil {
 			return fmt.Errorf("template: %q is an incomplete or empty template%s", t.Name(), t.DefinedTemplates())
@@ -124,6 +140,7 @@ func (t *Template) ExecuteTemplate(wr io.Writer, name string, data interface{})
 func (t *Template) lookupAndEscapeTemplate(name string) (tmpl *Template, err error) {
 	t.nameSpace.mu.Lock()
 	defer t.nameSpace.mu.Unlock()
+	t.nameSpace.escaped = true
 	tmpl = t.set[name]
 	if tmpl == nil {
 		return nil, fmt.Errorf("html/template: %q is undefined", name)
@@ -155,21 +172,22 @@ func (t *Template) DefinedTemplates() string {
 // define additional templates associated with t and are removed from the
 // definition of t itself.
 //
+// Templates can be redefined in successive calls to Parse,
+// before the first use of Execute on t or any associated template.
 // A template definition with a body containing only white space and comments
-// is considered empty and is not recorded as the template's body.
-// Each template can be given a non-empty definition at most once.
-// That is, Parse may be called multiple times to parse definitions of templates
-// to associate with t, but at most one such call can include a non-empty body for
-// t itself, and each named associated template can be given at most one
-// non-empty definition.
+// is considered empty and will not replace an existing template's body.
+// This allows using Parse to add new named template definitions without
+// overwriting the main template body.
 func (t *Template) Parse(text string) (*Template, error) {
-	t.nameSpace.mu.Lock()
-	t.escapeErr = nil
-	t.nameSpace.mu.Unlock()
+	if err := t.checkCanParse(); err != nil {
+		return nil, err
+	}
+
 	ret, err := t.text.Parse(text)
 	if err != nil {
 		return nil, err
 	}
+
 	// In general, all the named templates might have changed underfoot.
 	// Regardless, some new ones may have been defined.
 	// The template.Template set has been updated; update ours.
@@ -180,11 +198,7 @@ func (t *Template) Parse(text string) (*Template, error) {
 		tmpl := t.set[name]
 		if tmpl == nil {
 			tmpl = t.new(name)
-		} else if tmpl.escapeErr != nil {
-			return nil, fmt.Errorf("html/template: cannot redefine %q after it has executed", name)
 		}
-		// Restore our record of this text/template to its unescaped original state.
-		tmpl.escapeErr = nil
 		tmpl.text = v
 		tmpl.Tree = v.Tree
 	}
@@ -194,13 +208,14 @@ func (t *Template) Parse(text string) (*Template, error) {
 // AddParseTree creates a new template with the name and parse tree
 // and associates it with t.
 //
-// It returns an error if t has already been executed.
+// It returns an error if t or any associated template has already been executed.
 func (t *Template) AddParseTree(name string, tree *parse.Tree) (*Template, error) {
+	if err := t.checkCanParse(); err != nil {
+		return nil, err
+	}
+
 	t.nameSpace.mu.Lock()
 	defer t.nameSpace.mu.Unlock()
-	if t.escapeErr != nil {
-		return nil, fmt.Errorf("html/template: cannot AddParseTree to %q after it has executed", t.Name())
-	}
 	text, err := t.text.AddParseTree(name, tree)
 	if err != nil {
 		return nil, err
@@ -368,6 +383,8 @@ func ParseFiles(filenames ...string) (*Template, error) {
 //
 // When parsing multiple files with the same name in different directories,
 // the last one mentioned will be the one that results.
+//
+// ParseFiles returns an error if t or any associated template has already been executed.
 func (t *Template) ParseFiles(filenames ...string) (*Template, error) {
 	return parseFiles(t, filenames...)
 }
@@ -375,6 +392,10 @@ func (t *Template) ParseFiles(filenames ...string) (*Template, error) {
 // parseFiles is the helper for the method and function. If the argument
 // template is nil, it is created from the first file.
 func parseFiles(t *Template, filenames ...string) (*Template, error) {
+	if err := t.checkCanParse(); err != nil {
+		return nil, err
+	}
+
 	if len(filenames) == 0 {
 		// Not really a problem, but be consistent.
 		return nil, fmt.Errorf("html/template: no files named in call to ParseFiles")
@@ -429,12 +450,17 @@ func ParseGlob(pattern string) (*Template, error) {
 //
 // When parsing multiple files with the same name in different directories,
 // the last one mentioned will be the one that results.
+//
+// ParseGlob returns an error if t or any associated template has already been executed.
 func (t *Template) ParseGlob(pattern string) (*Template, error) {
 	return parseGlob(t, pattern)
 }
 
 // parseGlob is the implementation of the function and method ParseGlob.
 func parseGlob(t *Template, pattern string) (*Template, error) {
+	if err := t.checkCanParse(); err != nil {
+		return nil, err
+	}
 	filenames, err := filepath.Glob(pattern)
 	if err != nil {
 		return nil, err

src/html/template/template_test.go

-package template
+// Copyright 2016 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package template_test
 
 import (
 	"bytes"
+	. "html/template"
+	"strings"
 	"testing"
 )
 
@@ -27,3 +33,125 @@ func TestTemplateClone(t *testing.T) {
 		t.Fatalf("got %q; want %q", got, want)
 	}
 }
+
+func TestRedefineNonEmptyAfterExecution(t *testing.T) {
+	c := newTestCase(t)
+	c.mustParse(c.root, `foo`)
+	c.mustExecute(c.root, nil, "foo")
+	c.mustNotParse(c.root, `bar`)
+}
+
+func TestRedefineEmptyAfterExecution(t *testing.T) {
+	c := newTestCase(t)
+	c.mustParse(c.root, ``)
+	c.mustExecute(c.root, nil, "")
+	c.mustNotParse(c.root, `foo`)
+	c.mustExecute(c.root, nil, "")
+}
+
+func TestRedefineAfterNonExecution(t *testing.T) {
+	c := newTestCase(t)
+	c.mustParse(c.root, `{{if .}}<{{template "X"}}>{{end}}{{define "X"}}foo{{end}}`)
+	c.mustExecute(c.root, 0, "")
+	c.mustNotParse(c.root, `{{define "X"}}bar{{end}}`)
+	c.mustExecute(c.root, 1, "&lt;foo>")
+}
+
+func TestRedefineAfterNamedExecution(t *testing.T) {
+	c := newTestCase(t)
+	c.mustParse(c.root, `<{{template "X" .}}>{{define "X"}}foo{{end}}`)
+	c.mustExecute(c.root, nil, "&lt;foo>")
+	c.mustNotParse(c.root, `{{define "X"}}bar{{end}}`)
+	c.mustExecute(c.root, nil, "&lt;foo>")
+}
+
+func TestRedefineNestedByNameAfterExecution(t *testing.T) {
+	c := newTestCase(t)
+	c.mustParse(c.root, `{{define "X"}}foo{{end}}`)
+	c.mustExecute(c.lookup("X"), nil, "foo")
+	c.mustNotParse(c.root, `{{define "X"}}bar{{end}}`)
+	c.mustExecute(c.lookup("X"), nil, "foo")
+}
+
+func TestRedefineNestedByTemplateAfterExecution(t *testing.T) {
+	c := newTestCase(t)
+	c.mustParse(c.root, `{{define "X"}}foo{{end}}`)
+	c.mustExecute(c.lookup("X"), nil, "foo")
+	c.mustNotParse(c.lookup("X"), `bar`)
+	c.mustExecute(c.lookup("X"), nil, "foo")
+}
+
+func TestRedefineSafety(t *testing.T) {
+	c := newTestCase(t)
+	c.mustParse(c.root, `<html><a href="{{template "X"}}">{{define "X"}}{{end}}`)
+	c.mustExecute(c.root, nil, `<html><a href="">`)
+	// Note: Every version of Go prior to Go 1.8 accepted the redefinition of "X"
+	// on the next line, but luckily kept it from being used in the outer template.
+	// Now we reject it, which makes clearer that we're not going to use it.
+	c.mustNotParse(c.root, `{{define "X"}}" bar="baz{{end}}`)
+	c.mustExecute(c.root, nil, `<html><a href="">`)
+}
+
+func TestRedefineTopUse(t *testing.T) {
+	c := newTestCase(t)
+	c.mustParse(c.root, `{{template "X"}}{{.}}{{define "X"}}{{end}}`)
+	c.mustExecute(c.root, 42, `42`)
+	c.mustNotParse(c.root, `{{define "X"}}<script>{{end}}`)
+	c.mustExecute(c.root, 42, `42`)
+}
+
+func TestRedefineOtherParsers(t *testing.T) {
+	c := newTestCase(t)
+	c.mustParse(c.root, ``)
+	c.mustExecute(c.root, nil, ``)
+	if _, err := c.root.ParseFiles("no.template"); err == nil || !strings.Contains(err.Error(), "Execute") {
+		t.Errorf("ParseFiles: %v\nwanted error about already having Executed", err)
+	}
+	if _, err := c.root.ParseGlob("*.no.template"); err == nil || !strings.Contains(err.Error(), "Execute") {
+		t.Errorf("ParseGlob: %v\nwanted error about already having Executed", err)
+	}
+	if _, err := c.root.AddParseTree("t1", c.root.Tree); err == nil || !strings.Contains(err.Error(), "Execute") {
+		t.Errorf("AddParseTree: %v\nwanted error about already having Executed", err)
+	}
+}
+
+type testCase struct {
+	t    *testing.T
+	root *Template
+}
+
+func newTestCase(t *testing.T) *testCase {
+	return &testCase{
+		t:    t,
+		root: New("root"),
+	}
+}
+
+func (c *testCase) lookup(name string) *Template {
+	return c.root.Lookup(name)
+}
+
+func (c *testCase) mustParse(t *Template, text string) {
+	_, err := t.Parse(text)
+	if err != nil {
+		c.t.Fatalf("parse: %v", err)
+	}
+}
+
+func (c *testCase) mustNotParse(t *Template, text string) {
+	_, err := t.Parse(text)
+	if err == nil {
+		c.t.Fatalf("parse: unexpected success")
+	}
+}
+
+func (c *testCase) mustExecute(t *Template, val interface{}, want string) {
+	var buf bytes.Buffer
+	err := t.Execute(&buf, val)
+	if err != nil {
+		c.t.Fatalf("execute: %v", err)
+	}
+	if buf.String() != want {
+		c.t.Fatalf("template output:\n%s\nwant:\n%s", buf.String(), want)
+	}
+}

src/text/template/template.go

@@ -186,13 +186,11 @@ func (t *Template) Lookup(name string) *Template {
 // define additional templates associated with t and are removed from the
 // definition of t itself.
 //
+// Templates can be redefined in successive calls to Parse.
 // A template definition with a body containing only white space and comments
-// is considered empty and is not recorded as the template's body.
-// Each template can be given a non-empty definition at most once.
-// That is, Parse may be called multiple times to parse definitions of templates
-// to associate with t, but at most one such call can include a non-empty body for
-// t itself, and each named associated template can be given at most one
-// non-empty definition.
+// is considered empty and will not replace an existing template's body.
+// This allows using Parse to add new named template definitions without
+// overwriting the main template body.
 func (t *Template) Parse(text string) (*Template, error) {
 	t.init()
 	t.muFuncs.RLock()