[clang-tidy] ElseAfterReturn check wont suggest fixes if preprocessor branches are involved

[njames93]

Summary

Consider this code:

if (Cond) {
#ifdef X_SUPPORTED
  X();
#else
  return;
#endif
} else {
  Y();
}
Z();

In this example, if X_SUPPORTED is not defined, currently we'll get a warning from the else-after-return check. However If we apply that fix, and then the code is recompiled with X_SUPPORTED defined, we have inadvertently changed the behaviour of the if statement due to the else being removed. Code flow when Cond is true will be:

X();
Y();
Z();

where as before the fix it was:

X();
Z();

This patch adds checks that guard against #endif directives appearing between the control flow interrupter and the else and not applying the fix if they are detected.

[njames93]

Fix assertions failing with included files.
Ran this over the entire clang directory, which has a lot of macro (ab)use, and no crashes or otherwise unexpected behaviour.

[aaron.ballman]

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

118

Unrelated formatting change?

189

Should the map be passed by const ref to avoid copies?

203

Should this be const auto & to be clear there are no modifications to the map?

214

Missing a full stop at the end of the comment.

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

268

Same -> same
Also, missing a full stop.

286

ensure -> Ensure
Also, missing a full stop.

305

diagnost -> diagnose

314

We should probably add some tests for more pathological cases, like:

#if 1
if (true) {
  return;
#else
{
#endif
} else {
  return;
}

[njames93]

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

314

I'm don't even think its worth adding tests for that kind of code. Its impossible to reason about at the AST level(we dont get any AST for discarded pp conditional branches). So for these or cases like :

if (true) {
#if 1
  return;
} else {
#endif
  return;
}

Its just simpler to leave the behaviour as is and hope that a user sees the change and addresses (or suppresses) it manually.

[njames93]

Address comments.

[njames93]

Whoops missed one.

[aaron.ballman]

LGTM aside from the testing request.

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

314

I was not suggesting that we add the tests because we want to elegantly handle such eldritch horrors, but because I want to be sure we're not going to crash/assert/etc if we encounter them. My suggestion is to add the test cases and if you think the behavior isn't ideal, just stick a FIXME comment on the test case to show that we don't like the behavior but aren't dealing with it right now.

[steveire]

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

34

auto needs to be replaced/expanded.

208

auto needs to be replaced/expanded.

[njames93]

Added test cases to ensure clang-tidy doesn't crash.
Expanded auto out.

[aaron.ballman]

LGTM!

clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return-pp-no-crash.cpp

1

You can drop the -- -std=c++11 from this.

clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.h

 //===--- ElseAfterReturnCheck.h - clang-tidy---------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_ELSEAFTERRETURNCHECK_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_ELSEAFTERRETURNCHECK_H
 
 #include "../ClangTidyCheck.h"
+#include "llvm/ADT/DenseMap.h"
 
 namespace clang {
 namespace tidy {
 namespace readability {
 
 /// Flags the usages of `else` after `return`.
 ///
 /// http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return
 class ElseAfterReturnCheck : public ClangTidyCheck {
 public:
   ElseAfterReturnCheck(StringRef Name, ClangTidyContext *Context);
 
   void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+  void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
+                           Preprocessor *ModuleExpanderPP) override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 
+  using ConditionalBranchMap =
+      llvm::DenseMap<FileID, SmallVector<SourceRange, 1>>;
+
 private:
   const bool WarnOnUnfixable;
   const bool WarnOnConditionVariables;
+  ConditionalBranchMap PPConditionals;
 };
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_ELSEAFTERRETURNCHECK_H

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

 //===--- ElseAfterReturnCheck.cpp - clang-tidy-----------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "ElseAfterReturnCheck.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Lex/Lexer.h"
+#include "clang/Lex/Preprocessor.h"
 #include "clang/Tooling/FixIt.h"
 #include "llvm/ADT/SmallVector.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace readability {
 
-static const char ReturnStr[] = "return";
-static const char ContinueStr[] = "continue";
-static const char BreakStr[] = "break";
-static const char ThrowStr[] = "throw";
+namespace {
+
+class PPConditionalCollector : public PPCallbacks {
+public:
+  PPConditionalCollector(
+      ElseAfterReturnCheck::ConditionalBranchMap &Collections,
+      const SourceManager &SM)
+      : Collections(Collections), SM(SM) {}
+  void Endif(SourceLocation Loc, SourceLocation IfLoc) override {
+    if (!SM.isWrittenInSameFile(Loc, IfLoc))
+      return;
+    SmallVectorImpl<SourceRange> &Collection = Collections[SM.getFileID(Loc)];
+    assert(Collection.empty() || Collection.back().getEnd() < Loc);
+    Collection.emplace_back(IfLoc, Loc);
+  }
+
+private:
+  ElseAfterReturnCheck::ConditionalBranchMap &Collections;
+  const SourceManager &SM;
+};
+
+} // namespace
+
+static const char InterruptingStr[] = "interrupting";
 static const char WarningMessage[] = "do not use 'else' after '%0'";
 static const char WarnOnUnfixableStr[] = "WarnOnUnfixable";
 static const char WarnOnConditionVariablesStr[] = "WarnOnConditionVariables";
 
 static const DeclRefExpr *findUsage(const Stmt *Node, int64_t DeclIdentifier) {
   if (!Node)
     return nullptr;
   if (const auto *DeclRef = dyn_cast<DeclRefExpr>(Node)) {
static bool containsDeclInScope(const Stmt *Node) {
   if (const auto *Compound = dyn_cast<CompoundStmt>(Node))
     return llvm::any_of(Compound->body(), [](const Stmt *SubNode) {
       return isa<DeclStmt>(SubNode);
     });
   return false;
 }
 
 static void removeElseAndBrackets(DiagnosticBuilder &Diag, ASTContext &Context,
                            const Stmt *Else, SourceLocation ElseLoc) {
   auto Remap = [&](SourceLocation Loc) {
     return Context.getSourceManager().getExpansionLoc(Loc);
   };
   auto TokLen = [&](SourceLocation Loc) {
     return Lexer::MeasureTokenLength(Loc, Context.getSourceManager(),
                                      Context.getLangOpts());
   };
 
: ClangTidyCheck(Name, Context),
       WarnOnConditionVariables(Options.get(WarnOnConditionVariablesStr, true)) {
 }
 
 void ElseAfterReturnCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, WarnOnUnfixableStr, WarnOnUnfixable);
   Options.store(Opts, WarnOnConditionVariablesStr, WarnOnConditionVariables);
 }
 
+void ElseAfterReturnCheck::registerPPCallbacks(const SourceManager &SM,
+                                               Preprocessor *PP,
+                                               Preprocessor *ModuleExpanderPP) {
+  PP->addPPCallbacks(
+      std::make_unique<PPConditionalCollector>(this->PPConditionals, SM));
+}
+
 void ElseAfterReturnCheck::registerMatchers(MatchFinder *Finder) {
-  const auto InterruptsControlFlow =
-      stmt(anyOf(returnStmt().bind(ReturnStr), continueStmt().bind(ContinueStr),
-                 breakStmt().bind(BreakStr),
-                 expr(ignoringImplicit(cxxThrowExpr().bind(ThrowStr)))));
+  const auto InterruptsControlFlow = stmt(anyOf(
+      returnStmt().bind(InterruptingStr), continueStmt().bind(InterruptingStr),
+      breakStmt().bind(InterruptingStr),
+      expr(ignoringImplicit(cxxThrowExpr().bind(InterruptingStr)))));
   Finder->addMatcher(
       compoundStmt(
           forEach(ifStmt(unless(isConstexpr()),
                          hasThen(stmt(
                              anyOf(InterruptsControlFlow,
                                    compoundStmt(has(InterruptsControlFlow))))),
                          hasElse(stmt().bind("else")))
                       .bind("if")))
           .bind("cs"),
       this);
 }
 
+static bool hasPreprocessorBranchEndBetweenLocations(
+    const ElseAfterReturnCheck::ConditionalBranchMap &ConditionalBranchMap,
+    const SourceManager &SM, SourceLocation StartLoc, SourceLocation EndLoc) {
+
+  SourceLocation ExpandedStartLoc = SM.getExpansionLoc(StartLoc);
+  SourceLocation ExpandedEndLoc = SM.getExpansionLoc(EndLoc);
+  if (!SM.isWrittenInSameFile(ExpandedStartLoc, ExpandedEndLoc))
+    return false;
+
+  // StartLoc and EndLoc expand to the same macro.
+  if (ExpandedStartLoc == ExpandedEndLoc)
+    return false;
+
+  assert(ExpandedStartLoc < ExpandedEndLoc);
+
+  auto Iter = ConditionalBranchMap.find(SM.getFileID(ExpandedEndLoc));
+
+  if (Iter == ConditionalBranchMap.end() || Iter->getSecond().empty())
+    return false;
+
+  const SmallVectorImpl<SourceRange> &ConditionalBranches = Iter->getSecond();
+
+  assert(llvm::is_sorted(ConditionalBranches,
+                         [](const SourceRange &LHS, const SourceRange &RHS) {
+                           return LHS.getEnd() < RHS.getEnd();
+                         }));
+
+  // First conditional block that ends after ExpandedStartLoc.
+  const auto *Begin =
+      llvm::lower_bound(ConditionalBranches, ExpandedStartLoc,
+                        [](const SourceRange &LHS, const SourceLocation &RHS) {
+                          return LHS.getEnd() < RHS;
+                        });
+  const auto *End = ConditionalBranches.end();
+  for (; Begin != End && Begin->getEnd() < ExpandedEndLoc; ++Begin)
+    if (Begin->getBegin() < ExpandedStartLoc)
+      return true;
+  return false;
+}
+
+static StringRef getControlFlowString(const Stmt &Stmt) {
+  if (isa<ReturnStmt>(Stmt))
+    return "return";
+  if (isa<ContinueStmt>(Stmt))
+    return "continue";
+  if (isa<BreakStmt>(Stmt))
+    return "break";
+  if (isa<CXXThrowExpr>(Stmt))
+    return "throw";
+  llvm_unreachable("Unknown control flow interruptor");
+}
+
 void ElseAfterReturnCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *If = Result.Nodes.getNodeAs<IfStmt>("if");
   const auto *Else = Result.Nodes.getNodeAs<Stmt>("else");
   const auto *OuterScope = Result.Nodes.getNodeAs<CompoundStmt>("cs");
-
-  bool IsLastInScope = OuterScope->body_back() == If;
+  const auto *Interrupt = Result.Nodes.getNodeAs<Stmt>(InterruptingStr);
   SourceLocation ElseLoc = If->getElseLoc();
 
-  auto ControlFlowInterruptor = [&]() -> llvm::StringRef {
-    for (llvm::StringRef BindingName :
-         {ReturnStr, ContinueStr, BreakStr, ThrowStr})
-      if (Result.Nodes.getNodeAs<Stmt>(BindingName))
-        return BindingName;
-    return {};
-  }();
+  if (hasPreprocessorBranchEndBetweenLocations(
+          PPConditionals, *Result.SourceManager, Interrupt->getBeginLoc(),
+          ElseLoc))
+    return;
+
+  bool IsLastInScope = OuterScope->body_back() == If;
+  StringRef ControlFlowInterruptor = getControlFlowString(*Interrupt);
 
   if (!IsLastInScope && containsDeclInScope(Else)) {
     if (WarnOnUnfixable) {
       // Warn, but don't attempt an autofix.
       diag(ElseLoc, WarningMessage) << ControlFlowInterruptor;
     }
     return;
   }

clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return-pp-no-crash.cpp

+// RUN: clang-tidy %s -checks=-*,readability-else-after-return -- -std=c++11
+
+// We aren't concerned about the output here, just want to ensure clang-tidy doesn't crash.
+void foo() {
+#if 1
+  if (true) {
+    return;
+#else
+  {
+#endif
+  } else {
+    return;
+  }
+
+  if (true) {
+#if 1
+    return;
+  } else {
+#endif
+    return;
+  }
+}

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

void test_B44745() {
   if (auto X = false) {
     return;
   } else {
     for (;;) {
     }
   }
   return;
 }
+
+void testPPConditionals() {
+
+  // These cases the return isn't inside the conditional so diagnose as normal.
+  if (true) {
+    return;
+#if 1
+#endif
+  } else {
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
+    return;
+  }
+  if (true) {
+#if 1
+#endif
+    return;
+  } else {
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
+    return;
+  }
+
+  // No return here in the AST, no special handling needed.
+  if (true) {
+#if 0
+    return;
+#endif
+  } else {
+    return;
+  }
+
+  // Return here is inside a preprocessor conditional block, ignore this case.
+  if (true) {
+#if 1
+    return;
+#endif
+  } else {
+    return;
+  }
+
+  // These cases, same as above but with an #else block.
+  if (true) {
+#if 1
+    return;
+#else
+#endif
+  } else {
+    return;
+  }
+  if (true) {
+#if 0
+#else
+    return;
+#endif
+  } else {
+    return;
+  }
+
+// Ensure it can handle macros.
+#define RETURN return
+  if (true) {
+#if 1
+    RETURN;
+#endif
+  } else {
+    return;
+  }
+#define ELSE else
+  if (true) {
+#if 1
+    return;
+#endif
+  }
+  ELSE {
+    return;
+  }
+
+  // Whole statement is in a conditional block so diagnose as normal.
+#if 1
+  if (true) {
+    return;
+  } else {
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
+    return;
+  }
+#endif
+}