[coroutines] Fix fallthrough diagnostics for coroutines

[EricWF]

Summary

This patch fixes a number of issues with the analysis warnings emitted when a coroutine may reach the end of the function w/o returning.

• Fix bug where coroutines with return_value are incorrectly diagnosed as missing co_return's.
• Rework diagnostic message to no longer say "non-void coroutine", because that implies the coroutine doesn't have a void return type, which it might. In this case a non-void coroutine is one who's promise type does not contain return_void()

As a side-effect of this patch, coroutine bodies that contain an invalid coroutine promise objects are marked as invalid.

[GorNishanov]

lib/Sema/AnalysisBasedWarnings.cpp

378

Is this check no longer needed because of the changes to AnalysisDeclContext.cpp?

At some point it was needed because we were emitting "return gro" to produce the immediate return value from the coroutine, but, we wanted to flag the case when the user forgot to add "co_return"

[EricWF]

lib/Sema/AnalysisBasedWarnings.cpp

378

I couldn't exactly figure out why this was no longer needed, but it appears we don't traverse those implicitly created return statements.

I made sure to test this particular part of the change to ensure it didn't regress behavior. If you look at the changes for coreturn.cpp you should see that case exercised.

[GorNishanov]

LGTM

include/clang/Basic/DiagnosticSemaKinds.td

 def warn_falloff_nonvoid_function : Warning<
   "control reaches end of non-void function">,
   InGroup<ReturnType>;
 def err_maybe_falloff_nonvoid_block : Error<
   "control may reach end of non-void block">;
 def err_falloff_nonvoid_block : Error<
   "control reaches end of non-void block">;
 def warn_maybe_falloff_nonvoid_coroutine : Warning<
-  "control may reach end of non-void coroutine">,
+  "control may reach end of coroutine; which is undefined behavior because the promise type %0 does not declare 'return_void()'">,
   InGroup<ReturnType>;
 def warn_falloff_nonvoid_coroutine : Warning<
-  "control reaches end of non-void coroutine">,
+  "control reaches end of coroutine; which is undefined behavior because the promise type %0 does not declare 'return_void()'">,
   InGroup<ReturnType>;
 def warn_suggest_noreturn_function : Warning<
   "%select{function|method}0 %1 could be declared with attribute 'noreturn'">,
   InGroup<MissingNoreturn>, DefaultIgnore;
 def warn_suggest_noreturn_block : Warning<
   "block could be declared with attribute 'noreturn'">,
   InGroup<MissingNoreturn>, DefaultIgnore;
 

include/clang/Sema/ScopeInfo.h

public:
   }
 
   bool NeedsScopeChecking() const {
     return !HasDroppedStmt &&
         (HasIndirectGoto ||
           (HasBranchProtectedScope && HasBranchIntoScope));
   }
 
+  bool isCoroutine() const { return !FirstCoroutineStmtLoc.isInvalid(); }
+
   void setFirstCoroutineStmt(SourceLocation Loc, StringRef Keyword) {
     assert(FirstCoroutineStmtLoc.isInvalid() &&
                    "first coroutine statement location already set");
     FirstCoroutineStmtLoc = Loc;
     FirstCoroutineStmtKind = llvm::StringSwitch<unsigned char>(Keyword)
             .Case("co_return", 0)
             .Case("co_await", 1)
             .Case("co_yield", 2);

lib/Analysis/AnalysisDeclContext.cpp

static BodyFarm &getBodyFarm(ASTContext &C, CodeInjector *injector = nullptr) {
   static BodyFarm *BF = new BodyFarm(C, injector);
   return *BF;
 }
 
 Stmt *AnalysisDeclContext::getBody(bool &IsAutosynthesized) const {
   IsAutosynthesized = false;
   if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) {
     Stmt *Body = FD->getBody();
+    if (auto *CoroBody = dyn_cast_or_null<CoroutineBodyStmt>(Body))
+      Body = CoroBody->getBody();
     if (Manager && Manager->synthesizeBodies()) {
       Stmt *SynthesizedBody =
           getBodyFarm(getASTContext(), Manager->Injector.get()).getBody(FD);
       if (SynthesizedBody) {
         Body = SynthesizedBody;
         IsAutosynthesized = true;
       }
     }

lib/Sema/AnalysisBasedWarnings.cpp

static ControlFlowKind CheckFallThrough(AnalysisDeclContext &AC) {
   // Now we know what is live, we check the live precessors of the exit block
   // and look for fall through paths, being careful to ignore normal returns,
   // and exceptional paths.
   bool HasLiveReturn = false;
   bool HasFakeEdge = false;
   bool HasPlainEdge = false;
   bool HasAbnormalEdge = false;
 
-  // In a coroutine, only co_return statements count as normal returns. Remember
-  // if we are processing a coroutine or not.
-  const bool IsCoroutine = isa<CoroutineBodyStmt>(AC.getBody());
-
   // Ignore default cases that aren't likely to be reachable because all
   // enums in a switch(X) have explicit case statements.
   CFGBlock::FilterOptions FO;
   FO.IgnoreDefaultsWithCoveredEnums = 1;
 
   for (CFGBlock::filtered_pred_iterator
    I = cfg->getExit().filtered_pred_start_end(FO); I.hasMore(); ++I) {
     const CFGBlock& B = **I;
if (ri == re) {
       }
       // A labeled empty statement, or the entry block...
       HasPlainEdge = true;
       continue;
     }
 
     CFGStmt CS = ri->castAs<CFGStmt>();
     const Stmt *S = CS.getStmt();
-    if ((isa<ReturnStmt>(S) && !IsCoroutine) || isa<CoreturnStmt>(S)) {
+    if (isa<ReturnStmt>(S) || isa<CoreturnStmt>(S)) {
       HasLiveReturn = true;
       continue;
     }
     if (isa<ObjCAtThrowStmt>(S)) {
       HasFakeEdge = true;
       continue;
     }
     if (isa<CXXThrowExpr>(S)) {

 /// noreturn will return.
 static void CheckFallThroughForBody(Sema &S, const Decl *D, const Stmt *Body,
                                     const BlockExpr *blkExpr,
                                     const CheckFallThroughDiagnostics& CD,
                                     AnalysisDeclContext &AC) {
 
   bool ReturnsVoid = false;
   bool HasNoReturn = false;
+  bool IsCoroutine = S.getCurFunction() && S.getCurFunction()->isCoroutine();
 
   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 (isa<BlockDecl>(D)) {
     }
   }
 
   DiagnosticsEngine &Diags = S.getDiagnostics();
 
   // Short circuit for compilation speed.
   if (CD.checkDiagnostics(Diags, ReturnsVoid, HasNoReturn))
       return;
-
   SourceLocation LBrace = Body->getLocStart(), RBrace = Body->getLocEnd();
+  auto EmitDiag = [&](SourceLocation Loc, unsigned DiagID) {
+    if (IsCoroutine)
+      S.Diag(Loc, DiagID) << S.getCurFunction()->CoroutinePromise->getType();
+    else
+      S.Diag(Loc, DiagID);
+  };
   // Either in a function body compound statement, or a function-try-block.
   switch (CheckFallThrough(AC)) {
     case UnknownFallThrough:
       break;
 
     case MaybeFallThrough:
       if (HasNoReturn)
-        S.Diag(RBrace, CD.diag_MaybeFallThrough_HasNoReturn);
+        EmitDiag(RBrace, CD.diag_MaybeFallThrough_HasNoReturn);
       else if (!ReturnsVoid)
-        S.Diag(RBrace, CD.diag_MaybeFallThrough_ReturnsNonVoid);
+        EmitDiag(RBrace, CD.diag_MaybeFallThrough_ReturnsNonVoid);
       break;
     case AlwaysFallThrough:
       if (HasNoReturn)
-        S.Diag(RBrace, CD.diag_AlwaysFallThrough_HasNoReturn);
+        EmitDiag(RBrace, CD.diag_AlwaysFallThrough_HasNoReturn);
       else if (!ReturnsVoid)
-        S.Diag(RBrace, CD.diag_AlwaysFallThrough_ReturnsNonVoid);
+        EmitDiag(RBrace, CD.diag_AlwaysFallThrough_ReturnsNonVoid);
       break;
     case NeverFallThroughOrReturn:
       if (ReturnsVoid && !HasNoReturn && CD.diag_NeverFallThroughOrReturn) {
         if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) {
           S.Diag(LBrace, CD.diag_NeverFallThroughOrReturn) << 0 << FD;
         } else if (const ObjCMethodDecl *MD = dyn_cast<ObjCMethodDecl>(D)) {
           S.Diag(LBrace, CD.diag_NeverFallThroughOrReturn) << 1 << MD;
         } else {
if (!fscope->PossiblyUnreachableDiags.empty()) {
     }
 
     if (!analyzed)
       flushDiagnostics(S, fscope);
   }
   
   // Warning: check missing 'return'
   if (P.enableCheckFallThrough) {
-    auto IsCoro = [&]() {
-      if (auto *FD = dyn_cast<FunctionDecl>(D))
-        if (FD->getBody() && isa<CoroutineBodyStmt>(FD->getBody()))
-          return true;
-      return false;
-    };
     const CheckFallThroughDiagnostics &CD =
         (isa<BlockDecl>(D)
              ? CheckFallThroughDiagnostics::MakeForBlock()
              : (isa<CXXMethodDecl>(D) &&
                 cast<CXXMethodDecl>(D)->getOverloadedOperator() == OO_Call &&
                 cast<CXXMethodDecl>(D)->getParent()->isLambda())
                    ? CheckFallThroughDiagnostics::MakeForLambda()
-                   : (IsCoro()
+                   : (fscope->isCoroutine()
                           ? CheckFallThroughDiagnostics::MakeForCoroutine(D)
                           : CheckFallThroughDiagnostics::MakeForFunction(D)));
     CheckFallThroughForBody(S, D, Body, blkExpr, CD, AC);
   }
 
   // Warning: check for unreachable code
   if (P.enableCheckUnreachable) {
     // Only check for unreachable code on non-template instantiations.

lib/Sema/SemaCoroutine.cpp

static FunctionDecl *findDeleteForPromise(Sema &S, SourceLocation Loc,
   }
   S.MarkFunctionReferenced(Loc, OperatorDelete);
   return OperatorDelete;
 }
 
 
 void Sema::CheckCompletedCoroutineBody(FunctionDecl *FD, Stmt *&Body) {
   FunctionScopeInfo *Fn = getCurFunction();
-  assert(Fn && Fn->CoroutinePromise && "not a coroutine");
-
+  assert(Fn && Fn->isCoroutine() && "not a coroutine");
   if (!Body) {
     assert(FD->isInvalidDecl() &&
            "a null body is only allowed for invalid declarations");
     return;
   }
+  // We have a function that uses coroutine keywords, but we failed to build
+  // the promise type.
+  if (!Fn->CoroutinePromise)
+    return FD->setInvalidDecl();
 
   if (isa<CoroutineBodyStmt>(Body)) {
     // Nothing todo. the body is already a transformed coroutine body statement.
     return;
   }
 
   // Coroutines [stmt.return]p1:
   //   A return statement shall not appear in a coroutine.

lib/Sema/SemaDecl.cpp

 
 Decl *Sema::ActOnFinishFunctionBody(Decl *dcl, Stmt *Body,
                                     bool IsInstantiation) {
   FunctionDecl *FD = dcl ? dcl->getAsFunction() : nullptr;
 
   sema::AnalysisBasedWarnings::Policy WP = AnalysisWarnings.getDefaultPolicy();
   sema::AnalysisBasedWarnings::Policy *ActivePolicy = nullptr;
 
-  if (getLangOpts().CoroutinesTS && getCurFunction()->CoroutinePromise)
+  if (getLangOpts().CoroutinesTS && getCurFunction()->isCoroutine())
     CheckCompletedCoroutineBody(FD, Body);
 
   if (FD) {
     FD->setBody(Body);
 
     if (getLangOpts().CPlusPlus14) {
       if (!FD->isInvalidDecl() && Body && !FD->isDependentContext() &&
           FD->getReturnType()->isUndeducedType()) {

test/SemaCXX/coreturn.cpp

 struct promise_void {
   void get_return_object();
   suspend_always initial_suspend();
   suspend_always final_suspend();
   void return_void();
   void unhandled_exception();
 };
 
+struct promise_void_return_value {
+  void get_return_object();
+  suspend_always initial_suspend();
+  suspend_always final_suspend();
+  void unhandled_exception();
+  void return_value(int);
+};
+
+struct VoidTagNoReturn {
+  struct promise_type {
+    VoidTagNoReturn get_return_object();
+    suspend_always initial_suspend();
+    suspend_always final_suspend();
+    void unhandled_exception();
+  };
+};
+
+struct VoidTagReturnValue {
+  struct promise_type {
+    VoidTagReturnValue get_return_object();
+    suspend_always initial_suspend();
+    suspend_always final_suspend();
+    void unhandled_exception();
+    void return_value(int);
+  };
+};
+
+struct VoidTagReturnVoid {
+  struct promise_type {
+    VoidTagReturnVoid get_return_object();
+    suspend_always initial_suspend();
+    suspend_always final_suspend();
+    void unhandled_exception();
+    void return_void();
+  };
+};
+
 struct promise_float {
   float get_return_object();
   suspend_always initial_suspend();
   suspend_always final_suspend();
   void return_void();
   void unhandled_exception();
 };
 
 struct promise_int {
   int get_return_object();
   suspend_always initial_suspend();
   suspend_always final_suspend();
   void return_value(int);
   void unhandled_exception();
 };
 
-template <typename... T>
-struct std::experimental::coroutine_traits<void, T...> { using promise_type = promise_void; };
+template <>
+struct std::experimental::coroutine_traits<void> { using promise_type = promise_void; };
+
+template <typename T1>
+struct std::experimental::coroutine_traits<void, T1> { using promise_type = promise_void_return_value; };
 
 template <typename... T>
 struct std::experimental::coroutine_traits<float, T...> { using promise_type = promise_float; };
 
 template <typename... T>
 struct std::experimental::coroutine_traits<int, T...> { using promise_type = promise_int; };
 
 void test0() { co_await a; }
 float test1() { co_await a; }
 
 int test2() {
   co_await a;
-} // expected-warning {{control reaches end of non-void coroutine}}
+} // expected-warning {{control reaches end of coroutine; which is undefined behavior because the promise type 'std::experimental::coroutine_traits<int>::promise_type' (aka 'promise_int') does not declare 'return_void()'}}
+
+int test2a(bool b) {
+  if (b)
+    co_return 42;
+} // expected-warning {{control may reach end of coroutine; which is undefined behavior because the promise type 'std::experimental::coroutine_traits<int, bool>::promise_type' (aka 'promise_int') does not declare 'return_void()'}}
 
 int test3() {
   co_await a;
 b:
   goto b;
 }
+
+int test4() {
+  co_return 42;
+}
+
+void test5(int) {
+  co_await a;
+} // expected-warning {{control reaches end of coroutine; which is undefined behavior because}}
+
+void test6(int x) {
+  if (x)
+    co_return 42;
+} // expected-warning {{control may reach end of coroutine; which is undefined behavior because}}
+
+void test7(int y) {
+  if (y)
+    co_return 42;
+  else
+    co_return 101;
+}
+
+VoidTagReturnVoid test8() {
+  co_await a;
+}
+
+VoidTagReturnVoid test9(bool b) {
+  if (b)
+    co_return;
+}
+
+VoidTagReturnValue test10() {
+  co_await a;
+} // expected-warning {{control reaches end of coroutine}}
+
+VoidTagReturnValue test11(bool b) {
+  if (b)
+    co_return 42;
+} // expected-warning {{control may reach end of coroutine}}
+
+// FIXME: Promise types that declare neither return_value nor return_void
+// should be ill-formed. This test should be removed when that is fixed.
+VoidTagNoReturn test12() {
+  co_await a;
+} // expected-warning {{control reaches end of coroutine}}
+
+// FIXME: Promise types that declare neither return_value nor return_void
+// should be ill-formed. This test should be removed when that is fixed.
+VoidTagNoReturn test13(bool b) {
+  if (b)
+    co_await a;
+} // expected-warning {{control reaches end of coroutine}}

test/SemaCXX/coroutines.cpp

struct std::experimental::coroutine_traits<int, mismatch_gro_type_tag4> {
   };
 };
 
 extern "C" int f(mismatch_gro_type_tag4) { 
   // expected-error@-1 {{cannot initialize return object of type 'int' with an rvalue of type 'char *'}}
   co_return; //expected-note {{function is a coroutine due to use of 'co_return' here}}
 }
 
+struct bad_promise_no_return_func {
+  coro<bad_promise_no_return_func> get_return_object();
+  suspend_always initial_suspend();
+  suspend_always final_suspend();
+  void unhandled_exception();
+};
+// FIXME: Make this ill-formed because the promise type declares
+// neither return_value(...) or return_void().
+coro<bad_promise_no_return_func> no_return_value_or_return_void() {
+  co_await a;
+}