[clang-tidy] Fixed readability-else-after-return for cascade statements

[idlecode]

Summary

Fix generated by this check changed program semantics
in the case where 'if' was a part (direct child) of other statement.

Fixes PR30652

[djasper]

clang-tidy/readability/ElseAfterReturnCheck.cpp

28 ↗

(On Diff #76328)

I think this now effectively does:

stmt(forEach(ifStmt(unless(hasParent(ifSttmt())), ...)

I think that's equivalent to:

stmt(unless(ifStmt()), forEach(ifStmt(...)))

[idlecode]

clang-tidy/readability/ElseAfterReturnCheck.cpp

28 ↗

(On Diff #76328)

Apparently you are right - does that mean that this matcher actually matches node above ifStmt (and just binds ifStmt)?
If so maybe such expression would suffice? (it passes tests)

Finder->addMatcher( // note lack of stmt(forEach(
              ifStmt(unless(hasParent(ifStmt())),
                 hasThen(stmt(
                     anyOf(ControlFlowInterruptorMatcher,
                           compoundStmt(has(ControlFlowInterruptorMatcher))))),
                 hasElse(stmt().bind("else")))
              .bind("if"),
      this);

But I'm not sure if this would be any better than your version.

[djasper]

clang-tidy/readability/ElseAfterReturnCheck.cpp

28 ↗

(On Diff #76328)

Yeah. That makes sense to me. The difference is that we would start binding ifStmts() that are direct children of declarations. But I don't know whether we have excluded those intentionally. Do any tests break if you make this change?

[idlecode]

clang-tidy/readability/ElseAfterReturnCheck.cpp

28 ↗

(On Diff #76328)

I have just ran make check-clang-tools on updated source tree - no new failures. Upload the change?
What do you mean by 'if statement as children of declaration'? (I'm new to compilers and just curious :) )

[djasper]

clang-tidy/readability/ElseAfterReturnCheck.cpp

28 ↗

(On Diff #76328)

I did some more digging. It appears this bug was introduced in https://reviews.llvm.org/rL278257. The old code specifically did the forEach stuff to ensure that the ifStmt was a direct child of a compound stmt. I think we should investigate why this happened and probably undo it. There are similar to this one here if the ifStmt is a child of a for or while loop without a compound stmt.

[idlecode]

clang-tidy/readability/ElseAfterReturnCheck.cpp

28 ↗

(On Diff #76328)

Indeed, my fix fails for loops.
Also, do you see any reason why 'goto' was not included in interrupt statements?

[djasper]

clang-tidy/readability/ElseAfterReturnCheck.cpp

28 ↗

(On Diff #76328)

I don't think anyone cares about readability when there is "goto" ;)

[mgehre]

With this fix, is there still a warning on the following code?

if(b) {
   ...
   if(c)
    return;
   else
    doSomething()
   ...
}

I would expect that the check still warns on it.

[idlecode]

@mgehre: Yes it does - in your case AST looks like ifStmt(b) - CompoundStmt - ifStmt(c), so ifStmt's don't have direct parent-child relations.
But there is another problem:

while(a)
  if (b)
    return 1;
  else
    doSomething(2);

In such case, generated fix (with and without this fix) will change semantics of program.
As @djasper suggested, I need to take a look at this check history and update patch accordingly.

[idlecode]

I have reverted matcher to the state before https://reviews.llvm.org/D23265 (tests are passing with compoundStmt instead of stmt and this way is better than mine).

[djasper]

Looks good. Thanks for fixing this!

clang-tools-extra/trunk/clang-tidy/readability/ElseAfterReturnCheck.cpp

 namespace tidy {
 namespace readability {
 
 void ElseAfterReturnCheck::registerMatchers(MatchFinder *Finder) {
   const auto ControlFlowInterruptorMatcher =
       stmt(anyOf(returnStmt().bind("return"), continueStmt().bind("continue"),
                  breakStmt().bind("break"), cxxThrowExpr().bind("throw")));
   Finder->addMatcher(
-      stmt(forEach(
+      compoundStmt(forEach(
           ifStmt(hasThen(stmt(
                      anyOf(ControlFlowInterruptorMatcher,
                            compoundStmt(has(ControlFlowInterruptorMatcher))))),
                  hasElse(stmt().bind("else")))
               .bind("if"))),
       this);
 }
 

clang-tools-extra/trunk/test/clang-tidy/readability-else-after-return.cpp

if (a > 0) {
     return;
   }
 
   if (a > 0)
     f(0);
   else if (a > 10)
     return;
   else // comment-2
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not use 'else' after 'return'
-  // CHECK-FIXES: {{^}}  // comment-2
+  // CHECK-FIXES-NOT: {{^}}  // comment-2
+    f(0);
+
+  if (a > 0)
+    if (a < 10)
+      return;
+    else // comment-3
+    // CHECK-FIXES-NOT: {{^}}    // comment-3
       f(0);
+  else
+    if (a > 10)
+      return;
+    else // comment-4
+    // CHECK-FIXES-NOT: {{^}}    // comment-4
+      f(0);
+
+  if (a > 0) {
+    if (a < 10)
+      return;
+    else // comment-5
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
+    // CHECK-FIXES: {{^}}    // comment-5
+      f(0);
+  } else {
+    if (a > 10)
+      return;
+    else // comment-6
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
+    // CHECK-FIXES: {{^}}    // comment-6
+      f(0);
+  }
 }
 
 void foo() {
   for (unsigned x = 0; x < 42; ++x) {
     if (x) {
       continue;
-    } else { // comment-3
+    } else { // comment-7
     // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not use 'else' after 'continue'
-    // CHECK-FIXES: {{^}}    } // comment-3
+    // CHECK-FIXES: {{^}}    } // comment-7
       x++;
     }
     if (x) {
       break;
-    } else { // comment-4
+    } else { // comment-8
     // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not use 'else' after 'break'
-    // CHECK-FIXES: {{^}}    } // comment-4
+    // CHECK-FIXES: {{^}}    } // comment-8
       x++;
     }
     if (x) {
       throw 42;
-    } else { // comment-5
+    } else { // comment-9
     // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not use 'else' after 'throw'
-    // CHECK-FIXES: {{^}}    } // comment-5
+    // CHECK-FIXES: {{^}}    } // comment-9
       x++;
     }
   }
 }