text/template/parse: error on bad range variables

The package used to accept invalid range pipelines, such as:

	{{range $k, .}}
	{{range $k, 123 := .}}

This is because the logic that allowed a range pipeline to declare
multiple variables was broken. When encountering a single comma inside a
range pipeline, it would happily continue parsing a second variable,
even if we didn't have a variable token at all.

Then, the loop would immediately break, and we'd parse the pipeline we'd
be ranging over. That is, we'd parse {{range $k, .}} as if it were
{{range $k = .}}.

To fix this, only allow the loop to continue if we know we're going to
parse another variable or a token that would end the pipeline. Also add
a few test cases for these error edge cases.

While at it, make use of T.Run, which was useful in debugging
Tree.pipeline via print statements.

Fixes #28437.

Change-Id: Idc9966bf643f0f3bc1b052620357e5b0aa2022ea
Reviewed-on: https://go-review.googlesource.com/c/145282
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Bjørn Erik Pedersen <bjorn.erik.pedersen@gmail.com>
Reviewed-by: Rob Pike <r@golang.org>

diff --git a/src/text/template/parse/parse.go b/src/text/template/parse/parse.go
index efdad3297c686bfc69c99f1978c7cba9043a1933..5195694d421947575e963064c2165895ded5caaf 100644 (file)
--- a/src/text/template/parse/parse.go
+++ b/src/text/template/parse/parse.go
@@ -385,6 +385,7 @@ func (t *Tree) pipeline(context string) (pipe *PipeNode) {
        token := t.peekNonSpace()
        pos := token.pos
        // Are there declarations or assignments?
+       // TODO(mvdan): simplify the loop break/continue logic
        for {
                if v := t.peekNonSpace(); v.typ == itemVariable {
                        t.next()
@@ -406,7 +407,12 @@ func (t *Tree) pipeline(context string) (pipe *PipeNode) {
                                }
                                if next.typ == itemChar && next.val == "," {
                                        if context == "range" && len(vars) < 2 {
-                                               continue
+                                               switch t.peekNonSpace().typ {
+                                               case itemVariable, itemRightDelim, itemRightParen:
+                                                       continue
+                                               default:
+                                                       t.errorf("range can only initialize variables")
+                                               }
                                        }
                                        t.errorf("too many declarations in %s", context)
                                }

diff --git a/src/text/template/parse/parse_test.go b/src/text/template/parse/parse_test.go
index d03987581c958d4acfb56604689d1f0565e364b1..15cc65670adbb9b5dadcfc51287b821e4af34817 100644 (file)
--- a/src/text/template/parse/parse_test.go
+++ b/src/text/template/parse/parse_test.go
@@ -450,18 +450,37 @@ var errorTests = []parseTest{
        {"multilinerawstring",
                "{{ $v := `\n` }} {{",
                hasError, `multilinerawstring:2: unexpected unclosed action`},
+       {"rangeundefvar",
+               "{{range $k}}{{end}}",
+               hasError, `undefined variable`},
+       {"rangeundefvars",
+               "{{range $k, $v}}{{end}}",
+               hasError, `undefined variable`},
+       {"rangemissingvalue1",
+               "{{range $k,}}{{end}}",
+               hasError, `missing value for range`},
+       {"rangemissingvalue2",
+               "{{range $k, $v := }}{{end}}",
+               hasError, `missing value for range`},
+       {"rangenotvariable1",
+               "{{range $k, .}}{{end}}",
+               hasError, `range can only initialize variables`},
+       {"rangenotvariable2",
+               "{{range $k, 123 := .}}{{end}}",
+               hasError, `range can only initialize variables`},
 }
 
 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)
-               }
+               t.Run(test.name, func(t *testing.T) {
+                       _, err := New(test.name).Parse(test.input, "", "", make(map[string]*Tree))
+                       if err == nil {
+                               t.Fatalf("expected error %q, got nil", test.result)
+                       }
+                       if !strings.Contains(err.Error(), test.result) {
+                               t.Fatalf("error %q does not contain %q", err, test.result)
+                       }
+               })
        }
 }