Reintroduce template evaluation in variables

With a recent commit, template evaluation for variables in tasks got
broken. This reindroudces temmplate evaluation in taks, and resolves
a series of issues that where previouisly present on master, such as:

- Taskvars did not get evaluated as templates.
- Taskvars would, in contrast to the documentation, _override_ task
  variables for the taks called directly via `Executor.Run(args
  ...string)`. This caused different behaviour in the "default" task
  v.s. other tasks.

This commit ensures:
 - Priority order for variables is now according to the documentation,
   also for the "default" task.
 - Variables gets resolved in a particular order to ensure logical
   access to varaibles on template compile time, and that template
   compilation finds place _before_ resolution of dynamic variables.

This change also allows the following to work:

    task:
      vars:
        A: "52"
        B: "{{.A}}"

However, the following will always replace C with the uncompiled
`{{.A}}`:

    task:
      vars:
        A: "52"
        C: "{{.B}}"
        B: "{{.A}}"

Several tests have also been added to prevent this feature from breaking
again. This should hopefully finally resolve issue #40.

task.go

@@ -102,7 +102,7 @@ func (e *Executor) Run(args ...string) error {
 	}
 
 	for _, a := range args {
-		if err := e.RunTask(context.Background(), Call{Task: a, Vars: e.taskvars}); err != nil {
+		if err := e.RunTask(context.Background(), Call{Task: a, Vars: nil}); err != nil {
 			return err
 		}
 	}
@@ -111,22 +111,20 @@ func (e *Executor) Run(args ...string) error {
 
 // RunTask runs a task by its name
 func (e *Executor) RunTask(ctx context.Context, call Call) error {
-	task, ok := e.Tasks[call.Task]
+	origTask, ok := e.Tasks[call.Task]
 	if !ok {
 		return &taskNotFoundError{call.Task}
 	}
-
 	if atomic.AddInt32(e.taskCallCount[call.Task], 1) >= MaximumTaskCall {
 		return &MaximumTaskCallExceededError{task: call.Task}
 	}
 
-	var err error
-	call.Vars, err = e.getVariables(task, call)
+	vars, err := e.getVariables(call)
 	if err != nil {
 		return err
 	}
 
-	t, err := task.ReplaceVariables(call.Vars)
+	t, err := origTask.ReplaceVariables(vars)
 	if err != nil {
 		return err
 	}
@@ -138,11 +136,11 @@ func (e *Executor) RunTask(ctx context.Context, call Call) error {
 	// FIXME: doing again, since a var may have been overriden
 	// using the `set:` attribute of a dependecy.
 	// Remove this when `set` (that is deprecated) be removed
-	call.Vars, err = e.getVariables(task, call)
+	vars, err = e.getVariables(call)
 	if err != nil {
 		return err
 	}
-	t, err = task.ReplaceVariables(call.Vars)
+	t, err = origTask.ReplaceVariables(vars)
 	if err != nil {
 		return err
 	}

task_test.go

@@ -14,6 +14,121 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
+// fileContentTest provides a basic reusable test-case for running a Taskfile
+// and inspect generated files.
+type fileContentTest struct {
+	Dir       string
+	Target    string
+	TrimSpace bool
+	Files     map[string]string
+}
+
+func (fct fileContentTest) name(file string) string {
+	return fmt.Sprintf("target=%q,file=%q", fct.Target, file)
+}
+
+func (fct fileContentTest) Run(t *testing.T) {
+	for f := range fct.Files {
+		_ = os.Remove(filepath.Join(fct.Dir, f))
+	}
+
+	e := &task.Executor{
+		Dir:    fct.Dir,
+		Stdout: ioutil.Discard,
+		Stderr: ioutil.Discard,
+	}
+	assert.NoError(t, e.ReadTaskfile(), "e.ReadTaskfile()")
+	assert.NoError(t, e.Run(fct.Target), "e.Run(target)")
+
+	for name, expectContent := range fct.Files {
+		t.Run(fct.name(name), func(t *testing.T) {
+			b, err := ioutil.ReadFile(filepath.Join(fct.Dir, name))
+			assert.NoError(t, err, "Error reading file")
+			s := string(b)
+			if fct.TrimSpace {
+				s = strings.TrimSpace(s)
+			}
+			assert.Equal(t, expectContent, s, "unexpected file content")
+		})
+	}
+
+}
+
+func TestVars(t *testing.T) {
+	tt := fileContentTest{
+		Dir:       "testdata/vars",
+		Target:    "default",
+		TrimSpace: true,
+		Files: map[string]string{
+			// hello task:
+			"foo.txt":              "foo",
+			"bar.txt":              "bar",
+			"baz.txt":              "baz",
+			"tmpl_foo.txt":         "foo",
+			"tmpl_bar.txt":         "<no value>",
+			"tmpl_foo2.txt":        "foo2",
+			"tmpl_bar2.txt":        "bar2",
+			"shtmpl_foo.txt":       "foo",
+			"shtmpl_foo2.txt":      "foo2",
+			"nestedtmpl_foo.txt":   "{{.FOO}}",
+			"nestedtmpl_foo2.txt":  "foo2",
+			"foo2.txt":             "foo2",
+			"bar2.txt":             "bar2",
+			"baz2.txt":             "baz2",
+			"tmpl2_foo.txt":        "<no value>",
+			"tmpl2_foo2.txt":       "foo2",
+			"tmpl2_bar.txt":        "<no value>",
+			"tmpl2_bar2.txt":       "<no value>",
+			"shtmpl2_foo.txt":      "<no value>",
+			"shtmpl2_foo2.txt":     "foo2",
+			"nestedtmpl2_foo2.txt": "{{.FOO2}}",
+			"equal.txt":            "foo=bar",
+			"override.txt":         "bar",
+		},
+	}
+	tt.Run(t)
+	// Ensure identical results when running hello task directly.
+	tt.Target = "hello"
+	tt.Run(t)
+}
+
+func TestVarsInvalidTmpl(t *testing.T) {
+	const (
+		dir         = "testdata/vars"
+		target      = "invalid-var-tmpl"
+		expectError = "template: :1: unexpected EOF"
+	)
+
+	e := &task.Executor{
+		Dir:    dir,
+		Stdout: ioutil.Discard,
+		Stderr: ioutil.Discard,
+	}
+	assert.NoError(t, e.ReadTaskfile(), "e.ReadTaskfile()")
+	assert.EqualError(t, e.Run(target), expectError, "e.Run(target)")
+}
+
+func TestParams(t *testing.T) {
+	tt := fileContentTest{
+		Dir:       "testdata/params",
+		Target:    "default",
+		TrimSpace: false,
+		Files: map[string]string{
+			"hello.txt":       "Hello\n",
+			"world.txt":       "World\n",
+			"exclamation.txt": "!\n",
+			"dep1.txt":        "Dependence1\n",
+			"dep2.txt":        "Dependence2\n",
+			"spanish.txt":     "¡Holla mundo!\n",
+			"spanish-dep.txt": "¡Holla dependencia!\n",
+			"portuguese.txt":  "Olá, mundo!\n",
+			"portuguese2.txt": "Olá, mundo!\n",
+			"german.txt":      "Welt!\n",
+		},
+	}
+	tt.Run(t)
+}
+
 func TestDeps(t *testing.T) {
 	const dir = "testdata/deps"
 
@@ -52,48 +167,6 @@ func TestDeps(t *testing.T) {
 	}
 }
 
-func TestVars(t *testing.T) {
-	const dir = "testdata/vars"
-
-	files := []struct {
-		file    string
-		content string
-	}{
-		{"foo.txt", "foo"},
-		{"bar.txt", "bar"},
-		{"baz.txt", "baz"},
-		{"foo2.txt", "foo2"},
-		{"bar2.txt", "bar2"},
-		{"baz2.txt", "baz2"},
-		{"equal.txt", "foo=bar"},
-	}
-
-	for _, f := range files {
-		_ = os.Remove(filepath.Join(dir, f.file))
-	}
-
-	e := &task.Executor{
-		Dir:    dir,
-		Stdout: ioutil.Discard,
-		Stderr: ioutil.Discard,
-	}
-	assert.NoError(t, e.ReadTaskfile())
-	assert.NoError(t, e.Run("default"))
-
-	for _, f := range files {
-		d, err := ioutil.ReadFile(filepath.Join(dir, f.file))
-		if err != nil {
-			t.Errorf("Error reading %s: %v", f.file, err)
-		}
-		s := string(d)
-		s = strings.TrimSpace(s)
-
-		if s != f.content {
-			t.Errorf("File content should be %s but is %s", f.content, s)
-		}
-	}
-}
-
 func TestTaskCall(t *testing.T) {
 	const dir = "testdata/task_call"
 
@@ -225,41 +298,6 @@ func TestInit(t *testing.T) {
 	}
 }
 
-func TestParams(t *testing.T) {
-	const dir = "testdata/params"
-	var files = []struct {
-		file    string
-		content string
-	}{
-		{"hello.txt", "Hello\n"},
-		{"world.txt", "World\n"},
-		{"exclamation.txt", "!\n"},
-		{"dep1.txt", "Dependence1\n"},
-		{"dep2.txt", "Dependence2\n"},
-		{"spanish.txt", "¡Holla mundo!\n"},
-		{"spanish-dep.txt", "¡Holla dependencia!\n"},
-		{"portuguese.txt", "Olá, mundo!\n"},
-	}
-
-	for _, f := range files {
-		_ = os.Remove(filepath.Join(dir, f.file))
-	}
-
-	e := task.Executor{
-		Dir:    dir,
-		Stdout: ioutil.Discard,
-		Stderr: ioutil.Discard,
-	}
-	assert.NoError(t, e.ReadTaskfile())
-	assert.NoError(t, e.Run("default"))
-
-	for _, f := range files {
-		content, err := ioutil.ReadFile(filepath.Join(dir, f.file))
-		assert.NoError(t, err)
-		assert.Equal(t, f.content, string(content))
-	}
-}
-
 func TestCyclicDep(t *testing.T) {
 	const dir = "testdata/cyclic"
 

testdata/params/Taskfile.yml

@@ -1,7 +1,8 @@
 default:
   vars:
     SPANISH: ¡Holla mundo!
-    PORTUGUESE: "{{.PORTUGUESE}}"
+    PORTUGUESE: "{{.PORTUGUESE_HELLO_WORLD}}"
+    GERMAN: "Welt!"
   deps:
     - task: write-file
       vars: {CONTENT: Dependence1, FILE: dep1.txt}
@@ -20,7 +21,17 @@ default:
       vars: {CONTENT: "{{.SPANISH}}", FILE: spanish.txt}
     - task: write-file
       vars: {CONTENT: "{{.PORTUGUESE}}", FILE: portuguese.txt}
+    - task: write-file
+      vars: {CONTENT: "{{.GERMAN}}", FILE: german.txt}
+    - task: non-default
 
 write-file:
   cmds:
     - echo {{.CONTENT}} > {{.FILE}}
+
+non-default:
+  vars:
+    PORTUGUESE: "{{.PORTUGUESE_HELLO_WORLD}}"
+  cmds:
+    - task: write-file
+      vars: {CONTENT: "{{.PORTUGUESE}}", FILE: portuguese2.txt}

testdata/params/Taskvars.yml

@@ -1 +1,2 @@
-PORTUGUESE: Olá, mundo!
+PORTUGUESE_HELLO_WORLD: Olá, mundo!
+GERMAN: "Hello"

testdata/vars/Taskfile.yml

@@ -7,17 +7,49 @@ hello:
     - echo {{.FOO}} > foo.txt
     - echo {{.BAR}} > bar.txt
     - echo {{.BAZ}} > baz.txt
+    - echo '{{.TMPL_FOO}}' > tmpl_foo.txt
+    - echo '{{.TMPL_BAR}}' > tmpl_bar.txt
+    - echo '{{.TMPL_FOO2}}' > tmpl_foo2.txt
+    - echo '{{.TMPL_BAR2}}' > tmpl_bar2.txt
+    - echo '{{.SHTMPL_FOO}}' > shtmpl_foo.txt
+    - echo '{{.SHTMPL_FOO2}}' > shtmpl_foo2.txt
+    - echo '{{.NESTEDTMPL_FOO}}' > nestedtmpl_foo.txt
+    - echo '{{.NESTEDTMPL_FOO2}}' > nestedtmpl_foo2.txt
     - echo {{.FOO2}} > foo2.txt
     - echo {{.BAR2}} > bar2.txt
     - echo {{.BAZ2}} > baz2.txt
+    - echo '{{.TMPL2_FOO}}' > tmpl2_foo.txt
+    - echo '{{.TMPL2_BAR}}' > tmpl2_bar.txt
+    - echo '{{.TMPL2_FOO2}}' > tmpl2_foo2.txt
+    - echo '{{.TMPL2_BAR2}}' > tmpl2_bar2.txt
+    - echo '{{.SHTMPL2_FOO}}' > shtmpl2_foo.txt
+    - echo '{{.SHTMPL2_FOO2}}' > shtmpl2_foo2.txt
+    - echo '{{.NESTEDTMPL2_FOO2}}' > nestedtmpl2_foo2.txt
     - echo {{.EQUAL}} > equal.txt
+    - echo {{.OVERRIDE}} > override.txt
   vars:
     FOO: foo
     BAR: $echo bar
     BAZ:
       sh: echo baz
+    TMPL_FOO: "{{.FOO}}"
+    TMPL_BAR: "{{.BAR}}"
+    TMPL_FOO2: "{{.FOO2}}"
+    TMPL_BAR2: "{{.BAR2}}"
+    SHTMPL_FOO:
+      sh: "echo '{{.FOO}}'"
+    SHTMPL_FOO2:
+      sh: "echo '{{.FOO2}}'"
+    NESTEDTMPL_FOO: "{{.TMPL_FOO}}"
+    NESTEDTMPL_FOO2: "{{.TMPL2_FOO2}}"
+    OVERRIDE: "bar"
 
 set-equal:
   set: EQUAL
   cmds:
     - echo foo=bar
+
+invalid-var-tmpl:
+  vars:
+    CHARS: "abcd"
+    INVALID: "{{range .CHARS}}no end"

testdata/vars/Taskvars.yml

@@ -2,3 +2,11 @@ FOO2: foo2
 BAR2: $echo bar2
 BAZ2:
   sh: echo baz2
+TMPL2_FOO: "{{.FOO}}"
+TMPL2_BAR: "{{.BAR}}"
+TMPL2_FOO2: "{{.FOO2}}"
+TMPL2_BAR2: "{{.BAR2}}"
+SHTMPL2_FOO2:
+  sh: "echo '{{.FOO2}}'"
+NESTEDTMPL2_FOO2: "{{.TMPL2_FOO2}}"
+OVERRIDE: "foo"

variables.go

@@ -33,6 +33,11 @@ type Var struct {
 func (vs Vars) toStringMap() (m map[string]string) {
 	m = make(map[string]string, len(vs))
 	for k, v := range vs {
+		if v.Sh != "" {
+			// Dynamic variable is not yet resolved; trigger
+			// <no value> to be used in templates.
+			continue
+		}
 		m[k] = v.Static
 	}
 	return
@@ -95,33 +100,82 @@ func init() {
 	}
 }
 
-func (e *Executor) getVariables(t *Task, call Call) (Vars, error) {
-	result := make(Vars, len(t.Vars)+len(e.taskvars)+len(call.Vars))
+// getVariables returns fully resolved variables following the priorty order:
+// 1. Call variables (should already have been resolved)
+// 2. Environment (should not need to be resolved)
+// 3. Task variables, resolved with access to:
+//    - call, taskvars and environement variables
+// 4. Taskvars variables, resolved with access to:
+//    - environment variables
+func (e *Executor) getVariables(call Call) (Vars, error) {
+	t, ok := e.Tasks[call.Task]
+	if !ok {
+		return nil, &taskNotFoundError{call.Task}
+	}
 
-	merge := func(vars Vars) error {
-		for k, v := range vars {
-			v, err := e.handleDynamicVariableContent(v)
+	merge := func(dest Vars, srcs ...Vars) {
+		for _, src := range srcs {
+			for k, v := range src {
+				dest[k] = v
+			}
+		}
+	}
+	varsKeys := func(srcs ...Vars) []string {
+		m := make(map[string]struct{})
+		for _, src := range srcs {
+			for k := range src {
+				m[k] = struct{}{}
+			}
+		}
+		lst := make([]string, 0, len(m))
+		for k := range m {
+			lst = append(lst, k)
+		}
+		return lst
+	}
+	replaceVars := func(dest Vars, keys []string) error {
+		r := varReplacer{vars: dest}
+		for _, k := range keys {
+			v := dest[k]
+			dest[k] = Var{
+				Static: r.replace(v.Static),
+				Sh:     r.replace(v.Sh),
+			}
+		}
+		return r.err
+	}
+	resolveShell := func(dest Vars, keys []string) error {
+		for _, k := range keys {
+			v := dest[k]
+			static, err := e.handleDynamicVariableContent(v)
 			if err != nil {
 				return err
 			}
-			result[k] = Var{Static: v}
+			dest[k] = Var{Static: static}
 		}
 		return nil
 	}
-
-	if err := merge(e.taskvars); err != nil {
-		return nil, err
-	}
-	if err := merge(t.Vars); err != nil {
-		return nil, err
-	}
-	if err := merge(getEnvironmentVariables()); err != nil {
-		return nil, err
-	}
-	if err := merge(call.Vars); err != nil {
-		return nil, err
+	update := func(dest Vars, srcs ...Vars) error {
+		merge(dest, srcs...)
+		// updatedKeys ensures template evaluation is run only once.
+		updatedKeys := varsKeys(srcs...)
+		if err := replaceVars(dest, updatedKeys); err != nil {
+			return err
+		}
+		return resolveShell(dest, updatedKeys)
 	}
 
+	// Resolve taskvars variables to "result" with environment override variables.
+	override := getEnvironmentVariables()
+	result := make(Vars, len(e.taskvars)+len(t.Vars)+len(override))
+	if err := update(result, e.taskvars, override); err != nil {
+		return nil, err
+	}
+	// Resolve task variables to "result" with environment and call override variables.
+	merge(override, call.Vars)
+	if err := update(result, t.Vars, override); err != nil {
+		return nil, err
+	}
 	return result, nil
 }
 
@@ -176,13 +230,14 @@ func (e *Executor) handleDynamicVariableContent(v Var) (string, error) {
 // variables in almost all properties using the Go template package
 func (t *Task) ReplaceVariables(vars Vars) (*Task, error) {
 	r := varReplacer{vars: vars}
+
 	new := Task{
 		Desc:      r.replace(t.Desc),
 		Sources:   r.replaceSlice(t.Sources),
 		Generates: r.replaceSlice(t.Generates),
 		Status:    r.replaceSlice(t.Status),
 		Dir:       r.replace(t.Dir),
-		Vars:      r.replaceVars(t.Vars),
+		Vars:      nil,
 		Set:       r.replace(t.Set),
 		Env:       r.replaceVars(t.Env),
 		Silent:    t.Silent,