-
Notifications
You must be signed in to change notification settings - Fork 28
Add new rule to check if null checking is being done using short circuit #238
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,60 @@ | ||||||
# terraform_no_short_circuit_evaluation | ||||||
|
||||||
Disallow using logical operators (`&&`, `||`) with null checks that could lead to errors due to lack of short-circuit evaluation. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Doesn't seem like null is an essential part of the description, you might be looking at length or any other intended pre-condition. |
||||||
|
||||||
> This rule is enabled by "recommended" preset. | ||||||
|
||||||
## Example | ||||||
|
||||||
```hcl | ||||||
# This will error if var.obj is null | ||||||
resource "aws_instance" "example" { | ||||||
count = var.obj != null && var.obj.enabled ? 1 : 0 | ||||||
} | ||||||
|
||||||
# This is the safe way to write it | ||||||
resource "aws_instance" "example" { | ||||||
count = var.obj != null ? var.obj.enabled ? 1 : 0 : 0 | ||||||
} | ||||||
``` | ||||||
|
||||||
``` | ||||||
$ tflint | ||||||
1 issue(s) found: | ||||||
|
||||||
Warning: Short-circuit evaluation is not supported in Terraform. Use a conditional expression (condition ? true : false) instead. (terraform_no_short_circuit_evaluation) | ||||||
|
||||||
on main.tf line 3: | ||||||
3: count = var.obj != null && var.obj.enabled ? 1 : 0 | ||||||
|
||||||
Reference: https://github.com/terraform-linters/tflint-ruleset-terraform/blob/v0.1.0/docs/rules/terraform_no_short_circuit_evaluation.md | ||||||
``` | ||||||
|
||||||
## Why | ||||||
|
||||||
Unlike many programming languages, Terraform's logical operators (`&&` and `||`) do not short-circuit. This means that in an expression like `var.obj != null && var.obj.enabled`, both sides will be evaluated even if `var.obj` is null, which will result in an error. | ||||||
|
||||||
This is a common source of confusion for users coming from other programming languages where short-circuit evaluation is standard behavior. The issue is particularly problematic when checking for null before accessing object attributes. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Compared to what? |
||||||
|
||||||
## How To Fix | ||||||
|
||||||
Use nested conditional expressions instead of logical operators when you need short-circuit behavior. For example: | ||||||
|
||||||
```hcl | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Split this into multiple code blocks for readability |
||||||
# Instead of this: | ||||||
var.obj != null && var.obj.enabled | ||||||
|
||||||
# Use this: | ||||||
var.obj != null ? var.obj.enabled : false | ||||||
|
||||||
# For more complex conditions: | ||||||
var.obj != null ? (var.obj.enabled ? var.obj.value > 0 : false) : false | ||||||
``` | ||||||
|
||||||
You can also use the `try()` function in some cases, though this may mask errors you want to catch: | ||||||
|
||||||
```hcl | ||||||
try(var.obj.enabled, false) | ||||||
``` | ||||||
|
||||||
For more information, see [hashicorp/terraform#24128](https://github.com/hashicorp/terraform/issues/24128). | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is too open-ended for users, too much history to sort through. Instead link to the official docs on this limitation: https://developer.hashicorp.com/terraform/language/expressions/operators#logical-operators And then perhaps summarize any version-specific context, e.g. if it only affects Terraform versions earlier than x. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,7 @@ var PresetRules = map[string][]tflint.Rule{ | |
NewTerraformUnusedDeclarationsRule(), | ||
NewTerraformUnusedRequiredProvidersRule(), | ||
NewTerraformWorkspaceRemoteRule(), | ||
NewTerraformNoShortCircuitEvaluationRule(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this rule should be added to the recommended preset. |
||
}, | ||
"recommended": { | ||
NewTerraformDeprecatedIndexRule(), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,139 @@ | ||
package rules | ||
|
||
import ( | ||
"github.com/hashicorp/hcl/v2" | ||
"github.com/hashicorp/hcl/v2/hclsyntax" | ||
"github.com/terraform-linters/tflint-plugin-sdk/tflint" | ||
"github.com/terraform-linters/tflint-ruleset-terraform/project" | ||
) | ||
|
||
// TerraformNoShortCircuitEvaluationRule checks for attempts to use short-circuit evaluation | ||
type TerraformNoShortCircuitEvaluationRule struct { | ||
tflint.DefaultRule | ||
} | ||
|
||
// NewTerraformNoShortCircuitEvaluationRule returns a new rule | ||
func NewTerraformNoShortCircuitEvaluationRule() *TerraformNoShortCircuitEvaluationRule { | ||
return &TerraformNoShortCircuitEvaluationRule{} | ||
} | ||
|
||
// Name returns the rule name | ||
func (r *TerraformNoShortCircuitEvaluationRule) Name() string { | ||
return "terraform_no_short_circuit_evaluation" | ||
} | ||
|
||
// Enabled returns whether the rule is enabled by default | ||
func (r *TerraformNoShortCircuitEvaluationRule) Enabled() bool { | ||
return true | ||
} | ||
|
||
// Severity returns the rule severity | ||
func (r *TerraformNoShortCircuitEvaluationRule) Severity() tflint.Severity { | ||
return tflint.WARNING | ||
} | ||
|
||
// Link returns the rule reference link | ||
func (r *TerraformNoShortCircuitEvaluationRule) Link() string { | ||
return project.ReferenceLink(r.Name()) | ||
} | ||
|
||
// Check checks for attempts to use short-circuit evaluation | ||
func (r *TerraformNoShortCircuitEvaluationRule) Check(runner tflint.Runner) error { | ||
path, err := runner.GetModulePath() | ||
if err != nil { | ||
return err | ||
} | ||
if !path.IsRoot() { | ||
// This rule does not evaluate child modules | ||
return nil | ||
} | ||
|
||
diags := runner.WalkExpressions(tflint.ExprWalkFunc(func(expr hcl.Expression) hcl.Diagnostics { | ||
if binaryOpExpr, ok := expr.(*hclsyntax.BinaryOpExpr); ok { | ||
if binaryOpExpr.Op == hclsyntax.OpLogicalAnd || binaryOpExpr.Op == hclsyntax.OpLogicalOr { | ||
// Check if left side is a null check | ||
if isNullCheck(binaryOpExpr.LHS) { | ||
// Check if right side references the same variable as the left side | ||
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.", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
binaryOpExpr.Range(), | ||
); err != nil { | ||
return hcl.Diagnostics{ | ||
{ | ||
Severity: hcl.DiagError, | ||
Summary: "Failed to emit issue", | ||
Detail: err.Error(), | ||
}, | ||
} | ||
} | ||
} | ||
} | ||
} | ||
} | ||
return nil | ||
})) | ||
|
||
if diags.HasErrors() { | ||
return diags | ||
} | ||
return nil | ||
} | ||
|
||
// 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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Strictly speaking, the only valid combinations are |
||
// Check if either side is a null literal | ||
if isNullLiteral(binaryOpExpr.RHS) || isNullLiteral(binaryOpExpr.LHS) { | ||
return true | ||
} | ||
} | ||
} | ||
return false | ||
} | ||
|
||
// isNullLiteral checks if the expression is a null literal | ||
func isNullLiteral(expr hcl.Expression) bool { | ||
if literalExpr, ok := expr.(*hclsyntax.LiteralValueExpr); ok { | ||
return literalExpr.Val.IsNull() | ||
} | ||
return false | ||
} | ||
|
||
// 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of searching for the variable reference again in |
||
var varName string | ||
if binaryOpExpr, ok := nullCheck.(*hclsyntax.BinaryOpExpr); ok { | ||
// Try to get variable name from LHS | ||
if scopeTraversalExpr, ok := binaryOpExpr.LHS.(*hclsyntax.ScopeTraversalExpr); ok { | ||
if len(scopeTraversalExpr.Traversal) > 0 { | ||
varName = scopeTraversalExpr.Traversal.RootName() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The var.foo != null && var.bar ? 1 : 0 |
||
} | ||
} | ||
// If not found in LHS, try RHS | ||
if varName == "" { | ||
if scopeTraversalExpr, ok := binaryOpExpr.RHS.(*hclsyntax.ScopeTraversalExpr); ok { | ||
if len(scopeTraversalExpr.Traversal) > 0 { | ||
varName = scopeTraversalExpr.Traversal.RootName() | ||
} | ||
} | ||
} | ||
} | ||
|
||
// If we couldn't find a variable name, return false | ||
if varName == "" { | ||
return false | ||
} | ||
|
||
// Check if the expression references the same variable | ||
vars := expr.Variables() | ||
for _, v := range vars { | ||
if len(v) > 0 && v.RootName() == varName { | ||
return true | ||
} | ||
} | ||
return false | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,109 @@ | ||
package rules | ||
|
||
import ( | ||
"testing" | ||
|
||
hcl "github.com/hashicorp/hcl/v2" | ||
"github.com/terraform-linters/tflint-plugin-sdk/helper" | ||
) | ||
|
||
func Test_TerraformNoShortCircuitEvaluationRule(t *testing.T) { | ||
cases := []struct { | ||
Name string | ||
Content string | ||
Expected helper.Issues | ||
}{ | ||
{ | ||
Name: "short circuit with null check and &&", | ||
Content: ` | ||
resource "aws_instance" "example" { | ||
count = var.obj != null && var.obj.enabled ? 1 : 0 | ||
}`, | ||
Expected: helper.Issues{ | ||
{ | ||
Rule: NewTerraformNoShortCircuitEvaluationRule(), | ||
Message: "Short-circuit evaluation is not supported in Terraform. Use a conditional expression (condition ? true : false) instead.", | ||
Range: hcl.Range{ | ||
Filename: "resource.tf", | ||
Start: hcl.Pos{Line: 3, Column: 11}, | ||
End: hcl.Pos{Line: 3, Column: 45}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
{ | ||
Name: "short circuit with null check and ||", | ||
Content: ` | ||
resource "aws_instance" "example" { | ||
count = var.obj == null || var.obj.enabled ? 1 : 0 | ||
}`, | ||
Expected: helper.Issues{ | ||
{ | ||
Rule: NewTerraformNoShortCircuitEvaluationRule(), | ||
Message: "Short-circuit evaluation is not supported in Terraform. Use a conditional expression (condition ? true : false) instead.", | ||
Range: hcl.Range{ | ||
Filename: "resource.tf", | ||
Start: hcl.Pos{Line: 3, Column: 11}, | ||
End: hcl.Pos{Line: 3, Column: 45}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
{ | ||
Name: "correct conditional usage", | ||
Content: ` | ||
resource "aws_instance" "example" { | ||
count = var.obj == null ? 0 : (var.obj.enabled ? 1 : 0) | ||
}`, | ||
Expected: helper.Issues{}, | ||
}, | ||
{ | ||
Name: "valid use of logical operators with independent values", | ||
Content: ` | ||
resource "aws_instance" "example" { | ||
count = var.value > 3 || var.other_value < 10 ? 1 : 0 | ||
}`, | ||
Expected: helper.Issues{}, | ||
}, | ||
{ | ||
Name: "valid use of logical operators with same object", | ||
Content: ` | ||
resource "aws_instance" "example" { | ||
count = var.obj > 3 || var.obj < 10 ? 1 : 0 | ||
}`, | ||
Expected: helper.Issues{}, | ||
}, | ||
{ | ||
Name: "multiple null checks in one expression", | ||
Content: ` | ||
resource "aws_instance" "example" { | ||
count = var.obj != null && var.obj.enabled && var.obj.property > 0 ? 1 : 0 | ||
}`, | ||
Expected: helper.Issues{ | ||
{ | ||
Rule: NewTerraformNoShortCircuitEvaluationRule(), | ||
Message: "Short-circuit evaluation is not supported in Terraform. Use a conditional expression (condition ? true : false) instead.", | ||
Range: hcl.Range{ | ||
Filename: "resource.tf", | ||
Start: hcl.Pos{Line: 3, Column: 11}, | ||
End: hcl.Pos{Line: 3, Column: 45}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
} | ||
|
||
rule := NewTerraformNoShortCircuitEvaluationRule() | ||
|
||
for _, tc := range cases { | ||
t.Run(tc.Name, func(t *testing.T) { | ||
runner := helper.TestRunner(t, map[string]string{"resource.tf": tc.Content}) | ||
|
||
if err := rule.Check(runner); err != nil { | ||
t.Fatalf("Unexpected error occurred: %s", err) | ||
} | ||
|
||
helper.AssertIssues(t, tc.Expected, runner.Issues) | ||
}) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
terraform_logical_operator_short_circuit
would be a better name. Most of the other rules are anti-patterns but none uses ano
prefix.