hclsyntax: Improve conditional type mismatch errors (somewhat)

hclsyntax/expression.go

@@ -2,6 +2,7 @@ package hclsyntax
 
 import (
 	"fmt"
+	"sort"
 	"sync"
 
 	"github.com/hashicorp/hcl/v2"
@@ -615,12 +616,8 @@ func (e *ConditionalExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostic
 				Severity: hcl.DiagError,
 				Summary:  "Inconsistent conditional result types",
 				Detail: fmt.Sprintf(
-					// FIXME: Need a helper function for showing natural-language type diffs,
-					// since this will generate some useless messages in some cases, like
-					// "These expressions are object and object respectively" if the
-					// object types don't exactly match.
-					"The true and false result expressions must have consistent types. The given expressions are %s and %s, respectively.",
-					trueResult.Type().FriendlyName(), falseResult.Type().FriendlyName(),
+					"The true and false result expressions must have consistent types. %s.",
+					describeConditionalTypeMismatch(trueResult.Type(), falseResult.Type()),
 				),
 				Subject:     hcl.RangeBetween(e.TrueResult.Range(), e.FalseResult.Range()).Ptr(),
 				Context:     &e.SrcRange,
@@ -652,7 +649,7 @@ func (e *ConditionalExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostic
 		diags = append(diags, &hcl.Diagnostic{
 			Severity:    hcl.DiagError,
 			Summary:     "Incorrect condition type",
-			Detail:      fmt.Sprintf("The condition expression must be of type bool."),
+			Detail:      "The condition expression must be of type bool.",
 			Subject:     e.Condition.Range().Ptr(),
 			Context:     &e.SrcRange,
 			Expression:  e.Condition,
@@ -712,6 +709,144 @@ func (e *ConditionalExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostic
 	}
 }
 
+// describeConditionalTypeMismatch makes a best effort to describe the
+// difference between types in the true and false arms of a conditional
+// expression in a way that would be useful to someone trying to understand
+// why their conditional expression isn't valid.
+//
+// NOTE: This function is only designed to deal with situations
+// where trueTy and falseTy are different. Calling it with two equal
+// types will produce a nonsense result. This function also only really
+// deals with situations that type unification can't resolve, so we should
+// call this function only after trying type unification first.
+func describeConditionalTypeMismatch(trueTy, falseTy cty.Type) string {
+	// The main tricky cases here are when both trueTy and falseTy are
+	// of the same structural type kind, such as both being object types
+	// or both being tuple types. In that case the "FriendlyName" method
+	// returns only "object" or "tuple" and so we need to do some more
+	// work to describe what's different inside them.
+
+	switch {
+	case trueTy.IsObjectType() && falseTy.IsObjectType():
+		// We'll first gather up the attribute names and sort them. In the
+		// event that there are multiple attributes that disagree across
+		// the two types, we'll prefer to report the one that sorts lexically
+		// least just so that our error message is consistent between
+		// evaluations.
+		var trueAttrs, falseAttrs []string
+		for name := range trueTy.AttributeTypes() {
+			trueAttrs = append(trueAttrs, name)
+		}
+		sort.Strings(trueAttrs)
+		for name := range falseTy.AttributeTypes() {
+			falseAttrs = append(falseAttrs, name)
+		}
+		sort.Strings(falseAttrs)
+
+		for _, name := range trueAttrs {
+			if !falseTy.HasAttribute(name) {
+				return fmt.Sprintf("The 'true' value includes object attribute %q, which is absent in the 'false' value", name)
+			}
+			trueAty := trueTy.AttributeType(name)
+			falseAty := falseTy.AttributeType(name)
+			if !trueAty.Equals(falseAty) {
+				// For deeply-nested differences this will likely get very
+				// clunky quickly by nesting these messages inside one another,
+				// but we'll accept that for now in the interests of producing
+				// _some_ useful feedback, even if it isn't as concise as
+				// we'd prefer it to be. Deeply-nested structures in
+				// conditionals are thankfully not super common.
+				return fmt.Sprintf(
+					"Type mismatch for object attribute %q: %s",
+					name, describeConditionalTypeMismatch(trueAty, falseAty),
+				)
+			}
+		}
+		for _, name := range falseAttrs {
+			if !trueTy.HasAttribute(name) {
+				return fmt.Sprintf("The 'false' value includes object attribute %q, which is absent in the 'true' value", name)
+			}
+			// NOTE: We don't need to check the attribute types again, because
+			// any attribute that both types have in common would already have
+			// been checked in the previous loop.
+		}
+	case trueTy.IsTupleType() && falseTy.IsTupleType():
+		trueEtys := trueTy.TupleElementTypes()
+		falseEtys := falseTy.TupleElementTypes()
+
+		if trueCount, falseCount := len(trueEtys), len(falseEtys); trueCount != falseCount {
+			return fmt.Sprintf("The 'true' tuple has length %d, but the 'false' tuple has length %d", trueCount, falseCount)
+		}
+
+		// NOTE: Thanks to the condition above, we know that both tuples are
+		// of the same length and so they must have some differing types
+		// instead.
+		for i := range trueEtys {
+			trueEty := trueEtys[i]
+			falseEty := falseEtys[i]
+
+			if !trueEty.Equals(falseEty) {
+				// For deeply-nested differences this will likely get very
+				// clunky quickly by nesting these messages inside one another,
+				// but we'll accept that for now in the interests of producing
+				// _some_ useful feedback, even if it isn't as concise as
+				// we'd prefer it to be. Deeply-nested structures in
+				// conditionals are thankfully not super common.
+				return fmt.Sprintf(
+					"Type mismatch for tuple element %d: %s",
+					i, describeConditionalTypeMismatch(trueEty, falseEty),
+				)
+			}
+		}
+	case trueTy.IsCollectionType() && falseTy.IsCollectionType():
+		// For this case we're specifically interested in the situation where:
+		// - both collections are of the same kind, AND
+		// - the element types of both are either object or tuple types.
+		// This is just to avoid writing a useless statement like
+		// "The 'true' value is list of object, but the 'false' value is list of object".
+		// This still doesn't account for more awkward cases like collections
+		// of collections of structural types, but we won't let perfect be
+		// the enemy of the good.
+		trueEty := trueTy.ElementType()
+		falseEty := falseTy.ElementType()
+		if (trueTy.IsListType() && falseTy.IsListType()) || (trueTy.IsMapType() && falseTy.IsMapType()) || (trueTy.IsSetType() && falseTy.IsSetType()) {
+			if (trueEty.IsObjectType() && falseEty.IsObjectType()) || (trueEty.IsTupleType() && falseEty.IsTupleType()) {
+				noun := "collection"
+				switch { // NOTE: We now know that trueTy and falseTy have the same collection kind
+				case trueTy.IsListType():
+					noun = "list"
+				case trueTy.IsSetType():
+					noun = "set"
+				case trueTy.IsMapType():
+					noun = "map"
+				}
+				return fmt.Sprintf(
+					"Mismatched %s element types: %s",
+					noun, describeConditionalTypeMismatch(trueEty, falseEty),
+				)
+			}
+		}
+	}
+
+	// If we don't manage any more specialized message, we'll just report
+	// what the two types are.
+	trueName := trueTy.FriendlyName()
+	falseName := falseTy.FriendlyName()
+	if trueName == falseName {
+		// Absolute last resort for when we have no special rule above but
+		// we have two types with the same friendly name anyway. This is
+		// the most vague of all possible messages but is reserved for
+		// particularly awkward cases, like lists of lists of differing tuple
+		// types.
+		return "At least one deeply-nested attribute or element is not compatible across both the 'true' and the 'false' value"
+	}
+	return fmt.Sprintf(
+		"The 'true' value is %s, but the 'false' value is %s",
+		trueTy.FriendlyName(), falseTy.FriendlyName(),
+	)
+
+}
+
 func (e *ConditionalExpr) Range() hcl.Range {
 	return e.SrcRange
 }

hclsyntax/expression_test.go

@@ -1892,6 +1892,127 @@ EOT
 
 }
 
+func TestExpressionErrorMessages(t *testing.T) {
+	tests := []struct {
+		input       string
+		ctx         *hcl.EvalContext
+		wantSummary string
+		wantDetail  string
+	}{
+		// Error messages describing inconsistent result types for conditional expressions.
+		{
+			"true ? 1 : true",
+			nil,
+			"Inconsistent conditional result types",
+			"The true and false result expressions must have consistent types. The 'true' value is number, but the 'false' value is bool.",
+		},
+		{
+			"true ? [1] : [true]",
+			nil,
+			"Inconsistent conditional result types",
+			"The true and false result expressions must have consistent types. Type mismatch for tuple element 0: The 'true' value is number, but the 'false' value is bool.",
+		},
+		{
+			"true ? [1] : [1, true]",
+			nil,
+			"Inconsistent conditional result types",
+			"The true and false result expressions must have consistent types. The 'true' tuple has length 1, but the 'false' tuple has length 2.",
+		},
+		{
+			"true ? { a = 1 } : { a = true }",
+			nil,
+			"Inconsistent conditional result types",
+			"The true and false result expressions must have consistent types. Type mismatch for object attribute \"a\": The 'true' value is number, but the 'false' value is bool.",
+		},
+		{
+			"true ? { a = true, b = 1 } : { a = true }",
+			nil,
+			"Inconsistent conditional result types",
+			"The true and false result expressions must have consistent types. The 'true' value includes object attribute \"b\", which is absent in the 'false' value.",
+		},
+		{
+			"true ? { a = true } : { a = true, b = 1 }",
+			nil,
+			"Inconsistent conditional result types",
+			"The true and false result expressions must have consistent types. The 'false' value includes object attribute \"b\", which is absent in the 'true' value.",
+		},
+		{
+			"true ? listOf1Tuple : listOf0Tuple",
+			&hcl.EvalContext{
+				Variables: map[string]cty.Value{
+					"listOf1Tuple": cty.ListVal([]cty.Value{cty.TupleVal([]cty.Value{cty.True})}),
+					"listOf0Tuple": cty.ListVal([]cty.Value{cty.EmptyTupleVal}),
+				},
+			},
+			"Inconsistent conditional result types",
+			"The true and false result expressions must have consistent types. Mismatched list element types: The 'true' tuple has length 1, but the 'false' tuple has length 0.",
+		},
+		{
+			"true ? setOf1Tuple : setOf0Tuple",
+			&hcl.EvalContext{
+				Variables: map[string]cty.Value{
+					"setOf1Tuple": cty.SetVal([]cty.Value{cty.TupleVal([]cty.Value{cty.True})}),
+					"setOf0Tuple": cty.SetVal([]cty.Value{cty.EmptyTupleVal}),
+				},
+			},
+			"Inconsistent conditional result types",
+			"The true and false result expressions must have consistent types. Mismatched set element types: The 'true' tuple has length 1, but the 'false' tuple has length 0.",
+		},
+		{
+			"true ? mapOf1Tuple : mapOf2Tuple",
+			&hcl.EvalContext{
+				Variables: map[string]cty.Value{
+					"mapOf1Tuple": cty.MapVal(map[string]cty.Value{"a": cty.TupleVal([]cty.Value{cty.True})}),
+					"mapOf2Tuple": cty.MapVal(map[string]cty.Value{"a": cty.TupleVal([]cty.Value{cty.True, cty.Zero})}),
+				},
+			},
+			"Inconsistent conditional result types",
+			"The true and false result expressions must have consistent types. Mismatched map element types: The 'true' tuple has length 1, but the 'false' tuple has length 2.",
+		},
+		{
+			"true ? listOfListOf1Tuple : listOfListOf0Tuple",
+			&hcl.EvalContext{
+				Variables: map[string]cty.Value{
+					"listOfListOf1Tuple": cty.ListVal([]cty.Value{cty.ListVal([]cty.Value{cty.TupleVal([]cty.Value{cty.True})})}),
+					"listOfListOf0Tuple": cty.ListVal([]cty.Value{cty.ListVal([]cty.Value{cty.EmptyTupleVal})}),
+				},
+			},
+			"Inconsistent conditional result types",
+			// This is our totally non-specific last-resort of an error message,
+			// for situations that are too complex for any of our rules to
+			// describe coherently.
+			"The true and false result expressions must have consistent types. At least one deeply-nested attribute or element is not compatible across both the 'true' and the 'false' value.",
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.input, func(t *testing.T) {
+			var diags hcl.Diagnostics
+			expr, parseDiags := ParseExpression([]byte(test.input), "", hcl.Pos{Line: 1, Column: 1, Byte: 0})
+			diags = append(diags, parseDiags...)
+			_, valDiags := expr.Value(test.ctx)
+			diags = append(diags, valDiags...)
+
+			if !diags.HasErrors() {
+				t.Fatalf("unexpected success\nwant error:\n%s; %s", test.wantSummary, test.wantDetail)
+			}
+
+			for _, diag := range diags {
+				if diag.Severity != hcl.DiagError {
+					continue
+				}
+				if diag.Summary == test.wantSummary && diag.Detail == test.wantDetail {
+					// Success! We'll return early to conclude this test case.
+					return
+				}
+			}
+			// If we fall out here then we didn't find the diagnostic
+			// we were looking for.
+			t.Fatalf("missing expected error\ngot:\n%s\n\nwant error:\n%s; %s", diags.Error(), test.wantSummary, test.wantDetail)
+		})
+	}
+}
+
 func TestFunctionCallExprValue(t *testing.T) {
 	funcs := map[string]function.Function{
 		"length":     stdlib.StrlenFunc,

hclsyntax/expression_vars_gen.go

@@ -4,6 +4,7 @@
 // just wraps the package-level function "Variables" and uses an AST walk
 // to do its work.
 
+//go:build ignore
 // +build ignore
 
 package main