Repository: go-critic/go-critic (go)

Issue:
badCond: reports wrong suspicious condition
Found a bug in `badCond`: 

```
if paramType == lastParam.GetType() && paramType == bgp.BGP_ASPATH_ATTR_TYPE_SEQ {
```

Which indeed looks suspicious but here is the thing: `GetType()` might return `BGP_ASPATH_ATTR_TYPE_SEQ` which is a correct behaviour.

I think in `badCond` we should compare only simple/pure/safe expressions.


Pull request:
checker/badCond: rhs should be also check for safe expr
Added checks for RHS also, in case of unsafe expression we should not throw errors.

@cristaloleg 

Fixes #1485 

Patch:
diff --git a/checkers/badCond_checker.go b/checkers/badCond_checker.go
index 9be45cc..22a6267 100644
--- a/checkers/badCond_checker.go
+++ b/checkers/badCond_checker.go
@@ -87,7 +87,8 @@ func (c *badCondChecker) checkExpr(expr ast.Expr) {
 
 func (c *badCondChecker) equalToBoth(lhs, rhs *ast.BinaryExpr) bool {
 	return lhs.Op == token.EQL && rhs.Op == token.EQL &&
-		astequal.Expr(lhs.X, rhs.X)
+		astequal.Expr(lhs.X, rhs.X) &&
+		typep.SideEffectFree(c.ctx.TypesInfo, lhs.Y) && typep.SideEffectFree(c.ctx.TypesInfo, rhs.Y)
 }
 
 func (c *badCondChecker) lessAndGreater(lhs, rhs *ast.BinaryExpr) bool {


Test patch:
diff --git a/checkers/testdata/badCond/negative_tests.go b/checkers/testdata/badCond/negative_tests.go
index 1bbf9ae..437bb89 100644
--- a/checkers/testdata/badCond/negative_tests.go
+++ b/checkers/testdata/badCond/negative_tests.go
@@ -2,6 +2,8 @@ package checker_test
 
 func getIntPtr(v *int) {}
 
+func newError() error { return nil }
+
 func fixed1(retVal []int, start int) {
 	for i := 0; i < start; i++ {
 		retVal[i] = 0
@@ -62,4 +64,8 @@ func fixed3(x int) {
 	// in another way, like `x == 10 && y == 10`.
 	var y int
 	_ = x == 10 && y == 10
+
+	_ = err == nil && err == newError()
+
+	_ = err == newError() && err == nil
 }
diff --git a/checkers/testdata/badCond/positive_tests.go b/checkers/testdata/badCond/positive_tests.go
index aa93ae7..4df8b07 100644
--- a/checkers/testdata/badCond/positive_tests.go
+++ b/checkers/testdata/badCond/positive_tests.go
@@ -1,7 +1,5 @@
 package checker_test
 
-func newError() error { return nil }
-
 func bad1(retVal []int, start int) {
 	/*! `i > start` in loop; probably meant `i < start`? */
 	for i := 0; i > start; i++ {
@@ -37,10 +35,6 @@ func bad3(x int) {
 	/*! `(x == 10) && (x == 20)` condition is suspicious */
 	_ = (x == 10) && (x == 20)
 
-	var err error
-	/*! `err == nil && err == newError()` condition is suspicious */
-	_ = err == nil && err == newError()
-
 	// This one is (probably) not an error, but can be written
 	// in another way, like `x == 10 && y == 10`.
 	var y int