core: Restore our EvalReadData behavior

In an earlier commit we changed objchange.ProposedNewObject so that the
task of populating unknown values for attributes not known during apply
is the responsibility of the provider's PlanResourceChange method, rather
than being handled automatically.

However, we were also using objchange.ProposedNewObject to construct the
placeholder new object for a deferred data resource read, and so we
inadvertently broke that deferral behavior. Here we restore the old
behavior by introducing a new function objchange.PlannedDataResourceObject
which is a specialized version of objchange.ProposedNewObject that
includes the forced behavior of populating unknown values, because the
provider gets no opportunity to customize a deferred read.

TestContext2Plan_createBeforeDestroy_depends_datasource required some
updates here because its implementation of PlanResourceChange was not
handling the insertion of the unknown value for attribute "computed".
The other changes here are just in an attempt to make the flow of this
test more obvious, by clarifying that it is simulating a -refresh=false
run, which effectively forces a deferred read since we skip the eager
read that would normally happen in the refresh step.

plans/objchange/objchange.go

@@ -32,6 +32,32 @@ func ProposedNewObject(schema *configschema.Block, prior, config cty.Value) cty.
 		// below by giving us one non-null level of object to pull values from.
 		prior = AllAttributesNull(schema)
 	}
+	return proposedNewObject(schema, prior, config)
+}
+
+// PlannedDataResourceObject is similar to ProposedNewObject but tailored for
+// planning data resources in particular. Specifically, it replaces the values
+// of any Computed attributes not set in the configuration with an unknown
+// value, which serves as a placeholder for a value to be filled in by the
+// provider when the data resource is finally read.
+//
+// Data resources are different because the planning of them is handled
+// entirely within Terraform Core and not subject to customization by the
+// provider. This function is, in effect, producing an equivalent result to
+// passing the ProposedNewObject result into a provider's PlanResourceChange
+// function, assuming a fixed implementation of PlanResourceChange that just
+// fills in unknown values as needed.
+func PlannedDataResourceObject(schema *configschema.Block, config cty.Value) cty.Value {
+	// Our trick here is to run the ProposedNewObject logic with an
+	// entirely-unknown prior value. Because of cty's unknown short-circuit
+	// behavior, any operation on prior returns another unknown, and so
+	// unknown values propagate into all of the parts of the resulting value
+	// that would normally be filled in by preserving the prior state.
+	prior := cty.UnknownVal(schema.ImpliedType())
+	return proposedNewObject(schema, prior, config)
+}
+
+func proposedNewObject(schema *configschema.Block, prior, config cty.Value) cty.Value {
 	if config.IsNull() || !config.IsKnown() {
 		// This is a weird situation, but we'll allow it anyway to free
 		// callers from needing to specifically check for these cases.

terraform/context_plan_test.go

@@ -4990,8 +4990,20 @@ func TestContext2Plan_createBeforeDestroy_depends_datasource(t *testing.T) {
 		},
 	}
 	p.PlanResourceChangeFn = func(req providers.PlanResourceChangeRequest) providers.PlanResourceChangeResponse {
+		computedVal := req.ProposedNewState.GetAttr("computed")
+		if computedVal.IsNull() {
+			computedVal = cty.UnknownVal(cty.String)
+		}
 		return providers.PlanResourceChangeResponse{
-			PlannedState: req.ProposedNewState,
+			PlannedState: cty.ObjectVal(map[string]cty.Value{
+				"num":      req.ProposedNewState.GetAttr("num"),
+				"computed": computedVal,
+			}),
+		}
+	}
+	p.ReadDataSourceFn = func(req providers.ReadDataSourceRequest) providers.ReadDataSourceResponse {
+		return providers.ReadDataSourceResponse{
+			Diagnostics: tfdiags.Diagnostics(nil).Append(fmt.Errorf("ReadDataSource called, but should not have been")),
 		}
 	}
 
@@ -5004,11 +5016,20 @@ func TestContext2Plan_createBeforeDestroy_depends_datasource(t *testing.T) {
 		),
 	})
 
+	// We're skipping ctx.Refresh here, which simulates what happens when
+	// running "terraform plan -refresh=false". As a result, we don't get our
+	// usual opportunity to read the data source during the refresh step and
+	// thus the plan call below is forced to produce a deferred read action.
+
 	plan, diags := ctx.Plan()
+	if p.ReadDataSourceCalled {
+		t.Errorf("ReadDataSource was called on the provider, but should not have been because we didn't refresh")
+	}
 	if diags.HasErrors() {
 		t.Fatalf("unexpected errors: %s", diags.Err())
 	}
 
+	seenAddrs := make(map[string]struct{})
 	for _, res := range plan.Changes.Resources {
 		var schema *configschema.Block
 		switch res.Addr.Resource.Resource.Mode {
@@ -5023,6 +5044,8 @@ func TestContext2Plan_createBeforeDestroy_depends_datasource(t *testing.T) {
 			t.Fatal(err)
 		}
 
+		seenAddrs[ric.Addr.String()] = struct{}{}
+
 		t.Run(ric.Addr.String(), func(t *testing.T) {
 			switch i := ric.Addr.String(); i {
 			case "aws_instance.foo[0]":
@@ -5046,6 +5069,10 @@ func TestContext2Plan_createBeforeDestroy_depends_datasource(t *testing.T) {
 					t.Fatalf("resource %s should be read, got %s", ric.Addr, ric.Action)
 				}
 				checkVals(t, objectVal(t, schema, map[string]cty.Value{
+					// In a normal flow we would've read an exact value in
+					// ReadDataSource, but because this test doesn't run
+					// cty.Refresh we have no opportunity to do that lookup
+					// and a deferred read is forced.
 					"id":  cty.UnknownVal(cty.String),
 					"foo": cty.StringVal("0"),
 				}), ric.After)
@@ -5054,6 +5081,10 @@ func TestContext2Plan_createBeforeDestroy_depends_datasource(t *testing.T) {
 					t.Fatalf("resource %s should be read, got %s", ric.Addr, ric.Action)
 				}
 				checkVals(t, objectVal(t, schema, map[string]cty.Value{
+					// In a normal flow we would've read an exact value in
+					// ReadDataSource, but because this test doesn't run
+					// cty.Refresh we have no opportunity to do that lookup
+					// and a deferred read is forced.
 					"id":  cty.UnknownVal(cty.String),
 					"foo": cty.StringVal("1"),
 				}), ric.After)
@@ -5062,6 +5093,16 @@ func TestContext2Plan_createBeforeDestroy_depends_datasource(t *testing.T) {
 			}
 		})
 	}
+
+	wantAddrs := map[string]struct{}{
+		"aws_instance.foo[0]": struct{}{},
+		"aws_instance.foo[1]": struct{}{},
+		"data.aws_vpc.bar[0]": struct{}{},
+		"data.aws_vpc.bar[1]": struct{}{},
+	}
+	if !cmp.Equal(seenAddrs, wantAddrs) {
+		t.Errorf("incorrect addresses in changeset:\n%s", cmp.Diff(wantAddrs, seenAddrs))
+	}
 }
 
 // interpolated lists need to be stored in the original order.

terraform/eval_read_data.go

@@ -104,7 +104,7 @@ func (n *EvalReadData) Eval(ctx EvalContext) (interface{}, error) {
 		return nil, diags.Err()
 	}
 
-	proposedNewVal := objchange.ProposedNewObject(schema, priorVal, configVal)
+	proposedNewVal := objchange.PlannedDataResourceObject(schema, configVal)
 
 	// If our configuration contains any unknown values then we must defer the
 	// read to the apply phase by producing a "Read" change for this resource,
@@ -119,7 +119,11 @@ func (n *EvalReadData) Eval(ctx EvalContext) (interface{}, error) {
 				absAddr,
 			)
 		}
+		if n.ForcePlanRead {
+			log.Printf("[TRACE] EvalReadData: %s configuration is fully known, but we're forcing a read plan to be created", absAddr)
+		} else {
 			log.Printf("[TRACE] EvalReadData: %s configuration not fully known yet, so deferring to apply phase", absAddr)
+		}
 
 		err := ctx.Hook(func(h Hook) (HookAction, error) {
 			return h.PreDiff(absAddr, states.CurrentGen, priorVal, proposedNewVal)