types: Deprecate attr.Value type Null, Unknown, and Value fields

Reference: #447

When the framework type system was originally being developed, the value types were introduced with exported fields which also served as the internal details of whether a value was null, unknown, or a known value of a friendlier Go type. It was known that there was the potential for issues, but the simplified developer experience seemed to outweigh the potential for developer issues. Fast forward a few months, this decision appears to have two consequences that the framework maintainers hear about across various forums.

One issue is that the value types directly expose their internal implementation details and support the three states of a Terraform type value: being null, unknown, or a known value. Only one state should ever be set, but provider developers can make a value that is any combination of those states. This makes the framework behavior potentially indeterminate from the provider developer perspective whether, for example, a null AND unknown value becomes null OR unknown as it works its way through the framework.

```go
In this example, the logic has created an invalid null AND unknown value:

type ThingResourceModel struct{
  Computed types.String `tfsdk:"computed"`
}

func (r ThingResource) Create(ctx context.Context, req resource.CreateResource, resp *resource.CreateResponse) {
  var data ThingResourceModel

  resp.Diagnostics.Append(req.Plan.Get(ctx, &data))

  tflog.Trace(ctx, "Data Values", map[string]any{
    // Unknown value: types.String{Null: false, Unknown: true, Value: ""}
    "computed": plan.Computed,
  })

  // Maybe some external API responses here, but showing hardcoded updates for
  // brevity. This will make the value invalid by enabling Null without
  // disabling Unknown.
  data.Computed.Null = true

  tflog.Trace(ctx, "Data Values", map[string]any{
    // Invalid value: types.String{Null: true, Unknown: true, Value: ""}
    "computed": data.Computed,
  })

  // The invalid value will be either null or unknown, depending on the
  // type implementation. If unknown, Terraform will error, since unknown
  // values are never allowed in state.
  resp.Diagnostics.Append(resp.State.Set(ctx, &data))
}
```

Another issue is that the default (zero-value) state for an "unset" value type turns into a known value, which is confusing since these values explicitly support being null. This causes Terraform errors which would surface to practitioners (especially when untested) that provider developers then have to troubleshoot the error message containing Terraform's type system details, potentially discover the reason why it is happening by looking at the framework type source code, then figure out a workable solution. It's not intuitive.

```go
type ThingResourceModel struct{
  // let's assume this is left unconfigured (null in config and plan)
  Optional types.String `tfsdk:"optional"`
}

func (r ThingResource) Create(ctx context.Context, req resource.CreateResource, resp *resource.CreateResponse) {
  // Providers can opt to use a single variable that is updated based on an
  // external response, however that logic can be more difficult sometimes,
  // so it can be easier to split them. Showing the split way to exemplify
  // the "unset" problem.
  var plan, state ThingResourceModel

  resp.Diagnostics.Append(req.Plan.Get(ctx, &plan))

  tflog.Trace(ctx, "Plan Values", map[string]any{
    // Null value: types.String{Null: true, Unknown: false, Value: ""}
    "optional": plan.Optional,
  })

  // Maybe some external API responses here, but intentionally not
  // doing any state.Optional setting, which might happen if the
  // external response for that data was null for example.

  tflog.Trace(ctx, "State Values", map[string]any{
    // Zero-value: types.String{Null: false, Unknown: false, Value: ""}
    "optional": state.Optional,
  })

  // The state zero-value will later cause Terraform to error, such as:
  // Error: Provider produced inconsistent result after apply
  // ... expected cty.NullVal(cty.String), got cty.StringVal("")
  // Since the plan value said it would be null.
  resp.Diagnostics.Append(resp.State.Set(ctx, &state))
}
```

This deprecation of the fields in preference of functions and methods aims to unexport the internal details and treat the value types as immutable once they are created. When provider developers switch over to the new model, any errant changes to the deprecated exported fields will have no effect. A future version will remove the exported fields entirely.

Author: bflad

.changelog/pending.txt

@@ -0,0 +1,11 @@
+```release-note:note
+types: The `Bool` type `Null`, `Unknown`, and `Value` fields have been deprecated in preference of the `NullBool()`, `UnknownBool()`, and `ValueBool()` creation functions and `BoolValue()`, `IsNull()`, and `IsUnknown()` methods. The fields will be removed in a future release.
+```
+
+```release-note:enhancement
+types: Added `NullBool()`, `UnknownBool()`, `ValueBool` functions, which create immutable `Bool` values
+```
+
+```release-note:enhancement
+types: Added `Bool` type `BoolValue()` method, which returns the `bool` of the known value or `false` if null or unknown
+```

internal/reflect/primitive_test.go

@@ -160,9 +160,7 @@ func TestFromBool(t *testing.T) {
 			val: true,
 			typ: testtypes.BoolTypeWithValidateWarning{},
 			expected: testtypes.Bool{
-				Bool: types.Bool{
-					Value: true,
-				},
+				Bool:      types.ValueBool(true),
 				CreatedBy: testtypes.BoolTypeWithValidateWarning{},
 			},
 			expectedDiags: diag.Diagnostics{

internal/testing/types/bool.go

@@ -41,13 +41,13 @@ func (t BoolType) TerraformType(_ context.Context) tftypes.Type {
 func (t BoolType) ValueFromTerraform(ctx context.Context, in tftypes.Value) (attr.Value, error) {
 	if in.IsNull() {
 		return Bool{
-			Bool:      types.Bool{Null: true},
+			Bool:      types.NullBool(),
 			CreatedBy: t,
 		}, nil
 	}
 	if !in.IsKnown() {
 		return Bool{
-			Bool:      types.Bool{Unknown: true},
+			Bool:      types.UnknownBool(),
 			CreatedBy: t,
 		}, nil
 	}
@@ -56,7 +56,7 @@ func (t BoolType) ValueFromTerraform(ctx context.Context, in tftypes.Value) (att
 	if err != nil {
 		return nil, err
 	}
-	return Bool{Bool: types.Bool{Value: b}, CreatedBy: t}, nil
+	return Bool{Bool: types.ValueBool(b), CreatedBy: t}, nil
 }
 
 // ValueType returns the Value type.
@@ -91,13 +91,5 @@ func (b Bool) IsUnknown() bool {
 }
 
 func (b Bool) String() string {
-	if b.Bool.IsUnknown() {
-		return attr.UnknownValueString
-	}
-
-	if b.Bool.IsNull() {
-		return attr.NullValueString
-	}
-
-	return fmt.Sprintf("%t", b.Value)
+	return b.Bool.String()
 }

types/bool.go

@@ -12,37 +12,113 @@ var (
 	_ attr.Value = Bool{}
 )
 
+// NullBool creates a Bool with a null value. Determine whether the value is
+// null via the Bool type IsNull method.
+//
+// Setting the deprecated Bool type Null, Unknown, or Value fields after
+// creating a Bool with this function has no effect.
+func NullBool() Bool {
+	return Bool{
+		state: valueStateNull,
+	}
+}
+
+// UnknownBool creates a Bool with an unknown value. Determine whether the
+// value is unknown via the Bool type IsUnknown method.
+//
+// Setting the deprecated Bool type Null, Unknown, or Value fields after
+// creating a Bool with this function has no effect.
+func UnknownBool() Bool {
+	return Bool{
+		state: valueStateUnknown,
+	}
+}
+
+// ValueBool creates a Bool with a known value. Access the value via the Bool
+// type BoolValue method.
+//
+// Setting the deprecated Bool type Null, Unknown, or Value fields after
+// creating a Bool with this function has no effect.
+func ValueBool(value bool) Bool {
+	return Bool{
+		state: valueStateKnown,
+		value: value,
+	}
+}
+
 func boolValueFromTerraform(ctx context.Context, in tftypes.Value) (attr.Value, error) {
 	if in.IsNull() {
 		return Bool{
-			Null: true,
+			Null:  true,
+			state: valueStateDeprecated,
 		}, nil
 	}
 	if !in.IsKnown() {
 		return Bool{
 			Unknown: true,
+			state:   valueStateDeprecated,
 		}, nil
 	}
 	var b bool
 	err := in.As(&b)
 	if err != nil {
 		return nil, err
 	}
-	return Bool{Value: b}, nil
+	return Bool{
+		Value: b,
+		state: valueStateDeprecated,
+	}, nil
 }
 
 // Bool represents a boolean value.
 type Bool struct {
 	// Unknown will be true if the value is not yet known.
+	//
+	// If the Bool was created with the BoolValue, NullBool, or UnknownBool
+	// functions, changing this field has no effect.
+	//
+	// Deprecated: Use the [UnknownBool] function to create an unknown Bool
+	// value or use the [IsUnknown] method to determine whether the Bool value
+	// is unknown instead.
 	Unknown bool
 
 	// Null will be true if the value was not set, or was explicitly set to
 	// null.
+	//
+	// If the Bool was created with the BoolValue, NullBool, or UnknownBool
+	// functions, changing this field has no effect.
+	//
+	// Deprecated: Use the [NullBool] function to create a null Bool value or
+	// use the [IsNull] method to determine whether the Bool value is null
+	// instead.
 	Null bool
 
 	// Value contains the set value, as long as Unknown and Null are both
 	// false.
+	//
+	// If the Bool was created with the BoolValue, NullBool, or UnknownBool
+	// functions, changing this field has no effect.
+	//
+	// Deprecated: Use the [BoolValue] function to create a known Bool value or
+	// use the [BoolValue] method to retrieve the Bool value instead.
 	Value bool
+
+	// state represents whether the value is null, unknown, or known. The
+	// zero-value for this field is null.
+	state valueState
+
+	// value contains the known value, if not null or unknown.
+	value bool
+}
+
+// BoolValue returns the known bool value. If Bool is null or unknown, returns
+// false.
+func (b Bool) BoolValue() bool {
+	if b.state == valueStateDeprecated {
+		return b.Value
+	}
+
+	return b.value
 }
 
 // Type returns a BoolType.
@@ -52,16 +128,31 @@ func (b Bool) Type(_ context.Context) attr.Type {
 
 // ToTerraformValue returns the data contained in the Bool as a tftypes.Value.
 func (b Bool) ToTerraformValue(_ context.Context) (tftypes.Value, error) {
-	if b.Null {
+	switch b.state {
+	case valueStateDeprecated:
+		if b.Null {
+			return tftypes.NewValue(tftypes.Bool, nil), nil
+		}
+		if b.Unknown {
+			return tftypes.NewValue(tftypes.Bool, tftypes.UnknownValue), nil
+		}
+		if err := tftypes.ValidateValue(tftypes.Bool, b.Value); err != nil {
+			return tftypes.NewValue(tftypes.Bool, tftypes.UnknownValue), err
+		}
+		return tftypes.NewValue(tftypes.Bool, b.Value), nil
+	case valueStateKnown:
+		if err := tftypes.ValidateValue(tftypes.Bool, b.value); err != nil {
+			return tftypes.NewValue(tftypes.Bool, tftypes.UnknownValue), err
+		}
+
+		return tftypes.NewValue(tftypes.Bool, b.value), nil
+	case valueStateNull:
 		return tftypes.NewValue(tftypes.Bool, nil), nil
-	}
-	if b.Unknown {
+	case valueStateUnknown:
 		return tftypes.NewValue(tftypes.Bool, tftypes.UnknownValue), nil
+	default:
+		panic(fmt.Sprintf("unhandled Bool state in ToTerraformValue: %s", b.state))
 	}
-	if err := tftypes.ValidateValue(tftypes.Bool, b.Value); err != nil {
-		return tftypes.NewValue(tftypes.Bool, tftypes.UnknownValue), err
-	}
-	return tftypes.NewValue(tftypes.Bool, b.Value), nil
 }
 
 // Equal returns true if `other` is a *Bool and has the same value as `b`.
@@ -70,6 +161,12 @@ func (b Bool) Equal(other attr.Value) bool {
 	if !ok {
 		return false
 	}
+	if b.state != o.state {
+		return false
+	}
+	if b.state == valueStateKnown {
+		return b.value == o.value
+	}
 	if b.Unknown != o.Unknown {
 		return false
 	}
@@ -81,25 +178,37 @@ func (b Bool) Equal(other attr.Value) bool {
 
 // IsNull returns true if the Bool represents a null value.
 func (b Bool) IsNull() bool {
-	return b.Null
+	if b.state == valueStateNull {
+		return true
+	}
+
+	return b.state == valueStateDeprecated && b.Null
 }
 
 // IsUnknown returns true if the Bool represents a currently unknown value.
 func (b Bool) IsUnknown() bool {
-	return b.Unknown
+	if b.state == valueStateUnknown {
+		return true
+	}
+
+	return b.state == valueStateDeprecated && b.Unknown
 }
 
 // String returns a human-readable representation of the Bool value.
 // The string returned here is not protected by any compatibility guarantees,
 // and is intended for logging and error reporting.
 func (b Bool) String() string {
-	if b.Unknown {
+	if b.IsUnknown() {
 		return attr.UnknownValueString
 	}
 
-	if b.Null {
+	if b.IsNull() {
 		return attr.NullValueString
 	}
 
+	if b.state == valueStateKnown {
+		return fmt.Sprintf("%t", b.value)
+	}
+
 	return fmt.Sprintf("%t", b.Value)
 }