[coroutines] Fix fallthrough diagnostics for coroutines

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.

Reviewers: GorNishanov, rsmith, aaron.ballman, majnemer

Reviewed By: GorNishanov

Subscribers: cfe-commits

Differential Revision: https://reviews.llvm.org/D33532

llvm-svn: 303831

8 files changed, 135 insertions, 25 deletions

diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 2013036d579..60039fac6eb 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -537,10 +537,10 @@ def err_maybe_falloff_nonvoid_block : Error<
 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'">,
diff --git a/clang/include/clang/Sema/ScopeInfo.h b/clang/include/clang/Sema/ScopeInfo.h
index 4487c7c2ccb..4251fa649a8 100644
--- a/clang/include/clang/Sema/ScopeInfo.h
+++ b/clang/include/clang/Sema/ScopeInfo.h
@@ -388,6 +388,8 @@ public:
           (HasBranchProtectedScope && HasBranchIntoScope));
   }
 
+  bool isCoroutine() const { return !FirstCoroutineStmtLoc.isInvalid(); }
+
   void setFirstCoroutineStmt(SourceLocation Loc, StringRef Keyword) {
     assert(FirstCoroutineStmtLoc.isInvalid() &&
                    "first coroutine statement location already set");
diff --git a/clang/lib/Analysis/AnalysisDeclContext.cpp b/clang/lib/Analysis/AnalysisDeclContext.cpp
index 6b58916162f..7c0f5543da0 100644
--- a/clang/lib/Analysis/AnalysisDeclContext.cpp
+++ b/clang/lib/Analysis/AnalysisDeclContext.cpp
@@ -92,6 +92,8 @@ 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);
diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp
index 895d00b3ebf..db688b12cbc 100644
--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -334,10 +334,6 @@ static ControlFlowKind CheckFallThrough(AnalysisDeclContext &AC) {
   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;
@@ -379,7 +375,7 @@ static ControlFlowKind CheckFallThrough(AnalysisDeclContext &AC) {
 
     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;
     }
@@ -546,6 +542,7 @@ static void CheckFallThroughForBody(Sema &S, const Decl *D, const Stmt *Body,
 
   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))
@@ -574,8 +571,13 @@ static void CheckFallThroughForBody(Sema &S, const Decl *D, const Stmt *Body,
   // 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:
@@ -583,15 +585,15 @@ static void CheckFallThroughForBody(Sema &S, const Decl *D, const Stmt *Body,
 
     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) {
@@ -2031,12 +2033,6 @@ AnalysisBasedWarnings::IssueWarnings(sema::AnalysisBasedWarnings::Policy P,
   
   // 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()
@@ -2044,7 +2040,7 @@ AnalysisBasedWarnings::IssueWarnings(sema::AnalysisBasedWarnings::Policy P,
                 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);
diff --git a/clang/lib/Sema/SemaCoroutine.cpp b/clang/lib/Sema/SemaCoroutine.cpp
index 8f7011c985e..45b3a48d158 100644
--- a/clang/lib/Sema/SemaCoroutine.cpp
+++ b/clang/lib/Sema/SemaCoroutine.cpp
@@ -719,13 +719,16 @@ static FunctionDecl *findDeleteForPromise(Sema &S, SourceLocation Loc,
 
 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.
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 27c23e4f11a..fd33001f921 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -12179,7 +12179,7 @@ Decl *Sema::ActOnFinishFunctionBody(Decl *dcl, Stmt *Body,
   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) {
diff --git a/clang/test/SemaCXX/coreturn.cpp b/clang/test/SemaCXX/coreturn.cpp
index bdad227ea44..8bdf03ec67b 100644
--- a/clang/test/SemaCXX/coreturn.cpp
+++ b/clang/test/SemaCXX/coreturn.cpp
@@ -18,6 +18,43 @@ struct promise_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();
@@ -34,8 +71,11 @@ struct promise_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; };
@@ -48,10 +88,66 @@ 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}}
diff --git a/clang/test/SemaCXX/coroutines.cpp b/clang/test/SemaCXX/coroutines.cpp
index a39ad1b4016..4588586d004 100644
--- a/clang/test/SemaCXX/coroutines.cpp
+++ b/clang/test/SemaCXX/coroutines.cpp
@@ -818,3 +818,14 @@ extern "C" int f(mismatch_gro_type_tag4) {
   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;
+}