[Clang][Analysis] Don't treat the default switch path as unreachable when all enumerators are covered

clang/docs/ReleaseNotes.rst

@@ -81,6 +81,26 @@ code bases.
   ``-fno-strict-overflow`` to opt-in to a language dialect where signed integer
   and pointer overflow are well-defined.
 
+- ``-Wreturn-type`` now diagnoses missing returns when the default branch of a
+  switch statement's default branch misses a return, even if the switch covers
+  all enum values.
+
+  This was previously ok and is now warned about:
+
+  .. code-block:: c++
+
+    enum E { a, b };
+
+    int foo(E e) {
+        switch(e) {
+        case a:
+            return 20;
+        case b:
+            return 30;
+        }
+        // warning: non-void function does not return a value in all control paths
+    }
+
 C/C++ Language Potentially Breaking Changes
 -------------------------------------------
 

clang/lib/Analysis/CFG.cpp

@@ -4408,17 +4408,9 @@ CFGBlock *CFGBuilder::VisitSwitchStmt(SwitchStmt *Terminator) {
   }
 
   // If we have no "default:" case, the default transition is to the code
-  // following the switch body.  Moreover, take into account if all the
-  // cases of a switch are covered (e.g., switching on an enum value).
-  //
-  // Note: We add a successor to a switch that is considered covered yet has no
-  //       case statements if the enumeration has no enumerators.
-  bool SwitchAlwaysHasSuccessor = false;
-  SwitchAlwaysHasSuccessor |= switchExclusivelyCovered;
-  SwitchAlwaysHasSuccessor |= Terminator->isAllEnumCasesCovered() &&
-                              Terminator->getSwitchCaseList();
+  // following the switch body.
   addSuccessor(SwitchTerminatedBlock, DefaultCaseBlock,
-               !SwitchAlwaysHasSuccessor);
+               !switchExclusivelyCovered);
 
   // Add the terminator and condition in the switch block.
   SwitchTerminatedBlock->setTerminator(Terminator);

clang/lib/Sema/AnalysisBasedWarnings.cpp

@@ -456,10 +456,7 @@ static ControlFlowKind CheckFallThrough(AnalysisDeclContext &AC) {
   bool HasPlainEdge = false;
   bool HasAbnormalEdge = false;
 
-  // 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);

clang/test/Analysis/cfg.cpp

@@ -178,7 +178,7 @@ namespace NoReturnSingleSuccessor {
 // CHECK-NEXT:    1: x
 // CHECK-NEXT:    2: [B1.1] (ImplicitCastExpr, LValueToRValue, int)
 // CHECK-NEXT:    3: return [B1.2];
-// CHECK-NEXT:    Preds (5): B3 B4 B5 B6 B2(Unreachable)
+// CHECK-NEXT:    Preds (5): B3 B4 B5 B6 B2
 // CHECK-NEXT:    Succs (1): B0
 // CHECK:  [B2]
 // CHECK-NEXT:    1: 0
@@ -188,7 +188,7 @@ namespace NoReturnSingleSuccessor {
 // CHECK-NEXT:    5: [B2.4] (ImplicitCastExpr, IntegralCast, int)
 // CHECK-NEXT:    T: switch [B2.5]
 // CHECK-NEXT:    Preds (1): B7
-// CHECK-NEXT:    Succs (5): B3 B4 B5 B6 B1(Unreachable)
+// CHECK-NEXT:    Succs (5): B3 B4 B5 B6 B1
 // CHECK:  [B3]
 // CHECK-NEXT:   case D:
 // CHECK-NEXT:    1: 4
@@ -254,14 +254,14 @@ int test_enum_with_extension(enum MyEnum value) {
 // CHECK-NEXT:    5: [B2.4] (ImplicitCastExpr, IntegralCast, int)
 // CHECK-NEXT:    T: switch [B2.5]
 // CHECK-NEXT:    Preds (1): B7
-// CHECK-NEXT:    Succs (4): B4 B5 B6 B3(Unreachable)
+// CHECK-NEXT:    Succs (4): B4 B5 B6 B3
 // CHECK:  [B3]
 // CHECK-NEXT:   default:
 // CHECK-NEXT:    1: 4
 // CHECK-NEXT:    2: x
 // CHECK-NEXT:    3: [B3.2] = [B3.1]
 // CHECK-NEXT:    T: break;
-// CHECK-NEXT:    Preds (1): B2(Unreachable)
+// CHECK-NEXT:    Preds (1): B2
 // CHECK-NEXT:    Succs (1): B1
 // CHECK:  [B4]
 // CHECK-NEXT:   case C:

clang/test/Analysis/cxx-uninitialized-object-unguarded-access.cpp

@@ -383,6 +383,7 @@ class SwitchGuardedFieldsTest {
     case Kind::V:
       return -1;
     }
+    halt();
   }
 
   int operator+() {
@@ -392,6 +393,7 @@ class SwitchGuardedFieldsTest {
     case Kind::V:
       return -1;
     }
+    halt();
   }
 };
 

clang/test/CodeGenObjCXX/return.mm

@@ -12,19 +12,7 @@ - (int)method {
 
 @end
 
-enum Enum {
-  a
-};
-
-int (^block)(Enum) = ^int(Enum e) {
-  switch (e) {
-  case a:
-    return 1;
-  }
-};
-
-// Ensure that both methods and blocks don't use the -fstrict-return undefined
-// behaviour optimization.
+// Ensure that methods don't use the -fstrict-return undefined behaviour optimization.
 
 // CHECK-NOT: call void @llvm.trap
 // CHECK-NOT: unreachable

clang/test/Sema/return.c

@@ -265,7 +265,7 @@ int test_enum_cases(enum Cases C) {
   case C4: return 3;
   case C3: return 4;
   }
-} // no-warning
+} // expected-warning {{non-void function does not return a value in all control paths}}
 
 // PR12318 - Don't give a may reach end of non-void function warning.
 int test34(int x) {

clang/test/SemaCXX/array-bounds.cpp

@@ -133,7 +133,7 @@ int test_pr9296() {
 
 int test_sizeof_as_condition(int flag) {
   int arr[2] = { 0, 0 }; // expected-note {{array 'arr' declared here}}
-  if (flag) 
+  if (flag)
     return sizeof(char) != sizeof(char) ? arr[2] : arr[1];
   return sizeof(char) == sizeof(char) ? arr[2] : arr[1]; // expected-warning {{array index 2 is past the end of the array (that has type 'int[2]')}}
 }
@@ -171,17 +171,17 @@ void test_nested_switch() {
 }
 
 // Test that if all the values of an enum covered, that the 'default' branch
-// is unreachable.
+// is reachable.
 enum Values { A, B, C, D };
 void test_all_enums_covered(enum Values v) {
-  int x[2];
+  int x[2]; // expected-note {{array 'x' declared here}}
   switch (v) {
   case A: return;
   case B: return;
   case C: return;
   case D: return;
   }
-  x[2] = 0; // no-warning
+  x[2] = 0; // expected-warning {{array index 2 is past the end of the array (that has type 'int[2]')}}
 }
 
 namespace tailpad {
@@ -244,7 +244,7 @@ void test_pr10771() {
 }
 
 int test_pr11007_aux(const char * restrict, ...);
-  
+
 // Test checking with varargs.
 void test_pr11007() {
   double a[5]; // expected-note {{array 'a' declared here}}

clang/test/SemaObjC/warn-called-once.m

@@ -444,27 +444,6 @@ void never_called_switch_none(int cond, void (^callback)(void) CALLED_ONCE) {
   }
 }
 
-enum YesNoOrMaybe {
-  YES,
-  NO,
-  MAYBE
-};
-
-void exhaustive_switch(enum YesNoOrMaybe cond, void (^callback)(void) CALLED_ONCE) {
-  switch (cond) {
-  case YES:
-    callback();
-    break;
-  case NO:
-    callback();
-    break;
-  case MAYBE:
-    callback();
-    break;
-  }
-  // no-warning
-}
-
 void called_twice_exceptions(void (^callback)(void) CALLED_ONCE) {
   // TODO: Obj-C exceptions are not supported in CFG,
   //       we should report warnings in these as well.