[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

20

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)

21

so switch in switch is allowed ? why

32

put some meaningful names of variables according to LLVM standard

clang-tools-extra/docs/ReleaseNotes.rst

222 ↗

(On Diff #539390)

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 ↗

(On Diff #539390)

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

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

2

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

16–29 ↗

(On Diff #539602)

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)

19 ↗

(On Diff #539602)

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.

20 ↗

(On Diff #539602)

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 ↗

(On Diff #539602)

put checks in alphabetical order

225 ↗

(On Diff #539602)

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

226 ↗

(On Diff #539602)

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

27 ↗

(On Diff #539602)

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 ↗

(On Diff #539602)

Sure.

226 ↗

(On Diff #539602)

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

4 ↗

(On Diff #539857)

[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

16–29 ↗

(On Diff #539602)

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

19 ↗

(On Diff #539602)

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

16–29 ↗

(On Diff #539602)

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 ↗

(On Diff #540798)

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

[PiotrZSL]

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

19 ↗

(On Diff #539602)

Add getCheckTraversalKind and check again....

[xgupta]

Address comments

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

19 ↗

(On Diff #539602)

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

29 ↗

(On Diff #540818)

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

1 ↗

(On Diff #540818)

19 ↗

(On Diff #540818)

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

16–25 ↗

(On Diff #540845)

Put this into anonymous namespace to avoid ODR issues

40–41 ↗

(On Diff #540845)

should never happend

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

18 ↗

(On Diff #540845)

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

9 ↗

(On Diff #540845)

16 ↗

(On Diff #540845)

23 ↗

(On Diff #540845)

[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/misc/CMakeLists.txt

 add_custom_target(genconfusable DEPENDS Confusables.inc)
 
 add_clang_library(clangTidyMiscModule
   ConstCorrectnessCheck.cpp
   DefinitionsInHeadersCheck.cpp
   ConfusableIdentifierCheck.cpp
   HeaderIncludeCycleCheck.cpp
   IncludeCleanerCheck.cpp
+  IncompleteSwitchCheck.cpp
   MiscTidyModule.cpp
   MisleadingBidirectional.cpp
   MisleadingIdentifier.cpp
   MisplacedConstCheck.cpp
   NewDeleteOverloadsCheck.cpp
   NoRecursionCheck.cpp
   NonCopyableObjects.cpp
   NonPrivateMemberVariablesInClassesCheck.cpp

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

+//===--- IncompleteSwitchCheck.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_MISC_INCOMPLETESWITCHCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_INCOMPLETESWITCHCHECK_H
+
+#include "../ClangTidyCheck.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+
+namespace clang::tidy::misc {
+
+class IncompleteSwitchCheck : public ClangTidyCheck {
+public:
+  IncompleteSwitchCheck(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::misc
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_INCOMPLETESWITCHCHECK_H

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

+//===--- IncompleteSwitchCheck.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 "IncompleteSwitchCheck.h"
+#include "clang/AST/ASTContext.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::misc {
+
+void IncompleteSwitchCheck::registerMatchers(
+    ast_matchers::MatchFinder *Finder) {
+  Finder->addMatcher(
+      switchStmt(has(implicitCastExpr().bind("cast")),
+                 unless(hasAncestor(switchStmt(has(defaultStmt())))))
+          .bind("switch"),
+      this);
+}
+
+void IncompleteSwitchCheck::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::misc

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

 //===--- MiscTidyModule.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 "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "ConfusableIdentifierCheck.h"
 #include "ConstCorrectnessCheck.h"
 #include "DefinitionsInHeadersCheck.h"
 #include "HeaderIncludeCycleCheck.h"
 #include "IncludeCleanerCheck.h"
+#include "IncompleteSwitchCheck.h"
 #include "MisleadingBidirectional.h"
 #include "MisleadingIdentifier.h"
 #include "MisplacedConstCheck.h"
 #include "NewDeleteOverloadsCheck.h"
 #include "NoRecursionCheck.h"
 #include "NonCopyableObjects.h"
 #include "NonPrivateMemberVariablesInClassesCheck.h"
 #include "RedundantExpressionCheck.h"
CheckFactories.registerCheck<ConfusableIdentifierCheck>(
         "misc-confusable-identifiers");
     CheckFactories.registerCheck<ConstCorrectnessCheck>(
         "misc-const-correctness");
     CheckFactories.registerCheck<DefinitionsInHeadersCheck>(
         "misc-definitions-in-headers");
     CheckFactories.registerCheck<HeaderIncludeCycleCheck>(
         "misc-header-include-cycle");
     CheckFactories.registerCheck<IncludeCleanerCheck>("misc-include-cleaner");
+    CheckFactories.registerCheck<IncompleteSwitchCheck>(
+        "misc-incomplete-switch");
     CheckFactories.registerCheck<MisleadingBidirectionalCheck>(
         "misc-misleading-bidirectional");
     CheckFactories.registerCheck<MisleadingIdentifierCheck>(
         "misc-misleading-identifier");
     CheckFactories.registerCheck<MisplacedConstCheck>("misc-misplaced-const");
     CheckFactories.registerCheck<NewDeleteOverloadsCheck>(
         "misc-new-delete-overloads");
     CheckFactories.registerCheck<NoRecursionCheck>("misc-no-recursion");

clang-tools-extra/test/clang-tidy/checkers/misc/incomplete-switch.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;
+  }
+}