From 4a5a226236144e8374359ea94770f17e306cb958 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Mon, 21 Jun 2021 14:37:15 -0700 Subject: [PATCH] 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 | 63 ++++++++++++++++++------ hclsyntax/expression_test.go | 92 ++++++++++++++++++++++++++++++++++++ 2 files changed, 140 insertions(+), 15 deletions(-) diff --git a/hclsyntax/expression.go b/hclsyntax/expression.go index 63f2e88e..df615281 100644 --- a/hclsyntax/expression.go +++ b/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. diff --git a/hclsyntax/expression_test.go b/hclsyntax/expression_test.go index b420d933..10e924ed 100644 --- a/hclsyntax/expression_test.go +++ b/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,