Improve error messages for unexpected target types (#247)

terraform-linters · Mar 26, 2023 · 0dc8880 · 0dc8880
1 parent 8f09608
commit 0dc8880
Show file tree

Hide file tree

Showing 3 changed files with 69 additions and 58 deletions.
diff --git a/helper/runner.go b/helper/runner.go
@@ -203,16 +203,26 @@ var errRefTy = reflect.TypeOf((*error)(nil)).Elem()
 // EvaluateExpr returns a value of the passed expression.
 // Note that some features are limited
 func (r *Runner) EvaluateExpr(expr hcl.Expression, target interface{}, opts *tflint.EvaluateExprOption) error {
-	var callback bool
 	rval := reflect.ValueOf(target)
 	rty := rval.Type()
-	// Callback must meet the following requirements:
-	//   - It must be a function
-	//   - It must take an argument
-	//   - It must return an error
-	if rty.Kind() == reflect.Func && rty.NumIn() == 1 && rty.NumOut() == 1 && rty.Out(0).Implements(errRefTy) {
+
+	var callback bool
+	switch rty.Kind() {
+	case reflect.Func:
+		// Callback must meet the following requirements:
+		//   - It must be a function
+		//   - It must take an argument
+		//   - It must return an error
+		if !(rty.NumIn() == 1 && rty.NumOut() == 1 && rty.Out(0).Implements(errRefTy)) {
+			panic(`callback must be of type "func (v T) error"`)
+		}
 		callback = true
 		target = reflect.New(rty.In(0)).Interface()
+
+	case reflect.Pointer:
+		// ok
+	default:
+		panic("target value is not a pointer or function")
 	}
 
 	err := r.evaluateExpr(expr, target, opts)
@@ -246,19 +256,19 @@ func (r *Runner) evaluateExpr(expr hcl.Expression, target interface{}, opts *tfl
 		ty = *opts.WantType
 	} else {
 		switch target.(type) {
-		case *string, string:
+		case *string:
 			ty = cty.String
-		case *int, int:
+		case *int:
 			ty = cty.Number
-		case *[]string, []string:
+		case *[]string:
 			ty = cty.List(cty.String)
-		case *[]int, []int:
+		case *[]int:
 			ty = cty.List(cty.Number)
-		case *map[string]string, map[string]string:
+		case *map[string]string:
 			ty = cty.Map(cty.String)
-		case *map[string]int, map[string]int:
+		case *map[string]int:
 			ty = cty.Map(cty.Number)
-		case cty.Value, *cty.Value:
+		case *cty.Value:
 			ty = cty.DynamicPseudoType
 		default:
 			return fmt.Errorf("unsupported target type: %T", target)

diff --git a/plugin/plugin2host/client.go b/plugin/plugin2host/client.go
@@ -281,16 +281,26 @@ var errRefTy = reflect.TypeOf((*error)(nil)).Elem()
 // Passing a callback function instead of a value as the target will invoke the callback,
 // passing the evaluated value to the argument.
 func (c *GRPCClient) EvaluateExpr(expr hcl.Expression, target interface{}, opts *tflint.EvaluateExprOption) error {
-	var callback bool
 	rval := reflect.ValueOf(target)
 	rty := rval.Type()
-	// Callback must meet the following requirements:
-	//   - It must be a function
-	//   - It must take an argument
-	//   - It must return an error
-	if rty.Kind() == reflect.Func && rty.NumIn() == 1 && rty.NumOut() == 1 && rty.Out(0).Implements(errRefTy) {
+
+	var callback bool
+	switch rty.Kind() {
+	case reflect.Func:
+		// Callback must meet the following requirements:
+		//   - It must be a function
+		//   - It must take an argument
+		//   - It must return an error
+		if !(rty.NumIn() == 1 && rty.NumOut() == 1 && rty.Out(0).Implements(errRefTy)) {
+			panic(`callback must be of type "func (v T) error"`)
+		}
 		callback = true
 		target = reflect.New(rty.In(0)).Interface()
+
+	case reflect.Pointer:
+		// ok
+	default:
+		panic("target value is not a pointer or function")
 	}
 
 	err := c.evaluateExpr(expr, target, opts)
@@ -324,19 +334,19 @@ func (c *GRPCClient) evaluateExpr(expr hcl.Expression, target interface{}, opts
 		ty = *opts.WantType
 	} else {
 		switch target.(type) {
-		case *string, string:
+		case *string:
 			ty = cty.String
-		case *int, int:
+		case *int:
 			ty = cty.Number
-		case *[]string, []string:
+		case *[]string:
 			ty = cty.List(cty.String)
-		case *[]int, []int:
+		case *[]int:
 			ty = cty.List(cty.Number)
-		case *map[string]string, map[string]string:
+		case *map[string]string:
 			ty = cty.Map(cty.String)
-		case *map[string]int, map[string]int:
+		case *map[string]int:
 			ty = cty.Map(cty.Number)
-		case cty.Value, *cty.Value:
+		case *cty.Value:
 			ty = cty.DynamicPseudoType
 		default:
 			panic(fmt.Sprintf("unsupported target type: %T", target))

diff --git a/tflint/interface.go b/tflint/interface.go
@@ -159,18 +159,14 @@ type Runner interface {
 	// See the hclext.DecodeBody documentation and examples for more details.
 	DecodeRuleConfig(ruleName string, ret interface{}) error
 
-	// EvaluateExpr evaluates the given expression and assigns the value to the Go value target.
-	// The target must be a pointer. Otherwise, it will cause a panic.
+	// EvaluateExpr evaluates an expression and assigns its value to a Go value target,
+	// which must be a pointer or a function. Any other type of target will trigger a panic.
 	//
-	// If the value cannot be assigned to the target, it returns an error.
-	// There are particularly examples such as:
+	// For pointers, if the expression value cannot be assigned to the target, an error is returned.
+	// Some examples of this include unknown values (like variables without defaults or
+	// aws_instance.foo.arn), null values, and sensitive values (for variables with sensitive = true).
 	//
-	//   1. Unknown value (e.g. variables without defaults, `aws_instance.foo.arn`)
-	//   2. NULL
-	//   3. Sensitive value (variables with `sensitive = true`)
-	//
-	// However, if the target is cty.Value, these errors will not be returned.
-	// These errors can be handled with errors.Is().
+	// These errors be handled with errors.Is():
 	//
 	// ```
 	// var val string
@@ -192,29 +188,11 @@ type Runner interface {
 	// }
 	// ```
 	//
-	// The following are the types that can be passed as the target:
-	//
-	//   1. string
-	//   2. int
-	//   3. []string
-	//   4. []int
-	//   5. map[string]string
-	//   6. map[string]int
-	//   7. cty.Value
-	//   8. func (v T) error
-	//
-	// Passing any other type will cause a panic. If you pass a function, the assigned value
-	// will be used as an argument to execute the function. In this case, if a value cannot be
-	// assigned to the argument type, instead of returning an error, the execution is skipped.
-	// This is useful when it is always acceptable to ignore exceptional values.
-	//
-	// ```
-	// runner.EvaluateExpr(expr, func (val string) error {
-	//   // Test value
-	// }, nil)
-	// ```
+	// However, if the target is cty.Value, these errors will not be returned.
 	//
-	// Besides this, you can pass a structure. In that case, you need to explicitly pass wantType.
+	// Here are the types that can be passed as the target: string, int, []string, []int,
+	// map[string]string, map[string]int, and cty.Value. Passing any other type will
+	// result in a panic, but you can make an exception by passing wantType as an option.
 	//
 	// ```
 	// type complexVal struct {
@@ -230,6 +208,19 @@ type Runner interface {
 	// var complexVals []complexVal
 	// runner.EvaluateExpr(expr, &compleVals, &tflint.EvaluateExprOption{WantType: &wantType})
 	// ```
+	//
+	// For functions (callbacks), the assigned value is used as an argument to execute
+	// the function. If a value cannot be assigned to the argument type, the execution
+	// is skipped instead of returning an error. This is useful when it's always acceptable
+	// to ignore exceptional values.
+	//
+	// Here's an example of how you can pass a function to EvaluateExpr:
+	//
+	// ```
+	// runner.EvaluateExpr(expr, func (val string) error {
+	//   // Test value
+	// }, nil)
+	// ```
 	EvaluateExpr(expr hcl.Expression, target interface{}, option *EvaluateExprOption) error
 
 	// EmitIssue sends an issue to TFLint. You need to pass the message of the issue and the range.