[-Wmissing-noreturn] Detect non-void noreturn function candidates

[steakhal]

Summary

Previously, only void returning functions were considered for noreturn
attribute candidates. This patch removes this artificial restriction.

Notice the zero-initializations of the CheckFallThroughDiagnostics
objects. In some cases, such as inside MakeForCoroutine(), some struct
members were left uninitialized (probably by accident).
This way all the fields will be zero-initialized, preventing accidental
uninitialized reads during diagnostics construction.

[aaron.ballman]

Previously, only void returning functions were considered for noreturn attribute candidates. This patch removes this artificial restriction.

C2x 6.7.12.6p6: The implementation should produce a diagnostic message for a function declared with a noreturn attribute that appears to be capable of returning to its caller.

p7 has an example showing [[noreturn]] int h(void); with the comment "Implementations are similarly encouraged to diagnose the declaration of h() because it appears capable of returning to its caller due to the non-void return type."

So is this really an artificial restriction? To me, putting the noreturn attribute on a function with a return type makes no sense whatsoever. The interface is saying "I promise that calling me will not return" and "when I return, this is the type of the value I will give you."

[steakhal]

In D139195#3966675, @aaron.ballman wrote:

Previously, only void returning functions were considered for noreturn attribute candidates. This patch removes this artificial restriction.

C2x 6.7.12.6p6: The implementation should produce a diagnostic message for a function declared with a noreturn attribute that appears to be capable of returning to its caller.

p7 has an example showing [[noreturn]] int h(void); with the comment "Implementations are similarly encouraged to diagnose the declaration of h() because it appears capable of returning to its caller due to the non-void return type."

So is this really an artificial restriction? To me, putting the noreturn attribute on a function with a return type makes no sense whatsoever. The interface is saying "I promise that calling me will not return" and "when I return, this is the type of the value I will give you."

Hmm, I implicitly took the __attribute__((noreturn)) suggestion and the non-void return type as a code-smell for the exact same reason.
You are right, we should have a clean diagnostic message for these cases instead of this implied suggestion.
IDK why I haven't thought about this 😅

clang/lib/Sema/AnalysisBasedWarnings.cpp

static ControlFlowKind CheckFallThrough(AnalysisDeclContext &AC) {
   // 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 {
 
 struct CheckFallThroughDiagnostics {
-  unsigned diag_MaybeFallThrough_HasNoReturn;
-  unsigned diag_MaybeFallThrough_ReturnsNonVoid;
-  unsigned diag_AlwaysFallThrough_HasNoReturn;
-  unsigned diag_AlwaysFallThrough_ReturnsNonVoid;
-  unsigned diag_NeverFallThroughOrReturn;
+  unsigned diag_MaybeFallThrough_HasNoReturn = 0;
+  unsigned diag_MaybeFallThrough_ReturnsNonVoid = 0;
+  unsigned diag_AlwaysFallThrough_HasNoReturn = 0;
+  unsigned diag_AlwaysFallThrough_ReturnsNonVoid = 0;
+  unsigned diag_NeverFallThroughOrReturn = 0;
   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;
static CheckFallThroughDiagnostics MakeForFunction(const Decl *Func) {
     // Don't suggest that template instantiations be marked "noreturn"
     bool isTemplateInstantiation = false;
     if (const FunctionDecl *Function = dyn_cast<FunctionDecl>(Func))
       isTemplateInstantiation = Function->isTemplateInstantiation();
 
     if (!isVirtualMethod && !isTemplateInstantiation)
       D.diag_NeverFallThroughOrReturn =
         diag::warn_suggest_noreturn_function;
-    else
-      D.diag_NeverFallThroughOrReturn = 0;
 
     D.funMode = Function;
     return D;
   }
 
   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 ||
case MaybeFallThrough:
       break;
     case AlwaysFallThrough:
       if (HasNoReturn)
         EmitDiag(RBrace, CD.diag_AlwaysFallThrough_HasNoReturn);
       else if (!ReturnsVoid)
         EmitDiag(RBrace, CD.diag_AlwaysFallThrough_ReturnsNonVoid);
       break;
     case NeverFallThroughOrReturn:
-      if (ReturnsVoid && !HasNoReturn && CD.diag_NeverFallThroughOrReturn) {
+      if (!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 {
           S.Diag(LBrace, CD.diag_NeverFallThroughOrReturn);
         }
       }

clang/test/SemaCXX/return-noreturn.cpp

 };
 
 struct other { ~other() {} };
 
 // Ensure that destructors from objects are properly modeled in the CFG despite
 // the presence of switches, case statements, labels, and blocks. These tests
 // try to cover bugs reported in both PR6884 and PR10063.
 namespace abort_struct_complex_cfgs {
-  int basic(int x) {
+  __attribute__((noreturn)) int basic(int x) {
     switch (x) { default: pr6884_abort(); }
   }
-  int f1(int x) {
+  __attribute__((noreturn)) int f1(int x) {
     switch (x) default: pr6884_abort_struct();
   }
-  int f2(int x) {
+  __attribute__((noreturn)) int f2(int x) {
     switch (x) { default: pr6884_abort_struct(); }
   }
   int f2_positive(int x) {
     switch (x) { default: ; }
   } // expected-warning {{non-void function does not return a value}}
-  int f3(int x) {
+  __attribute__((noreturn)) int f3(int x) {
     switch (x) { default: { pr6884_abort_struct(); } }
   }
-  int f4(int x) {
+  __attribute__((noreturn)) int f4(int x) {
     switch (x) default: L1: L2: case 4: pr6884_abort_struct();
   }
-  int f5(int x) {
+  __attribute__((noreturn)) int f5(int x) {
     switch (x) default: L1: { L2: case 4: pr6884_abort_struct(); }
   }
-  int f6(int x) {
+  __attribute__((noreturn)) int f6(int x) {
     switch (x) default: L1: L2: case 4: { pr6884_abort_struct(); }
   }
 
   // FIXME: detect noreturn destructors triggered by calls to delete.
   int f7(int x) {
     switch (x) default: L1: L2: case 4: {
       pr6884_abort_struct *p = new pr6884_abort_struct();
       delete p;
     }
   } // expected-warning {{non-void function does not return a value}}
 
   // Test that these constructs work even when extraneous blocks are created
   // before and after the switch due to implicit destructors.
-  int g1(int x) {
+  __attribute__((noreturn)) int g1(int x) {
     other o;
     switch (x) default: pr6884_abort_struct();
   }
-  int g2(int x) {
+  __attribute__((noreturn)) int g2(int x) {
     other o;
     switch (x) { default: pr6884_abort_struct(); }
   }
   int g2_positive(int x) {
     other o;
     switch (x) { default: ; }
   } // expected-warning {{non-void function does not return a value}}
-  int g3(int x) {
+  __attribute__((noreturn)) int g3(int x) {
     other o;
     switch (x) { default: { pr6884_abort_struct(); } }
   }
-  int g4(int x) {
+  __attribute__((noreturn)) int g4(int x) {
     other o;
     switch (x) default: L1: L2: case 4: pr6884_abort_struct();
   }
-  int g5(int x) {
+  __attribute__((noreturn)) int g5(int x) {
     other o;
     switch (x) default: L1: { L2: case 4: pr6884_abort_struct(); }
   }
-  int g6(int x) {
+  __attribute__((noreturn)) int g6(int x) {
     other o;
     switch (x) default: L1: L2: case 4: { pr6884_abort_struct(); }
   }
 
   // Test that these constructs work even with variables carrying the no-return
   // destructor instead of temporaries.
-  int h1(int x) {
+  __attribute__((noreturn)) int h1(int x) {
     other o;
     switch (x) default: pr6884_abort_struct a;
   }
-  int h2(int x) {
+  __attribute__((noreturn)) int h2(int x) {
     other o;
     switch (x) { default: pr6884_abort_struct a; }
   }
-  int h3(int x) {
+  __attribute__((noreturn)) int h3(int x) {
     other o;
     switch (x) { default: { pr6884_abort_struct a; } }
   }
-  int h4(int x) {
+  __attribute__((noreturn)) int h4(int x) {
     other o;
     switch (x) default: L1: L2: case 4: pr6884_abort_struct a;
   }
-  int h5(int x) {
+  __attribute__((noreturn)) int h5(int x) {
     other o;
     switch (x) default: L1: { L2: case 4: pr6884_abort_struct a; }
   }
-  int h6(int x) {
+  __attribute__((noreturn)) int h6(int x) {
     other o;
     switch (x) default: L1: L2: case 4: { pr6884_abort_struct a; }
   }
 }
 
 // PR9380
 struct PR9380 {
   ~PR9380();
struct NoReturn {
   ~NoReturn() __attribute__((noreturn));
   operator bool() const;
 };
 struct Return {
   ~Return();
   operator bool() const;
 };
 
-int testTernaryUnconditionalNoreturn() {
+__attribute__((noreturn)) int testTernaryUnconditionalNoreturn() {
   true ? NoReturn() : NoReturn();
 }
 
-int testTernaryStaticallyConditionalNoretrunOnTrue() {
+__attribute__((noreturn)) int testTernaryStaticallyConditionalNoretrunOnTrue() {
   true ? NoReturn() : Return();
 }
 
 int testTernaryStaticallyConditionalRetrunOnTrue() {
   true ? Return() : NoReturn();
 } // expected-warning {{non-void function does not return a value}}
 
-int testTernaryStaticallyConditionalNoretrunOnFalse() {
+__attribute__((noreturn)) int testTernaryStaticallyConditionalNoretrunOnFalse() {
   false ? Return() : NoReturn();
 }
 
 int testTernaryStaticallyConditionalRetrunOnFalse() {
   false ? NoReturn() : Return();
 } // expected-warning {{non-void function does not return a value}}
 
 int testTernaryConditionalNoreturnTrueBranch(bool value) {
   value ? (NoReturn() || NoReturn()) : Return();
 } // expected-warning {{non-void function does not return a value in all control paths}}
 
 int testTernaryConditionalNoreturnFalseBranch(bool value) {
   value ? Return() : (NoReturn() || NoReturn());
 } // expected-warning {{non-void function does not return a value in all control paths}}
 
 int testConditionallyExecutedComplexTernaryTrueBranch(bool value) {
   value || (true ? NoReturn() : true);
 } // expected-warning {{non-void function does not return a value in all control paths}}
 
 int testConditionallyExecutedComplexTernaryFalseBranch(bool value) {
   value || (false ? true : NoReturn());
 } // expected-warning {{non-void function does not return a value in all control paths}}
 
-int testStaticallyExecutedLogicalOrBranch() {
+__attribute__((noreturn)) int testStaticallyExecutedLogicalOrBranch() {
   false || NoReturn();
 }
 
-int testStaticallyExecutedLogicalAndBranch() {
+__attribute__((noreturn)) int testStaticallyExecutedLogicalAndBranch() {
   true && NoReturn();
 }
 
 int testStaticallySkippedLogicalOrBranch() {
   true || NoReturn();
 } // expected-warning {{non-void function does not return a value}}
 
 int testStaticallySkppedLogicalAndBranch() {
   false && NoReturn();
 } // expected-warning {{non-void function does not return a value}}
 
 int testConditionallyExecutedComplexLogicalBranch(bool value) {
   value || (true && NoReturn());
 } // expected-warning {{non-void function does not return a value in all control paths}}
 
 int testConditionallyExecutedComplexLogicalBranch2(bool value) {
   (true && value) || (true && NoReturn());
 } // expected-warning {{non-void function does not return a value in all control paths}}
 
-int testConditionallyExecutedComplexLogicalBranch3(bool value) {
+__attribute__((noreturn)) int testConditionallyExecutedComplexLogicalBranch3(bool value) {
   (false && (Return() || true)) || (true && NoReturn());
 }
 
-int testConditionallyExecutedComplexLogicalBranch4(bool value) {
+__attribute__((noreturn)) int testConditionallyExecutedComplexLogicalBranch4(bool value) {
   false || ((Return() || true) && (true && NoReturn()));
 }
 
 #if __cplusplus >= 201103L
 namespace LambdaVsTemporaryDtor {
   struct Y { ~Y(); };
   struct X { template<typename T> X(T, Y = Y()) {} };
 
   struct Fatal { ~Fatal() __attribute__((noreturn)); };
   struct FatalCopy { FatalCopy(); FatalCopy(const FatalCopy&, Fatal F = Fatal()); };
 
   void foo();
 
   int bar() {
     X work([](){ Fatal(); });
     foo();
   } // expected-warning {{non-void function does not return a value}}
 
-  int baz() {
+  __attribute__((noreturn)) int baz() {
     FatalCopy fc;
     X work([fc](){});
     foo();
   } // ok, initialization of lambda does not return
 }
 #endif
 
 // Ensure that function-try-blocks also check for return values properly.

clang/test/SemaObjC/return.m

 // RUN: %clang_cc1 -fsyntax-only -verify -Wmissing-noreturn -fobjc-exceptions -Wno-objc-root-class %s
 
-int test1(void) {
+__attribute__((noreturn)) int test1(void) {
   id a;
   @throw a;
 }
 
 // PR5286
 void test2(int a) {
   while (1) {
     if (a)