From dc8a2b03987f1266ace9a7cdcba92f5e4ccdb8ed Mon Sep 17 00:00:00 2001
From: Justin Chadwell <me@jedevc.com>
Date: Mon, 6 Feb 2023 14:29:02 +0000
Subject: [PATCH] bake: avoid early-exit for resolution failures

With changes made to allow lazy evaluation, we were early exiting if an
undefined name was detected, either for a variable or a function.

This had two key implications:

1. The error messages changed, and became significantly less
   informative.

   For example, we went from:

   > Unknown variable; There is no variable named "FO". Did you mean "FOO"?, and 1 other diagnostic(s)

   To

   > Invalid expression; undefined variable "FO"

2. Any issues in our function detection from funcCalls which cause JSON
   functions to be erroneously detected cause invalid functions to be
   resolved, which causes new name resolution errors.

To avoid the above problems, we can defer the error from an undefined
name until HCL evaluation - which produces the more informative errors,
and does not suffer from incorrectly detecting JSON functions.

Signed-off-by: Justin Chadwell <me@jedevc.com>
---
 bake/hcl_test.go            | 18 ++++++++++++++++++
 bake/hclparser/hclparser.go | 23 +++++++++++++++++------
 2 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/bake/hcl_test.go b/bake/hcl_test.go
index 6d6fda0e276..56d705add04 100644
--- a/bake/hcl_test.go
+++ b/bake/hcl_test.go
@@ -670,6 +670,24 @@ func TestJSONFunctions(t *testing.T) {
 	require.Equal(t, ptrstr("pre-<FOO-abc>"), c.Targets[0].Args["v1"])
 }
 
+func TestJSONInvalidFunctions(t *testing.T) {
+	dt := []byte(`{
+	"target": {
+		"app": {
+			"args": {
+				"v1": "myfunc(\"foo\")"
+			}
+		}
+	}}`)
+
+	c, err := ParseFile(dt, "docker-bake.json")
+	require.NoError(t, err)
+
+	require.Equal(t, 1, len(c.Targets))
+	require.Equal(t, c.Targets[0].Name, "app")
+	require.Equal(t, ptrstr(`myfunc("foo")`), c.Targets[0].Args["v1"])
+}
+
 func TestHCLFunctionInAttr(t *testing.T) {
 	dt := []byte(`
 	function "brace" {
diff --git a/bake/hclparser/hclparser.go b/bake/hclparser/hclparser.go
index 6a8a7926064..23477c43e05 100644
--- a/bake/hclparser/hclparser.go
+++ b/bake/hclparser/hclparser.go
@@ -62,7 +62,9 @@ type parser struct {
 	doneB     map[*hcl.Block]map[string]struct{}
 }
 
-func (p *parser) loadDeps(exp hcl.Expression, exclude map[string]struct{}) hcl.Diagnostics {
+var errUndefined = errors.New("undefined")
+
+func (p *parser) loadDeps(exp hcl.Expression, exclude map[string]struct{}, allowMissing bool) hcl.Diagnostics {
 	fns, hcldiags := funcCalls(exp)
 	if hcldiags.HasErrors() {
 		return hcldiags
@@ -70,6 +72,9 @@ func (p *parser) loadDeps(exp hcl.Expression, exclude map[string]struct{}) hcl.D
 
 	for _, fn := range fns {
 		if err := p.resolveFunction(fn); err != nil {
+			if allowMissing && errors.Is(err, errUndefined) {
+				continue
+			}
 			return hcl.Diagnostics{
 				&hcl.Diagnostic{
 					Severity: hcl.DiagError,
@@ -128,6 +133,9 @@ func (p *parser) loadDeps(exp hcl.Expression, exclude map[string]struct{}) hcl.D
 				}
 			}
 			if err := p.resolveBlock(blocks[0], target); err != nil {
+				if allowMissing && errors.Is(err, errUndefined) {
+					continue
+				}
 				return hcl.Diagnostics{
 					&hcl.Diagnostic{
 						Severity: hcl.DiagError,
@@ -140,6 +148,9 @@ func (p *parser) loadDeps(exp hcl.Expression, exclude map[string]struct{}) hcl.D
 			}
 		} else {
 			if err := p.resolveValue(v.RootName()); err != nil {
+				if allowMissing && errors.Is(err, errUndefined) {
+					continue
+				}
 				return hcl.Diagnostics{
 					&hcl.Diagnostic{
 						Severity: hcl.DiagError,
@@ -167,7 +178,7 @@ func (p *parser) resolveFunction(name string) error {
 		if _, ok := p.ectx.Functions[name]; ok {
 			return nil
 		}
-		return errors.Errorf("undefined function %s", name)
+		return errors.Wrapf(errUndefined, "function %q does not exit", name)
 	}
 	if _, ok := p.progressF[name]; ok {
 		return errors.Errorf("function cycle not allowed for %s", name)
@@ -217,7 +228,7 @@ func (p *parser) resolveFunction(name string) error {
 		return diags
 	}
 
-	if diags := p.loadDeps(f.Result.Expr, params); diags.HasErrors() {
+	if diags := p.loadDeps(f.Result.Expr, params, false); diags.HasErrors() {
 		return diags
 	}
 
@@ -255,7 +266,7 @@ func (p *parser) resolveValue(name string) (err error) {
 	if _, builtin := p.opt.Vars[name]; !ok && !builtin {
 		vr, ok := p.vars[name]
 		if !ok {
-			return errors.Errorf("undefined variable %q", name)
+			return errors.Wrapf(errUndefined, "variable %q does not exit", name)
 		}
 		def = vr.Default
 	}
@@ -270,7 +281,7 @@ func (p *parser) resolveValue(name string) (err error) {
 		return
 	}
 
-	if diags := p.loadDeps(def.Expr, nil); diags.HasErrors() {
+	if diags := p.loadDeps(def.Expr, nil, true); diags.HasErrors() {
 		return diags
 	}
 	vv, diags := def.Expr.Value(p.ectx)
@@ -395,7 +406,7 @@ func (p *parser) resolveBlock(block *hcl.Block, target *hcl.BodySchema) (err err
 		return diag
 	}
 	for _, a := range content.Attributes {
-		diag := p.loadDeps(a.Expr, nil)
+		diag := p.loadDeps(a.Expr, nil, true)
 		if diag.HasErrors() {
 			return diag
 		}