Skip to content
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

terraform_empty_list_equality: improve expression detection #1426

Merged

Conversation

bmbferreira
Copy link
Contributor

Fixes partially #1424 to be able to also detect empty lists on more complex conditional expressions such as var.enabled && var.proxy_subnets != [] ? 1 : 0.

As @bendrucker referred here, the evaluation of complex attribute values is a deeper issue (#1257) and this PR does not fix that part.

@bmbferreira bmbferreira force-pushed the fix-empty-list-equality branch from e72f1a5 to 2095371 Compare July 1, 2022 01:27
@bmbferreira
Copy link
Contributor Author

Hi @bendrucker , thanks for the swift review. 🙇
Addressed all your comments, please let me know if there's anything else to fix/improve.

Have a great weekend!

@bmbferreira bmbferreira requested a review from bendrucker July 3, 2022 00:38
@bmbferreira bmbferreira force-pushed the fix-empty-list-equality branch from 8063659 to 35eab2b Compare July 3, 2022 00:59
Copy link
Member

@bendrucker bendrucker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, getting closer!

rules/terraformrules/terraform_empty_list_equality.go Outdated Show resolved Hide resolved
rules/terraformrules/terraform_empty_list_equality.go Outdated Show resolved Hide resolved
rules/terraformrules/terraform_empty_list_equality_test.go Outdated Show resolved Hide resolved
@bmbferreira bmbferreira requested a review from bendrucker July 4, 2022 23:22
@bendrucker
Copy link
Member

Thanks, will be away for a few days, can give this another review in about a week.

@bmbferreira
Copy link
Contributor Author

Thanks, will be away for a few days, can give this another review in about a week.

Ok! Thank you! 🙇

@wata727
Copy link
Member

wata727 commented Jul 10, 2022

While working on another issue, added a change to walk expressions inside expressions in WalkExpressions #1432
Perhaps this issue has also been resolved. Can you confirm it?

@wata727
Copy link
Member

wata727 commented Jul 27, 2022

I checked it again on the latest master branch, but it didn't fix the issue. This fix is still needed.

issuesRangeSet[exprRange] = struct{}{}
}
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this search logic can be rewritten fairly easily by #1432.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @wata727 ! Thanks for the suggestion and sorry for being silent in the last couple of weeks. Would you mind to ellaborate on your suggestion please? 😕 I'm calling WalkExpressions here but as you noticed the latest change didn't fix the issue, therefore I think I still need to process the expression recursively here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the pattern that this rule essentially wants to detect is any == []. In other words, it might be better to check against BinaryOpExpr while walking through all the expressions, rather than checking for ConditionalExpr first.

Imagine something like:

runner.WalkExpression(func(expr hcl.Expression) error {
  if binaryOpExpr, ok := expr.(*hclsyntax.BinaryOpExpr); ok && binaryOpExpr.Op.Type == cty.Bool {
    if tupleConsExpr, ok := binaryOpExpr.RHS.(*hclsyntax.TupleConsExpr); ok && len(tupleConsExpr.Exprs) == 0 {
      runner.EmitIssue(...)
    }

    if tupleConsExpr, ok := binaryOpExpr.LHS.(*hclsyntax.TupleConsExpr); ok && len(tupleConsExpr.Exprs) == 0 {
      runner.EmitIssue(...)
    }
  }
})

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahh! got it 🙇 Just tried it and it is much better now! I could even remove the map to avoid duplicated ranges 👌 thank you!

@bmbferreira bmbferreira force-pushed the fix-empty-list-equality branch from dd24efe to 02b1bd8 Compare August 9, 2022 01:27
@bmbferreira bmbferreira requested a review from wata727 August 9, 2022 01:30
Copy link
Member

@bendrucker bendrucker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small style change, would apply myself but I don't seem to have edit access to push to your PR branch

diff --git a/rules/terraformrules/terraform_empty_list_equality.go b/rules/terraformrules/terraform_empty_list_equality.go
index 25baf37a..83bbfd7a 100644
--- a/rules/terraformrules/terraform_empty_list_equality.go
+++ b/rules/terraformrules/terraform_empty_list_equality.go
@@ -58,17 +58,17 @@ func (r *TerraformEmptyListEqualityRule) checkEmptyList(runner *tflint.Runner) e
 	return runner.WalkExpressions(func(expr hcl.Expression) error {
 		if binaryOpExpr, ok := expr.(*hclsyntax.BinaryOpExpr); ok && binaryOpExpr.Op.Type == cty.Bool {
 			if tupleConsExpr, ok := binaryOpExpr.LHS.(*hclsyntax.TupleConsExpr); ok && len(tupleConsExpr.Exprs) == 0 {
-				r.emitEmptyListEqualityIssue(binaryOpExpr.Range(), runner)
+				r.emitIssue(binaryOpExpr.Range(), runner)
 			} else if tupleConsExpr, ok := binaryOpExpr.RHS.(*hclsyntax.TupleConsExpr); ok && len(tupleConsExpr.Exprs) == 0 {
-				r.emitEmptyListEqualityIssue(binaryOpExpr.Range(), runner)
+				r.emitIssue(binaryOpExpr.Range(), runner)
 			}
 		}
 		return nil
 	})
 }
 
-// emitEmptyListEqualityIssue emits issue for comparison with static empty list
-func (r *TerraformEmptyListEqualityRule) emitEmptyListEqualityIssue(exprRange hcl.Range, runner *tflint.Runner) {
+// emitIssue emits issue for comparison with static empty list
+func (r *TerraformEmptyListEqualityRule) emitIssue(exprRange hcl.Range, runner *tflint.Runner) {
 	runner.EmitIssue(
 		r,
 		"Comparing a collection with an empty list is invalid. To detect an empty collection, check its length.",

You can copy the above and pipe to git apply.

Otherwise LGTM

@bmbferreira
Copy link
Contributor Author

One small style change, would apply myself but I don't seem to have edit access to push to your PR branch

diff --git a/rules/terraformrules/terraform_empty_list_equality.go b/rules/terraformrules/terraform_empty_list_equality.go
index 25baf37a..83bbfd7a 100644
--- a/rules/terraformrules/terraform_empty_list_equality.go
+++ b/rules/terraformrules/terraform_empty_list_equality.go
@@ -58,17 +58,17 @@ func (r *TerraformEmptyListEqualityRule) checkEmptyList(runner *tflint.Runner) e
 	return runner.WalkExpressions(func(expr hcl.Expression) error {
 		if binaryOpExpr, ok := expr.(*hclsyntax.BinaryOpExpr); ok && binaryOpExpr.Op.Type == cty.Bool {
 			if tupleConsExpr, ok := binaryOpExpr.LHS.(*hclsyntax.TupleConsExpr); ok && len(tupleConsExpr.Exprs) == 0 {
-				r.emitEmptyListEqualityIssue(binaryOpExpr.Range(), runner)
+				r.emitIssue(binaryOpExpr.Range(), runner)
 			} else if tupleConsExpr, ok := binaryOpExpr.RHS.(*hclsyntax.TupleConsExpr); ok && len(tupleConsExpr.Exprs) == 0 {
-				r.emitEmptyListEqualityIssue(binaryOpExpr.Range(), runner)
+				r.emitIssue(binaryOpExpr.Range(), runner)
 			}
 		}
 		return nil
 	})
 }
 
-// emitEmptyListEqualityIssue emits issue for comparison with static empty list
-func (r *TerraformEmptyListEqualityRule) emitEmptyListEqualityIssue(exprRange hcl.Range, runner *tflint.Runner) {
+// emitIssue emits issue for comparison with static empty list
+func (r *TerraformEmptyListEqualityRule) emitIssue(exprRange hcl.Range, runner *tflint.Runner) {
 	runner.EmitIssue(
 		r,
 		"Comparing a collection with an empty list is invalid. To detect an empty collection, check its length.",

You can copy the above and pipe to git apply.

Otherwise LGTM

thanks again for the review @bendrucker. just applied the suggested change!

@bmbferreira bmbferreira requested a review from bendrucker August 10, 2022 00:21
@bendrucker
Copy link
Member

Getting some 500s right now from GitHub for merge status. I'll revisit this and merge tomorrow.

@bmbferreira
Copy link
Contributor Author

Getting some 500s right now from GitHub for merge status. I'll revisit this and merge tomorrow.

yup, np. Seems they're having some issues: https://www.githubstatus.com/
image

@bendrucker bendrucker changed the title fix: improves empty list equality check on conditional expressions terraform_empty_list_equality: improve expression detection Aug 10, 2022
@bendrucker bendrucker merged commit 53b1ede into terraform-linters:master Aug 10, 2022
@bmbferreira bmbferreira deleted the fix-empty-list-equality branch August 15, 2022 01:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants