[clang-tidy] Fix false positives in `misc-redundant-expression` check

[fwolff]

Summary

It turns out that the right-hand sides of overloaded comparison operators are currently not checked at all in misc-redundant-expression, leading to such false positives as given in #54011 or here. This patch fixes this.

[aaron.ballman]

clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp

605

This seems incorrect to me -- the LHS and RHS can be swapped along with which operator is used and still achieve the same semantic results.

While playing around to test the behavior we currently have, I was a bit surprised at the difference in results here: https://godbolt.org/z/hE4qfca1b.

Also, do we need to bind? Nothing seems to look at that binding that I can see.

[fwolff]

Thanks for the quick review @aaron.ballman! I think I've addressed your comments.

clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp

605

This seems incorrect to me -- the LHS and RHS can be swapped along with which operator is used and still achieve the same semantic results.

Fixed.

While playing around to test the behavior we currently have, I was a bit surprised at the difference in results here: https://godbolt.org/z/hE4qfca1b.

All four conditions get a warning with my changes. I've added your example to the test file.

Also, do we need to bind? Nothing seems to look at that binding that I can see.

Yes, it is hard to see, but here the constant is actually read.

[aaron.ballman]

LGTM! Please also add a release note for the fix when you land it.

clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp

605

Yes, it is hard to see, but here the constant is actually read.

Oh, thanks for the info!

clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp

 
 // Matches relational expressions: 'Expr <op> k' (i.e. x < 2, x != 3, 12 <= x).
 static ast_matchers::internal::Matcher<Expr>
 matchRelationalIntegerConstantExpr(StringRef Id) {
   std::string CastId = (Id + "-cast").str();
   std::string SwapId = (Id + "-swap").str();
   std::string NegateId = (Id + "-negate").str();
   std::string OverloadId = (Id + "-overload").str();
+  std::string ConstId = (Id + "-const").str();
 
   const auto RelationalExpr = ignoringParenImpCasts(binaryOperator(
       isComparisonOperator(), expr().bind(Id),
       anyOf(allOf(hasLHS(matchSymbolicExpr(Id)),
                   hasRHS(matchIntegerConstantExpr(Id))),
             allOf(hasLHS(matchIntegerConstantExpr(Id)),
                   hasRHS(matchSymbolicExpr(Id)), expr().bind(SwapId)))));
 
const auto NegateNegateRelationalExpr =
                     hasUnaryOperand(unaryOperator(
                         hasOperatorName("!"),
                         hasUnaryOperand(anyOf(CastExpr, RelationalExpr)))));
 
   const auto OverloadedOperatorExpr =
       cxxOperatorCallExpr(
           hasAnyOverloadedOperatorName("==", "!=", "<", "<=", ">", ">="),
           // Filter noisy false positives.
-          unless(isMacro()), unless(isInTemplateInstantiation()))
+          unless(isMacro()), unless(isInTemplateInstantiation()),
+          anyOf(hasLHS(ignoringParenImpCasts(integerLiteral().bind(ConstId))),
+                hasRHS(ignoringParenImpCasts(integerLiteral().bind(ConstId)))))
           .bind(OverloadId);
 
   return anyOf(RelationalExpr, CastExpr, NegateRelationalExpr,
                NegateNegateRelationalExpr, OverloadedOperatorExpr);
 }
 
 // Checks whether a function param is non constant reference type, and may
 // be modified in the function.
if (const auto *Bin = Result.Nodes.getNodeAs<BinaryOperator>(Id)) {
     Opcode = BO_NE;
     OperandExpr = Cast;
     Value = APSInt(32, false);
   } else if (const auto *OverloadedOperatorExpr =
                  Result.Nodes.getNodeAs<CXXOperatorCallExpr>(OverloadId)) {
     if (canOverloadedOperatorArgsBeModified(OverloadedOperatorExpr, false))
       return false;
 
+    bool IntegerConstantIsFirstArg = false;
+
     if (const auto *Arg = OverloadedOperatorExpr->getArg(1)) {
       if (!Arg->isValueDependent() &&
+          !Arg->isIntegerConstantExpr(*Result.Context)) {
+        IntegerConstantIsFirstArg = true;
+        if (const auto *Arg = OverloadedOperatorExpr->getArg(0)) {
+          if (!Arg->isValueDependent() &&
               !Arg->isIntegerConstantExpr(*Result.Context))
             return false;
+        } else
+          return false;
       }
-    Symbol = OverloadedOperatorExpr->getArg(0);
+    } else
+      return false;
+
+    Symbol = OverloadedOperatorExpr->getArg(IntegerConstantIsFirstArg ? 1 : 0);
     OperandExpr = OverloadedOperatorExpr;
     Opcode = BinaryOperator::getOverloadedOpcode(OverloadedOperatorExpr->getOperator());
 
-    return BinaryOperator::isComparisonOp(Opcode);
+    if (!retrieveIntegerConstantExpr(Result, Id, Value, ConstExpr))
+      return false;
+
+    if (!BinaryOperator::isComparisonOp(Opcode))
+      return false;
+
+    // The call site of this function expects the constant on the RHS,
+    // so change the opcode accordingly.
+    if (IntegerConstantIsFirstArg)
+      Opcode = BinaryOperator::reverseComparisonOp(Opcode);
+
+    return true;
   } else {
     return false;
   }
 
   if (!retrieveSymbolicExpr(Result, Id, Symbol))
     return false;
 
   if (Result.Nodes.getNodeAs<Expr>(SwapId))

clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp

struct S {
   bool operator!=(int i) const { return x != i; } // not modifying
   bool operator>(const int &i) const { return x > i; } // not modifying
   bool operator<(int i) const { return x < i; } // not modifying
 };
 
 bool operator<=(const S &s, int i) { return s.x <= i; } // not modifying
 bool operator>=(const S &s, const int &i) { return s.x >= i; } // not modifying
 
+bool operator==(int i, const S &s) { return s == i; } // not modifying
+bool operator<(const int &i, const S &s) { return s > i; } // not modifying
+bool operator<=(const int &i, const S &s) { return s >= i; } // not modifying
+bool operator>(const int &i, const S &s) { return s < i; } // not modifying
+
 struct S2 {
   S2() { x = 1; }
   int x;
   // Overloaded comparison operators that are able to modify their params.
   bool operator==(const int &i) {
     this->x++;
     return x == i;
   }
#undef CONFIG
   if (s1 == 1 && s1 == 1) return true;
   // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: equivalent expression on both sides of logical operator
   if (s1 == 1 || s1 != 1) return true;
   // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: logical expression is always true
   if (s1 > 1 && s1 < 1) return true;
   // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: logical expression is always false
   if (s1 >= 1 || s1 <= 1) return true;
   // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: logical expression is always true
+  if (s1 >= 2 && s1 <= 0) return true;
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: logical expression is always false
+
+  // Same test as above but with swapped LHS/RHS on one side of the logical operator.
+  if (1 == s1 && s1 == 1) return true;
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: equivalent expression on both sides of logical operator
+  if (1 == s1 || s1 != 1) return true;
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: logical expression is always true
+  if (1 < s1 && s1 < 1) return true;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: logical expression is always false
+  if (1 <= s1 || s1 <= 1) return true;
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: logical expression is always true
+  if (2 < s1 && 0 > s1) return true;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: logical expression is always false
+
+  // Test for absence of false positives (issue #54011).
+  if (s1 == 1 || s1 == 2) return true;
+  if (s1 > 1 && s1 < 3) return true;
+  if (s1 >= 2 || s1 <= 0) return true;
 
   // Test for overloaded operators that may modify their params.
   S2 s2;
   if (s2 == 1 || s2 != 1) return true;
   if (s2 == 1 || s2 == 1) return true;
   if (s2 > 1 && s2 < 1) return true;
   if (s2 >= 1 || s2 <= 1) return true;
 }