EnsureNoError is a bad interface

[wata727]

In TFLint, there are three patterns of expression evaluation results.

• Success. The inspection can be continued based on correct evaluation results.
• Failure. All inspections should be stopped because they are likely to give incorrect results.
• Known issues. You don't have to stop all inspections, but you should skip this rule.

It was a difficult problem to handle these 3 results appropriately. The interface had to be designed so that plugin developers didn't know the details of the known issues and only knew if succeeded or failed.

EnsureNoError was designed as a last resort. We made it a convention for developers to pass the error returned by EvaluateExpr to this function. This aims to hide this problem by only calling the callback if no known issues occur.

But this doesn't seem to work very well. By convention in Go, it is common to test the returned error by if err != nil, and EnsureNoError is not intuitive. Therefore, it confuses developers as they don't know where this function is to use.

We need a more intuitive interface. For example, with the type parameter added in Go 1.18, we can pass a callback function to be executed when the evaluation is successful to EvaluateExpr.

func EvaluateExpr[T any](expr hcl.Expression, callback func (ret T) error) error {
  val, err := getExprValueFromGRPCServer(expr)
  if errors.Is(err, knownErr) {
    // Skipped
    return nil
  }

  var out T
  gocty.FromCtyValue(val, &out)
  return callback(out)
}

func main() {
  EvaluateExpr(expr, func (ret string) error {
    fmt.Printf("evaluated value: %s", ret)
  })
}

[wata727]

Type parameters in methods do not work... golang/go#49085

[wata727]

There is a way to check for known issues in rulesets instead of rules. Change the ruleset implementation as follows:

func (r *BuiltinRuleSet) Check(runner Runner) error {
	for _, rule := range r.EnabledRules {
		if err := rule.Check(runner); err != nil {
			if errors.Is(err, tflint.ErrUnevaluable) || errors.Is(err, tflint.ErrNullValue) || errors.Is(err, tflint.ErrUnknownValue) || errors.Is(err, tflint.ErrSensitive) {
				continue
			}
			return fmt.Errorf("Failed to check `%s` rule: %s", rule.Name(), err)
		}
	}
	return nil
}

https://github.com/terraform-linters/tflint-plugin-sdk/blob/v0.15.0/tflint/ruleset.go#L101-L109

In this case, the rule can simply check for errors without using EnsureNoError.

// Before
err := runner.EvaluteExpr(...)
err = runner.EnsureNoError(err, func () error {
	// Do something
})
if err != nil {
	return err
}

// After
err := runner.EvaluateExpr(...)
if err != nil {
	return err
}

However, we should be careful with early returns. An example like the following will unintentionally break prematurely:

for _, expr := range expressions {
	err := runner.EvaluateExpr(expr, ...)
	if err != nil {
		return err
	}
}

To avoid this, rules should join errors whenever possible. errors.Join, introduced in Go 1.20, is perfect for this purpose:

var errs []error
for _, expr := range expressions {
	err := runner.EvaluateExpr(expr, ...)
	if err != nil {
		errs = append(errs, err)
	}
}
return errors.Join(errs...)

golang/go@4a0a2b3

The ruleset unwraps the error and removes only the known errors:

if err := rule.Check(runner); err != nil {
	if errs, ok := err.(interface { Unwrap() []error }); ok {
		var ers []error
		for _, e := range errs.Unwrap() {
			if errors.Is(err, tflint.ErrUnevaluable) || errors.Is(err, tflint.ErrNullValue) || errors.Is(err, tflint.ErrUnknownValue) || errors.Is(err, tflint.ErrSensitive) {
				// skip
			} else {
				ers = append(ers, e)
			}
		}
		err = errors.Join(ers...)
		if err == nil {
			continue
		}
	}
	return fmt.Errorf("Failed to check `%s` rule: %s", rule.Name(), err)
}

[wata727]

Or maybe the plugin developer should be aware of the known errors.

ErrUnknownValue can be ignored in most cases, but whether ErrNullValue can be ignored depends on the context. It might be better to leave the responsibility of determining the error on the developer by not providing helpers.

[wata727]

I made a modification to EvaluateExpr to allow passing a function callback. Unfortunately, this could not be achieved with generics, but it is supported by reflection.