internal/fwserver: Ensure Attribute and Block plan modification retur…

…ns custom value type implementations (#768)

Reference: #754
Reference: #715 (precursor)
Reference: #767 (followup)

The framework recently was updated to perform stricter value type checking against the defined schema type when setting data. This was to prevent panics or other potentially confusing behaviors with mismatched types. The attribute-based plan modification logic in the framework however was missing some value type conversion, which could generate unavoidable `Value Conversion Error` diagnostics for provider developers when attributes/blocks used both `CustomType` and `PlanModifiers` fields.

This changeset ensures that attribute-based plan modification will always return the custom value type implementation of a value after a plan modifier response. All plan modifier responses are currently using the base value type, so the framework logic must handle converting it back. Even if the plan modifier responses were updated to use the `Valuable` interfaces, the framework would still need to perform this logic in case a plan modifier implementation opted to return the base value type, since all base value types implement the `Valuable` interfaces currently.

These changes handle custom types on all attribute and block implementations, however a non-trivial amount of internal code refactoring is necessary to fix this same issue for `NestedAttributeObject` and `NestedBlockObject` implementations that contain both `CustomType` and `PlanModifiers` fields. That effort will follow this one separately to reduce review burden.

Previously before logic updates:

```
--- FAIL: TestServerPlanResourceChange (0.00s)
    --- FAIL: TestServerPlanResourceChange/create-attributeplanmodifier-response-attributeplan-custom-type (0.00s)
        /Users/bflad/src/github.com/hashicorp/terraform-plugin-framework/internal/fwserver/server_planresourcechange_test.go:3848: unexpected difference:   &fwserver.PlanResourceChangeResponse{
            - 	Diagnostics: diag.Diagnostics{
            - 		diag.withPath{
            - 			Diagnostic: diag.ErrorDiagnostic{detail: "An unexpected error was encounte"..., summary: "Value Conversion Error"},
            - 			path:       s"test_computed",
            - 		},
            - 	},
            + 	Diagnostics:    nil,
              	PlannedPrivate: &{Provider: &{data: {}}},
              	PlannedState: &tfsdk.State{
            - 		Raw:    s`tftypes.Object["test_computed":tftypes.String, "test_other_computed":tftypes.String, "test_required":tftypes.String]<"test_computed":tftypes.String<unknown>, "test_other_computed":tftypes.String<unknown>, "test_required":tftypes.String<"test-config-value">>`,
            + 		Raw:    s`tftypes.Object["test_computed":tftypes.String, "test_other_computed":tftypes.String, "test_required":tftypes.String]<"test_computed":tftypes.String<"test-attributeplanmodifier-value">, "test_other_computed":tftypes.String<unknown>, "test_required":tftypes.`...,
              		Schema: schema.Schema{Attributes: {"test_computed": schema.StringAttribute{CustomType: s"StringTypeWithSemanticEquals(false)", Computed: true, PlanModifiers: {...}}, "test_other_computed": schema.StringAttribute{Computed: true}, "test_required": schema.StringAttribute{Required: true}}},
              	},
              	RequiresReplace: s"[]",
              }

--- FAIL: TestAttributePlanModifyString (0.00s)
    --- FAIL: TestAttributePlanModifyString/response-planvalue-custom-type (0.00s)
        /Users/bflad/src/github.com/hashicorp/terraform-plugin-framework/internal/fwserver/attribute_plan_modification_test.go:8315: unexpected difference:   &fwserver.ModifyAttributePlanResponse{
            - 	AttributePlan:   basetypes.StringValue{state: 2, value: "testvalue"},
            + 	AttributePlan:   types.StringValueWithSemanticEquals{StringValue: basetypes.StringValue{state: 2, value: "testvalue"}},
              	Diagnostics:     nil,
              	RequiresReplace: s"[]",
              	Private:         nil,
              }

--- FAIL: TestAttributeModifyPlan (0.00s)
    --- FAIL: TestAttributeModifyPlan/attribute-list-nested-usestateforunknown-custom-type (0.00s)
        /Users/bflad/src/github.com/hashicorp/terraform-plugin-framework/internal/fwserver/attribute_plan_modification_test.go:1733: Unexpected response (-wanted, +got):   fwserver.ModifyAttributePlanResponse{
            - 	AttributePlan: types.ListValueWithSemanticEquals{
            - 		ListValue: basetypes.ListValue{
            - 			elements: []attr.Value{
            - 				basetypes.ObjectValue{
            - 					attributes:     map[string]attr.Value{...},
            - 					attributeTypes: map[string]attr.Type{...},
            - 					state:          2,
            - 				},
            - 			},
            - 			elementType: basetypes.ObjectType{AttrTypes: map[string]attr.Type{"nested_computed": basetypes.StringType{}}},
            - 			state:       2,
            - 		},
            - 	},
            + 	AttributePlan: basetypes.ListValue{
            + 		elements: []attr.Value{
            + 			basetypes.ObjectValue{
            + 				attributes:     map[string]attr.Value{"nested_computed": basetypes.StringValue{...}},
            + 				attributeTypes: map[string]attr.Type{"nested_computed": basetypes.StringType{}},
            + 				state:          2,
            + 			},
            + 		},
            + 		elementType: basetypes.ObjectType{AttrTypes: map[string]attr.Type{"nested_computed": basetypes.StringType{}}},
            + 		state:       2,
            + 	},
              	Diagnostics:     nil,
              	RequiresReplace: s"[]",
              	Private:         nil,
              }

--- FAIL: TestBlockModifyPlan (0.00s)
    --- FAIL: TestBlockModifyPlan/block-list-usestateforunknown-custom-type (0.00s)
        /Users/bflad/src/github.com/hashicorp/terraform-plugin-framework/internal/fwserver/block_plan_modification_test.go:3099: Unexpected response (+wanted, -got):   fwserver.ModifyAttributePlanResponse{
            - 	AttributePlan: types.ListValueWithSemanticEquals{
            - 		ListValue: basetypes.ListValue{
            - 			elements: []attr.Value{
            - 				basetypes.ObjectValue{
            - 					attributes:     map[string]attr.Value{...},
            - 					attributeTypes: map[string]attr.Type{...},
            - 					state:          2,
            - 				},
            - 			},
            - 			elementType: basetypes.ObjectType{AttrTypes: map[string]attr.Type{"nested_computed": basetypes.StringType{}}},
            - 			state:       2,
            - 		},
            - 	},
            + 	AttributePlan: basetypes.ListValue{
            + 		elements: []attr.Value{
            + 			basetypes.ObjectValue{
            + 				attributes:     map[string]attr.Value{"nested_computed": basetypes.StringValue{...}},
            + 				attributeTypes: map[string]attr.Type{"nested_computed": basetypes.StringType{}},
            + 				state:          2,
            + 			},
            + 		},
            + 		elementType: basetypes.ObjectType{AttrTypes: map[string]attr.Type{"nested_computed": basetypes.StringType{}}},
            + 		state:       2,
            + 	},
              	Diagnostics:     nil,
              	RequiresReplace: s"[]",
              	Private:         nil,
              }

--- FAIL: TestBlockPlanModifyList (0.00s)
    --- FAIL: TestBlockPlanModifyList/response-planvalue-custom-type (0.00s)
        /Users/bflad/src/github.com/hashicorp/terraform-plugin-framework/internal/fwserver/block_plan_modification_test.go:3528: unexpected difference:   &fwserver.ModifyAttributePlanResponse{
            - 	AttributePlan: basetypes.ListValue{
            - 		elements:    []attr.Value{basetypes.StringValue{state: 2, value: "testvalue"}},
            - 		elementType: basetypes.StringType{},
            - 		state:       2,
            - 	},
            + 	AttributePlan: types.ListValueWithSemanticEquals{
            + 		ListValue: basetypes.ListValue{
            + 			elements:    []attr.Value{basetypes.StringValue{state: 2, value: "testvalue"}},
            + 			elementType: basetypes.StringType{},
            + 			state:       2,
            + 		},
            + 	},
              	Diagnostics:     nil,
              	RequiresReplace: s"[]",
              	Private:         nil,
              }
```

.changes/unreleased/BUG FIXES-20230613-133402.yaml

@@ -0,0 +1,6 @@
+kind: BUG FIXES
+body: 'resource/schema: Prevented `Value Conversion Error` diagnostics for attributes
+  and blocks implementing both `CustomType` and `PlanModifiers` fields'
+time: 2023-06-13T13:34:02.465635-04:00
+custom:
+  Issue: "754"

internal/fwserver/attr_type.go

@@ -0,0 +1,147 @@
+// Copyright (c) HashiCorp, Inc.
+// SPDX-License-Identifier: MPL-2.0
+
+package fwserver
+
+import (
+	"context"
+
+	"github.com/hashicorp/terraform-plugin-framework/diag"
+	"github.com/hashicorp/terraform-plugin-framework/path"
+	"github.com/hashicorp/terraform-plugin-framework/types/basetypes"
+)
+
+func coerceBoolTypable(ctx context.Context, schemaPath path.Path, valuable basetypes.BoolValuable) (basetypes.BoolTypable, diag.Diagnostics) {
+	typable, ok := valuable.Type(ctx).(basetypes.BoolTypable)
+
+	// Type() of a Valuable should always be a Typable to recreate the Valuable,
+	// but if for some reason it is not, raise an implementation error instead
+	// of a panic.
+	if !ok {
+		return nil, diag.Diagnostics{
+			attributePlanModificationTypableError(schemaPath, valuable),
+		}
+	}
+
+	return typable, nil
+}
+
+func coerceFloat64Typable(ctx context.Context, schemaPath path.Path, valuable basetypes.Float64Valuable) (basetypes.Float64Typable, diag.Diagnostics) {
+	typable, ok := valuable.Type(ctx).(basetypes.Float64Typable)
+
+	// Type() of a Valuable should always be a Typable to recreate the Valuable,
+	// but if for some reason it is not, raise an implementation error instead
+	// of a panic.
+	if !ok {
+		return nil, diag.Diagnostics{
+			attributePlanModificationTypableError(schemaPath, valuable),
+		}
+	}
+
+	return typable, nil
+}
+
+func coerceInt64Typable(ctx context.Context, schemaPath path.Path, valuable basetypes.Int64Valuable) (basetypes.Int64Typable, diag.Diagnostics) {
+	typable, ok := valuable.Type(ctx).(basetypes.Int64Typable)
+
+	// Type() of a Valuable should always be a Typable to recreate the Valuable,
+	// but if for some reason it is not, raise an implementation error instead
+	// of a panic.
+	if !ok {
+		return nil, diag.Diagnostics{
+			attributePlanModificationTypableError(schemaPath, valuable),
+		}
+	}
+
+	return typable, nil
+}
+
+func coerceListTypable(ctx context.Context, schemaPath path.Path, valuable basetypes.ListValuable) (basetypes.ListTypable, diag.Diagnostics) {
+	typable, ok := valuable.Type(ctx).(basetypes.ListTypable)
+
+	// Type() of a Valuable should always be a Typable to recreate the Valuable,
+	// but if for some reason it is not, raise an implementation error instead
+	// of a panic.
+	if !ok {
+		return nil, diag.Diagnostics{
+			attributePlanModificationTypableError(schemaPath, valuable),
+		}
+	}
+
+	return typable, nil
+}
+
+func coerceMapTypable(ctx context.Context, schemaPath path.Path, valuable basetypes.MapValuable) (basetypes.MapTypable, diag.Diagnostics) {
+	typable, ok := valuable.Type(ctx).(basetypes.MapTypable)
+
+	// Type() of a Valuable should always be a Typable to recreate the Valuable,
+	// but if for some reason it is not, raise an implementation error instead
+	// of a panic.
+	if !ok {
+		return nil, diag.Diagnostics{
+			attributePlanModificationTypableError(schemaPath, valuable),
+		}
+	}
+
+	return typable, nil
+}
+
+func coerceNumberTypable(ctx context.Context, schemaPath path.Path, valuable basetypes.NumberValuable) (basetypes.NumberTypable, diag.Diagnostics) {
+	typable, ok := valuable.Type(ctx).(basetypes.NumberTypable)
+
+	// Type() of a Valuable should always be a Typable to recreate the Valuable,
+	// but if for some reason it is not, raise an implementation error instead
+	// of a panic.
+	if !ok {
+		return nil, diag.Diagnostics{
+			attributePlanModificationTypableError(schemaPath, valuable),
+		}
+	}
+
+	return typable, nil
+}
+
+func coerceObjectTypable(ctx context.Context, schemaPath path.Path, valuable basetypes.ObjectValuable) (basetypes.ObjectTypable, diag.Diagnostics) {
+	typable, ok := valuable.Type(ctx).(basetypes.ObjectTypable)
+
+	// Type() of a Valuable should always be a Typable to recreate the Valuable,
+	// but if for some reason it is not, raise an implementation error instead
+	// of a panic.
+	if !ok {
+		return nil, diag.Diagnostics{
+			attributePlanModificationTypableError(schemaPath, valuable),
+		}
+	}
+
+	return typable, nil
+}
+
+func coerceSetTypable(ctx context.Context, schemaPath path.Path, valuable basetypes.SetValuable) (basetypes.SetTypable, diag.Diagnostics) {
+	typable, ok := valuable.Type(ctx).(basetypes.SetTypable)
+
+	// Type() of a Valuable should always be a Typable to recreate the Valuable,
+	// but if for some reason it is not, raise an implementation error instead
+	// of a panic.
+	if !ok {
+		return nil, diag.Diagnostics{
+			attributePlanModificationTypableError(schemaPath, valuable),
+		}
+	}
+
+	return typable, nil
+}
+
+func coerceStringTypable(ctx context.Context, schemaPath path.Path, valuable basetypes.StringValuable) (basetypes.StringTypable, diag.Diagnostics) {
+	typable, ok := valuable.Type(ctx).(basetypes.StringTypable)
+
+	// Type() of a Valuable should always be a Typable to recreate the Valuable,
+	// but if for some reason it is not, raise an implementation error instead
+	// of a panic.
+	if !ok {
+		return nil, diag.Diagnostics{
+			attributePlanModificationTypableError(schemaPath, valuable),
+		}
+	}
+
+	return typable, nil
+}

internal/fwserver/attr_value.go

@@ -16,52 +16,112 @@ import (
 	"github.com/hashicorp/terraform-plugin-framework/types/basetypes"
 )
 
-func coerceListValue(ctx context.Context, schemaPath path.Path, value attr.Value) (types.List, diag.Diagnostics) {
-	listVal, ok := value.(basetypes.ListValuable)
+func coerceListValuable(_ context.Context, schemaPath path.Path, value attr.Value) (basetypes.ListValuable, diag.Diagnostics) {
+	listValuable, ok := value.(basetypes.ListValuable)
 
 	if !ok {
 		return types.ListNull(nil), diag.Diagnostics{
 			schemaDataWalkError(schemaPath, value),
 		}
 	}
 
-	return listVal.ToListValue(ctx)
+	return listValuable, nil
 }
 
-func coerceMapValue(ctx context.Context, schemaPath path.Path, value attr.Value) (types.Map, diag.Diagnostics) {
-	mapVal, ok := value.(basetypes.MapValuable)
+func coerceListValue(ctx context.Context, schemaPath path.Path, value attr.Value) (types.List, diag.Diagnostics) {
+	listValuable, diags := coerceListValuable(ctx, schemaPath, value)
+
+	if diags.HasError() {
+		return types.ListNull(nil), diags
+	}
+
+	listValue, listValueDiags := listValuable.ToListValue(ctx)
+
+	// Ensure prior warnings are preserved.
+	diags.Append(listValueDiags...)
+
+	return listValue, diags
+}
+
+func coerceMapValuable(_ context.Context, schemaPath path.Path, value attr.Value) (basetypes.MapValuable, diag.Diagnostics) {
+	mapValuable, ok := value.(basetypes.MapValuable)
 
 	if !ok {
 		return types.MapNull(nil), diag.Diagnostics{
 			schemaDataWalkError(schemaPath, value),
 		}
 	}
 
-	return mapVal.ToMapValue(ctx)
+	return mapValuable, nil
 }
 
-func coerceObjectValue(ctx context.Context, schemaPath path.Path, value attr.Value) (types.Object, diag.Diagnostics) {
-	objectVal, ok := value.(basetypes.ObjectValuable)
+func coerceMapValue(ctx context.Context, schemaPath path.Path, value attr.Value) (types.Map, diag.Diagnostics) {
+	mapValuable, diags := coerceMapValuable(ctx, schemaPath, value)
+
+	if diags.HasError() {
+		return types.MapNull(nil), diags
+	}
+
+	mapValue, mapValueDiags := mapValuable.ToMapValue(ctx)
+
+	// Ensure prior warnings are preserved.
+	diags.Append(mapValueDiags...)
+
+	return mapValue, diags
+}
+
+func coerceObjectValuable(_ context.Context, schemaPath path.Path, value attr.Value) (basetypes.ObjectValuable, diag.Diagnostics) {
+	objectValuable, ok := value.(basetypes.ObjectValuable)
 
 	if !ok {
 		return types.ObjectNull(nil), diag.Diagnostics{
 			schemaDataWalkError(schemaPath, value),
 		}
 	}
 
-	return objectVal.ToObjectValue(ctx)
+	return objectValuable, nil
 }
 
-func coerceSetValue(ctx context.Context, schemaPath path.Path, value attr.Value) (types.Set, diag.Diagnostics) {
-	setVal, ok := value.(basetypes.SetValuable)
+func coerceObjectValue(ctx context.Context, schemaPath path.Path, value attr.Value) (types.Object, diag.Diagnostics) {
+	objectValuable, diags := coerceObjectValuable(ctx, schemaPath, value)
+
+	if diags.HasError() {
+		return types.ObjectNull(nil), diags
+	}
+
+	objectValue, objectValueDiags := objectValuable.ToObjectValue(ctx)
+
+	// Ensure prior warnings are preserved.
+	diags.Append(objectValueDiags...)
+
+	return objectValue, diags
+}
+
+func coerceSetValuable(_ context.Context, schemaPath path.Path, value attr.Value) (basetypes.SetValuable, diag.Diagnostics) {
+	setValuable, ok := value.(basetypes.SetValuable)
 
 	if !ok {
 		return types.SetNull(nil), diag.Diagnostics{
 			schemaDataWalkError(schemaPath, value),
 		}
 	}
 
-	return setVal.ToSetValue(ctx)
+	return setValuable, nil
+}
+
+func coerceSetValue(ctx context.Context, schemaPath path.Path, value attr.Value) (types.Set, diag.Diagnostics) {
+	setValuable, diags := coerceSetValuable(ctx, schemaPath, value)
+
+	if diags.HasError() {
+		return types.SetNull(nil), diags
+	}
+
+	setValue, setValueDiags := setValuable.ToSetValue(ctx)
+
+	// Ensure prior warnings are preserved.
+	diags.Append(setValueDiags...)
+
+	return setValue, diags
 }
 
 func listElemObject(ctx context.Context, schemaPath path.Path, list types.List, index int, description fwschemadata.DataDescription) (types.Object, diag.Diagnostics) {