From a226ea120d0e3c23f71f3b89be79db1ff9a8971f Mon Sep 17 00:00:00 2001
From: Martin Atkins <mart@degeneration.co.uk>
Date: Sun, 18 Jun 2017 09:36:06 -0700
Subject: [PATCH] zclsyntax: catch and test for more errors in for expressions

---
 zcl/zclsyntax/expression.go      | 50 +++++++++++++++++++++++++++-----
 zcl/zclsyntax/expression_test.go | 40 ++++++++++++++++++++-----
 2 files changed, 75 insertions(+), 15 deletions(-)

diff --git a/zcl/zclsyntax/expression.go b/zcl/zclsyntax/expression.go
index 203dd14..9a69956 100644
--- a/zcl/zclsyntax/expression.go
+++ b/zcl/zclsyntax/expression.go
@@ -695,6 +695,44 @@ func (e *ForExpr) Value(ctx *zcl.EvalContext) (cty.Value, zcl.Diagnostics) {
 	childCtx := ctx.NewChild()
 	childCtx.Variables = map[string]cty.Value{}
 
+	// Before we start we'll do an early check to see if any CondExpr we've
+	// been given is of the wrong type. This isn't 100% reliable (it may
+	// be DynamicVal until real values are given) but it should catch some
+	// straightforward cases and prevent a barrage of repeated errors.
+	if e.CondExpr != nil {
+		if e.KeyVar != "" {
+			childCtx.Variables[e.KeyVar] = cty.DynamicVal
+		}
+		childCtx.Variables[e.ValVar] = cty.DynamicVal
+
+		result, condDiags := e.CondExpr.Value(childCtx)
+		diags = append(diags, condDiags...)
+		if result.IsNull() {
+			diags = append(diags, &zcl.Diagnostic{
+				Severity: zcl.DiagError,
+				Summary:  "Condition is null",
+				Detail:   "The value of the 'if' clause must not be null.",
+				Subject:  e.CondExpr.Range().Ptr(),
+				Context:  &e.SrcRange,
+			})
+			return cty.DynamicVal, diags
+		}
+		_, err := convert.Convert(result, cty.Bool)
+		if err != nil {
+			diags = append(diags, &zcl.Diagnostic{
+				Severity: zcl.DiagError,
+				Summary:  "Invalid 'for' condition",
+				Detail:   fmt.Sprintf("The 'if' clause value is invalid: %s.", err.Error()),
+				Subject:  e.CondExpr.Range().Ptr(),
+				Context:  &e.SrcRange,
+			})
+			return cty.DynamicVal, diags
+		}
+		if condDiags.HasErrors() {
+			return cty.DynamicVal, diags
+		}
+	}
+
 	if e.KeyExpr != nil {
 		// Producing an object
 		var vals map[string]cty.Value
@@ -731,14 +769,6 @@ func (e *ForExpr) Value(ctx *zcl.EvalContext) (cty.Value, zcl.Diagnostics) {
 					known = false
 					continue
 				}
-				if !includeRaw.IsKnown() {
-					// We will eventually return DynamicVal, but we'll continue
-					// iterating in case there are other diagnostics to gather
-					// for later elements.
-					known = false
-					continue
-				}
-
 				include, err := convert.Convert(includeRaw, cty.Bool)
 				if err != nil {
 					if known {
@@ -753,6 +783,10 @@ func (e *ForExpr) Value(ctx *zcl.EvalContext) (cty.Value, zcl.Diagnostics) {
 					known = false
 					continue
 				}
+				if !include.IsKnown() {
+					known = false
+					continue
+				}
 
 				if include.False() {
 					// Skip this element
diff --git a/zcl/zclsyntax/expression_test.go b/zcl/zclsyntax/expression_test.go
index 9766dc9..b717ece 100644
--- a/zcl/zclsyntax/expression_test.go
+++ b/zcl/zclsyntax/expression_test.go
@@ -607,30 +607,56 @@ upper(
 			1, // can't iterate over a string (even if it's unknown)
 		},
 		{
-			`[for v in ["a", "b"]: v if unk]`,
+			`[for v in ["a", "b"]: v if unkbool]`,
 			&zcl.EvalContext{
 				Variables: map[string]cty.Value{
-					"unk": cty.UnknownVal(cty.Bool),
+					"unkbool": cty.UnknownVal(cty.Bool),
 				},
 			},
 			cty.DynamicVal,
 			0,
 		},
 		{
-			`[for v in ["a", "b"]: v if unk]`,
+			`[for v in ["a", "b"]: v if nullbool]`,
 			&zcl.EvalContext{
 				Variables: map[string]cty.Value{
-					"unk": cty.UnknownVal(cty.Number),
+					"nullbool": cty.NullVal(cty.Bool),
 				},
 			},
 			cty.DynamicVal,
-			0, // if expression must be bool
+			1, // value of if clause must not be null
 		},
 		{
-			`[for v in ["a", "b"]: unk]`,
+			`[for v in ["a", "b"]: v if dyn]`,
 			&zcl.EvalContext{
 				Variables: map[string]cty.Value{
-					"unk": cty.UnknownVal(cty.String),
+					"dyn": cty.DynamicVal,
+				},
+			},
+			cty.DynamicVal,
+			0,
+		},
+		{
+			`[for v in ["a", "b"]: v if unknum]`,
+			&zcl.EvalContext{
+				Variables: map[string]cty.Value{
+					"unknum": cty.UnknownVal(cty.List(cty.Number)),
+				},
+			},
+			cty.DynamicVal,
+			1, // if expression must be bool
+		},
+		{
+			`[for i, v in ["a", "b"]: v if i + i]`,
+			nil,
+			cty.DynamicVal,
+			1, // if expression must be bool
+		},
+		{
+			`[for v in ["a", "b"]: unkstr]`,
+			&zcl.EvalContext{
+				Variables: map[string]cty.Value{
+					"unkstr": cty.UnknownVal(cty.String),
 				},
 			},
 			cty.TupleVal([]cty.Value{