[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

222

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.

225

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

222

put checks in alphabetical order

225

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

226

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

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

4 ↗

(On Diff #539602)

6 ↗

(On Diff #539602)

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

8 ↗

(On Diff #539602)

13 ↗

(On Diff #539602)

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

41–44 ↗

(On Diff #539602)

this probably should be before example

46–49 ↗

(On Diff #539602)

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

52 ↗

(On Diff #539602)

would be nice to list them

58 ↗

(On Diff #539602)

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

225

Sure.

226

Corrected.

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

46–49 ↗

(On Diff #539602)

Sure, will remove.

52 ↗

(On Diff #539602)

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

58 ↗

(On Diff #539602)

Accurate, thanks.

[PiotrZSL]

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

52 ↗

(On Diff #539602)

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

52 ↗

(On Diff #539602)

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 "DynamicStaticInitializersCheck.h"
 #include "EasilySwappableParametersCheck.h"
 #include "ExceptionEscapeCheck.h"
 #include "FoldInitTypeCheck.h"
 #include "ForwardDeclarationNamespaceCheck.h"
 #include "ForwardingReferenceOverloadCheck.h"
 #include "ImplicitWideningOfMultiplicationResultCheck.h"
 #include "InaccurateEraseCheck.h"
+#include "SwitchMissingDefaultCaseCheck.h"
 #include "IncorrectRoundingsCheck.h"
 #include "InfiniteLoopCheck.h"
 #include "IntegerDivisionCheck.h"
 #include "LambdaFunctionNameCheck.h"
 #include "MacroParenthesesCheck.h"
 #include "MacroRepeatedSideEffectsCheck.h"
 #include "MisplacedOperatorInStrlenInAllocCheck.h"
 #include "MisplacedPointerArithmeticInAllocCheck.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 {
+
+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;
+};
+
+} // 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 {
+
+void SwitchMissingDefaultCaseCheck::registerMatchers(
+    ast_matchers::MatchFinder *Finder) {
+  Finder->addMatcher(
+      switchStmt(has(implicitCastExpr().bind("cast")),
+                 unless(hasAncestor(switchStmt(has(defaultStmt())))))
+          .bind("switch"),
+      this);
+}
+
+void SwitchMissingDefaultCaseCheck::check(
+    const ast_matchers::MatchFinder::MatchResult &Result) {
+  const auto *c = Result.Nodes.getNodeAs<ImplicitCastExpr>("cast");
+  if (c->getCastKind() == CK_IntegralCast)
+    return;
+
+  const auto *s = Result.Nodes.getNodeAs<SwitchStmt>("switch");
+  diag(s->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:`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 :doc:`bugprone-switch-missing-default-case
+  <clang-tidy/checks/bugprone/switch-missing-default-case>` check.
+
+  Detects incomplete switch statements without a default case.
+
 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-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>`_, "Yes"
    `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: clang-tidy --checks='-*,misc-incomplete-switch' %s -- | FileCheck -implicit-check-not="{{warning|error}}:" %s
+
+void Positive() {
+  int i = 0;
+  // CHECK: [[@LINE+1]]:11: warning: switching on non-enum value without default case may not cover all cases [misc-incomplete-switch]
+  switch (i) {
+  case 0:
+    break;
+  }
+}
+
+void Negative() {
+  enum E { eE1 };
+  E e = eE1;
+  switch (e) { // no-warning
+  case eE1:
+    break;
+  }
+
+  int i = 0;
+  switch (i) { // no-warning
+  case 0:
+    break;
+  default:
+    break;
+  }
+}