Skip evaluation of module arguments if the module call is not evaluat…

…ed (#1415)

tflint/runner.go

@@ -142,11 +142,11 @@ func NewRunner(c *Config, files map[string]*hcl.File, ants map[string]Annotation
 		content.Blocks = overrideBlocks(content.Blocks, c.Blocks)
 	}
 	for _, resource := range content.Blocks {
-		ok, err := runner.willEvaluateResource(resource)
+		evaluable, err := runner.isEvaluableResource(resource)
 		if err != nil {
 			return runner, err
 		}
-		if ok {
+		if evaluable {
 			resourceType := resource.Labels[0]
 			resourceName := resource.Labels[1]
 
@@ -164,6 +164,7 @@ func NewRunner(c *Config, files map[string]*hcl.File, ants map[string]Annotation
 // Recursively search modules and generate Runners
 // In order to propagate attributes of moduleCall as variables to the module,
 // evaluate the variables. If it cannot be evaluated, treat it as unknown
+// Modules that are not evaluated (`count` is 0 or `for_each` is empty) are ignored.
 func NewModuleRunners(parent *Runner) ([]*Runner, error) {
 	runners := []*Runner{}
 
@@ -176,6 +177,18 @@ func NewModuleRunners(parent *Runner) ([]*Runner, error) {
 			log.Printf("[INFO] Ignore `%s` module", moduleCall.Name)
 			continue
 		}
+		evaluable, err := parent.isEvaluableModuleCall(moduleCall)
+		if err != nil {
+			return runners, fmt.Errorf(
+				"failed to eval count/for_each meta-arguments in %s:%d; %w",
+				moduleCall.DeclRange.Filename,
+				moduleCall.DeclRange.Start.Line,
+				err,
+			)
+		}
+		if !evaluable {
+			continue
+		}
 
 		attributes, diags := moduleCall.Config.JustAttributes()
 		if diags.HasErrors() {

tflint/runner_eval.go

@@ -9,6 +9,7 @@ import (
 	"github.com/terraform-linters/tflint-plugin-sdk/hclext"
 	sdk "github.com/terraform-linters/tflint-plugin-sdk/tflint"
 	"github.com/terraform-linters/tflint/terraform/addrs"
+	"github.com/terraform-linters/tflint/terraform/configs"
 	"github.com/terraform-linters/tflint/terraform/lang"
 	"github.com/zclconf/go-cty/cty"
 	"github.com/zclconf/go-cty/cty/gocty"
@@ -116,60 +117,88 @@ func isEvaluableRef(ref *addrs.Reference) bool {
 	}
 }
 
-// willEvaluateResource checks whether the passed resource will be evaluated.
-// If `count` is 0 or `for_each` is empty, Terraform will not evaluate the attributes of that resource.
+// isEvaluableMetaArguments checks whether the passed resource meta-arguments (count/for_each)
+// indicate the resource will be evaluated.
+// If `count` is 0 or `for_each` is empty, Terraform will not evaluate the attributes of
+// that resource.
 // Terraform doesn't expect to pass null for these attributes (it will cause an error),
 // but we'll treat them as if they were undefined.
-func (r *Runner) willEvaluateResource(resource *hclext.Block) (bool, error) {
-	if attr, exists := resource.Body.Attributes["count"]; exists {
-		val, err := r.EvaluateExpr(attr.Expr, cty.Number)
-		if err != nil {
-			return willEvaluateResourceOnError(err)
-		}
+func (r *Runner) isEvaluableResource(resource *hclext.Block) (bool, error) {
+	if count, exists := resource.Body.Attributes["count"]; exists {
+		return r.isEvaluableCountArgument(count.Expr)
+	}
 
-		count := 1
-		if err := gocty.FromCtyValue(val, &count); err != nil {
-			return false, err
-		}
-		if count == 0 {
-			// `count = 0` is not evaluated
-			return false, nil
-		}
-		// `count > 1` is evaluated`
-		return true, nil
+	if forEach, exists := resource.Body.Attributes["for_each"]; exists {
+		return r.isEvaluableForEachArgument(forEach.Expr)
 	}
 
-	if attr, exists := resource.Body.Attributes["for_each"]; exists {
-		forEach, err := r.EvaluateExpr(attr.Expr, cty.DynamicPseudoType)
-		if err != nil {
-			return willEvaluateResourceOnError(err)
-		}
+	// If `count` or `for_each` is not defined, it will be evaluated by default
+	return true, nil
+}
 
-		if forEach.IsNull() {
-			// null value means that attribute is not set
-			return true, nil
-		}
-		if !forEach.IsKnown() {
-			// unknown value is non-deterministic
-			return false, nil
-		}
-		if !forEach.CanIterateElements() {
-			// uniteratable values (string, number, etc.) are invalid
-			return false, fmt.Errorf("The `for_each` value is not iterable in %s:%d", attr.Expr.Range().Filename, attr.Expr.Range().Start.Line)
-		}
-		if forEach.LengthInt() == 0 {
-			// empty `for_each` is not evaluated
-			return false, nil
-		}
-		// `for_each` with non-empty elements is evaluated
-		return true, nil
+// isEvaluableModuleCall checks whether the passed module-call meta-arguments (count/for_each)
+// indicate the module-call will be evaluated.
+// If `count` is 0 or `for_each` is empty, Terraform will not evaluate the attributes of that module.
+// Terraform doesn't expect to pass null for these attributes (it will cause an error),
+// but we'll treat them as if they were undefined.
+func (r *Runner) isEvaluableModuleCall(moduleCall *configs.ModuleCall) (bool, error) {
+	if moduleCall.Count != nil {
+		return r.isEvaluableCountArgument(moduleCall.Count)
+	}
+
+	if moduleCall.ForEach != nil {
+		return r.isEvaluableForEachArgument(moduleCall.ForEach)
 	}
 
 	// If `count` or `for_each` is not defined, it will be evaluated by default
 	return true, nil
 }
 
-func willEvaluateResourceOnError(err error) (bool, error) {
+func (r *Runner) isEvaluableCountArgument(expr hcl.Expression) (bool, error) {
+	val, err := r.EvaluateExpr(expr, cty.Number)
+	if err != nil {
+		return isEvaluableMetaArgumentsOnError(err)
+	}
+
+	count := 1
+	if err := gocty.FromCtyValue(val, &count); err != nil {
+		return false, err
+	}
+	if count == 0 {
+		// `count = 0` is not evaluated
+		return false, nil
+	}
+	// `count > 1` is evaluated`
+	return true, nil
+}
+
+func (r *Runner) isEvaluableForEachArgument(expr hcl.Expression) (bool, error) {
+	val, err := r.EvaluateExpr(expr, cty.DynamicPseudoType)
+	if err != nil {
+		return isEvaluableMetaArgumentsOnError(err)
+	}
+
+	if val.IsNull() {
+		// null value means that attribute is not set
+		return true, nil
+	}
+	if !val.IsKnown() {
+		// unknown value is non-deterministic
+		return false, nil
+	}
+	if !val.CanIterateElements() {
+		// uniteratable values (string, number, etc.) are invalid
+		return false, fmt.Errorf("The `for_each` value is not iterable in %s:%d", expr.Range().Filename, expr.Range().Start.Line)
+	}
+	if val.LengthInt() == 0 {
+		// empty `for_each` is not evaluated
+		return false, nil
+	}
+	// `for_each` with non-empty elements is evaluated
+	return true, nil
+}
+
+func isEvaluableMetaArgumentsOnError(err error) (bool, error) {
 	if errors.Is(err, sdk.ErrNullValue) {
 		// null value means that attribute is not set
 		return true, nil

tflint/runner_eval_test.go

@@ -796,7 +796,7 @@ resource "null_resource" "test" {
 	}
 }
 
-func Test_willEvaluateResource(t *testing.T) {
+func Test_isEvaluableResource(t *testing.T) {
 	cases := []struct {
 		Name     string
 		Content  string

tflint/runner_test.go

@@ -140,6 +140,31 @@ func Test_NewModuleRunners_nestedModules(t *testing.T) {
 	})
 }
 
+func Test_NewModuleRunners_withZeroCount(t *testing.T) {
+	withinFixtureDir(t, "module_with_count_for_each", func() {
+		runner := testRunnerWithOsFs(t, moduleConfig())
+
+		runners, err := NewModuleRunners(runner)
+		if err != nil {
+			t.Fatalf("Unexpected error occurred: %s", err)
+		}
+
+		if len(runners) != 2 {
+			t.Fatalf("This function must return 2 runners, but returned %d", len(runners))
+		}
+
+		moduleNames := make([]string, 2)
+		for idx, r := range runners {
+			moduleNames[idx] = r.TFConfig.Path.String()
+		}
+		expected := []string{"module.count_is_one", "module.for_each_is_not_empty"}
+		less := func(a, b string) bool { return a < b }
+		if diff := cmp.Diff(moduleNames, expected, cmpopts.SortSlices(less)); diff != "" {
+			t.Fatal(diff)
+		}
+	})
+}
+
 func Test_NewModuleRunners_modVars(t *testing.T) {
 	withinFixtureDir(t, "nested_module_vars", func() {
 		runner := testRunnerWithOsFs(t, moduleConfig())

tflint/test-fixtures/module_with_count_for_each/.terraform/modules/modules.json

@@ -0,0 +1 @@
+{"Modules":[{"Key":"","Source":"","Dir":"."},{"Key":"count_is_zero","Source":"./module","Dir":"module"},{"Key":"for_each_is_empty","Source":"./module","Dir":"module"},{"Key":"count_is_one","Source":"./module","Dir":"module"},{"Key":"for_each_is_not_empty","Source":"./module","Dir":"module"}]}

tflint/test-fixtures/module_with_count_for_each/module.tf

@@ -0,0 +1,42 @@
+variable "config" {
+  type = object({ instance_type = string })
+  default = null
+}
+
+module "count_is_zero" {
+  source = "./module"
+  count = var.config != null ? 1 : 0
+
+  instance_type = var.config.instance_type
+}
+
+module "count_is_one" {
+  source = "./module"
+  count = var.config != null ? 0 : 1
+
+  instance_type = "t2.micro"
+}
+
+variable "instance_types" {
+  type = list(string)
+  default = []
+}
+
+module "for_each_is_empty" {
+  source = "./module"
+  for_each = var.instance_types
+
+  instance_type = each.value
+}
+
+variable "instance_types_with_default" {
+  type = list(string)
+  default = ["t2.micro"]
+}
+
+module "for_each_is_not_empty" {
+  source = "./module"
+  for_each = var.instance_types_with_default
+
+  instance_type = each.value
+}

tflint/test-fixtures/module_with_count_for_each/module/main.tf

@@ -0,0 +1,6 @@
+variable "instance_type" {}
+
+resource "aws_instance" "foo" {
+  ami = "ami-12345678"
+  instance_type = var.instance_type
+}