text/template: towards better errors

Give the right name for errors, and add a test to check we're
getting the errors we expect.
Also fix an ordering bug (calling add after stopParse) that
caused a nil indirection rather than a helpful error.
Fixes #3280.

R=golang-dev, adg
CC=golang-dev
https://golang.org/cl/6520043

2 files changed, 79 insertions, 5 deletions

diff --git a/src/pkg/text/template/parse/parse.go b/src/pkg/text/template/parse/parse.go
index 6177e32e73..c52e41d166 100644
--- a/src/pkg/text/template/parse/parse.go
+++ b/src/pkg/text/template/parse/parse.go
@@ -18,8 +18,9 @@ import (
 
 // Tree is the representation of a single parsed template.
 type Tree struct {
-	Name string    // name of the template represented by the tree.
-	Root *ListNode // top-level root of the tree.
+	Name      string    // name of the template represented by the tree.
+	ParseName string    // name of the top-level template during parsing, for error messages.
+	Root      *ListNode // top-level root of the tree.
 	// Parsing only; cleared after parse.
 	funcs     []map[string]interface{}
 	lex       *lexer
@@ -114,7 +115,7 @@ func New(name string, funcs ...map[string]interface{}) *Tree {
 // errorf formats the error and terminates processing.
 func (t *Tree) errorf(format string, args ...interface{}) {
 	t.Root = nil
-	format = fmt.Sprintf("template: %s:%d: %s", t.Name, t.lex.lineNumber(), format)
+	format = fmt.Sprintf("template: %s:%d: %s", t.ParseName, t.lex.lineNumber(), format)
 	panic(fmt.Errorf(format, args...))
 }
 
@@ -203,6 +204,7 @@ func (t *Tree) atEOF() bool {
 // the treeSet map.
 func (t *Tree) Parse(s, leftDelim, rightDelim string, treeSet map[string]*Tree, funcs ...map[string]interface{}) (tree *Tree, err error) {
 	defer t.recover(&err)
+	t.ParseName = t.Name
 	t.startParse(funcs, lex(t.Name, s, leftDelim, rightDelim))
 	t.parse(treeSet)
 	t.add(treeSet)
@@ -257,6 +259,7 @@ func (t *Tree) parse(treeSet map[string]*Tree) (next Node) {
 			delim := t.next()
 			if t.nextNonSpace().typ == itemDefine {
 				newT := New("definition") // name will be updated once we know it.
+				newT.ParseName = t.ParseName
 				newT.startParse(t.funcs, t.lex)
 				newT.parseDefinition(treeSet)
 				continue
@@ -289,8 +292,8 @@ func (t *Tree) parseDefinition(treeSet map[string]*Tree) {
 	if end.Type() != nodeEnd {
 		t.errorf("unexpected %s in %s", end, context)
 	}
-	t.stopParse()
 	t.add(treeSet)
+	t.stopParse()
 }
 
 // itemList:
diff --git a/src/pkg/text/template/parse/parse_test.go b/src/pkg/text/template/parse/parse_test.go
index 3838250ef2..4be4ca077d 100644
--- a/src/pkg/text/template/parse/parse_test.go
+++ b/src/pkg/text/template/parse/parse_test.go
@@ -7,10 +7,11 @@ package parse
 import (
 	"flag"
 	"fmt"
+	"strings"
 	"testing"
 )
 
-var debug = flag.Bool("debug", false, "show the errors produced by the tests")
+var debug = flag.Bool("debug", false, "show the errors produced by the main tests")
 
 type numberTest struct {
 	text      string
@@ -321,3 +322,73 @@ func TestIsEmpty(t *testing.T) {
 		}
 	}
 }
+
+// All failures, and the result is a string that must appear in the error message.
+var errorTests = []parseTest{
+	// Check line numbers are accurate.
+	{"unclosed1",
+		"line1\n{{",
+		hasError, `unclosed1:2: unexpected unclosed action in command`},
+	{"unclosed2",
+		"line1\n{{define `x`}}line2\n{{",
+		hasError, `unclosed2:3: unexpected unclosed action in command`},
+	// Specific errors.
+	{"function",
+		"{{foo}}",
+		hasError, `function "foo" not defined`},
+	{"comment",
+		"{{/*}}",
+		hasError, `unclosed comment`},
+	{"lparen",
+		"{{.X (1 2 3}}",
+		hasError, `unclosed left paren`},
+	{"rparen",
+		"{{.X 1 2 3)}}",
+		hasError, `unexpected ")"`},
+	{"space",
+		"{{`x`3}}",
+		hasError, `missing space?`},
+	{"idchar",
+		"{{a#}}",
+		hasError, `'#'`},
+	{"charconst",
+		"{{'a}}",
+		hasError, `unterminated character constant`},
+	{"stringconst",
+		`{{"a}}`,
+		hasError, `unterminated quoted string`},
+	{"rawstringconst",
+		"{{`a}}",
+		hasError, `unterminated raw quoted string`},
+	{"number",
+		"{{0xi}}",
+		hasError, `number syntax`},
+	{"multidefine",
+		"{{define `a`}}a{{end}}{{define `a`}}b{{end}}",
+		hasError, `multiple definition of template`},
+	{"eof",
+		"{{range .X}}",
+		hasError, `unexpected EOF`},
+	{"variable",
+		"{{$a.b := 23}}",
+		hasError, `illegal variable in declaration`},
+	{"multidecl",
+		"{{$a,$b,$c := 23}}",
+		hasError, `too many declarations`},
+	{"undefvar",
+		"{{$a}}",
+		hasError, `undefined variable`},
+}
+
+func TestErrors(t *testing.T) {
+	for _, test := range errorTests {
+		_, err := New(test.name).Parse(test.input, "", "", make(map[string]*Tree))
+		if err == nil {
+			t.Errorf("%q: expected error", test.name)
+			continue
+		}
+		if !strings.Contains(err.Error(), test.result) {
+			t.Errorf("%q: error %q does not contain %q", test.name, err, test.result)
+		}
+	}
+}