Add new rule to check if null checking is being done using short circuit #238

bmbferreira · 2025-02-09T00:38:49Z

This adds a new rule terraform_no_short_circuit_evaluation that checks for attempts to use logical operators (&&, ||) with null checks that could lead to errors due to lack of short-circuit evaluation.

This is one of the most common mistakes I usually see in Terraform for beginners. While this issue was recently fixed in Terraform core (hashicorp/terraform#24128), older Terraform versions don't support short-circuit evaluation. Therefore, it would be helpful to have TFLint check this optionally by enabling this rule.

The rule will warn about code patterns like:

# This will error if var.obj is null
count = var.obj != null && var.obj.enabled ? 1 : 0
# Should be written as:
count = var.obj != null ? var.obj.enabled ? 1 : 0 : 0

The rule could be enabled by default and included in the "recommended" preset since this is a common source of errors for Terraform users.

wata727 · 2025-02-09T16:44:45Z

Nice! This is one of the surprising behaviors of the Terraform language, and it's great to be able to find the possible error statically.
It's probably impossible to find all issues, but it's a good idea to narrow the scope to common patterns.

Furthermore, it would be nice to be able to auto-fix the issue.

I'll review this again later.

wata727 · 2025-02-10T15:00:52Z

rules/terraform_no_short_circuit_evaluation.go

+		// Try to get variable name from LHS
+		if scopeTraversalExpr, ok := binaryOpExpr.LHS.(*hclsyntax.ScopeTraversalExpr); ok {
+			if len(scopeTraversalExpr.Traversal) > 0 {
+				varName = scopeTraversalExpr.Traversal.RootName()


The RootName will only return var for addresses like var.foo. So there are false positives in expressions like:

var.foo != null && var.bar ? 1 : 0

wata727 · 2025-02-10T15:05:53Z

rules/terraform_no_short_circuit_evaluation.go

+
+// referencesNullCheckedVar checks if the right side expression references the same variable that was null checked
+func referencesNullCheckedVar(nullCheck, expr hcl.Expression) bool {
+	// Get the variable name from the null check


Instead of searching for the variable reference again in referencesNullCheckedVar, it would be better for isNullCheck to return it.

wata727 · 2025-02-10T15:13:52Z

rules/terraform_no_short_circuit_evaluation.go

+// isNullCheck determines if an expression is checking for null
+func isNullCheck(expr hcl.Expression) bool {
+	if binaryOpExpr, ok := expr.(*hclsyntax.BinaryOpExpr); ok {
+		if binaryOpExpr.Op == hclsyntax.OpEqual || binaryOpExpr.Op == hclsyntax.OpNotEqual {


Strictly speaking, the only valid combinations are OpLogicalAnd with OpNotEqual and OpLogicalOr with OpEqual. However, in practice, an expression like var.obj == null && var.obj.enabled is rarely useful, so this may be sufficient as it is.

wata727 · 2025-02-10T15:38:43Z

rules/terraform_no_short_circuit_evaluation.go

+					if referencesNullCheckedVar(binaryOpExpr.LHS, binaryOpExpr.RHS) {
+						if err := runner.EmitIssue(
+							r,
+							"Short-circuit evaluation is not supported in Terraform. Use a conditional expression (condition ? true : false) instead.",


It's debatable whether it's appropriate to always suggest a conditional expression as a fix. According to hashicorp/terraform#24128 there are a variety of different solutions.
As you already mentioned in the documentation, another option is to use the try function.

wata727 · 2025-02-10T15:42:25Z

rules/preset.go

@@ -22,6 +22,7 @@ var PresetRules = map[string][]tflint.Rule{
 		NewTerraformUnusedDeclarationsRule(),
 		NewTerraformUnusedRequiredProvidersRule(),
 		NewTerraformWorkspaceRemoteRule(),
+		NewTerraformNoShortCircuitEvaluationRule(),


I think this rule should be added to the recommended preset.

bendrucker

If an upcoming version of Terraform eliminates this footgun that's a great reason for this not to be enabled by default. Users of Terraform who are sufficiently up to date should be free to use the more familiar/concise syntax. Authors of reusable modules need to be more concerned about backward compatibility.

bendrucker · 2025-02-10T22:39:36Z