clang-tidy: Add initial check for "Don't use else after return".

[djasper]

Summary

As per the LLVM coding standards:
http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return

Initial version, probably still quite a bit to do until this is really useful.

[dblaikie]

Not your problem, but I'm wondering: If/when/how we'll be able to integrate clang-tidy checks into the compile step for developers. This warning and many others in clang-tidy ought to be cheap enough to run at compile time and as hard errors just like many real clang warnings (the only reason they're not is that they're stylistic in nature and so don't meet that bar for the compiler warnings - not because they aren't cheap, low false positive, etc). It'd be nice not to have these as asynchronous results but as errors during the build.

clang-tidy/misc/ElseAfterReturnCheck.cpp

38 ↗

(On Diff #18020)

Is this the "quite a bit to do" you are referring to? Or are there other holes you know need fixing? (perhaps some could be included in the test case to give an idea of why this isn't ready for prime-time yet)

[silvas]

In D6927#107453, @dblaikie wrote:

Not your problem, but I'm wondering: If/when/how we'll be able to integrate clang-tidy checks into the compile step for developers. This warning and many others in clang-tidy ought to be cheap enough to run at compile time and as hard errors just like many real clang warnings (the only reason they're not is that they're stylistic in nature and so don't meet that bar for the compiler warnings - not because they aren't cheap, low false positive, etc). It'd be nice not to have these as asynchronous results but as errors during the build.

Maybe there's scope for a way to add warnings/errors which are run as a separate AST consumer, rather than integrated into the parsing/lexing code path. That way, you don't pay for them if you don't use them (also you don't pay for them in added complexity within the parsing/lexing code). I think most AST-based clang-tidy checks would fit that model. We also have a warning we added internally that would be very nice to cleanly segregate out of the main code path like that.

[alexfh]

The rule applies to other control statements as well, such as break, continue and throw (may be also goto, but who cares about this rule when a goto is used?). Can we handle them in the same check? Then the name may need to be changed.

clang-tidy/misc/ElseAfterReturnCheck.cpp

34 ↗

(On Diff #18020)

clang-format?

35 ↗

(On Diff #18020)

Maybe explain what "here" means? How about "don't use else after a return"?

clang-tidy/misc/MiscTidyModule.cpp

32 ↗

(On Diff #18020)

It's definitely a readability check, and it's style-specific. Thus, I think, it's better to put this check in the readability/ directory and then include in the styles it's relevant to (currently, llvm, maybe also google, if a similar rule is added there).

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

15 ↗

(On Diff #18020)

Please add a line number to avoid incorrect matches.

[djasper]

It does apply to other statements and we are going to implement those in this check, but I'd like the check to have the same name as the rule in the coding standard. Also, after "return" seems to be by far the most frequent case.

clang-tidy/misc/ElseAfterReturnCheck.cpp

34 ↗

(On Diff #18020)

Done.

35 ↗

(On Diff #18020)

Done.

38 ↗

(On Diff #18020)

That's part of the analysis. Happy to do all that, but don't have time at the moment. I think the check is useful as is, though.

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

15 ↗

(On Diff #18020)

Done.

[alexfh]

clang-tidy/readability/ElseAfterReturnCheck.cpp

22

Please add a FIXME to support continue, break and throw.

35

Looks like a debug code. Is it needed?

40

const auto *CS?

41

Please add a FIXME to come up with a good way to ensure that the formatting is updated for the whole compound statement. I guess that the "format changed lines" strategy will fail here. Maybe we'll need to remove some leading whitespace from each line to ensure it's treated as changed.

[djasper]

clang-tidy/readability/ElseAfterReturnCheck.cpp

22

Done.

35

Done.

40

Done.

41

If it doesn't work, I actually want to fix clang-format to handle this correctly. Might come up in other workflows, too and we want it to work anywhere (git clang-format, etc.).

[djasper]

Submitted as r226025.

clang-tidy/readability/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyReadabilityModule
   BracesAroundStatementsCheck.cpp
+  ElseAfterReturnCheck.cpp
   FunctionSize.cpp
   NamespaceCommentCheck.cpp
   ReadabilityTidyModule.cpp
   RedundantSmartptrGet.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTooling
   )

clang-tidy/readability/ElseAfterReturnCheck.h

+//===--- ElseAfterReturnCheck.h - clang-tidy---------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_ELSE_AFTER_RETURN_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_ELSE_AFTER_RETURN_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+
+class ElseAfterReturnCheck : public ClangTidyCheck {
+public:
+  ElseAfterReturnCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_ELSE_AFTER_RETURN_H
+

clang-tidy/readability/ElseAfterReturnCheck.cpp

+//===--- ElseAfterReturnCheck.cpp - clang-tidy-----------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "ElseAfterReturnCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+
+void ElseAfterReturnCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      ifStmt(
+          hasThen(stmt(anyOf(returnStmt(), compoundStmt(has(returnStmt()))))),
+          hasElse(stmt().bind("else"))).bind("if"),
+      this);
+}
+
+static FixItHint removeToken(SourceLocation Loc) {
+  return FixItHint::CreateRemoval(CharSourceRange::getTokenRange(Loc, Loc));
+}
+
+void ElseAfterReturnCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *If = Result.Nodes.getNodeAs<IfStmt>("if");
+  SourceLocation ElseLoc = If->getElseLoc();
+  if (ElseLoc.isInvalid())
+    If->dump();
+  DiagnosticBuilder Diag = diag(ElseLoc, "don't use else after return");
+  Diag << removeToken(ElseLoc);
+
+  // FIXME: Removing the braces isn't always safe. Do a more careful analysis.
+  if (const CompoundStmt *CS = Result.Nodes.getNodeAs<CompoundStmt>("else"))
+    Diag << removeToken(CS->getLBracLoc()) << removeToken(CS->getRBracLoc());
+}
+
+} // namespace tidy
+} // namespace clang
+

clang-tidy/readability/ReadabilityTidyModule.cpp

 //===--- ReadabilityTidyModule.cpp - clang-tidy ---------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "BracesAroundStatementsCheck.h"
+#include "ElseAfterReturnCheck.h"
 #include "FunctionSize.h"
 #include "RedundantSmartptrGet.h"
 
 namespace clang {
 namespace tidy {
 namespace readability {
 
 class ReadabilityModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<BracesAroundStatementsCheck>(
         "readability-braces-around-statements");
+    CheckFactories.registerCheck<ElseAfterReturnCheck>(
+        "readability-else-after-return");
     CheckFactories.registerCheck<FunctionSizeCheck>(
         "readability-function-size");
     CheckFactories.registerCheck<RedundantSmartptrGet>(
         "readability-redundant-smartptr-get");
   }
 };
 
 } // namespace readability

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

+// RUN: $(dirname %s)/check_clang_tidy.sh %s readability-else-after-return %t
+// REQUIRES: shell
+
+void f(int a) {
+  if (a > 0)
+    return;
+  else // comment
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: don't use else after return
+// CHECK-FIXES: {{^}}  // comment
+    return;
+
+  if (a > 0) {
+    return;
+  } else { // comment
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: don't use else after return
+// CHECK-FIXES:  } // comment
+    return;
+  }
+
+  if (a > 0) {
+    f(0);
+    if (a > 10)
+      return;
+  } else {
+    return;
+  }
+}
+