[clang-tidy] Add check for possibly incomplete switch statements

[xgupta]

Summary

While clang warns about a possibly incomplete switch statement when switching over an enum variable and failing to cover all enum values (either explicitly or with a default case), no such warning is emitted if a plain integer variable is used as switch variable.

Add a clang-tidy check to diagnose these scenarios.

No fixit hint is provided since there are multiple possible solutions.

[bkramer]

clang-tidy/misc/IncompleteSwitchCheck.cpp

11 ↗

(On Diff #12172)

This will also match casts that occur somewhere inside of the switch cases, you probably want to use a plain has instead of hasDescendent.

12 ↗

(On Diff #12172)

Similar problem, what if one switch with a default case is contained within a switch without one?

19 ↗

(On Diff #12172)

I think actually checking that you're casting from an enum would be a good idea, integral casts are common with integers too.

short foo;
switch (foo) {// Implicit int promotion.
  ...
}

test/clang-tidy/incomplete-switch.cpp

1 ↗

(On Diff #12172)

Please add -implicit-check-not="{{warning|error}}:" here, otherwise it doesn't check if additional warnings are emitted.

[xgupta]

Just updated the patch to trunk

[xgupta]

.

[xgupta]

Address bkramer's comments except one for "checking that you're casting from an enum".

[xgupta]

Update ReleaseNotes.rst

[PiotrZSL]

Missing documentation for check.
Once you ready with update, please switch review into ready for review.

clang-tools-extra/clang-tidy/misc/IncompleteSwitchCheck.cpp

19 ↗

(On Diff #539390)

no need to check casts, just check if its not enum... or check if its integer (validate type of expression).
and use IgnoreUnlessSpelledInSource to exclude implicit code (like other checks)

20 ↗

(On Diff #539390)

so switch in switch is allowed ? why

31 ↗

(On Diff #539390)

put some meaningful names of variables according to LLVM standard

clang-tools-extra/docs/ReleaseNotes.rst

228

name is to generic. maybe misc-switch-missing-default-case or something similar, maybe it should be readability module or bugprone ? Still misc could be not bad trade-off.

231

put info about non enum, for enums we got compiler warnings already

clang-tools-extra/test/clang-tidy/checkers/misc/incomplete-switch.cpp

1 ↗

(On Diff #539390)

Check currently use different run command. Base on new one.

[xgupta]

Address some comments

[xgupta]

Address more comments

[xgupta]

Add documentation

[PiotrZSL]

clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.cpp

17–30

I do not like that implicitCastExpr.

this is AST for your example:

`-SwitchStmt <line:4:3, line:7:3>
   |-ImplicitCastExpr <line:4:11> 'int' <LValueToRValue>
   | `-DeclRefExpr <col:11> 'int' lvalue Var 0x555de93bc200 'i' 'int'
   `-CompoundStmt <col:14, line:7:3>
     `-CaseStmt <line:5:3, line:6:5>
       |-ConstantExpr <line:5:8> 'int'
       | |-value: Int 0
       | `-IntegerLiteral <col:8> 'int' 0
       `-BreakStmt <line:6:5>

look that case there is only because we got cast from rvalue to lvalue.
if you write example like this:

int getValue();

void Positive() {
  switch (getValue()) {
  case 0:
    break;
  }
}

there is not cast because AST will look like this:

-SwitchStmt <line:4:3, line:7:3>
      |-CallExpr <line:4:11, col:20> 'int'
      | `-ImplicitCastExpr <col:11> 'int (*)()' <FunctionToPointerDecay>
      |   `-DeclRefExpr <col:11> 'int ()' lvalue Function 0x5573a3b38100 'getValue' 'int ()'
      `-CompoundStmt <col:23, line:7:3>
        `-CaseStmt <line:5:3, line:6:5>
          |-ConstantExpr <line:5:8> 'int'
          | |-value: Int 0
          | `-IntegerLiteral <col:8> 'int' 0
          `-BreakStmt <line:6:5>

So add test with rvalue int... remove that implicit cast checks, and use IgnoreUnlessSpelledInSource (it will remove IntegralCast cast from enum to int, so hasCondition would match enum directly)

20

this should be something like:

hasCondition(expr(hasType(qualType(hasCanonicalType(unless(hasDeclaration(enumDecl()))))))

Or you can verify just if type is integral type.
Note that with modern C++ you may have init statements in enums.

21

this doesn't make sense, and i'm not sure why its even working.... for your case with default.
because proper solution would be something like:

AST_MATCHER(SwitchStmt, hasDefaultCase) {
 const SwitchCase *Case = Node.getSwitchCaseList();
 while(Case) {
    if (DefaultStmt::classof(Case))
        return true;

    Case = Case->getNextSwitchCase ();
 }
 return false;
}

and then just

unless(hasDefaultCase())

clang-tools-extra/docs/ReleaseNotes.rst

228

put checks in alphabetical order

231

actually that are complete switch statements, developer covered everything He/She wanted, so other wording should be used

232

this suggest that enums are covered by check, and thats false

clang-tools-extra/docs/clang-tidy/checks/bugprone/switch-missing-default-case.rst

5

7

first sentence in documentation should be in pair to first sentence in release notes and class comment.

9

14

avoid such long lines, no need to duplicate check name, and you can put warning before switch

42–45

this probably should be before example

47–50

this isnt needed, no options, not point to mention them

53

would be nice to list them

59

C.89 is C.89: Make a hash noexcept
There is "ES.79: Use default to handle common cases (only)" but thats only about enums., still could be put here as reference, in such case you should put link

[PiotrZSL]

clang-tools-extra/test/clang-tidy/checkers/bugprone/switch-missing-default-case.cpp

28

Add also tests with int/enum behind typedef and no default case.

[xgupta]

Address all comments excpet in SwitchMissingDefaultCaseCheck.cpp

[xgupta]

clang-tools-extra/docs/ReleaseNotes.rst

231

Sure.

232

Corrected.

clang-tools-extra/docs/clang-tidy/checks/bugprone/switch-missing-default-case.rst

47–50

Sure, will remove.

53

Didn't understand, what should I list, non-enum types?

59

Accurate, thanks.

[PiotrZSL]

clang-tools-extra/docs/clang-tidy/checks/bugprone/switch-missing-default-case.rst

53

No, not a types, compiler warnings (ones for enums). Simply so when user enable this check for integers, He/she could make sure that enum specific warnings are also enabled.
and remove this list that you added, as you cannot do switch on pointers or floating point types.

clang-tools-extra/test/clang-tidy/checkers/bugprone/switch-missing-default-case.cpp

5

[xgupta]

Updated SwitchMissingDefaultCaseCheck.cpp but still WIP
Will look again on saturday/sunday

[xgupta]

Address latest two comments

[xgupta]

clang-tools-extra/docs/clang-tidy/checks/bugprone/switch-missing-default-case.rst

53

I can see there is -Wswitch which I have mentioned.

[xgupta]

Address comment

[xgupta]

clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.cpp

17–30

Removed the implicit cast but couldn't get how to use IgnoreUnlessSpelledInSource here.

20

For some reason, the check is giving warning for enum cases and I couldn't understand why, can you please help?

[PiotrZSL]

clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.cpp

17–30

Just add

std::optional<TraversalKind> getCheckTraversalKind() const override {
   return TK_IgnoreUnlessSpelledInSource;
 }

to header, like it is in other checks.
this should also solve template handling, so add test like this:

template<typename T>
void testTemplate(T value)
{
   switch(value)  {
   ...
   }
}

testTemplate(5);
testTemplate(SomeEnum);

clang-tools-extra/docs/clang-tidy/checks/list.rst

95

check does not provide fixes, so remove that "Yes"

[PiotrZSL]

clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.cpp

20

Add getCheckTraversalKind and check again....

[xgupta]

Address comments

clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.cpp

20

Wow, feel like a magic!

[PiotrZSL]

Tests need to be reverted, and template handling corrected (check comments).
After that it looks good to me.

clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.cpp

30

clang-tools-extra/test/clang-tidy/checkers/bugprone/switch-missing-default-case.cpp

2

20

wrong, do not change everything to use only testTemplates, leave previous one, template should be additional test, this is why test fail. Previous tests were fine, just some additional should be added.

[xgupta]

Updated testcase [WIP]

[PiotrZSL]

Just few nits (column numbers in test, missing doxygen comment, ...).
Please fix those before committing.

Except that, looking good to me.

clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.cpp

17–26

Put this into anonymous namespace to avoid ODR issues

41–42

should never happend

clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.h

19

Missing class doxygen comment.
Check other classes, should be something like:

/// Ensures that switch statements without default cases are flagged, focuses only
/// on covering cases with non-enums where the compiler may not issue warnings.
///
/// For the user-facing documentation see:
/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/switch-missing-default-case.html

clang-tools-extra/test/clang-tidy/checkers/bugprone/switch-missing-default-case.cpp

10

17

24

[xgupta]

Address comments

[xgupta]

In D4784#4504690, @PiotrZSL wrote:

Just few nits (column numbers in test, missing doxygen comment, ...).
Please fix those before committing.

Except that, looking good to me.

Thanks for this nice code review.

[bbannier]

Thanks for the reviews and @xgupta for pushing this over the finish line. This had been lingering a long time.

clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp

 #include "SuspiciousIncludeCheck.h"
 #include "SuspiciousMemoryComparisonCheck.h"
 #include "SuspiciousMemsetUsageCheck.h"
 #include "SuspiciousMissingCommaCheck.h"
 #include "SuspiciousReallocUsageCheck.h"
 #include "SuspiciousSemicolonCheck.h"
 #include "SuspiciousStringCompareCheck.h"
 #include "SwappedArgumentsCheck.h"
+#include "SwitchMissingDefaultCaseCheck.h"
 #include "TerminatingContinueCheck.h"
 #include "ThrowKeywordMissingCheck.h"
 #include "TooSmallLoopVariableCheck.h"
 #include "UncheckedOptionalAccessCheck.h"
 #include "UndefinedMemoryManipulationCheck.h"
 #include "UndelegatedConstructorCheck.h"
 #include "UnhandledExceptionAtNewCheck.h"
 #include "UnhandledSelfAssignmentCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<ForwardDeclarationNamespaceCheck>(
         "bugprone-forward-declaration-namespace");
     CheckFactories.registerCheck<ForwardingReferenceOverloadCheck>(
         "bugprone-forwarding-reference-overload");
     CheckFactories.registerCheck<ImplicitWideningOfMultiplicationResultCheck>(
         "bugprone-implicit-widening-of-multiplication-result");
     CheckFactories.registerCheck<InaccurateEraseCheck>(
         "bugprone-inaccurate-erase");
+    CheckFactories.registerCheck<SwitchMissingDefaultCaseCheck>(
+        "bugprone-switch-missing-default-case");
     CheckFactories.registerCheck<IncorrectRoundingsCheck>(
         "bugprone-incorrect-roundings");
     CheckFactories.registerCheck<InfiniteLoopCheck>("bugprone-infinite-loop");
     CheckFactories.registerCheck<IntegerDivisionCheck>(
         "bugprone-integer-division");
     CheckFactories.registerCheck<LambdaFunctionNameCheck>(
         "bugprone-lambda-function-name");
     CheckFactories.registerCheck<MacroParenthesesCheck>(

clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt

add_clang_library(clangTidyBugproneModule
   DynamicStaticInitializersCheck.cpp
   EasilySwappableParametersCheck.cpp
   ExceptionEscapeCheck.cpp
   FoldInitTypeCheck.cpp
   ForwardDeclarationNamespaceCheck.cpp
   ForwardingReferenceOverloadCheck.cpp
   ImplicitWideningOfMultiplicationResultCheck.cpp
   InaccurateEraseCheck.cpp
+  SwitchMissingDefaultCaseCheck.cpp
   IncorrectRoundingsCheck.cpp
   InfiniteLoopCheck.cpp
   IntegerDivisionCheck.cpp
   LambdaFunctionNameCheck.cpp
   MacroParenthesesCheck.cpp
   MacroRepeatedSideEffectsCheck.cpp
   MisplacedOperatorInStrlenInAllocCheck.cpp
   MisplacedPointerArithmeticInAllocCheck.cpp

clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.h

+//===--- SwitchMissingDefaultCaseCheck.h - clang-tidy -----------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SWITCHMISSINGDEFAULTCASECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SWITCHMISSINGDEFAULTCASECHECK_H
+
+#include "../ClangTidyCheck.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+
+namespace clang::tidy::bugprone {
+
+/// Ensures that switch statements without default cases are flagged, focuses
+/// only on covering cases with non-enums where the compiler may not issue
+/// warnings.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/switch-missing-default-case.html
+class SwitchMissingDefaultCaseCheck : public ClangTidyCheck {
+public:
+  SwitchMissingDefaultCaseCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  std::optional<TraversalKind> getCheckTraversalKind() const override {
+    return TK_IgnoreUnlessSpelledInSource;
+  }
+};
+
+} // namespace clang::tidy::bugprone
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SWITCHMISSINGDEFAULTCASECHECK_H

clang-tools-extra/clang-tidy/bugprone/SwitchMissingDefaultCaseCheck.cpp

+//===--- SwitchMissingDefaultCaseCheck.cpp - clang-tidy -------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "SwitchMissingDefaultCaseCheck.h"
+#include "clang/AST/ASTContext.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::bugprone {
+
+namespace {
+
+AST_MATCHER(SwitchStmt, hasDefaultCase) {
+  const SwitchCase *Case = Node.getSwitchCaseList();
+  while (Case) {
+    if (DefaultStmt::classof(Case))
+      return true;
+
+    Case = Case->getNextSwitchCase();
+  }
+  return false;
+}
+} // namespace
+
+void SwitchMissingDefaultCaseCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      switchStmt(hasCondition(expr(unless(isInstantiationDependent()),
+                                   hasType(qualType(hasCanonicalType(
+                                       unless(hasDeclaration(enumDecl()))))))),
+                 unless(hasDefaultCase()))
+          .bind("switch"),
+      this);
+}
+
+void SwitchMissingDefaultCaseCheck::check(
+    const ast_matchers::MatchFinder::MatchResult &Result) {
+  const auto *Switch = Result.Nodes.getNodeAs<SwitchStmt>("switch");
+
+  diag(Switch->getSwitchLoc(), "switching on non-enum value without "
+                               "default case may not cover all cases");
+}
+} // namespace clang::tidy::bugprone

clang-tools-extra/docs/ReleaseNotes.rst

- New :doc:`bugprone-multiple-new-in-one-expression
   memory by the first ``new`` may leak if the second allocation fails and throws exception.
 
 - New :doc:`bugprone-non-zero-enum-to-bool-conversion
   <clang-tidy/checks/bugprone/non-zero-enum-to-bool-conversion>` check.
 
   Detect implicit and explicit casts of ``enum`` type into ``bool`` where ``enum`` type
   doesn't have a zero-value enumerator.
 
+- New :doc:`bugprone-switch-missing-default-case
+  <clang-tidy/checks/bugprone/switch-missing-default-case>` check.
+
+  Ensures that switch statements without default cases are flagged, focuses only
+  on covering cases with non-enums where the compiler may not issue warnings.
+
 - New :doc:`bugprone-unique-ptr-array-mismatch
   <clang-tidy/checks/bugprone/unique-ptr-array-mismatch>` check.
 
   Finds initializations of C++ unique pointers to non-array type that are
   initialized with an array.
 
 - New :doc:`bugprone-unsafe-functions
   <clang-tidy/checks/bugprone/unsafe-functions>` check.
- New :doc:`readability-avoid-unconditional-preprocessor-if
   ``#if 1``, etc.
 
 - New :doc:`readability-operators-representation
   <clang-tidy/checks/readability/operators-representation>` check.
 
   Enforces consistent token representation for invoked binary, unary and
   overloaded operators in C++ code.
 
 New check aliases
 ^^^^^^^^^^^^^^^^^
 
 - New alias :doc:`cert-msc24-c
   <clang-tidy/checks/cert/msc24-c>` to :doc:`bugprone-unsafe-functions
   <clang-tidy/checks/bugprone/unsafe-functions>` was added.
 
 - New alias :doc:`cert-msc33-c
   <clang-tidy/checks/cert/msc33-c>` to :doc:`bugprone-unsafe-functions
   <clang-tidy/checks/bugprone/unsafe-functions>` was added.
 
 - New alias :doc:`cppcoreguidelines-noexcept-destructor
   <clang-tidy/checks/cppcoreguidelines/noexcept-destructor>` to

clang-tools-extra/docs/clang-tidy/checks/bugprone/switch-missing-default-case.rst

+.. title:: clang-tidy - bugprone-switch-missing-default-case
+
+bugprone-switch-missing-default-case
+====================================
+
+Ensures that switch statements without default cases are flagged, focuses only
+on covering cases with non-enums where the compiler may not issue warnings.
+
+Switch statements without a default case can lead to unexpected
+behavior and incomplete handling of all possible cases. When a switch statement
+lacks a default case, if a value is encountered that does not match any of the
+specified cases, the program will continue execution without any defined
+behavior or handling.
+
+This check helps identify switch statements that are missing a default case,
+allowing developers to ensure that all possible cases are handled properly.
+Adding a default case allows for graceful handling of unexpected or unmatched
+values, reducing the risk of program errors and unexpected behavior.
+
+Example:
+
+.. code-block:: c++
+
+  // Example 1:
+  // warning: switching on non-enum value without default case may not cover all cases
+  switch (i) {
+  case 0:
+    break;
+  }
+
+  // Example 2:
+  enum E { eE1 };
+  E e = eE1;
+  switch (e) { // no-warning
+  case eE1:
+    break;
+  }
+
+  // Example 3:
+  int i = 0;
+  switch (i) { // no-warning
+  case 0:
+    break;
+  default:
+    break;
+  }
+
+.. note::
+   Enum types are already covered by compiler warnings (comes under -Wswitch)
+   when a switch statement does not handle all enum values. This check focuses
+   on non-enum types where the compiler warnings may not be present.
+
+.. seealso::
+   The `CppCoreGuideline ES.79 <https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-default>`_
+   provide guidelines on switch statements, including the recommendation to
+   always provide a default case.

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. csv-table::
    `bugprone-dynamic-static-initializers <bugprone/dynamic-static-initializers.html>`_,
    `bugprone-easily-swappable-parameters <bugprone/easily-swappable-parameters.html>`_,
    `bugprone-exception-escape <bugprone/exception-escape.html>`_,
    `bugprone-fold-init-type <bugprone/fold-init-type.html>`_,
    `bugprone-forward-declaration-namespace <bugprone/forward-declaration-namespace.html>`_,
    `bugprone-forwarding-reference-overload <bugprone/forwarding-reference-overload.html>`_,
    `bugprone-implicit-widening-of-multiplication-result <bugprone/implicit-widening-of-multiplication-result.html>`_, "Yes"
    `bugprone-inaccurate-erase <bugprone/inaccurate-erase.html>`_, "Yes"
+   `bugprone-switch-missing-default-case <bugprone/switch-missing-default-case.html>`_,
    `bugprone-incorrect-roundings <bugprone/incorrect-roundings.html>`_,
    `bugprone-infinite-loop <bugprone/infinite-loop.html>`_,
    `bugprone-integer-division <bugprone/integer-division.html>`_,
    `bugprone-lambda-function-name <bugprone/lambda-function-name.html>`_,
    `bugprone-macro-parentheses <bugprone/macro-parentheses.html>`_, "Yes"
    `bugprone-macro-repeated-side-effects <bugprone/macro-repeated-side-effects.html>`_,
    `bugprone-misplaced-operator-in-strlen-in-alloc <bugprone/misplaced-operator-in-strlen-in-alloc.html>`_, "Yes"
    `bugprone-misplaced-pointer-arithmetic-in-alloc <bugprone/misplaced-pointer-arithmetic-in-alloc.html>`_, "Yes"

clang-tools-extra/test/clang-tidy/checkers/bugprone/switch-missing-default-case.cpp

+// RUN: %check_clang_tidy %s bugprone-switch-missing-default-case %t -- -- -fno-delayed-template-parsing
+
+typedef int MyInt;
+enum EnumType { eE2 };
+typedef EnumType MyEnum;
+
+void positive() {
+  int I1 = 0;
+  // CHECK-MESSAGES: [[@LINE+1]]:3: warning: switching on non-enum value without default case may not cover all cases [bugprone-switch-missing-default-case]
+  switch (I1) {
+  case 0:
+    break;
+  }
+
+  MyInt I2 = 0;
+  // CHECK-MESSAGES: [[@LINE+1]]:3: warning: switching on non-enum value without default case may not cover all cases [bugprone-switch-missing-default-case]
+  switch (I2) {
+  case 0:
+    break;
+  }
+
+  int getValue(void);
+  // CHECK-MESSAGES: [[@LINE+1]]:3: warning: switching on non-enum value without default case may not cover all cases [bugprone-switch-missing-default-case]
+  switch (getValue()) {
+  case 0:
+    break;
+  }
+}
+
+void negative() {
+  enum E { eE1 };
+  E E1 = eE1;
+  switch (E1) { // no-warning
+  case eE1:
+    break;
+  }
+
+  MyEnum E2 = eE2;
+  switch (E2) { // no-warning
+  case eE2:
+    break;
+  }
+
+  int I1 = 0;
+  switch (I1) { // no-warning
+  case 0:
+    break;
+  default:
+    break;
+  }
+
+  MyInt I2 = 0;
+  switch (I2) { // no-warning
+  case 0:
+    break;
+  default:
+    break;
+  }
+
+  int getValue(void);
+  switch (getValue()) { // no-warning
+  case 0:
+    break;
+    default:
+    break;
+  }
+}
+
+template<typename T>
+void testTemplate(T Value) {
+  switch (Value) {
+    case 0:
+      break;
+  }
+}
+
+void exampleUsage() {
+  testTemplate(5);
+  testTemplate(EnumType::eE2);
+}