Proposal: adding IsNull() and IsUnknown() methods to attr.Value interface

[detro]

NOTE: This comes after a verbal conversation about this.

NOTE: This is a draft, to check a few things about the approach I'm taking. It's only applied to Bool, so it's not the final PR.

Background

As part of the work to port https://github.com/hashicorp/terraform-provider-tls/ to terraform-plugin-framework, a need arises as soon as I attempted to implement AttributeValidator and AttributePlanModifier.

About plan modifiers

For plan modifiers, it's possible to implement a defaultValueAttributePlanModifier that when used looks like:

PlanModifiers: []tfsdk.AttributePlanModifier{
  defaultValue(types.Bool{Value: false}),
},

And the implementation looks like:

type defaultValueAttributePlanModifier struct {
	DefaultValue attr.Value
}

var _ tfsdk.AttributePlanModifier = (*defaultValueAttributePlanModifier)(nil)

func defaultValue(v attr.Value) tfsdk.AttributePlanModifier {
	return &defaultValueAttributePlanModifier{v}
}

func (apm defaultValueAttributePlanModifier) Modify(ctx context.Context, req tfsdk.ModifyAttributePlanRequest, res *tfsdk.ModifyAttributePlanResponse) {
	// First, check the attribute configuration
	attrConfigValue, err := req.AttributeConfig.ToTerraformValue(ctx)
	if err != nil {
		res.Diagnostics.AddAttributeError(
			req.AttributePath,
			fmt.Sprintf("Unexpected error converting config value for %q", pathToString(req.AttributePath)),
			fmt.Sprintf("This is always a bug with the provider. Error: %s", err),
		)
		return
	}

	// If the attribute configuration is not null, we are done here
	if !attrConfigValue.IsNull() {
		return
	}

	// Second, check the attribute plan (so far):
	// this is necessary as other plan modifier might have already been applied, and we don't
	// want to interfere.
	attrPlanValue, err := req.AttributePlan.ToTerraformValue(ctx)
	if err != nil {
		res.Diagnostics.AddAttributeError(
			req.AttributePath,
			fmt.Sprintf("Unexpected error converting plan value for %q", pathToString(req.AttributePath)),
			fmt.Sprintf("This is always a bug with the provider. Error: %s", err),
		)
		return
	}

	// If the attribute plan is not null, or is fully known (objects), we are done here
	if !attrPlanValue.IsNull() || attrPlanValue.IsFullyKnown() {
		return
	}

	res.AttributePlan = apm.DefaultValue
}

If you look closely, we are having to convert back to TF types to check for null-ness. That feels sub-optimal (code smell?).

About validators

Similarly to the above, I wanted to implement AttributeValidator that can be used to verify that an attribute was/wasn't set, in respect to other fields. This is a very useful declarative feature of terraform-plugin-sdk/v2:

type Schema struct {
  // ...
  ConflictsWith [][string](https://pkg.go.dev/builtin#string)
  ExactlyOneOf  [][string](https://pkg.go.dev/builtin#string)
  AtLeastOneOf  [][string](https://pkg.go.dev/builtin#string)
  RequiredWith  [][string](https://pkg.go.dev/builtin#string)
  // ...
}

This can be achieved in the framework by leveraging tfsdk.ValidateAttributeReques.Config, that allows to interrogate the entire configuration while validating a single attribute.

For example, this is how the equivalent of RequiredWith looks like:

type requiredWithAttributeValidator struct {
	requiredAttributes []*tftypes.AttributePath
}

var _ tfsdk.AttributeValidator = (*requiredWithAttributeValidator)(nil)

func requiredWith(attributePaths ...*tftypes.AttributePath) tfsdk.AttributeValidator {
	return &requiredWithAttributeValidator{attributePaths}
}

func (av requiredWithAttributeValidator) Validate(ctx context.Context, req tfsdk.ValidateAttributeRequest, res *tfsdk.ValidateAttributeResponse) {
	tflog.Debug(ctx, "Validating attribute is set together with other required attributes", map[string]interface{}{
		"attribute":          pathToString(req.AttributePath),
		"requiredAttributes": av.requiredAttributes,
	})

	var v attr.Value
	diags := tfsdk.ValueAs(ctx, req.AttributeConfig, &v)
	if diags.HasError() {
		res.Diagnostics.Append(diags...)
		return
	}

	for _, path := range av.requiredAttributes {
		var o attr.Value
		req.Config.GetAttribute(ctx, path, &o)

		isVNull, err := isAttrValueNull(ctx, v)
		if err != nil {
			res.Diagnostics.AddAttributeError(
				req.AttributePath,
				fmt.Sprintf("Unexpected error converting attribute value for %q", pathToString(req.AttributePath)),
				fmt.Sprintf("This is always a bug with the provider. Error: %s", err),
			)
		}

		isONull, err := isAttrValueNull(ctx, o)
		if err != nil {
			res.Diagnostics.AddAttributeError(
				path,
				fmt.Sprintf("Unexpected error converting attribute value for %q", pathToString(path)),
				fmt.Sprintf("This is always a bug with the provider. Error: %s", err),
			)
		}

		if !isVNull && isONull {
			res.Diagnostics.AddAttributeError(
				req.AttributePath,
				fmt.Sprintf("Attribute %q missing", pathToString(path)),
				fmt.Sprintf("%q must be specified when %q is specified", pathToString(path), pathToString(req.AttributePath)),
			)
			return
		}
	}
}

Unfortunately, like for plan modifiers, the implementations needs to convert back to TF types to interrogate null-ness. Writing switch blocks and use type assertions could work, but it would then break for custom types, that is a feature of the new framework.

Proposal

attr.Value, the interface implemented by every types.*, should contain extra methods:

type Value interface {
  //...
  // IsNull returns true if the Value is not set, or is explicitly set to null.
  IsNull() bool

  // IsUnknown returns true if the value is not yet known.
  IsUnknown() bool
}

This would allow implementing key features like the above, way simpler/cleaner.

Future considerations

Follows a list of things we will not be addressing in this proposal PR, following the initial team consultation.

Un-export Unknown and Null fields on concrete types

It's a bit awkward to have on the concrete types both .Unknown fields, as well as .IsUnknown() methods exported. We should probably look to ways to move the boolean field away from the public interface of those types. Maybe .unknown and .null?

Constructor helpers

By virtue of the previous point, it would become necessary to provide some helper constructors to be able to create concrete types that set .unknown and .null for us.
For the types.Bool type, they could maybe look like:

func NewBool(v bool) Bool {}

func UnknownBool() Bool {}

func NullBool() Bool {}

tftypes.Value.IsFullyKnown()

TF types offer an additional method called IsFullyKnown(): this is the same as IsKnown() for primitive types, but it's useful for structured types like Objects, Maps etc. I could be beneficial for our structured types.*?

[detro]

🙏 I just need to stop worrying about me fat-fingering it and open a PR with a bunch of Jetbrains files in it.

[bflad]

Looks good to me 🚀

Let's ensure there is an associated changelog entry, e.g. .changelog/335.txt

```release-note:breaking-change
attr: The `Value` interface now requires the `IsNull()` and `IsUnknown()` methods
```

```release-note:enhancement
types: Added `IsNull()` and `IsUnknown()` methods to all types
```

[bflad]

Do you happen to have goimports enabled in your editor on save? It should revert this to the way it was.

[detro]

I have checked, and both Goland and goimports do the same thing: they seem to be putting "standard", then "3rd party", then "local" imports, separated by a space.

I thought that was the "Go way" to do imports, no?

[bflad]

https://github.com/golang/go/wiki/CodeReviewComments#imports tends to be my jam. Not sure why certain things would default to splitting out the "local" imports group versus not.

[bflad]

Thank you for adding these. 👍

[bendbennett]

LGTM. Nice!

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.