tfprotov5+tfprotov6: Remove unnecessary AttributePath_Step error return (#369)

Author: bflad

tfprotov5/internal/toproto/attribute_path.go

@@ -4,95 +4,89 @@
 package toproto
 
 import (
-	"errors"
+	"fmt"
 
 	"github.com/hashicorp/terraform-plugin-go/tfprotov5/internal/tfplugin5"
 	"github.com/hashicorp/terraform-plugin-go/tftypes"
 )
 
-var ErrUnknownAttributePathStepType = errors.New("unknown type of AttributePath_Step")
-
-func AttributePath(in *tftypes.AttributePath) (*tfplugin5.AttributePath, error) {
+func AttributePath(in *tftypes.AttributePath) *tfplugin5.AttributePath {
 	if in == nil {
-		return nil, nil
-	}
-
-	steps, err := AttributePath_Steps(in.Steps())
-
-	if err != nil {
-		return nil, err
+		return nil
 	}
 
 	resp := &tfplugin5.AttributePath{
-		Steps: steps,
+		Steps: AttributePath_Steps(in.Steps()),
 	}
 
-	return resp, nil
+	return resp
 }
 
-func AttributePaths(in []*tftypes.AttributePath) ([]*tfplugin5.AttributePath, error) {
+func AttributePaths(in []*tftypes.AttributePath) []*tfplugin5.AttributePath {
 	resp := make([]*tfplugin5.AttributePath, 0, len(in))
 
 	for _, a := range in {
-		attr, err := AttributePath(a)
-
-		if err != nil {
-			return resp, err
-		}
-
-		resp = append(resp, attr)
+		resp = append(resp, AttributePath(a))
 	}
 
-	return resp, nil
+	return resp
 }
 
-func AttributePath_Step(step tftypes.AttributePathStep) (*tfplugin5.AttributePath_Step, error) {
-	var resp tfplugin5.AttributePath_Step
-	if name, ok := step.(tftypes.AttributeName); ok {
-		resp.Selector = &tfplugin5.AttributePath_Step_AttributeName{
-			AttributeName: string(name),
-		}
-		return &resp, nil
+func AttributePath_Step(step tftypes.AttributePathStep) *tfplugin5.AttributePath_Step {
+	if step == nil {
+		return nil
 	}
-	if key, ok := step.(tftypes.ElementKeyString); ok {
-		resp.Selector = &tfplugin5.AttributePath_Step_ElementKeyString{
-			ElementKeyString: string(key),
+
+	switch step := step.(type) {
+	case tftypes.AttributeName:
+		return &tfplugin5.AttributePath_Step{
+			Selector: &tfplugin5.AttributePath_Step_AttributeName{
+				AttributeName: string(step),
+			},
 		}
-		return &resp, nil
-	}
-	if key, ok := step.(tftypes.ElementKeyInt); ok {
-		resp.Selector = &tfplugin5.AttributePath_Step_ElementKeyInt{
-			ElementKeyInt: int64(key),
+	case tftypes.ElementKeyInt:
+		return &tfplugin5.AttributePath_Step{
+			Selector: &tfplugin5.AttributePath_Step_ElementKeyInt{
+				ElementKeyInt: int64(step),
+			},
 		}
-		return &resp, nil
-	}
-	if _, ok := step.(tftypes.ElementKeyValue); ok {
-		// the protocol has no equivalent of an ElementKeyValue, so we
-		// return nil for both the step and the error here, to signal
-		// that we've hit a step we can't convey back to Terraform
-		return nil, nil
+	case tftypes.ElementKeyString:
+		return &tfplugin5.AttributePath_Step{
+			Selector: &tfplugin5.AttributePath_Step_ElementKeyString{
+				ElementKeyString: string(step),
+			},
+		}
+	case tftypes.ElementKeyValue:
+		// The protocol has no equivalent of an ElementKeyValue, so this
+		// returns nil for the step to signal a step we cannot convey back
+		// to Terraform.
+		return nil
 	}
-	return nil, ErrUnknownAttributePathStepType
+
+	// It is not currently possible to create tftypes.AttributePathStep
+	// implementations outside the tftypes package and these implementations
+	// should rarely change, if ever, since they are critical to how
+	// Terraform understands attribute paths. If this panic was reached, it
+	// implies that a new step type was introduced and needs to be
+	// implemented as a new case above or that this logic needs to be
+	// otherwise changed to handle some new attribute path system.
+	panic(fmt.Sprintf("unimplemented tftypes.AttributePathStep type: %T", step))
 }
 
-func AttributePath_Steps(in []tftypes.AttributePathStep) ([]*tfplugin5.AttributePath_Step, error) {
+func AttributePath_Steps(in []tftypes.AttributePathStep) []*tfplugin5.AttributePath_Step {
 	resp := make([]*tfplugin5.AttributePath_Step, 0, len(in))
+
 	for _, step := range in {
-		if step == nil {
-			resp = append(resp, nil)
-			continue
-		}
-		s, err := AttributePath_Step(step)
-		if err != nil {
-			return resp, err
-		}
-		// in the face of a set, the protocol has no way to represent
-		// the index, so we just bail and return the prefix we can
-		// return.
+		s := AttributePath_Step(step)
+
+		// In the face of a ElementKeyValue or missing step, Terraform has no
+		// way to represent the attribute path, so only return the prefix.
 		if s == nil {
-			return resp, nil
+			return resp
 		}
+
 		resp = append(resp, s)
 	}
-	return resp, nil
+
+	return resp
 }

tfprotov5/internal/toproto/attribute_path_test.go

@@ -50,11 +50,7 @@ func TestAttributePath(t *testing.T) {
 		t.Run(name, func(t *testing.T) {
 			t.Parallel()
 
-			// Intentionally not checking the error return as it is impossible
-			// to implement a test case which would raise an error. This return
-			// will be removed in preference of a panic a future change.
-			// Reference: https://github.com/hashicorp/terraform-plugin-go/issues/365
-			got, _ := toproto.AttributePath(testCase.in)
+			got := toproto.AttributePath(testCase.in)
 
 			// Protocol Buffers generated types must have unexported fields
 			// ignored or cmp.Diff() will raise an error. This is easier than
@@ -137,11 +133,7 @@ func TestAttributePaths(t *testing.T) {
 		t.Run(name, func(t *testing.T) {
 			t.Parallel()
 
-			// Intentionally not checking the error return as it is impossible
-			// to implement a test case which would raise an error. This return
-			// will be removed in preference of a panic a future change.
-			// Reference: https://github.com/hashicorp/terraform-plugin-go/issues/365
-			got, _ := toproto.AttributePaths(testCase.in)
+			got := toproto.AttributePaths(testCase.in)
 
 			// Protocol Buffers generated types must have unexported fields
 			// ignored or cmp.Diff() will raise an error. This is easier than
@@ -206,11 +198,7 @@ func TestAttributePath_Step(t *testing.T) {
 		t.Run(name, func(t *testing.T) {
 			t.Parallel()
 
-			// Intentionally not checking the error return as it is impossible
-			// to implement a test case which would raise an error. This return
-			// will be removed in preference of a panic a future change.
-			// Reference: https://github.com/hashicorp/terraform-plugin-go/issues/365
-			got, _ := toproto.AttributePath_Step(testCase.in)
+			got := toproto.AttributePath_Step(testCase.in)
 
 			// Protocol Buffers generated types must have unexported fields
 			// ignored or cmp.Diff() will raise an error. This is easier than
@@ -280,11 +268,7 @@ func TestAttributePath_Steps(t *testing.T) {
 		t.Run(name, func(t *testing.T) {
 			t.Parallel()
 
-			// Intentionally not checking the error return as it is impossible
-			// to implement a test case which would raise an error. This return
-			// will be removed in preference of a panic a future change.
-			// Reference: https://github.com/hashicorp/terraform-plugin-go/issues/365
-			got, _ := toproto.AttributePath_Steps(testCase.in)
+			got := toproto.AttributePath_Steps(testCase.in)
 
 			// Protocol Buffers generated types must have unexported fields
 			// ignored or cmp.Diff() will raise an error. This is easier than

tfprotov5/internal/toproto/data_source.go

@@ -20,39 +20,27 @@ func GetMetadata_DataSourceMetadata(in *tfprotov5.DataSourceMetadata) *tfplugin5
 	return resp
 }
 
-func ValidateDataSourceConfig_Response(in *tfprotov5.ValidateDataSourceConfigResponse) (*tfplugin5.ValidateDataSourceConfig_Response, error) {
+func ValidateDataSourceConfig_Response(in *tfprotov5.ValidateDataSourceConfigResponse) *tfplugin5.ValidateDataSourceConfig_Response {
 	if in == nil {
-		return nil, nil
-	}
-
-	diags, err := Diagnostics(in.Diagnostics)
-
-	if err != nil {
-		return nil, err
+		return nil
 	}
 
 	resp := &tfplugin5.ValidateDataSourceConfig_Response{
-		Diagnostics: diags,
+		Diagnostics: Diagnostics(in.Diagnostics),
 	}
 
-	return resp, nil
+	return resp
 }
 
-func ReadDataSource_Response(in *tfprotov5.ReadDataSourceResponse) (*tfplugin5.ReadDataSource_Response, error) {
+func ReadDataSource_Response(in *tfprotov5.ReadDataSourceResponse) *tfplugin5.ReadDataSource_Response {
 	if in == nil {
-		return nil, nil
-	}
-
-	diags, err := Diagnostics(in.Diagnostics)
-
-	if err != nil {
-		return nil, err
+		return nil
 	}
 
 	resp := &tfplugin5.ReadDataSource_Response{
-		Diagnostics: diags,
+		Diagnostics: Diagnostics(in.Diagnostics),
 		State:       DynamicValue(in.State),
 	}
 
-	return resp, nil
+	return resp
 }

tfprotov5/internal/toproto/data_source_test.go

@@ -107,11 +107,7 @@ func TestReadDataSource_Response(t *testing.T) {
 		t.Run(name, func(t *testing.T) {
 			t.Parallel()
 
-			// Intentionally not checking the error return as it is impossible
-			// to implement a test case which would raise an error. This return
-			// will be removed in preference of a panic a future change.
-			// Reference: https://github.com/hashicorp/terraform-plugin-go/issues/365
-			got, _ := toproto.ReadDataSource_Response(testCase.in)
+			got := toproto.ReadDataSource_Response(testCase.in)
 
 			// Protocol Buffers generated types must have unexported fields
 			// ignored or cmp.Diff() will raise an error. This is easier than
@@ -167,11 +163,7 @@ func TestValidateDataSourceConfig_Response(t *testing.T) {
 		t.Run(name, func(t *testing.T) {
 			t.Parallel()
 
-			// Intentionally not checking the error return as it is impossible
-			// to implement a test case which would raise an error. This return
-			// will be removed in preference of a panic a future change.
-			// Reference: https://github.com/hashicorp/terraform-plugin-go/issues/365
-			got, _ := toproto.ValidateDataSourceConfig_Response(testCase.in)
+			got := toproto.ValidateDataSourceConfig_Response(testCase.in)
 
 			// Protocol Buffers generated types must have unexported fields
 			// ignored or cmp.Diff() will raise an error. This is easier than

tfprotov5/internal/toproto/diagnostic.go

@@ -10,46 +10,34 @@ import (
 	"github.com/hashicorp/terraform-plugin-go/tfprotov5/internal/tfplugin5"
 )
 
-func Diagnostic(in *tfprotov5.Diagnostic) (*tfplugin5.Diagnostic, error) {
+func Diagnostic(in *tfprotov5.Diagnostic) *tfplugin5.Diagnostic {
 	if in == nil {
-		return nil, nil
-	}
-
-	attribute, err := AttributePath(in.Attribute)
-
-	if err != nil {
-		return nil, err
+		return nil
 	}
 
 	resp := &tfplugin5.Diagnostic{
-		Attribute:        attribute,
+		Attribute:        AttributePath(in.Attribute),
 		Detail:           ForceValidUTF8(in.Detail),
 		FunctionArgument: in.FunctionArgument,
 		Severity:         Diagnostic_Severity(in.Severity),
 		Summary:          ForceValidUTF8(in.Summary),
 	}
 
-	return resp, nil
+	return resp
 }
 
 func Diagnostic_Severity(in tfprotov5.DiagnosticSeverity) tfplugin5.Diagnostic_Severity {
 	return tfplugin5.Diagnostic_Severity(in)
 }
 
-func Diagnostics(in []*tfprotov5.Diagnostic) ([]*tfplugin5.Diagnostic, error) {
+func Diagnostics(in []*tfprotov5.Diagnostic) []*tfplugin5.Diagnostic {
 	resp := make([]*tfplugin5.Diagnostic, 0, len(in))
 
 	for _, diag := range in {
-		d, err := Diagnostic(diag)
-
-		if err != nil {
-			return resp, err
-		}
-
-		resp = append(resp, d)
+		resp = append(resp, Diagnostic(diag))
 	}
 
-	return resp, nil
+	return resp
 }
 
 // ForceValidUTF8 returns a string guaranteed to be valid UTF-8 even if the

tfprotov5/internal/toproto/diagnostic_test.go

@@ -105,11 +105,7 @@ func TestDiagnostic(t *testing.T) {
 		t.Run(name, func(t *testing.T) {
 			t.Parallel()
 
-			// Intentionally not checking the error return as it is impossible
-			// to implement a test case which would raise an error. This return
-			// will be removed in preference of a panic a future change.
-			// Reference: https://github.com/hashicorp/terraform-plugin-go/issues/365
-			got, _ := toproto.Diagnostic(testCase.in)
+			got := toproto.Diagnostic(testCase.in)
 
 			// Protocol Buffers generated types must have unexported fields
 			// ignored or cmp.Diff() will raise an error. This is easier than
@@ -187,11 +183,7 @@ func TestDiagnostics(t *testing.T) {
 		t.Run(name, func(t *testing.T) {
 			t.Parallel()
 
-			// Intentionally not checking the error return as it is impossible
-			// to implement a test case which would raise an error. This return
-			// will be removed in preference of a panic a future change.
-			// Reference: https://github.com/hashicorp/terraform-plugin-go/issues/365
-			got, _ := toproto.Diagnostics(testCase.in)
+			got := toproto.Diagnostics(testCase.in)
 
 			// Protocol Buffers generated types must have unexported fields
 			// ignored or cmp.Diff() will raise an error. This is easier than

tfprotov5/internal/toproto/function.go

@@ -10,23 +10,17 @@ import (
 	"github.com/hashicorp/terraform-plugin-go/tfprotov5/internal/tfplugin5"
 )
 
-func CallFunction_Response(in *tfprotov5.CallFunctionResponse) (*tfplugin5.CallFunction_Response, error) {
+func CallFunction_Response(in *tfprotov5.CallFunctionResponse) *tfplugin5.CallFunction_Response {
 	if in == nil {
-		return nil, nil
-	}
-
-	diags, err := Diagnostics(in.Diagnostics)
-
-	if err != nil {
-		return nil, err
+		return nil
 	}
 
 	resp := &tfplugin5.CallFunction_Response{
-		Diagnostics: diags,
+		Diagnostics: Diagnostics(in.Diagnostics),
 		Result:      DynamicValue(in.Result),
 	}
 
-	return resp, nil
+	return resp
 }
 
 func Function(in *tfprotov5.Function) (*tfplugin5.Function, error) {
@@ -136,14 +130,8 @@ func GetFunctions_Response(in *tfprotov5.GetFunctionsResponse) (*tfplugin5.GetFu
 		return nil, nil
 	}
 
-	diags, err := Diagnostics(in.Diagnostics)
-
-	if err != nil {
-		return nil, err
-	}
-
 	resp := &tfplugin5.GetFunctions_Response{
-		Diagnostics: diags,
+		Diagnostics: Diagnostics(in.Diagnostics),
 		Functions:   make(map[string]*tfplugin5.Function, len(in.Functions)),
 	}