warning when inner condition is identical

[danielmarjamaki]

Summary

The core.alpha.IdenticalExpr checker warns when some expressions are identical. For example when a if condition and else if condition are identical there is a warning.

This patch makes Clang warn also when an inner if condition is identical to the outer if condition.

[kimgr]

Added some ideas for tests that might break.

test/Analysis/identical-expressions.cpp

1543

How about a negative test here, eg

if (++x > 10) {
  if (++x > 10) {
  ...

Or one with a mod of x between the ifs, and one with an unrelated change between the ifs, eg ++y.

[danielmarjamaki]

In D10892#198699, @kimgr wrote:

Added some ideas for tests that might break.

good ideas.. I will add these tests.

[danielmarjamaki]

added a few test cases as suggested by Kim.

[danielmarjamaki]

I have tested it. report: https://drive.google.com/file/d/0BykPmWrCOxt2X3NYSWF4X21tR1k/view?usp=sharing

Statistics:
projects: 1398
files: 66578
warnings: 16

I have investigated all warnings and they are correct.

[kimgr]

Some style nits and a question around the purpose of Sr. It doesn't appear to do what I thought it would.

I'll leave further review in the hands of owners, I'm just passing through.

lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp

119

Could you say /*IgnoreSideEffects=*/ false for the last param, so it's more obvious that conditions with side effects are not considered?

120

Can you name this after its purpose? Something like OriginalCondRange...?

125

Is the Sr used for the note, and intended to point to the first if-statement? In your posted output the warning and the note both have the same location, and it would be neat if they pointed to the respective stmts.

test/Analysis/identical-expressions.cpp

1544

s/todo/FIXME/

Use proper sentence casing in comments.

s/written/emitted/ ?

1554

s/dont/Don't/

End sentence with a full stop.

1560

s/dont/Don't/ and end with a full stop.

[danielmarjamaki]

In D10892#202084, @kimgr wrote:

Some style nits and a question around the purpose of Sr. It doesn't appear to do what I thought it would.

I'll leave further review in the hands of owners, I'm just passing through.

Thanks!

I'll take care of these comments when I get back from my vacation.

[danielmarjamaki]

Minor cleanup

[zaks.anna]

Otherwise, LGTM.

Thanks!
Anna.

lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp

123

"conditions of the inner and outer statements are identical" ?

Ideally, we'd want to highlight both conditions but i am not sure if that is possible.

test/Analysis/identical-expressions.cpp

1535

What is the purpose of this '//' line?

[danielmarjamaki]

lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp

120

Removed

125

I removed Sr

lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp

if (isIdenticalStmt(AC->getASTContext(), RHS, LHS)) {
     reportIdenticalExpr(B, CheckBitwise, Sr);
   }
 }
 
 bool FindIdenticalExprVisitor::VisitIfStmt(const IfStmt *I) {
   const Stmt *Stmt1 = I->getThen();
   const Stmt *Stmt2 = I->getElse();
 
+  // Check for identical inner condition:
+  //
+  // if (x<10) {
+  //   if (x<10) {
+  //   ..
+  if (const CompoundStmt *CS = dyn_cast<CompoundStmt>(Stmt1)) {
+    if (!CS->body_empty()) {
+      const IfStmt *InnerIf = dyn_cast<IfStmt>(*CS->body_begin());
+      if (InnerIf && isIdenticalStmt(AC->getASTContext(), I->getCond(), InnerIf->getCond(), /*ignoreSideEffects=*/ false)) {
+        PathDiagnosticLocation ELoc(InnerIf->getCond(), BR.getSourceManager(), AC);
+        BR.EmitBasicReport(AC->getDecl(), Checker, "Identical conditions",
+          categories::LogicError,
+          "inner condition is identical to previous condition",
+          ELoc);
+      }
+    }
+  }
+
   // Check for identical conditions:
   //
   // if (b) {
   //   foo1();
   // } else if (b) {
   //   foo2();
   // }
   if (Stmt1 && Stmt2) {

test/Analysis/identical-expressions.cpp

void test_nowarn_long() {
   if (0) {
     b -= a;
     c = 0;
   } else { // no-warning
     b -= a;
     c = 0LL;
   }
 }
+
+
+///////////////////////////////////////////////////////////////////////////////
+// Identical inner conditions
+
+void test_warn_inner_if_1(int x) {
+  if (x == 1) {
+    if (x == 1) // expected-warning {{inner condition is identical to previous condition}}
+      ;
+  }
+
+  // FIXME: Should warn here. The warning is currently not emitted because there
+  // is code between the conditions.
+  if (x == 1) {
+    int y = x;
+    if (x == 1)
+      ;
+  }
+}
+
+void test_nowarn_inner_if_1(int x) {
+  // Don't warn when condition has side effects.
+  if (x++ == 1) {
+    if (x++ == 1)
+      ;
+  }
+
+  // Don't warn when x is changed before inner condition.
+  if (x < 10) {
+    x++;
+    if (x < 10)
+      ;
+  }
+}