[Diagnostics] Implement -Wswitch-default

[xbolva00]

Summary

For MSVC compatibility, based on request in https://bugs.llvm.org/show_bug.cgi?id=4546.

[xbolva00]

I will work on this after we land https://reviews.llvm.org/D63139.

[xbolva00]

lib/Sema/SemaStmt.cpp

869

TODO:

if (!SC) -> switch has no default and case labels (empty body is misleading.. switch(x) x = b)

switch (x) {

// warn_empty_switch_body

}

[xbolva00]

Rebased, improved.

[xbolva00]

In D63192#1539605, @xbolva00 wrote:

I will work on this after we land https://reviews.llvm.org/D63139.

^ not yet landed, but please @aaron.ballman take a look, this patch is ready for review.

[aaron.ballman]

I'm wary of adding this as an on-by-default warning, despite its presence in MSVC (and despite submitting a similar patch years ago to implement the same functionality). Machine-generated code runs into this one frequently (we've been bitten by it numerous times with our tablegen code, especially in the backends), but it does not seem likely to occur in hand-written code very often.

While it's pretty easy to implement this in the frontend, I sort of feel like this is a better candidate for clang-tidy's readability module.

[xbolva00]

I see. I should really check clang-tidy codebase :)

[aaron.ballman]

In D63192#1578867, @xbolva00 wrote:

I see. I should really check clang-tidy codebase :)

I have a hunch that this will be equally as trivially implementable in clang-tidy -- if you go that route, feel free to add me as a reviewer!

include/clang/Basic/DiagnosticGroups.td

def Shadow : DiagGroup<"shadow", [ShadowFieldInConstructorModified,
                                   ShadowIvar]>;
 def ShadowAll : DiagGroup<"shadow-all", [Shadow, ShadowFieldInConstructor,
                                          ShadowUncapturedLocal, ShadowField]>;
 
 def Shorten64To32 : DiagGroup<"shorten-64-to-32">;
 def : DiagGroup<"sign-promo">;
 def SignCompare : DiagGroup<"sign-compare">;
 def : DiagGroup<"stack-protector">;
-def : DiagGroup<"switch-default">;
 def : DiagGroup<"synth">;
 def SizeofArrayArgument : DiagGroup<"sizeof-array-argument">;
 def SizeofArrayDecay : DiagGroup<"sizeof-array-decay">;
 def SizeofPointerMemaccess : DiagGroup<"sizeof-pointer-memaccess">;
 def MemsetTransposedArgs : DiagGroup<"memset-transposed-args">;
 def DynamicClassMemaccess : DiagGroup<"dynamic-class-memaccess">;
 def NonTrivialMemaccess : DiagGroup<"nontrivial-memaccess">;
 def SuspiciousBzero : DiagGroup<"suspicious-bzero">;

 
 def InvalidOffsetof : DiagGroup<"invalid-offsetof">;
 def : DiagGroup<"strict-prototypes">;
 def StrictSelector : DiagGroup<"strict-selector-match">;
 def MethodDuplicate : DiagGroup<"duplicate-method-match">;
 def ObjCCStringFormat : DiagGroup<"cstring-format-directive">;
 def CoveredSwitchDefault : DiagGroup<"covered-switch-default">;
 def SwitchBool     : DiagGroup<"switch-bool">;
+def SwitchDefault  : DiagGroup<"switch-default">;
 def SwitchEnum     : DiagGroup<"switch-enum">;
 def Switch         : DiagGroup<"switch">;
 def EnumCompareSwitch : DiagGroup<"enum-compare-switch">;
 def EnumCompare       : DiagGroup<"enum-compare", [EnumCompareSwitch]>;
 def ImplicitFallthroughPerFunction :
   DiagGroup<"implicit-fallthrough-per-function">;
 def ImplicitFallthrough  : DiagGroup<"implicit-fallthrough",
                                      [ImplicitFallthroughPerFunction]>;

 def ThreadSafetyBeta : DiagGroup<"thread-safety-beta">;
 
 // Uniqueness Analysis warnings
 def Consumed       : DiagGroup<"consumed">;
 
 // Note that putting warnings in -Wall will not disable them by default. If a
 // warning should be active _only_ when -Wall is passed in, mark it as
 // DefaultIgnore in addition to putting it here.
-def All : DiagGroup<"all", [Most, Parentheses, Switch, SwitchBool]>;
+def All : DiagGroup<"all", [Most, Parentheses, Switch, SwitchBool, SwitchDefault]>;
 
 // Warnings that should be in clang-cl /w4.
 def : DiagGroup<"CL4", [All, Extra]>;
 
 // Warnings enabled by -pedantic.  This is magically filled in by TableGen.
 def Pedantic : DiagGroup<"pedantic">;
 
 // Aliases.

include/clang/Basic/DiagnosticSemaKinds.td

 def err_break_not_in_loop_or_switch : Error<
   "'break' statement not in loop or switch statement">;
 def warn_loop_ctrl_binds_to_inner : Warning<
   "'%0' is bound to current loop, GCC binds it to the enclosing loop">,
   InGroup<GccCompat>;
 def warn_break_binds_to_switch : Warning<
   "'break' is bound to loop, GCC binds it to switch">,
   InGroup<GccCompat>;
+def warn_no_cases_in_switch : Warning<
+  "switch has no default and case labels">, InGroup<Switch>, DefaultIgnore;
+def warn_default_only_in_switch : Warning<
+  "switch contains only default label">, InGroup<SwitchDefault>, DefaultIgnore;
 def err_default_not_in_switch : Error<
   "'default' statement not in switch statement">;
 def err_case_not_in_switch : Error<"'case' statement not in switch statement">;
 def warn_bool_switch_condition : Warning<
   "switch condition has boolean value">, InGroup<SwitchBool>;
 def warn_case_value_overflow : Warning<
   "overflow converting case value to switch condition type (%0 to %1)">,
   InGroup<Switch>;

lib/Sema/SemaStmt.cpp

Sema::ActOnFinishSwitchStmt(SourceLocation SwitchLoc, Stmt *Switch,
   // Keep track of any GNU case ranges we see.  The APSInt is the low value.
   typedef std::vector<std::pair<llvm::APSInt, CaseStmt*> > CaseRangesTy;
   CaseRangesTy CaseRanges;
 
   DefaultStmt *TheDefaultStmt = nullptr;
 
   bool CaseListIsErroneous = false;
 
-  for (SwitchCase *SC = SS->getSwitchCaseList(); SC && !HasDependentValue;
-       SC = SC->getNextSwitchCase()) {
+  SwitchCase *SC = SS->getSwitchCaseList();
+  if (!SC)
+    Diag(SS->getBeginLoc(), diag::warn_no_cases_in_switch);
+  else if (isa<DefaultStmt>(SC) && !SC->getNextSwitchCase())
+    Diag(SC->getBeginLoc(), diag::warn_default_only_in_switch);
+
+  for (; SC && !HasDependentValue; SC = SC->getNextSwitchCase()) {
 
     if (DefaultStmt *DS = dyn_cast<DefaultStmt>(SC)) {
       if (TheDefaultStmt) {
         Diag(DS->getDefaultLoc(), diag::err_multiple_default_labels_defined);
         Diag(TheDefaultStmt->getDefaultLoc(), diag::note_duplicate_case_prev);
 
         // FIXME: Remove the default statement from the switch block so that
         // we'll return a valid AST.  This requires recursing down the AST and

test/Sema/switch-availability.c

 // RUN: %clang_cc1 -verify -Wswitch -triple x86_64-apple-macosx10.12 %s
 
 enum SwitchOne {
   Unavail __attribute__((availability(macos, unavailable))),
 };
 
 void testSwitchOne(enum SwitchOne so) {
-  switch (so) {} // no warning
+  switch (so) {} // expected-warning {{switch has no default and case labels}}
 }
 
 enum SwitchTwo {
   Ed __attribute__((availability(macos, deprecated=10.12))),
   Vim __attribute__((availability(macos, deprecated=10.13))),
   Emacs,
 };
 
 void testSwitchTwo(enum SwitchTwo st) {
+  // expected-warning@+1 {{switch has no default and case labels}}
   switch (st) {} // expected-warning{{enumeration values 'Vim' and 'Emacs' not handled in switch}}
 }
 
 enum SwitchThree {
   New __attribute__((availability(macos, introduced=1000))),
 };
 
 void testSwitchThree(enum SwitchThree st) {
+  // expected-warning@+1 {{switch has no default and case labels}}
   switch (st) {} // expected-warning{{enumeration value 'New' not handled in switch}}
 }

test/SemaCXX/switch_default.cpp

+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wswitch-default %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wswitch-default %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wall %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wall %s
+
+void test(int x) {
+  switch (x) {
+  default: // expected-warning {{switch contains only default label}}
+    break;
+  }
+
+  switch (x) {
+  default: // expected-warning {{switch contains only default label}}
+    x++;
+  }
+
+  switch (x) {
+  default:; // expected-warning {{switch contains only default label}}
+  }
+
+  switch (x)
+  default: // expected-warning {{switch contains only default label}}
+    break;
+
+  switch (x) {
+  default:
+    return;
+  case 1:
+    break;
+  }
+
+  switch (x) {
+  case 1:
+    break;
+  default:
+    return;
+  }
+}

test/SemaCXX/uninitialized.cpp

if (b) {
     [fname]{};
   }
 }
 
 void if_switch_init_stmt(int k) {
   if (int n = 0; (n == k || k > 5)) {}
 
   if (int n; (n == k || k > 5)) {} // expected-warning {{uninitialized}} expected-note {{initialize}}
-
+  // expected-warning@+1 {{switch has no default and case labels}}
   switch (int n = 0; (n == k || k > 5)) {} // expected-warning {{boolean}}
-
+  // expected-warning@+1 {{switch has no default and case labels}}
   switch (int n; (n == k || k > 5)) {} // expected-warning {{uninitialized}} expected-note {{initialize}} expected-warning {{boolean}}
 }