Require provider-defined function parameter naming

.changes/unreleased/BREAKING CHANGES-20240320-095152.yaml

@@ -0,0 +1,5 @@
+kind: BREAKING CHANGES
+body: 'function: All parameters must be explicitly named'
+time: 2024-03-20T09:51:52.869254Z
+custom:
+  Issue: "964"

function/definition.go

@@ -89,41 +89,46 @@ func (d Definition) Parameter(ctx context.Context, position int) (Parameter, dia
 // implementation of the definition to prevent unexpected errors or panics. This
 // logic runs during the GetProviderSchema RPC, or via provider-defined unit
 // testing, and should never include false positives.
-func (d Definition) ValidateImplementation(ctx context.Context) diag.Diagnostics {
+func (d Definition) ValidateImplementation(ctx context.Context, funcName string) diag.Diagnostics {
 	var diags diag.Diagnostics
 
 	if d.Return == nil {
 		diags.AddError(
 			"Invalid Function Definition",
 			"When validating the function definition, an implementation issue was found. "+
 				"This is always an issue with the provider and should be reported to the provider developers.\n\n"+
-				"Definition Return field is undefined",
+				fmt.Sprintf("Function %q - Definition Return field is undefined", funcName),
 		)
 	} else if d.Return.GetType() == nil {
 		diags.AddError(
 			"Invalid Function Definition",
 			"When validating the function definition, an implementation issue was found. "+
 				"This is always an issue with the provider and should be reported to the provider developers.\n\n"+
-				"Definition return data type is undefined",
+				fmt.Sprintf("Function %q - Definition return data type is undefined", funcName),
 		)
 	}
 
 	paramNames := make(map[string]int, len(d.Parameters))
 	for pos, param := range d.Parameters {
 		name := param.GetName()
-		// If name is not set, default the param name based on position: "param1", "param2", etc.
+		// If name is not set, add an error diagnostic, parameter names are mandatory.
 		if name == "" {
-			name = fmt.Sprintf("%s%d", DefaultParameterNamePrefix, pos+1)
+			diags.AddError(
+				"Invalid Function Definition",
+				"When validating the function definition, an implementation issue was found. "+
+					"This is always an issue with the provider and should be reported to the provider developers.\n\n"+
+					fmt.Sprintf("Function %q - Parameter at position %d does not have a name", funcName, pos),
+			)
 		}
 
 		conflictPos, exists := paramNames[name]
-		if exists {
+		if exists && name != "" {
 			diags.AddError(
 				"Invalid Function Definition",
 				"When validating the function definition, an implementation issue was found. "+
 					"This is always an issue with the provider and should be reported to the provider developers.\n\n"+
 					"Parameter names must be unique. "+
-					fmt.Sprintf("Parameters at position %d and %d have the same name %q", conflictPos, pos, name),
+					fmt.Sprintf("Function %q - Parameters at position %d and %d have the same name %q", funcName, conflictPos, pos, name),
 			)
 			continue
 		}
@@ -133,19 +138,24 @@ func (d Definition) ValidateImplementation(ctx context.Context) diag.Diagnostics
 
 	if d.VariadicParameter != nil {
 		name := d.VariadicParameter.GetName()
-		// If name is not set, default the variadic param name
+		// If name is not set, add an error diagnostic, parameter names are mandatory.
 		if name == "" {
-			name = DefaultVariadicParameterName
+			diags.AddError(
+				"Invalid Function Definition",
+				"When validating the function definition, an implementation issue was found. "+
+					"This is always an issue with the provider and should be reported to the provider developers.\n\n"+
+					fmt.Sprintf("Function %q - The variadic parameter does not have a name", funcName),
+			)
 		}
 
 		conflictPos, exists := paramNames[name]
-		if exists {
+		if exists && name != "" {
 			diags.AddError(
 				"Invalid Function Definition",
 				"When validating the function definition, an implementation issue was found. "+
 					"This is always an issue with the provider and should be reported to the provider developers.\n\n"+
 					"Parameter names must be unique. "+
-					fmt.Sprintf("Parameter at position %d and the variadic parameter have the same name %q", conflictPos, name),
+					fmt.Sprintf("Function %q - Parameter at position %d and the variadic parameter have the same name %q", funcName, conflictPos, name),
 			)
 		}
 	}

function/definition_test.go

@@ -8,6 +8,7 @@ import (
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
+
 	"github.com/hashicorp/terraform-plugin-framework/diag"
 	"github.com/hashicorp/terraform-plugin-framework/function"
 )
@@ -157,29 +158,65 @@ func TestDefinitionValidateImplementation(t *testing.T) {
 				Return: function.StringReturn{},
 			},
 		},
-		"valid-only-variadic": {
+		"missing-variadic-param-name": {
 			definition: function.Definition{
 				VariadicParameter: function.StringParameter{},
 				Return:            function.StringReturn{},
 			},
+			expected: diag.Diagnostics{
+				diag.NewErrorDiagnostic(
+					"Invalid Function Definition",
+					"When validating the function definition, an implementation issue was found. "+
+						"This is always an issue with the provider and should be reported to the provider developers.\n\n"+
+						"Function \"test-function\" - The variadic parameter does not have a name",
+				),
+			},
 		},
-		"valid-param-name-defaults": {
+		"missing-param-names": {
 			definition: function.Definition{
 				Parameters: []function.Parameter{
 					function.StringParameter{},
 					function.StringParameter{},
 				},
 				Return: function.StringReturn{},
 			},
+			expected: diag.Diagnostics{
+				diag.NewErrorDiagnostic(
+					"Invalid Function Definition",
+					"When validating the function definition, an implementation issue was found. "+
+						"This is always an issue with the provider and should be reported to the provider developers.\n\n"+
+						"Function \"test-function\" - Parameter at position 0 does not have a name",
+				),
+				diag.NewErrorDiagnostic(
+					"Invalid Function Definition",
+					"When validating the function definition, an implementation issue was found. "+
+						"This is always an issue with the provider and should be reported to the provider developers.\n\n"+
+						"Function \"test-function\" - Parameter at position 1 does not have a name",
+				),
+			},
 		},
-		"valid-param-names-defaults-with-variadic": {
+		"missing-param-names-with-variadic": {
 			definition: function.Definition{
 				Parameters: []function.Parameter{
 					function.StringParameter{},
 				},
 				VariadicParameter: function.NumberParameter{},
 				Return:            function.StringReturn{},
 			},
+			expected: diag.Diagnostics{
+				diag.NewErrorDiagnostic(
+					"Invalid Function Definition",
+					"When validating the function definition, an implementation issue was found. "+
+						"This is always an issue with the provider and should be reported to the provider developers.\n\n"+
+						"Function \"test-function\" - Parameter at position 0 does not have a name",
+				),
+				diag.NewErrorDiagnostic(
+					"Invalid Function Definition",
+					"When validating the function definition, an implementation issue was found. "+
+						"This is always an issue with the provider and should be reported to the provider developers.\n\n"+
+						"Function \"test-function\" - The variadic parameter does not have a name",
+				),
+			},
 		},
 		"result-missing": {
 			definition: function.Definition{},
@@ -188,7 +225,7 @@ func TestDefinitionValidateImplementation(t *testing.T) {
 					"Invalid Function Definition",
 					"When validating the function definition, an implementation issue was found. "+
 						"This is always an issue with the provider and should be reported to the provider developers.\n\n"+
-						"Definition Return field is undefined",
+						"Function \"test-function\" - Definition Return field is undefined",
 				),
 			},
 		},
@@ -219,27 +256,7 @@ func TestDefinitionValidateImplementation(t *testing.T) {
 					"When validating the function definition, an implementation issue was found. "+
 						"This is always an issue with the provider and should be reported to the provider developers.\n\n"+
 						"Parameter names must be unique. "+
-						"Parameters at position 2 and 4 have the same name \"param-dup\"",
-				),
-			},
-		},
-		"conflicting-param-name-with-default": {
-			definition: function.Definition{
-				Parameters: []function.Parameter{
-					function.StringParameter{
-						Name: "param2",
-					},
-					function.Float64Parameter{}, // defaults to param2
-				},
-				Return: function.StringReturn{},
-			},
-			expected: diag.Diagnostics{
-				diag.NewErrorDiagnostic(
-					"Invalid Function Definition",
-					"When validating the function definition, an implementation issue was found. "+
-						"This is always an issue with the provider and should be reported to the provider developers.\n\n"+
-						"Parameter names must be unique. "+
-						"Parameters at position 0 and 1 have the same name \"param2\"",
+						"Function \"test-function\" - Parameters at position 2 and 4 have the same name \"param-dup\"",
 				),
 			},
 		},
@@ -267,7 +284,7 @@ func TestDefinitionValidateImplementation(t *testing.T) {
 					"When validating the function definition, an implementation issue was found. "+
 						"This is always an issue with the provider and should be reported to the provider developers.\n\n"+
 						"Parameter names must be unique. "+
-						"Parameter at position 1 and the variadic parameter have the same name \"param-dup\"",
+						"Function \"test-function\" - Parameter at position 1 and the variadic parameter have the same name \"param-dup\"",
 				),
 			},
 		},
@@ -301,41 +318,21 @@ func TestDefinitionValidateImplementation(t *testing.T) {
 					"When validating the function definition, an implementation issue was found. "+
 						"This is always an issue with the provider and should be reported to the provider developers.\n\n"+
 						"Parameter names must be unique. "+
-						"Parameters at position 0 and 2 have the same name \"param-dup\"",
+						"Function \"test-function\" - Parameters at position 0 and 2 have the same name \"param-dup\"",
 				),
 				diag.NewErrorDiagnostic(
 					"Invalid Function Definition",
 					"When validating the function definition, an implementation issue was found. "+
 						"This is always an issue with the provider and should be reported to the provider developers.\n\n"+
 						"Parameter names must be unique. "+
-						"Parameters at position 0 and 4 have the same name \"param-dup\"",
+						"Function \"test-function\" - Parameters at position 0 and 4 have the same name \"param-dup\"",
 				),
 				diag.NewErrorDiagnostic(
 					"Invalid Function Definition",
 					"When validating the function definition, an implementation issue was found. "+
 						"This is always an issue with the provider and should be reported to the provider developers.\n\n"+
 						"Parameter names must be unique. "+
-						"Parameter at position 0 and the variadic parameter have the same name \"param-dup\"",
-				),
-			},
-		},
-		"conflicting-param-name-with-variadic-default": {
-			definition: function.Definition{
-				Parameters: []function.Parameter{
-					function.Float64Parameter{
-						Name: function.DefaultVariadicParameterName,
-					},
-				},
-				VariadicParameter: function.BoolParameter{}, // defaults to varparam
-				Return:            function.StringReturn{},
-			},
-			expected: diag.Diagnostics{
-				diag.NewErrorDiagnostic(
-					"Invalid Function Definition",
-					"When validating the function definition, an implementation issue was found. "+
-						"This is always an issue with the provider and should be reported to the provider developers.\n\n"+
-						"Parameter names must be unique. "+
-						"Parameter at position 0 and the variadic parameter have the same name \"varparam\"",
+						"Function \"test-function\" - Parameter at position 0 and the variadic parameter have the same name \"param-dup\"",
 				),
 			},
 		},
@@ -347,7 +344,7 @@ func TestDefinitionValidateImplementation(t *testing.T) {
 		t.Run(name, func(t *testing.T) {
 			t.Parallel()
 
-			got := testCase.definition.ValidateImplementation(context.Background())
+			got := testCase.definition.ValidateImplementation(context.Background(), "test-function")
 
 			if diff := cmp.Diff(got, testCase.expected); diff != "" {
 				t.Errorf("unexpected difference: %s", diff)

function/func_error.go

@@ -69,6 +69,10 @@ func (fe *FuncError) Equal(other *FuncError) bool {
 
 // Error returns the error text.
 func (fe *FuncError) Error() string {
+	if fe == nil {
+		return ""
+	}
+
 	return fe.Text
 }
 

function/func_error_test.go

@@ -87,6 +87,42 @@ func TestFunctionError_Equal(t *testing.T) {
 	}
 }
 
+func TestFunctionError_Error(t *testing.T) {
+	t.Parallel()
+
+	testCases := map[string]struct {
+		funcErr  *function.FuncError
+		expected string
+	}{
+		"nil": {
+			expected: "",
+		},
+		"empty": {
+			funcErr:  &function.FuncError{},
+			expected: "",
+		},
+		"text": {
+			funcErr: &function.FuncError{
+				Text: "function error",
+			},
+			expected: "function error",
+		},
+	}
+
+	for name, tc := range testCases {
+		name, tc := name, tc
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+
+			got := tc.funcErr.Error()
+
+			if got != tc.expected {
+				t.Errorf("Unexpected response: got: %s, wanted: %s", got, tc.expected)
+			}
+		})
+	}
+}
+
 func TestConcatFuncErrors(t *testing.T) {
 	t.Parallel()
 

function/parameter.go

@@ -7,17 +7,6 @@ import (
 	"github.com/hashicorp/terraform-plugin-framework/attr"
 )
 
-const (
-	// DefaultParameterNamePrefix is the prefix used to default the name of parameters which do not declare
-	// a name. Use this to prevent Terraform errors for missing names. This prefix is used with the parameter
-	// position in a function definition to create a unique name (param1, param2, etc.)
-	DefaultParameterNamePrefix = "param"
-
-	// DefaultVariadicParameterName is the default name given to a variadic parameter that does not declare
-	// a name. Use this to prevent Terraform errors for missing names.
-	DefaultVariadicParameterName = "varparam"
-)
-
 // Parameter is the interface for defining function parameters.
 type Parameter interface {
 	// GetAllowNullValue should return if the parameter accepts a null value.

internal/fwserver/server_functions.go

@@ -98,7 +98,7 @@ func (s *Server) FunctionDefinitions(ctx context.Context) (map[string]function.D
 			continue
 		}
 
-		validateDiags := definitionResp.Definition.ValidateImplementation(ctx)
+		validateDiags := definitionResp.Definition.ValidateImplementation(ctx, name)
 
 		diags.Append(validateDiags...)
 

internal/fwserver/server_getfunctions_test.go

@@ -116,7 +116,7 @@ func TestServerGetFunctions(t *testing.T) {
 						"Invalid Function Definition",
 						"When validating the function definition, an implementation issue was found. "+
 							"This is always an issue with the provider and should be reported to the provider developers.\n\n"+
-							"Definition Return field is undefined",
+							"Function \"function1\" - Definition Return field is undefined",
 					),
 				},
 				FunctionDefinitions: map[string]function.Definition{},

internal/fwserver/server_getproviderschema_test.go

@@ -416,7 +416,7 @@ func TestServerGetProviderSchema(t *testing.T) {
 						"Invalid Function Definition",
 						"When validating the function definition, an implementation issue was found. "+
 							"This is always an issue with the provider and should be reported to the provider developers.\n\n"+
-							"Definition Return field is undefined",
+							"Function \"function1\" - Definition Return field is undefined",
 					),
 				},
 				FunctionDefinitions: nil,