Simplify CheckFallThroughForBody

[gromer]

Summary

Split CheckFallThroughForBody into separate implementations for blocks, lambdas, coroutines, and all other functions. This simplifies the code because virtually every part of it varies depending on what kind of function is being processed; it's both clearer and safer to branch once at the beginning, rather than branch repeatedly (sometimes on subtly different conditions) and/or rely on layers of indirection. Notice, for example, how this refactoring surfaces the inconsistency between the fast and slow paths for coroutines, and enables the fast-path condition for functions to take advantage of more information about what kinds of diagnostics the slow path can produce (see specifically how SuggestNoReturn now feeds into the early-return condition).

[rsmith]

I'm not a fan of the duplication introduced here, but the new code is definitely more obvious. On balance, this seems like a small improvement, so let's go for it.

b/llvm/tools/clang/lib/Sema/AnalysisBasedWarnings.cpp

541–543

Hmm. It really doesn't make any sense to apply cpu_dispatch to a lambda, because the call operator can't be overloaded. I don't think this special case warning suppression is worthwhile. (Rather, we should probably disallow the attribute on lambdas; I've asked Erich Keane about that.)

594

I know this is just rearranged from where it was before, but... can you remove the redundant parens here, and run the patch through clang-format?

619

This if and the one below are redundant now.

[gromer]

b/llvm/tools/clang/lib/Sema/AnalysisBasedWarnings.cpp

619

Leaving aside the mismatch between the locations passed to isIgnored and Diag, I believe this if still has an observable effect (namely suppressing the diagnostic) in the case where ReturnsValue is false, HasNoReturnAttr is true, and neither diagnostic is ignored.

Even if I'm wrong about that, it's not clear to me whether the error is here or in the early return condition (as noted in the FIXME above, the two do not appear to match up).

[gromer]

OK, the diffs are now un-borked. Sorry for the flailing incompetence.

b/llvm/tools/clang/lib/Sema/AnalysisBasedWarnings.cpp

static ControlFlowKind CheckFallThrough(AnalysisDeclContext &AC) {
   if (HasAbnormalEdge || HasFakeEdge || HasLiveReturn)
     return MaybeFallThrough;
   // This says AlwaysFallThrough for calls to functions that are not marked
   // noreturn, that don't return.  If people would like this warning to be more
   // accurate, such functions should be marked as noreturn.
   return AlwaysFallThrough;
 }
 
-namespace {
+static void CheckFallThroughForBlock(Sema &S, const Stmt *Body,
+                                     const BlockExpr *blkExpr,
+                                     AnalysisDeclContext &AC) {
+  const FunctionType *FT =
+      blkExpr->getType()->getPointeeType()->getAs<FunctionType>();
+  if (FT == nullptr) return;
 
-struct CheckFallThroughDiagnostics {
-  unsigned diag_MaybeFallThrough_HasNoReturn;
-  unsigned diag_MaybeFallThrough_ReturnsNonVoid;
-  unsigned diag_AlwaysFallThrough_HasNoReturn;
-  unsigned diag_AlwaysFallThrough_ReturnsNonVoid;
-  unsigned diag_NeverFallThroughOrReturn;
-  enum { Function, Block, Lambda, Coroutine } funMode;
-  SourceLocation FuncLoc;
-
-  static CheckFallThroughDiagnostics MakeForFunction(const Decl *Func) {
-    CheckFallThroughDiagnostics D;
-    D.FuncLoc = Func->getLocation();
-    D.diag_MaybeFallThrough_HasNoReturn =
-      diag::warn_falloff_noreturn_function;
-    D.diag_MaybeFallThrough_ReturnsNonVoid =
-      diag::warn_maybe_falloff_nonvoid_function;
-    D.diag_AlwaysFallThrough_HasNoReturn =
-      diag::warn_falloff_noreturn_function;
-    D.diag_AlwaysFallThrough_ReturnsNonVoid =
-      diag::warn_falloff_nonvoid_function;
+  bool ReturnsValue = !FT->getReturnType()->isVoidType();
+  bool HasNoReturnAttr = FT->getNoReturnAttr();
 
-    // Don't suggest that virtual functions be marked "noreturn", since they
-    // might be overridden by non-noreturn functions.
-    bool isVirtualMethod = false;
-    if (const CXXMethodDecl *Method = dyn_cast<CXXMethodDecl>(Func))
-      isVirtualMethod = Method->isVirtual();
+  // Short circuit for compilation speed.
+  if (!ReturnsValue && !HasNoReturnAttr) return;
 
-    // Don't suggest that template instantiations be marked "noreturn"
-    bool isTemplateInstantiation = false;
-    if (const FunctionDecl *Function = dyn_cast<FunctionDecl>(Func))
-      isTemplateInstantiation = Function->isTemplateInstantiation();
+  SourceLocation RBrace = Body->getEndLoc();
+  switch (CheckFallThrough(AC)) {
+    case MaybeFallThrough:
+      if (HasNoReturnAttr)
+        S.Diag(RBrace, diag::err_noreturn_block_has_return_expr);
+      else if (ReturnsValue)
+        S.Diag(RBrace, diag::err_maybe_falloff_nonvoid_block);
+      break;
 
-    if (!isVirtualMethod && !isTemplateInstantiation)
-      D.diag_NeverFallThroughOrReturn =
-        diag::warn_suggest_noreturn_function;
-    else
-      D.diag_NeverFallThroughOrReturn = 0;
+    case AlwaysFallThrough:
+      if (HasNoReturnAttr)
+        S.Diag(RBrace, diag::err_noreturn_block_has_return_expr);
+      else if (ReturnsValue)
+        S.Diag(RBrace, diag::err_falloff_nonvoid_block);
+      break;
 
-    D.funMode = Function;
-    return D;
+    case NeverFallThroughOrReturn:
+    case NeverFallThrough:
+    case UnknownFallThrough:
+      break;
   }
+}
+
+static void CheckFallThroughForLambda(Sema &S, const Decl *D, const Stmt *Body,
+                                      AnalysisDeclContext &AC,
+                                      sema::FunctionScopeInfo *FSI) {
+  const auto *FD = dyn_cast<FunctionDecl>(D);
+  if (FD == nullptr) return;
+
+  auto *LSI = cast<sema::LambdaScopeInfo>(FSI);
+  bool ReturnsValue = LSI->WrappedReturnType.isNull()
+                          ? !FD->getReturnType()->isVoidType()
+                          : LSI->CoroutineHasNonVoidReturn;
+  bool HasNoReturnAttr = FD->isNoReturn();
+
+  // Short circuit for compilation speed.
+  if (!ReturnsValue && !HasNoReturnAttr) return;
 
-  static CheckFallThroughDiagnostics MakeForCoroutine(const Decl *Func) {
-    CheckFallThroughDiagnostics D;
-    D.FuncLoc = Func->getLocation();
-    D.diag_MaybeFallThrough_HasNoReturn = 0;
-    D.diag_MaybeFallThrough_ReturnsNonVoid =
-        diag::warn_maybe_falloff_nonvoid_coroutine;
-    D.diag_AlwaysFallThrough_HasNoReturn = 0;
-    D.diag_AlwaysFallThrough_ReturnsNonVoid =
-        diag::warn_falloff_nonvoid_coroutine;
-    D.funMode = Coroutine;
-    return D;
-  }
-
-  static CheckFallThroughDiagnostics MakeForBlock() {
-    CheckFallThroughDiagnostics D;
-    D.diag_MaybeFallThrough_HasNoReturn =
-      diag::err_noreturn_block_has_return_expr;
-    D.diag_MaybeFallThrough_ReturnsNonVoid =
-      diag::err_maybe_falloff_nonvoid_block;
-    D.diag_AlwaysFallThrough_HasNoReturn =
-      diag::err_noreturn_block_has_return_expr;
-    D.diag_AlwaysFallThrough_ReturnsNonVoid =
-      diag::err_falloff_nonvoid_block;
-    D.diag_NeverFallThroughOrReturn = 0;
-    D.funMode = Block;
-    return D;
-  }
-
-  static CheckFallThroughDiagnostics MakeForLambda() {
-    CheckFallThroughDiagnostics D;
-    D.diag_MaybeFallThrough_HasNoReturn =
-      diag::err_noreturn_lambda_has_return_expr;
-    D.diag_MaybeFallThrough_ReturnsNonVoid =
-      diag::warn_maybe_falloff_nonvoid_lambda;
-    D.diag_AlwaysFallThrough_HasNoReturn =
-      diag::err_noreturn_lambda_has_return_expr;
-    D.diag_AlwaysFallThrough_ReturnsNonVoid =
-      diag::warn_falloff_nonvoid_lambda;
-    D.diag_NeverFallThroughOrReturn = 0;
-    D.funMode = Lambda;
-    return D;
-  }
-
-  bool checkDiagnostics(DiagnosticsEngine &D, bool ReturnsVoid,
-                        bool HasNoReturn) const {
-    if (funMode == Function) {
-      return (ReturnsVoid ||
-              D.isIgnored(diag::warn_maybe_falloff_nonvoid_function,
-                          FuncLoc)) &&
-             (!HasNoReturn ||
-              D.isIgnored(diag::warn_noreturn_function_has_return_expr,
-                          FuncLoc)) &&
-             (!ReturnsVoid ||
-              D.isIgnored(diag::warn_suggest_noreturn_block, FuncLoc));
-    }
-    if (funMode == Coroutine) {
-      return (ReturnsVoid ||
-              D.isIgnored(diag::warn_maybe_falloff_nonvoid_function, FuncLoc) ||
-              D.isIgnored(diag::warn_maybe_falloff_nonvoid_coroutine,
-                          FuncLoc)) &&
-             (!HasNoReturn);
-    }
-    // For blocks / lambdas.
-    return ReturnsVoid && !HasNoReturn;
-  }
-};
+  SourceLocation RBrace = Body->getEndLoc();
+  switch (CheckFallThrough(AC)) {
+    case MaybeFallThrough:
+      if (HasNoReturnAttr)
+        S.Diag(RBrace, diag::err_noreturn_lambda_has_return_expr);
+      else if (ReturnsValue)
+        S.Diag(RBrace, diag::warn_maybe_falloff_nonvoid_lambda);
+      break;
+    case AlwaysFallThrough:
+      if (HasNoReturnAttr)
+        S.Diag(RBrace, diag::err_noreturn_lambda_has_return_expr);
+      else if (ReturnsValue)
+        S.Diag(RBrace, diag::warn_falloff_nonvoid_lambda);
+      break;
+    case NeverFallThroughOrReturn:
+    case NeverFallThrough:
+    case UnknownFallThrough:
+      break;
 
-} // anonymous namespace
-
-/// CheckFallThroughForBody - Check that we don't fall off the end of a
-/// function that should return a value.  Check that we don't fall off the end
-/// of a noreturn function.  We assume that functions and blocks not marked
-/// noreturn will return.
-static void CheckFallThroughForBody(Sema &S, const Decl *D, const Stmt *Body,
-                                    const BlockExpr *blkExpr,
-                                    const CheckFallThroughDiagnostics &CD,
+static void CheckFallThroughForCoroutine(Sema &S, const Decl *D,
+                                         const Stmt *Body,
                                          AnalysisDeclContext &AC,
                                          sema::FunctionScopeInfo *FSI) {
+  const auto *FD = dyn_cast<FunctionDecl>(D);
+  if (FD == nullptr) return;
+
+  const auto *CBody = dyn_cast<CoroutineBodyStmt>(Body);
+  bool ReturnsValue = CBody ? (CBody->getFallthroughHandler() == nullptr)
+                            : !FD->getReturnType()->isVoidType();
+  bool HasNoReturnAttr = FD->isNoReturn();
+
+  // Short circuit for compilation speed.
+  //
+  // FIXME this logic does not appear to correspond to the logic on the
+  // slow path.
+  DiagnosticsEngine &Diags = S.getDiagnostics();
+  if ((!ReturnsValue ||
+       Diags.isIgnored(diag::warn_maybe_falloff_nonvoid_function,
+                       D->getLocation()) ||
+       Diags.isIgnored(diag::warn_maybe_falloff_nonvoid_coroutine,
+                       D->getLocation())) &&
+      !HasNoReturnAttr)
+    return;
+
+  SourceLocation RBrace = Body->getEndLoc();
+  switch (CheckFallThrough(AC)) {
+    case MaybeFallThrough:
+      if (ReturnsValue)
+        S.Diag(RBrace, diag::warn_maybe_falloff_nonvoid_coroutine)
+            << FSI->CoroutinePromise->getType();
+      break;
+    case AlwaysFallThrough:
+      if (ReturnsValue)
+        S.Diag(RBrace, diag::warn_falloff_nonvoid_coroutine)
+            << FSI->CoroutinePromise->getType();
+      break;
+    case NeverFallThroughOrReturn:
+    case NeverFallThrough:
+    case UnknownFallThrough:
+      break;
+  }
+}
 
+static void CheckFallThroughForFunction(Sema &S, const Decl *D,
+                                        const Stmt *Body,
+                                        AnalysisDeclContext &AC) {
   bool ReturnsVoid = false;
-  bool HasNoReturn = false;
-  bool IsCoroutine = FSI->isCoroutine();
+  bool HasNoReturnAttr = false;
+  bool SuggestNoReturn = true;
 
   if (const auto *FD = dyn_cast<FunctionDecl>(D)) {
-    if (const auto *CBody = dyn_cast<CoroutineBodyStmt>(Body))
-      ReturnsVoid = CBody->getFallthroughHandler() != nullptr;
-    else
-      ReturnsVoid = FD->getReturnType()->isVoidType();
-    HasNoReturn = FD->isNoReturn();
-  }
-  else if (const auto *MD = dyn_cast<ObjCMethodDecl>(D)) {
-    ReturnsVoid = MD->getReturnType()->isVoidType();
-    HasNoReturn = MD->hasAttr<NoReturnAttr>();
-  }
-  else if (isa<BlockDecl>(D)) {
-    QualType BlockTy = blkExpr->getType();
-    if (const FunctionType *FT =
-          BlockTy->getPointeeType()->getAs<FunctionType>()) {
-      if (FT->getReturnType()->isVoidType())
-        ReturnsVoid = true;
-      if (FT->getNoReturnAttr())
-        HasNoReturn = true;
-    }
-  }
+    HasNoReturnAttr = FD->isNoReturn();
 
-  DiagnosticsEngine &Diags = S.getDiagnostics();
+    // Don't suggest that virtual functions be marked "noreturn", since they
+    // might be overridden by non-noreturn functions.
+    bool isVirtualMethod = false;
+    if (const CXXMethodDecl *Method = dyn_cast<CXXMethodDecl>(FD))
+      isVirtualMethod = Method->isVirtual();
 
-  // Short circuit for compilation speed.
-  if (CD.checkDiagnostics(Diags, ReturnsVoid, HasNoReturn))
-      return;
-  SourceLocation LBrace = Body->getBeginLoc(), RBrace = Body->getEndLoc();
-  auto EmitDiag = [&](SourceLocation Loc, unsigned DiagID) {
-    if (IsCoroutine)
-      S.Diag(Loc, DiagID) << FSI->CoroutinePromise->getType();
-    else
-      S.Diag(Loc, DiagID);
-  };
+    // Don't suggest that template instantiations be marked "noreturn"
+    bool isTemplateInstantiation = false;
+    if (const FunctionDecl *Function = dyn_cast<FunctionDecl>(FD))
+      isTemplateInstantiation = Function->isTemplateInstantiation();
+
+    SuggestNoReturn = !isVirtualMethod && !isTemplateInstantiation;
+  } else if (const auto *MD = dyn_cast<ObjCMethodDecl>(D)) {
+    ReturnsVoid = MD->getReturnType()->isVoidType();
+    HasNoReturnAttr = MD->hasAttr<NoReturnAttr>();
+  }
 
   // cpu_dispatch functions permit empty function bodies for ICC compatibility.
   if (D->getAsFunction() && D->getAsFunction()->isCPUDispatchMultiVersion())
     return;
 
+  // Short circuit for compilation speed.
+  DiagnosticsEngine &Diags = S.getDiagnostics();
+  if ((ReturnsVoid || Diags.isIgnored(diag::warn_maybe_falloff_nonvoid_function,
+                                      D->getLocation())) &&
+      (!HasNoReturnAttr ||
+       Diags.isIgnored(diag::warn_noreturn_function_has_return_expr,
+                       D->getLocation())) &&
+      (!ReturnsVoid || !SuggestNoReturn ||
+       Diags.isIgnored(diag::warn_suggest_noreturn_block, D->getLocation())))
+    return;
+
   // Either in a function body compound statement, or a function-try-block.
+  SourceLocation LBrace = Body->getBeginLoc(), RBrace = Body->getEndLoc();
   switch (CheckFallThrough(AC)) {
-    case UnknownFallThrough:
-      break;
-
     case MaybeFallThrough:
-      if (HasNoReturn)
-        EmitDiag(RBrace, CD.diag_MaybeFallThrough_HasNoReturn);
+      if (HasNoReturnAttr)
+        S.Diag(RBrace, diag::warn_falloff_noreturn_function);
       else if (!ReturnsVoid)
-        EmitDiag(RBrace, CD.diag_MaybeFallThrough_ReturnsNonVoid);
+        S.Diag(RBrace, diag::warn_maybe_falloff_nonvoid_function);
       break;
     case AlwaysFallThrough:
-      if (HasNoReturn)
-        EmitDiag(RBrace, CD.diag_AlwaysFallThrough_HasNoReturn);
+      if (HasNoReturnAttr)
+        S.Diag(RBrace, diag::warn_falloff_noreturn_function);
       else if (!ReturnsVoid)
-        EmitDiag(RBrace, CD.diag_AlwaysFallThrough_ReturnsNonVoid);
+        S.Diag(RBrace, diag::warn_falloff_nonvoid_function);
       break;
     case NeverFallThroughOrReturn:
-      if (ReturnsVoid && !HasNoReturn && CD.diag_NeverFallThroughOrReturn) {
+      if (ReturnsVoid && !HasNoReturnAttr && SuggestNoReturn) {
         if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) {
-          S.Diag(LBrace, CD.diag_NeverFallThroughOrReturn) << 0 << FD;
+          S.Diag(LBrace, diag::warn_suggest_noreturn_function) << 0 << FD;
         } else if (const ObjCMethodDecl *MD = dyn_cast<ObjCMethodDecl>(D)) {
-          S.Diag(LBrace, CD.diag_NeverFallThroughOrReturn) << 1 << MD;
+          S.Diag(LBrace, diag::warn_suggest_noreturn_function) << 1 << MD;
         } else {
-          S.Diag(LBrace, CD.diag_NeverFallThroughOrReturn);
+          S.Diag(LBrace, diag::warn_suggest_noreturn_function);
         }
       }
       break;
     case NeverFallThrough:
+    case UnknownFallThrough:
       break;
   }
 }
 
+/// CheckFallThroughForBody - Check that we don't fall off the end of a
+/// function that should return a value.  Check that we don't fall off the end
+/// of a noreturn function.  We assume that functions and blocks not marked
+/// noreturn will return.
+static void CheckFallThroughForBody(Sema &S, const Decl *D, const Stmt *Body,
+                                    const BlockExpr *blkExpr,
+                                    AnalysisDeclContext &AC,
+                                    sema::FunctionScopeInfo *FSI) {
+  if (isa<BlockDecl>(D)) {
+    CheckFallThroughForBlock(S, Body, blkExpr, AC);
+  } else if (isa<CXXMethodDecl>(D) &&
+             cast<CXXMethodDecl>(D)->getOverloadedOperator() == OO_Call &&
+             cast<CXXMethodDecl>(D)->getParent()->isLambda()) {
+    CheckFallThroughForLambda(S, D, Body, AC, FSI);
+  } else if (FSI->isCoroutine()) {
+    CheckFallThroughForCoroutine(S, D, Body, AC, FSI);
+  } else {
+    CheckFallThroughForFunction(S, D, Body, AC);
+  }
+}
+
 //===----------------------------------------------------------------------===//
 // -Wuninitialized
 //===----------------------------------------------------------------------===//
 
 namespace {
 /// ContainsReference - A visitor class to search for references to
 /// a particular declaration (the needle) within any evaluated component of an
 /// expression (recursively).
if (!fscope->PossiblyUnreachableDiags.empty()) {
     }
 
     if (!analyzed)
       flushDiagnostics(S, fscope);
   }
 
   // Warning: check missing 'return'
   if (P.enableCheckFallThrough) {
-    const CheckFallThroughDiagnostics &CD =
-        (isa<BlockDecl>(D)
-             ? CheckFallThroughDiagnostics::MakeForBlock()
-             : (isa<CXXMethodDecl>(D) &&
-                cast<CXXMethodDecl>(D)->getOverloadedOperator() == OO_Call &&
-                cast<CXXMethodDecl>(D)->getParent()->isLambda())
-                   ? CheckFallThroughDiagnostics::MakeForLambda()
-                   : (fscope->isCoroutine()
-                          ? CheckFallThroughDiagnostics::MakeForCoroutine(D)
-                          : CheckFallThroughDiagnostics::MakeForFunction(D)));
-    CheckFallThroughForBody(S, D, Body, blkExpr, CD, AC, fscope);
+    CheckFallThroughForBody(S, D, Body, blkExpr, AC, fscope);
   }
 
   // Warning: check for unreachable code
   if (P.enableCheckUnreachable) {
     // Only check for unreachable code on non-template instantiations.
     // Different template instantiations can effectively change the control-flow
     // and it is very difficult to prove that a snippet of code in a template
     // is unreachable for all instantiations.