Skip to content

Commit

Permalink
bake: avoid early-exit for resolution failures
Browse files Browse the repository at this point in the history
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 <[email protected]>
  • Loading branch information
jedevc committed Feb 6, 2023
1 parent d9780e2 commit dc8a2b0
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 6 deletions.
18 changes: 18 additions & 0 deletions bake/hcl_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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" {
Expand Down
23 changes: 17 additions & 6 deletions bake/hclparser/hclparser.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,14 +62,19 @@ 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
}

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,
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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
}
Expand All @@ -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)
Expand Down Expand Up @@ -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
}
Expand Down

0 comments on commit dc8a2b0

Please sign in to comment.