hclsyntax: Don't panic when function returns invalid ArgError

When a function returns a functions.ArgError it's supposed to set the
argument index to a valid index within the range of the given argument
slice.

However, we typically try to be robust in the face of
incorrectly-implemented functions, so rather than panicking in that case
as we would've before we'll now return a slightly-less-precise error
message.

The "if i > len(e.Args)-1" case in the previous code here was actually
trying to handle this situation before, but it didn't quite manage to do
it because it was incorrectly referring to e.Args rather than "args"
(so it couldn't take into account expanded arguments) and because it
incorrectly assumed that there would always be more declared parameters
than arguments, which is actually never true in any valid call.

Now we'll handle out of range a little differently for variadic vs.
non-variadic functions. For variadics we'll assume that the function was
trying to talk about the variadic parameter and name that in the error
message. For non-variadics we can't do anything special so we end up
just treating it the same as any other error type, simply stating that
the function call failed without naming a particular argument.

Functions that end up in this error-handling codepath should still be
fixed, because they'll likely end up returning a slightly confusing error
message which doesn't accurately reflect the source of the problem. The
goal here is just avoid a panic in that case.

hclsyntax/expression.go

@@ -451,22 +451,55 @@ func (e *FunctionCallExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnosti
 				param = varParam
 			}
 
-			// this can happen if an argument is (incorrectly) null.
-			if i > len(e.Args)-1 {
-				diags = append(diags, &hcl.Diagnostic{
-					Severity: hcl.DiagError,
-					Summary:  "Invalid function argument",
-					Detail: fmt.Sprintf(
-						"Invalid value for %q parameter: %s.",
-						param.Name, err,
-					),
-					Subject:     args[len(params)].StartRange().Ptr(),
-					Context:     e.Range().Ptr(),
-					Expression:  e,
-					EvalContext: ctx,
-				})
+			if param == nil || i > len(args)-1 {
+				// Getting here means that the function we called has a bug:
+				// it returned an arg error that refers to an argument index
+				// that wasn't present in the call. For that situation
+				// we'll degrade to a less specific error just to give
+				// some sort of answer, but best to still fix the buggy
+				// function so that it only returns argument indices that
+				// are in range.
+				switch {
+				case param != nil:
+					// In this case we'll assume that the function was trying
+					// to talk about a final variadic parameter but the caller
+					// didn't actually provide any arguments for it. That means
+					// we can at least still name the parameter in the
+					// error message, but our source range will be the call
+					// as a whole because we don't have an argument expression
+					// to highlight specifically.
+					diags = append(diags, &hcl.Diagnostic{
+						Severity: hcl.DiagError,
+						Summary:  "Invalid function argument",
+						Detail: fmt.Sprintf(
+							"Invalid value for %q parameter: %s.",
+							param.Name, err,
+						),
+						Subject:     e.Range().Ptr(),
+						Expression:  e,
+						EvalContext: ctx,
+					})
+				default:
+					// This is the most degenerate case of all, where the
+					// index is out of range even for the declared parameters,
+					// and so we can't tell which parameter the function is
+					// trying to report an error for. Just a generic error
+					// report in that case.
+					diags = append(diags, &hcl.Diagnostic{
+						Severity: hcl.DiagError,
+						Summary:  "Error in function call",
+						Detail: fmt.Sprintf(
+							"Call to function %q failed: %s.",
+							e.Name, err,
+						),
+						Subject:     e.StartRange().Ptr(),
+						Context:     e.Range().Ptr(),
+						Expression:  e,
+						EvalContext: ctx,
+					})
+				}
 			} else {
-				argExpr := e.Args[i]
+				argExpr := args[i]
 
 				// TODO: we should also unpick a PathError here and show the
 				// path to the deep value where the error was detected.

hclsyntax/expression_test.go

@@ -337,6 +337,98 @@ upper(
 			cty.DynamicVal,
 			0,
 		},
+		{
+			`misbehave()`,
+			&hcl.EvalContext{
+				Functions: map[string]function.Function{
+					"misbehave": function.New(&function.Spec{
+						Type: func(args []cty.Value) (cty.Type, error) {
+							// This function misbehaves by indicating an error
+							// on an argument index that is out of range for
+							// its declared parameters. That would always be
+							// a bug in the function, but we want to avoid
+							// panicking in this case and just behave like it
+							// was a normal (non-arg) error.
+							return cty.NilType, function.NewArgErrorf(1, "out of range")
+						},
+					}),
+				},
+			},
+			cty.DynamicVal,
+			1, // Call to function "misbehave" failed: out of range
+		},
+		{
+			`misbehave() /* variadic */`,
+			&hcl.EvalContext{
+				Functions: map[string]function.Function{
+					"misbehave": function.New(&function.Spec{
+						VarParam: &function.Parameter{
+							Name: "foo",
+							Type: cty.String,
+						},
+						Type: func(args []cty.Value) (cty.Type, error) {
+							// This function misbehaves by indicating an error
+							// on an argument index that is out of range for
+							// the given arguments. That would always be a
+							// bug in the function, but to avoid panicking we
+							// just treat it like a problem related to the
+							// declared variadic argument.
+							return cty.NilType, function.NewArgErrorf(1, "out of range")
+						},
+					}),
+				},
+			},
+			cty.DynamicVal,
+			1, // Invalid value for "foo" parameter: out of range
+		},
+		{
+			`misbehave([]...)`,
+			&hcl.EvalContext{
+				Functions: map[string]function.Function{
+					"misbehave": function.New(&function.Spec{
+						VarParam: &function.Parameter{
+							Name: "foo",
+							Type: cty.String,
+						},
+						Type: func(args []cty.Value) (cty.Type, error) {
+							// This function misbehaves by indicating an error
+							// on an argument index that is out of range for
+							// the given arguments. That would always be a
+							// bug in the function, but to avoid panicking we
+							// just treat it like a problem related to the
+							// declared variadic argument.
+							return cty.NilType, function.NewArgErrorf(1, "out of range")
+						},
+					}),
+				},
+			},
+			cty.DynamicVal,
+			1, // Invalid value for "foo" parameter: out of range
+		},
+		{
+			`argerrorexpand(["a", "b"]...)`,
+			&hcl.EvalContext{
+				Functions: map[string]function.Function{
+					"argerrorexpand": function.New(&function.Spec{
+						VarParam: &function.Parameter{
+							Name: "foo",
+							Type: cty.String,
+						},
+						Type: func(args []cty.Value) (cty.Type, error) {
+							// We should be able to indicate an error in
+							// argument 1 because the indices are into the
+							// arguments _after_ "..." expansion. An earlier
+							// HCL version had a bug where it used the
+							// pre-expansion arguments and would thus panic
+							// in this case.
+							return cty.NilType, function.NewArgErrorf(1, "blah blah")
+						},
+					}),
+				},
+			},
+			cty.DynamicVal,
+			1, // Invalid value for "foo" parameter: blah blah
+		},
 		{
 			`[]`,
 			nil,