hclsyntax: Improve conditional type mismatch errors (somewhat)

apparentlymart · apparentlymart · commit 97d54a95e0a0 · 2022-04-21T09:30:58.000-07:00
For a long time now we've had a very simplistic error message for the
case of conditional expression result arms not having the same type, which
only works for situations where the two types have differing "friendly
names" down in the cty layer.

Unfortunately due to the typical complexity of the structural type kinds
(object and tuple types) their friendly names are just "object" and
"tuple", which tends to lead us to seemingly-incorrect error messages
like:

    The true and false result expressions must have consistent types.
    The given expressions are object and object, respectively.

This then is an attempt to use some more specialized messaging in some
of the situations that led to that sort of weird message before. In
particular, this handles:
 - both types are object types but their attributes don't match
 - both types are tuple types but their elements don't match
 - both types are the same kind of collection of either object or tuple
   types which don't match

These are the three _shallow_ cases that the previous logic wasn't able to
properly describe. This still leaves unaddressed a hopefully-less-common
case of nested collections with differing structural types in their
depths, but still avoids generating a confusing error message by instead
generating a _very vague but still correct_ error message:

    At least one deeply-nested attribute or element is not compatible
    across both the 'true' and the 'false' value.

My intent here is to make HCL return something precise enough _most of the
time_, without letting perfect be the enemy of the good. This will
generate some quite obnoxious long messages for particularly complex
nested structures, but so far it appears that such values are relatively
rare inside conditional expressions and so we'll wait to see what arises
in practice before trying to handle those situations more concisely.

Ideally I would like to include some actionable feedback that in some
cases it can help to explicitly convert ambiguously-typed expressions
like "null" or tuples intended to be lists to the intended type, so that
the type unification step has more information to infer the author intent.
However, HCL itself doesn't have any builtins for such conversions and so
today any messaging about that would need to be generated up at the
application layer so the application can refer to whatever functions/etc
it provides for type conversion. It isn't clear how to do that with the
current design, so we'll leave that to be addressed another day.
diff --git a/hclsyntax/expression.go b/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
 }
diff --git a/hclsyntax/expression_test.go b/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,
