Experimenting with more details for unknown for_each error

This was an attempt to try to report more context in an unknown for_each
error by dragging unknown value origin information through the normal
expression evaluation process using cty marks.

However, it doesn't actually work because Terraform evaluates each
expression separately in its own evaluation context, and so this trick
of sneakily re-evaluating an expression with marks only when we hit an
error can only ever find shallow references, which we can already find
statically anyway.

I'm just pushing this up to keep it for a possible second attempt later.
I think a second attempt would need something more like the analyzer
I built in #28944, but using marks and expression evaluation to deal with
its analyses instead of the custom static technique I tried there.

go.mod

@@ -114,7 +114,7 @@ require (
 	github.com/xanzy/ssh-agent v0.2.1
 	github.com/xiang90/probing v0.0.0-20160813154853-07dd2e8dfe18 // indirect
 	github.com/xlab/treeprint v0.0.0-20161029104018-1d6e34225557
-	github.com/zclconf/go-cty v1.8.4
+	github.com/zclconf/go-cty v1.8.5-0.20210621160911-b9c7d6be66e1
 	github.com/zclconf/go-cty-debug v0.0.0-20191215020915-b22d67c1ba0b
 	github.com/zclconf/go-cty-yaml v1.0.2
 	go.uber.org/atomic v1.3.2 // indirect

go.sum

@@ -614,6 +614,10 @@ github.com/zclconf/go-cty v1.2.0/go.mod h1:hOPWgoHbaTUnI5k4D2ld+GRpFJSCe6bCM7m1q
 github.com/zclconf/go-cty v1.8.0/go.mod h1:vVKLxnk3puL4qRAv72AO+W99LUD4da90g3uUAzyuvAk=
 github.com/zclconf/go-cty v1.8.4 h1:pwhhz5P+Fjxse7S7UriBrMu6AUJSZM5pKqGem1PjGAs=
 github.com/zclconf/go-cty v1.8.4/go.mod h1:vVKLxnk3puL4qRAv72AO+W99LUD4da90g3uUAzyuvAk=
+github.com/zclconf/go-cty v1.8.5-0.20210621160911-b9c7d6be66e1 h1:CDyjVKS+LzxBeEbAy5kmaCFGIwxLPKYa0FEX1gzaV8g=
+github.com/zclconf/go-cty v1.8.5-0.20210621160911-b9c7d6be66e1/go.mod h1:vVKLxnk3puL4qRAv72AO+W99LUD4da90g3uUAzyuvAk=
+github.com/zclconf/go-cty v1.8.5-0.20210625223433-df79fec1ecb9 h1:6nRiP2H2qtOJQi4wwdJICADhGGNRKcsDdw/cBJshZWI=
+github.com/zclconf/go-cty v1.8.5-0.20210625223433-df79fec1ecb9/go.mod h1:vVKLxnk3puL4qRAv72AO+W99LUD4da90g3uUAzyuvAk=
 github.com/zclconf/go-cty-debug v0.0.0-20191215020915-b22d67c1ba0b h1:FosyBZYxY34Wul7O/MSKey3txpPYyCqVO5ZyceuQJEI=
 github.com/zclconf/go-cty-debug v0.0.0-20191215020915-b22d67c1ba0b/go.mod h1:ZRKQfBXbGkpdV6QMzT3rU1kSTAnfu1dO8dPKjYprgj8=
 github.com/zclconf/go-cty-yaml v1.0.2 h1:dNyg4QLTrv2IfJpm7Wtxi55ed5gLGOlPrZ6kMd51hY0=

internal/lang/eval.go

@@ -11,6 +11,7 @@ import (
 	"github.com/hashicorp/terraform/internal/instances"
 	"github.com/hashicorp/terraform/internal/lang/blocktoattr"
 	"github.com/hashicorp/terraform/internal/tfdiags"
+	"github.com/zclconf/go-cty-debug/ctydebug"
 	"github.com/zclconf/go-cty/cty"
 	"github.com/zclconf/go-cty/cty/convert"
 )
@@ -167,6 +168,7 @@ func (s *Scope) EvalExpr(expr hcl.Expression, wantType cty.Type) (cty.Value, tfd
 		// it's likely evaluation will produce redundant copies of the same errors.
 		return cty.UnknownVal(wantType), diags
 	}
+	fmt.Print(ctydebug.ValueString(cty.ObjectVal(ctx.Variables)))
 
 	val, evalDiags := expr.Value(ctx)
 	diags = diags.Append(evalDiags)

internal/lang/marks/resource_derived.go

+package marks
+
+import (
+	"sort"
+
+	"github.com/hashicorp/terraform/internal/addrs"
+	"github.com/zclconf/go-cty/cty"
+)
+
+type resourceInstanceDerived struct {
+	instanceAddr addrs.AbsResourceInstance
+}
+
+// MarkAsResourceInstanceValue returns a version of the given value that
+// carries an additional mark recording that it originated from a particular
+// given resource instance.
+//
+// We don't use such markings for all values derived from resource instances,
+// but in some specialized situations we use this function and its companions
+// ResourceInstancesDerivedFrom and HasResourceInstanceValueMarks as part of
+// dynamic analysis to gather additional context to return in error messages.
+func MarkAsResourceInstanceValue(v cty.Value, instanceAddr addrs.AbsResourceInstance) cty.Value {
+	// We can't use an addrs.AbsResourceInstance _directly_ as a mark,
+	// because it contains a slice, but we'll wrap it up inside a
+	// pointer to a resourceInstanceDerived so cty can compare it by pointer
+	// equality. In practice that means that no two marks produced by
+	// this function will actually be equal, but that's okay because
+	// we'll dedupe them on the way back out in ResourceInstancesDerivedFrom.
+	// That defers the effort of deduping them to only if we actually end
+	// up using the marks, at the expense of some usually-minor additional
+	// overhead of potentially tracking the same address multiple times.
+	return v.Mark(&resourceInstanceDerived{instanceAddr})
+}
+
+// HasResourceInstanceValueMarks returns true if the given value carries any
+// marks that originated from a call to MarkAsResourceInstanceValue.
+//
+// This can be considerably cheaper than calling ResourceInstancesDerivedFrom
+// and testing whether the result is empty, because it avoids the need for
+// any deduping or sorting of the result, and for allocating a new slice to
+// return the results in.
+func HasResourceInstanceValueMarks(v cty.Value) bool {
+	marks := v.Marks()
+	for mark := range marks {
+		if _, ok := mark.(*resourceInstanceDerived); ok {
+			return true
+		}
+	}
+	return false
+}
+
+// ResourceInstancesDerivedFrom processes a value that was possibly previously
+// marked using MarkAsResourceInstanceValue, and if so returns a set
+// (presented as a slice) of all of the resource instance addresses the
+// value has been marked with.
+//
+// The result contains only one element for each distinct instance address,
+// and is in our usual sorting order for resource instance addresses.
+//
+// If the given value wasn't previously marked by MarkAsResourceInstanceValue
+// or derived from a value that was, the result will be empty.
+func ResourceInstancesDerivedFrom(v cty.Value) []addrs.AbsResourceInstance {
+	marks := v.Marks()
+	if len(marks) == 0 {
+		return nil // easy case for totally-unmarked values
+	}
+
+	// We need to do some extra work here to dedupe the addresses. We
+	// intentionally defer this to here, rather than doing it at marking
+	// time, so that we only pay the deduping overhead of doing this if we
+	// are actually going to use the result.
+	addrsUniq := make(map[string]addrs.AbsResourceInstance, len(marks))
+	for mark := range marks {
+		if mark, ok := mark.(*resourceInstanceDerived); ok {
+			// Here we assume that the string representation of an instance
+			// address is sufficiently unique, which it is in this case
+			// because we're only comparing instances to other instances.
+			// (This would not be valid if possibly comparing a mixture of
+			// resources and resource instances, due to ambiguity.)
+			addrsUniq[mark.instanceAddr.String()] = mark.instanceAddr
+		}
+	}
+
+	if len(addrsUniq) == 0 {
+		// Could get here if the value only has marks _other than_
+		// resourceInstanceDerived ones.
+		return nil
+	}
+
+	addrs := make([]addrs.AbsResourceInstance, 0, len(addrsUniq))
+	for _, addr := range addrsUniq {
+		addrs = append(addrs, addr)
+	}
+	sort.SliceStable(addrs, func(i, j int) bool {
+		return addrs[i].Less(addrs[j])
+	})
+	return addrs
+}

internal/terraform/eval_context.go

@@ -120,6 +120,25 @@ type EvalContext interface {
 	// addresses in this context.
 	EvaluationScope(self addrs.Referenceable, keyData InstanceKeyEvalData) *lang.Scope
 
+	// GetExprUnknownContributors takes the given HCL expression and, if it
+	// would return an unknown value when evaluated in the current context,
+	// returns a set of the resource instance addresses that the unknown value
+	// was derived from.
+	//
+	// This is a heuristic sort of function intended for returning additional
+	// context in error messages. Don't use it to control any main behavior.
+	//
+	// Callers must pass the same "self" and "keyData" they would've used to
+	// actually evaluate the expression, or the result may be incorrect.
+	//
+	// Don't use this function during a validation walk, because at that phase
+	// there can be unknown values for reasons other than pending resource
+	// instance changes, which this function cannot represent.
+	//
+	// If the given expression is not valid (that is, if it would've returned
+	// errors for normal evaluation) then the result may be incomplete.
+	GetExprUnknownContributors(expr hcl.Expression, self addrs.Referenceable, keyData InstanceKeyEvalData) []addrs.AbsResourceInstance
+
 	// SetModuleCallArguments defines values for the variables of a particular
 	// child module call.
 	//

internal/terraform/eval_context_builtin.go

@@ -7,6 +7,7 @@ import (
 	"sync"
 
 	"github.com/hashicorp/terraform/internal/instances"
+	"github.com/hashicorp/terraform/internal/lang/marks"
 	"github.com/hashicorp/terraform/internal/plans"
 	"github.com/hashicorp/terraform/internal/providers"
 	"github.com/hashicorp/terraform/internal/provisioners"
@@ -281,6 +282,17 @@ func (ctx *BuiltinEvalContext) EvaluateExpr(expr hcl.Expression, wantType cty.Ty
 	return scope.EvalExpr(expr, wantType)
 }
 
+func (ctx *BuiltinEvalContext) GetExprUnknownContributors(expr hcl.Expression, self addrs.Referenceable, keyData InstanceKeyEvalData) []addrs.AbsResourceInstance {
+	scope := ctx.EvaluationScope(self, keyData)
+	// We need to turn on the unknown source tracking mode in the underlying
+	// data source, which will then cause our result to be marked with
+	// unknown value source information that we can extract using
+	// marks.ResourceInstancesDerivedFrom.
+	scope.Data.(*evaluationStateData).TrackUnknownReferences = true
+	markedV, _ := scope.EvalExpr(expr, cty.DynamicPseudoType)
+	return marks.ResourceInstancesDerivedFrom(markedV)
+}
+
 func (ctx *BuiltinEvalContext) EvaluationScope(self addrs.Referenceable, keyData InstanceKeyEvalData) *lang.Scope {
 	if !ctx.pathSet {
 		panic("context path not set")

internal/terraform/eval_context_mock.go

@@ -100,6 +100,13 @@ type MockEvalContext struct {
 	EvaluateExprResult cty.Value
 	EvaluateExprDiags  tfdiags.Diagnostics
 
+	GetExprUnknownContributorsCalled     bool
+	GetExprUnknownContributorsExpr       hcl.Expression
+	GetExprUnknownContributorsSelf       addrs.Referenceable
+	GetExprUnknownContributorsKeyData    InstanceKeyEvalData
+	GetExprUnknownContributorsResultFunc func(hcl.Expression, addrs.Referenceable, InstanceKeyEvalData) []addrs.AbsResourceInstance
+	GetExprUnknownContributorsResult     []addrs.AbsResourceInstance
+
 	EvaluationScopeCalled  bool
 	EvaluationScopeSelf    addrs.Referenceable
 	EvaluationScopeKeyData InstanceKeyEvalData
@@ -244,6 +251,17 @@ func (c *MockEvalContext) EvaluateExpr(expr hcl.Expression, wantType cty.Type, s
 	return c.EvaluateExprResult, c.EvaluateExprDiags
 }
 
+func (c *MockEvalContext) GetExprUnknownContributors(expr hcl.Expression, self addrs.Referenceable, keyData InstanceKeyEvalData) []addrs.AbsResourceInstance {
+	c.GetExprUnknownContributorsCalled = true
+	c.GetExprUnknownContributorsExpr = expr
+	c.GetExprUnknownContributorsSelf = self
+	c.GetExprUnknownContributorsKeyData = keyData
+	if c.GetExprUnknownContributorsResultFunc != nil {
+		return c.GetExprUnknownContributorsResultFunc(expr, self, keyData)
+	}
+	return c.GetExprUnknownContributorsResult
+}
+
 // installSimpleEval is a helper to install a simple mock implementation of
 // both EvaluateBlock and EvaluateExpr into the receiver.
 //

internal/terraform/eval_for_each.go

@@ -2,6 +2,7 @@ package terraform
 
 import (
 	"fmt"
+	"strings"
 
 	"github.com/hashicorp/hcl/v2"
 	"github.com/hashicorp/terraform/internal/lang"
@@ -91,10 +92,12 @@ func evaluateForEachExpressionValue(expr hcl.Expression, ctx EvalContext, allowU
 		return nullMap, diags
 	case !forEachVal.IsKnown():
 		if !allowUnknown {
+			detailMsg := errInvalidForEachUnknownDetailWithSuggestions(ctx, expr)
+
 			diags = diags.Append(&hcl.Diagnostic{
 				Severity:    hcl.DiagError,
 				Summary:     "Invalid for_each argument",
-				Detail:      errInvalidForEachUnknownDetail,
+				Detail:      detailMsg,
 				Subject:     expr.Range().Ptr(),
 				Expression:  expr,
 				EvalContext: hclCtx,
@@ -126,10 +129,12 @@ func evaluateForEachExpressionValue(expr hcl.Expression, ctx EvalContext, allowU
 		// entire set as unknown
 		if !forEachVal.IsWhollyKnown() {
 			if !allowUnknown {
+				detailMsg := errInvalidForEachUnknownDetailWithSuggestions(ctx, expr)
+
 				diags = diags.Append(&hcl.Diagnostic{
 					Severity:    hcl.DiagError,
 					Summary:     "Invalid for_each argument",
-					Detail:      errInvalidForEachUnknownDetail,
+					Detail:      detailMsg,
 					Subject:     expr.Range().Ptr(),
 					Expression:  expr,
 					EvalContext: hclCtx,
@@ -174,6 +179,26 @@ func evaluateForEachExpressionValue(expr hcl.Expression, ctx EvalContext, allowU
 
 const errInvalidForEachUnknownDetail = `The "for_each" value depends on resource attributes that cannot be determined until apply, so Terraform cannot predict how many instances will be created. To work around this, use the -target argument to first apply only the resources that the for_each depends on.`
 
+func errInvalidForEachUnknownDetailWithSuggestions(ctx EvalContext, expr hcl.Expression) string {
+	// Hopefully we'll be able to give a hint about which resource
+	// instances we're deriving unknown values from here.
+	upstreamAddrs := ctx.GetExprUnknownContributors(expr, nil, EvalDataForNoInstanceKey)
+	switch {
+	case len(upstreamAddrs) == 1:
+		return fmt.Sprintf("%s\n\nThis expression seems to be derived from an attribute of %s.", errInvalidForEachUnknownDetail, upstreamAddrs[0])
+	case len(upstreamAddrs) != 0:
+		var msgBuf strings.Builder
+		msgBuf.WriteString(errInvalidForEachUnknownDetail)
+		msgBuf.WriteString("\n\nThis expression seems to be derived from attributes of some or all of the following resource instances:")
+		for _, addr := range upstreamAddrs {
+			fmt.Fprintf(&msgBuf, "\n  %s", addr)
+		}
+		return msgBuf.String()
+	default:
+		return errInvalidForEachUnknownDetail
+	}
+}
+
 // markSafeLengthInt allows calling LengthInt on marked values safely
 func markSafeLengthInt(val cty.Value) int {
 	v, _ := val.UnmarkDeep()

internal/terraform/evaluate.go

@@ -97,6 +97,17 @@ type evaluationStateData struct {
 	// Operation records the type of walk the evaluationStateData is being used
 	// for.
 	Operation walkOperation
+
+	// TrackUnknownReferences activates a special mode, off by default, where
+	// unknown values returned as part of resource instances will be marked
+	// by which resource instance they originated at, as an aid to giving
+	// better feedback in error messages for situations where unknown values
+	// are not allowed.
+	//
+	// To interpret the result of this mode, use
+	// marks.ResourceInstancesDerivedFrom with an unknown value returned
+	// from an evaluation context where this mode was enabled.
+	TrackUnknownReferences bool
 }
 
 // InstanceKeyEvalData is the old name for instances.RepetitionData, aliased
@@ -711,14 +722,17 @@ func (d *evaluationStateData) GetResource(addr addrs.Resource, rng tfdiags.Sourc
 	instances := map[addrs.InstanceKey]cty.Value{}
 	pendingDestroy := d.Evaluator.Changes.IsFullDestroy()
 	for key, is := range rs.Instances {
+		instAddr := addr.Instance(key).Absolute(d.ModulePath)
+
 		if is == nil || is.Current == nil {
 			// Assume we're dealing with an instance that hasn't been created yet.
 			instances[key] = cty.UnknownVal(ty)
+			if d.TrackUnknownReferences {
+				instances[key] = annotateUnknownValueResourceInstAddr(instances[key], instAddr)
+			}
 			continue
 		}
 
-		instAddr := addr.Instance(key).Absolute(d.ModulePath)
-
 		change := d.Evaluator.Changes.GetResourceInstanceChange(instAddr, states.CurrentGen)
 		if change != nil {
 			// Don't take any resources that are yet to be deleted into account.
@@ -765,6 +779,14 @@ func (d *evaluationStateData) GetResource(addr addrs.Resource, rng tfdiags.Sourc
 			}
 
 			instances[key] = val.MarkWithPaths(afterMarks)
+
+			if d.TrackUnknownReferences {
+				// In our reference-tracking mode we'll additionally transform
+				// the result so that any unknown values will be annotated
+				// as originating from this resource instance, if they did.
+				instances[key] = annotateUnknownValueResourceInstAddr(instances[key], instAddr)
+			}
+
 			continue
 		}
 
@@ -794,6 +816,8 @@ func (d *evaluationStateData) GetResource(addr addrs.Resource, rng tfdiags.Sourc
 			val = val.MarkWithPaths(marks)
 		}
 		instances[key] = val
+		// We don't need to deal with d.TrackUnknownReferences here because
+		// the value in the state can never have unknown values anyway.
 	}
 
 	var ret cty.Value
@@ -897,6 +921,26 @@ func (d *evaluationStateData) getResourceSchema(addr addrs.Resource, providerAdd
 	return schema
 }
 
+func annotateUnknownValueResourceInstAddr(obj cty.Value, addr addrs.AbsResourceInstance) cty.Value {
+	v, _ := cty.Transform(
+		obj,
+		func(path cty.Path, v cty.Value) (cty.Value, error) {
+			if !v.IsKnown() {
+				// We'll tend to only report the 'closest' resource
+				// instance to the expression here, because the
+				// values we're marking are coming from state or
+				// plan which doesn't preserve indirect instance
+				// marks and thus we'll "lose" the markings from
+				// upstream when a resource argument's value is
+				// derived from some other resource attribute.
+				return marks.MarkAsResourceInstanceValue(v, addr), nil
+			}
+			return v, nil
+		},
+	)
+	return v
+}
+
 func (d *evaluationStateData) GetTerraformAttr(addr addrs.TerraformAttr, rng tfdiags.SourceRange) (cty.Value, tfdiags.Diagnostics) {
 	var diags tfdiags.Diagnostics
 	switch addr.Name {