text/template: detect unexported fields better

Moves the error detection back into execution, where it used to be,
and improves the error message.
Rolls back most of 6009048, which broke lower-case keys in maps.
If it weren't for maps we could detect this at compile time rather than
execution time.

Fixes issue 3542.

R=golang-dev, dsymonds
CC=golang-dev
http://codereview.appspot.com/6098051

4 files changed, 24 insertions, 19 deletions

diff --git a/src/pkg/text/template/exec.go b/src/pkg/text/template/exec.go
index feb434a3b..b8d23d43f 100644
--- a/src/pkg/text/template/exec.go
+++ b/src/pkg/text/template/exec.go
@@ -12,6 +12,8 @@ import (
 	"sort"
 	"strings"
 	"text/template/parse"
+	"unicode"
+	"unicode/utf8"
 )
 
 // state represents the state of an execution. It's not part of the
@@ -414,9 +416,13 @@ func (s *state) evalField(dot reflect.Value, fieldName string, args []parse.Node
 		return s.evalCall(dot, method, fieldName, args, final)
 	}
 	hasArgs := len(args) > 1 || final.IsValid()
-	// It's not a method; is it a field of a struct?
+	// It's not a method; must be a field of a struct or an element of a map. The receiver must not be nil.
 	receiver, isNil := indirect(receiver)
-	if receiver.Kind() == reflect.Struct {
+	if isNil {
+		s.errorf("nil pointer evaluating %s.%s", typ, fieldName)
+	}
+	switch receiver.Kind() {
+	case reflect.Struct:
 		tField, ok := receiver.Type().FieldByName(fieldName)
 		if ok {
 			field := receiver.FieldByIndex(tField.Index)
@@ -428,9 +434,11 @@ func (s *state) evalField(dot reflect.Value, fieldName string, args []parse.Node
 				return field
 			}
 		}
-	}
-	// If it's a map, attempt to use the field name as a key.
-	if receiver.Kind() == reflect.Map {
+		if !isExported(fieldName) {
+			s.errorf("%s is not an exported field of struct type %s", fieldName, typ)
+		}
+	case reflect.Map:
+		// If it's a map, attempt to use the field name as a key.
 		nameVal := reflect.ValueOf(fieldName)
 		if nameVal.Type().AssignableTo(receiver.Type().Key()) {
 			if hasArgs {
@@ -439,13 +447,16 @@ func (s *state) evalField(dot reflect.Value, fieldName string, args []parse.Node
 			return receiver.MapIndex(nameVal)
 		}
 	}
-	if isNil {
-		s.errorf("nil pointer evaluating %s.%s", typ, fieldName)
-	}
 	s.errorf("can't evaluate field %s in type %s", fieldName, typ)
 	panic("not reached")
 }
 
+// isExported reports whether the field name (which starts with a period) can be accessed.
+func isExported(fieldName string) bool {
+	r, _ := utf8.DecodeRuneInString(fieldName[1:]) // drop the period
+	return unicode.IsUpper(r)
+}
+
 var (
 	errorType       = reflect.TypeOf((*error)(nil)).Elem()
 	fmtStringerType = reflect.TypeOf((*fmt.Stringer)(nil)).Elem()
diff --git a/src/pkg/text/template/exec_test.go b/src/pkg/text/template/exec_test.go
index 37d25f470..8f6c67bba 100644
--- a/src/pkg/text/template/exec_test.go
+++ b/src/pkg/text/template/exec_test.go
@@ -65,6 +65,8 @@ type T struct {
 	VariadicFuncInt func(int, ...string) string
 	// Template to test evaluation of templates.
 	Tmpl *Template
+	// Unexported field; cannot be accessed by template.
+	unexported int
 }
 
 type U struct {
@@ -232,6 +234,7 @@ var execTests = []execTest{
 	// Fields of structs.
 	{".X", "-{{.X}}-", "-x-", tVal, true},
 	{".U.V", "-{{.U.V}}-", "-v-", tVal, true},
+	{".unexported", "{{.unexported}}", "", tVal, false},
 
 	// Fields on maps.
 	{"map .one", "{{.MSI.one}}", "1", tVal, true},
@@ -473,6 +476,8 @@ var execTests = []execTest{
 	// Pipelined arg was not being type-checked.
 	{"bug8a", "{{3|oneArg}}", "", tVal, false},
 	{"bug8b", "{{4|dddArg 3}}", "", tVal, false},
+	// A bug was introduced that broke map lookups for lower-case names.
+	{"bug9", "{{.cause}}", "neglect", map[string]string{"cause": "neglect"}, true},
 }
 
 func zeroArgs() string {
diff --git a/src/pkg/text/template/parse/parse.go b/src/pkg/text/template/parse/parse.go
index cbb1686a7..7970b5fcc 100644
--- a/src/pkg/text/template/parse/parse.go
+++ b/src/pkg/text/template/parse/parse.go
@@ -14,7 +14,6 @@ import (
 	"runtime"
 	"strconv"
 	"unicode"
-	"unicode/utf8"
 )
 
 // Tree is the representation of a single parsed template.
@@ -474,9 +473,6 @@ Loop:
 		case itemVariable:
 			cmd.append(t.useVar(token.val))
 		case itemField:
-			if !isExported(token.val) {
-				t.errorf("field %q not exported; cannot be evaluated", token.val)
-			}
 			cmd.append(newField(token.val))
 		case itemBool:
 			cmd.append(newBool(token.val == "true"))
@@ -502,12 +498,6 @@ Loop:
 	return cmd
 }
 
-// isExported reports whether the field name (which starts with a period) can be accessed.
-func isExported(fieldName string) bool {
-	r, _ := utf8.DecodeRuneInString(fieldName[1:]) // drop the period
-	return unicode.IsUpper(r)
-}
-
 // hasFunction reports if a function name exists in the Tree's maps.
 func (t *Tree) hasFunction(name string) bool {
 	for _, funcMap := range t.funcs {
diff --git a/src/pkg/text/template/parse/parse_test.go b/src/pkg/text/template/parse/parse_test.go
index fb98613fe..b2e788238 100644
--- a/src/pkg/text/template/parse/parse_test.go
+++ b/src/pkg/text/template/parse/parse_test.go
@@ -230,7 +230,6 @@ var parseTests = []parseTest{
 	{"invalid punctuation", "{{printf 3, 4}}", hasError, ""},
 	{"multidecl outside range", "{{with $v, $u := 3}}{{end}}", hasError, ""},
 	{"too many decls in range", "{{range $u, $v, $w := 3}}{{end}}", hasError, ""},
-	{"unexported field", "{{.local}}", hasError, ""},
 	// Equals (and other chars) do not assignments make (yet).
 	{"bug0a", "{{$x := 0}}{{$x}}", noError, "{{$x := 0}}{{$x}}"},
 	{"bug0b", "{{$x = 1}}{{$x}}", hasError, ""},