[clang-tidy] readability-else-after-return: also diagnose noreturn function calls.

[lebedev.ri]

Summary

The readability-else-after-return check was not warning about an else after a call to the function that will not return. In particular, marker with a noreturn attribute, be it either C++11 [[noreturn]], or the __attribute__((noreturn)).
This differential adds support to diagnose normal function calls, and calls to member functions; but not constructors, destructors and special member functions.

A follow-up for D40505.

[lebedev.ri]

Depends on D41455.
I'm open to suggestions re diagnostic wording.
The current 'noreturn call' seems lazy, and is basically a placeholder.

[aaron.ballman]

clang-tidy/readability/ElseAfterReturnCheck.cpp

43

You've spotted the downside to this clever construct -- it is hard to make the wording not terrible here. :-)

I think the diagnostic should read do no use 'else' after a call to a function that does not return. This is trickier because the current %0 is always quoted.

docs/ReleaseNotes.rst

189

This seems to be > 80 cols, so please reformat. Also, it should be "that require cleanups and calls to functions marked with the noreturn attribute" (fixes a few grammatical issues at once).

[malcolm.parsons]

This check could also handle else after goto.

[lebedev.ri]

In D41456#961826, @malcolm.parsons wrote:

This check could also handle else after goto.

Yes, certainly. Though i'm not too sure on the restrictions.
The obvious precondition is, the label can to be defined anywhere except after the goto in the same (or child) compound statement:

if(condition) {
  goto label:
  label: ; // <- false-positive
} else ...

[alexfh]

The problem with this proposed functionality is that it's not always obvious at the call site whether a function is noreturn. exit() may be an easy case, but there may be much less obvious names and whether the function is noreturn may depend on compile-time configuration or be instantiation-dependent in template code (https://godbolt.org/g/fNtpCy):

template<typename T>
struct X {
  void f();
};
template<>
struct X<int> {
    [[noreturn]] void f();
};
template<typename T>
void f(int x) {
  if (x) {
    X<T>().f();
  } else {  // Should the check complain here?
    //...
  }
}

clang-tidy/readability/ElseAfterReturnCheck.cpp

 namespace clang {
 namespace tidy {
 namespace readability {
 
 void ElseAfterReturnCheck::registerMatchers(MatchFinder *Finder) {
   const auto ControlFlowInterruptorMatcher =
       stmt(anyOf(returnStmt().bind("return"), continueStmt().bind("continue"),
                  breakStmt().bind("break"),
-                 expr(ignoringImplicit(cxxThrowExpr().bind("throw")))));
+                 expr(ignoringImplicit(cxxThrowExpr().bind("throw"))),
+                 callExpr(hasDeclaration(functionDecl(isNoReturn())))
+                     .bind("noreturn call")));
   Finder->addMatcher(
       compoundStmt(forEach(
           ifStmt(hasThen(stmt(
                      anyOf(ControlFlowInterruptorMatcher,
                            compoundStmt(has(ControlFlowInterruptorMatcher))))),
                  hasElse(stmt().bind("else")))
               .bind("if"))),
       this);
 }
 
 void ElseAfterReturnCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *If = Result.Nodes.getNodeAs<IfStmt>("if");
   SourceLocation ElseLoc = If->getElseLoc();
   std::string ControlFlowInterruptor;
-  for (const auto *BindingName : {"return", "continue", "break", "throw"})
+  for (const auto *BindingName :
+       {"return", "continue", "break", "throw", "noreturn call"})
     if (Result.Nodes.getNodeAs<Stmt>(BindingName))
       ControlFlowInterruptor = BindingName;
 
   DiagnosticBuilder Diag = diag(ElseLoc, "do not use 'else' after '%0'")
                            << ControlFlowInterruptor;
   Diag << tooling::fixit::createRemoval(ElseLoc);
 
   // FIXME: Removing the braces isn't always safe. Do a more careful analysis.

docs/ReleaseNotes.rst

- New `objc-forbidden-subclassing
   designed to be subclassed.
 
 - New `objc-property-declaration
   <http://clang.llvm.org/extra/clang-tidy/checks/objc-property-declaration.html>`_ check
 
   Finds property declarations in Objective-C files that do not follow the
   pattern of property names in Apple's programming guide.
 
+- The 'readability-else-after-return' check was adjusted to handle throw expressions that require cleanups, calls to functions marked with noreturn attribute.
+
 - New `readability-static-accessed-through-instance
   <http://clang.llvm.org/extra/clang-tidy/checks/readability-static-accessed-through-instance.html>`_ check
 
   Finds member expressions that access static members through instances and
   replaces them with uses of the appropriate qualified-id.
 
 - The 'misc-argument-comment' check was renamed to `bugprone-argument-comment
   <http://clang.llvm.org/extra/clang-tidy/checks/bugprone-argument-comment.html>`_

test/clang-tidy/readability-else-after-return.cpp

 // RUN: %check_clang_tidy %s readability-else-after-return %t -- -- -std=c++11 -fexceptions
 
 namespace std {
 struct string {
   string(const char *);
   ~string();
 };
 } // namespace std
 
 struct my_exception {
   my_exception(const std::string &s);
 };
 
+__attribute__((noreturn)) void my_noreturn();
+[[noreturn]] void my_noreturn2();
+[[noreturn]] __attribute__((noreturn)) void my_noreturn3();
+struct my_struct {
+  [[noreturn]] static void test();
+  [[noreturn]] void test_2();
+};
+struct my_assign {
+  my_assign();
+  [[noreturn]] my_assign& operator=(const my_assign& c);
+};
+struct my_copy {
+  my_copy();
+  [[noreturn]] my_copy(const my_copy& c);
+};
+struct noreturn_ctor {
+  [[noreturn]] noreturn_ctor();
+};
+struct noreturn_dtor {
+  [[noreturn]] noreturn_dtor();
+};
+
 void f(int a) {
   if (a > 0)
     return;
   else // comment-0
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not use 'else' after 'return'
   // CHECK-FIXES: {{^}}  // comment-0
     return;
 
for (unsigned x = 0; x < 42; ++x) {
     }
     if (x) {
       throw my_exception("foo");
     } else { // comment-10
     // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not use 'else' after 'throw'
     // CHECK-FIXES: {{^}}    } // comment-10
       x++;
     }
+    if (x) {
+      my_noreturn();
+    } else { // comment-11
+    // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not use 'else' after 'noreturn call'
+    // CHECK-FIXES: {{^}}    } // comment-11
+      x++;
+    }
+    if (x) {
+      my_noreturn2();
+    } else { // comment-12
+    // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not use 'else' after 'noreturn call'
+    // CHECK-FIXES: {{^}}    } // comment-12
+      x++;
+    }
+    if (x) {
+      my_noreturn3();
+    } else { // comment-13
+    // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not use 'else' after 'noreturn call'
+    // CHECK-FIXES: {{^}}    } // comment-13
+      x++;
+    }
+    if (x) {
+      my_struct::test();
+    } else { // comment-14
+    // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not use 'else' after 'noreturn call'
+    // CH1ECK-FIXES: {{^}}    } // comment-14
+      x++;
+    }
+    if (x) {
+      my_struct s;
+      s.test_2();
+    } else { // comment-15
+    // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not use 'else' after 'noreturn call'
+    // CHECK-FIXES: {{^}}    } // comment-15
+      x++;
+    }
+    if (x) {
+      my_assign a0;
+      my_assign a1;
+      a1 = a0;
+    } else { // comment-16
+    // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not use 'else' after 'noreturn call'
+    // CHECK-FIXES: {{^}}    } // comment-16
+      x++;
+    }
+    if (x) {
+      // FIXME
+      my_copy c0;
+      my_copy c1(c0);
+    } else { // comment-17
+      x++;
+    }
+    if (x) {
+      // FIXME
+      noreturn_ctor n;
+    } else { // comment-18
+      x++;
+    }
+    if (x) {
+      // FIXME
+      noreturn_dtor n;
+    } else { // comment-19
+      x++;
+    }
   }
 }