[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

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

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

19

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

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 ↗

(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

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

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

 add_clang_library(clangTidyMiscModule
   ArgumentCommentCheck.cpp
   BoolPointerImplicitConversion.cpp
+  IncompleteSwitchCheck.cpp
   MiscTidyModule.cpp
   RedundantSmartptrGet.cpp
   SwappedArgumentsCheck.cpp

clang-tidy/misc/IncompleteSwitchCheck.h

+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_INCOMPLETESWITCHCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_INCOMPLETESWITCHCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+
+class IncompleteSwitchCheck : public ClangTidyCheck {
+public:
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+} // tidy
+} // clang
+
+#endif /* end of include guard: INCOMPLETESWITCHCHECK_H */

clang-tidy/misc/IncompleteSwitchCheck.cpp

+#include "IncompleteSwitchCheck.h"
+#include <iostream>
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+void
+IncompleteSwitchCheck::registerMatchers(ast_matchers::MatchFinder *Finder) {
+  Finder->addMatcher(
+      switchStmt(hasDescendant(implicitCastExpr().bind("cast")),
+                 unless(hasDescendant(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->getCond()->getLocStart(), "switching on non-enum value without "
+                                    "default case may not cover all cases");
+}
+} // tidy
+} // clang

clang-tidy/misc/MiscTidyModule.cpp

 #include "../ClangTidyModuleRegistry.h"
 #include "ArgumentCommentCheck.h"
 #include "BoolPointerImplicitConversion.h"
+#include "IncompleteSwitchCheck.h"
 #include "RedundantSmartptrGet.h"
 #include "SwappedArgumentsCheck.h"
 #include "UndelegatedConstructor.h"

         "misc-redundant-smartptr-get",
         new ClangTidyCheckFactory<RedundantSmartptrGet>());
     CheckFactories.addCheckFactory(
+        "misc-incomplete-switch",
+        new ClangTidyCheckFactory<IncompleteSwitchCheck>());
+    CheckFactories.addCheckFactory(
         "misc-swapped-arguments",
         new ClangTidyCheckFactory<SwappedArgumentsCheck>());
     CheckFactories.addCheckFactory(

test/clang-tidy/incomplete-switch.cpp

+// RUN: clang-tidy --checks='-*,misc-incomplete-switch' %s -- | FileCheck %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;
+  }
+}