[clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

Summary:
This check searches for missing `else` branches in `if-else if`-chains and
missing `default` labels in `switch` statements, that use integers as condition.

It is very similar to -Wswitch, but concentrates on integers only, since enums are
already covered.

The option to warn for missing `else` branches is deactivated by default, since it is
very noise on larger code bases.

Running it on LLVM:
{F5354858} for default configuration
{F5354866} just for llvm/lib/Analysis/ScalarEvolution.cpp, the else-path checker is very noisy!

Reviewers: alexfh, aaron.ballman, hokein

Reviewed By: aaron.ballman

Subscribers: lebedev.ri, Eugene.Zelenko, cfe-commits, mgorny, JDevlieghere, xazax.hun

Tags: #clang-tools-extra

Differential Revision: https://reviews.llvm.org/D37808

llvm-svn: 318600
This commit is contained in:
Jonas Toth 2017-11-18 19:48:33 +00:00
parent 41fc45c4e6
commit 9b1dc4c275
9 changed files with 859 additions and 0 deletions

View File

@ -2,6 +2,7 @@ set(LLVM_LINK_COMPONENTS support)
add_clang_library(clangTidyHICPPModule
ExceptionBaseclassCheck.cpp
MultiwayPathsCoveredCheck.cpp
NoAssemblerCheck.cpp
HICPPTidyModule.cpp
SignedBitwiseCheck.cpp

View File

@ -35,6 +35,7 @@
#include "../readability/FunctionSizeCheck.h"
#include "../readability/IdentifierNamingCheck.h"
#include "ExceptionBaseclassCheck.h"
#include "MultiwayPathsCoveredCheck.h"
#include "NoAssemblerCheck.h"
#include "SignedBitwiseCheck.h"
@ -53,6 +54,8 @@ public:
"hicpp-exception-baseclass");
CheckFactories.registerCheck<SignedBitwiseCheck>(
"hicpp-signed-bitwise");
CheckFactories.registerCheck<MultiwayPathsCoveredCheck>(
"hicpp-multiway-paths-covered");
CheckFactories.registerCheck<google::ExplicitConstructorCheck>(
"hicpp-explicit-conversions");
CheckFactories.registerCheck<readability::FunctionSizeCheck>(

View File

@ -0,0 +1,179 @@
//===--- MultiwayPathsCoveredCheck.cpp - clang-tidy------------------------===//
//
// The LLVM Compiler Infrastructure
//
// This file is distributed under the University of Illinois Open Source
// License. See LICENSE.TXT for details.
//
//===----------------------------------------------------------------------===//
#include "MultiwayPathsCoveredCheck.h"
#include "clang/AST/ASTContext.h"
#include <limits>
using namespace clang::ast_matchers;
namespace clang {
namespace tidy {
namespace hicpp {
void MultiwayPathsCoveredCheck::storeOptions(
ClangTidyOptions::OptionMap &Opts) {
Options.store(Opts, "WarnOnMissingElse", WarnOnMissingElse);
}
void MultiwayPathsCoveredCheck::registerMatchers(MatchFinder *Finder) {
Finder->addMatcher(
stmt(allOf(
anyOf(switchStmt(forEachSwitchCase(defaultStmt()))
.bind("switch-default"),
switchStmt(unless(hasDescendant(switchCase())))
.bind("degenerate-switch"),
// This matcher must be the last one of the three
// 'switchStmt' options.
// Otherwise the cases 'switch-default' and
// 'degenerate-switch' are not found correctly.
switchStmt(forEachSwitchCase(unless(defaultStmt())))
.bind("switch-no-default")),
switchStmt(hasCondition(allOf(
// Match on switch statements that have either a bit-field or an
// integer condition. The ordering in 'anyOf()' is important
// because the last condition is the most general.
anyOf(ignoringImpCasts(memberExpr(hasDeclaration(
fieldDecl(isBitField()).bind("bitfield")))),
hasDescendant(declRefExpr().bind("non-enum-condition"))),
// 'unless()' must be the last match here and must be bound,
// otherwise the matcher does not work correctly.
unless(hasDescendant(declRefExpr(hasType(enumType()))
.bind("enum-condition")))))))),
this);
// This option is noisy, therefore matching is configurable.
if (WarnOnMissingElse) {
Finder->addMatcher(
ifStmt(allOf(hasParent(ifStmt()), unless(hasElse(anything()))))
.bind("else-if"),
this);
}
}
static unsigned countCaseLabels(const SwitchStmt *Switch) {
unsigned CaseCount = 0;
const SwitchCase *CurrentCase = Switch->getSwitchCaseList();
while (CurrentCase) {
++CaseCount;
CurrentCase = CurrentCase->getNextSwitchCase();
}
return CaseCount;
}
/// This function calculate 2 ** Bits and returns
/// numeric_limits<std::size_t>::max() if an overflow occured.
static std::size_t twoPow(std::size_t Bits) {
return Bits >= std::numeric_limits<std::size_t>::digits
? std::numeric_limits<std::size_t>::max()
: static_cast<size_t>(1) << Bits;
}
/// Get the number of possible values that can be switched on for the type T.
///
/// \return - 0 if bitcount could not be determined
/// - numeric_limits<std::size_t>::max() when overflow appeared due to
/// more then 64 bits type size.
static std::size_t getNumberOfPossibleValues(QualType T,
const ASTContext &Context) {
// `isBooleanType` must come first because `bool` is an integral type as well
// and would not return 2 as result.
if (T->isBooleanType())
return 2;
else if (T->isIntegralType(Context))
return twoPow(Context.getTypeSize(T));
else
return 1;
}
void MultiwayPathsCoveredCheck::check(const MatchFinder::MatchResult &Result) {
if (const auto *ElseIfWithoutElse =
Result.Nodes.getNodeAs<IfStmt>("else-if")) {
diag(ElseIfWithoutElse->getLocStart(),
"potentially uncovered codepath; add an ending else statement");
return;
}
// Checks the sanity of 'switch' statements that actually do define
// a default branch but might be degenerated by having no or only one case.
if (const auto *SwitchWithDefault =
Result.Nodes.getNodeAs<SwitchStmt>("switch-default")) {
handleSwitchWithDefault(SwitchWithDefault);
return;
}
// Checks all 'switch' statements that do not define a default label.
// Here the heavy lifting happens.
if (const auto *SwitchWithoutDefault =
Result.Nodes.getNodeAs<SwitchStmt>("switch-no-default")) {
handleSwitchWithoutDefault(SwitchWithoutDefault, Result);
return;
}
// Warns for degenerated 'switch' statements that neither define a case nor
// a default label.
if (const auto *DegenerateSwitch =
Result.Nodes.getNodeAs<SwitchStmt>("degenerate-switch")) {
diag(DegenerateSwitch->getLocStart(), "degenerated switch without labels");
return;
}
llvm_unreachable("matched a case, that was not explicitly handled");
}
void MultiwayPathsCoveredCheck::handleSwitchWithDefault(
const SwitchStmt *Switch) {
unsigned CaseCount = countCaseLabels(Switch);
assert(CaseCount > 0 && "Switch statement with supposedly one default "
"branch did not contain any case labels");
if (CaseCount == 1 || CaseCount == 2)
diag(Switch->getLocStart(),
CaseCount == 1
? "degenerated switch with default label only"
: "switch could be better written as an if/else statement");
}
void MultiwayPathsCoveredCheck::handleSwitchWithoutDefault(
const SwitchStmt *Switch, const MatchFinder::MatchResult &Result) {
// The matcher only works because some nodes are explicitly matched and
// bound but ignored. This is necessary to build the excluding logic for
// enums and 'switch' statements without a 'default' branch.
assert(!Result.Nodes.getNodeAs<DeclRefExpr>("enum-condition") &&
"switch over enum is handled by warnings already, explicitly ignoring "
"them");
assert(!Result.Nodes.getNodeAs<SwitchStmt>("switch-default") &&
"switch stmts with default branch do cover all paths, explicitly "
"ignoring them");
// Determine the number of case labels. Since 'default' is not present
// and duplicating case labels is not allowed this number represents
// the number of codepaths. It can be directly compared to 'MaxPathsPossible'
// to see if some cases are missing.
unsigned CaseCount = countCaseLabels(Switch);
// CaseCount == 0 is caught in DegenerateSwitch. Necessary because the
// matcher used for here does not match on degenerate 'switch'.
assert(CaseCount > 0 && "Switch statement without any case found. This case "
"should be excluded by the matcher and is handled "
"separatly.");
std::size_t MaxPathsPossible = [&]() {
if (const auto *GeneralCondition =
Result.Nodes.getNodeAs<DeclRefExpr>("non-enum-condition"))
return getNumberOfPossibleValues(GeneralCondition->getType(),
*Result.Context);
if (const auto *BitfieldDecl =
Result.Nodes.getNodeAs<FieldDecl>("bitfield"))
return twoPow(BitfieldDecl->getBitWidthValue(*Result.Context));
llvm_unreachable("either bit-field or non-enum must be condition");
}();
// FIXME: Transform the 'switch' into an 'if' for CaseCount == 1.
if (CaseCount < MaxPathsPossible)
diag(Switch->getLocStart(),
CaseCount == 1 ? "switch with only one case; use an if statement"
: "potential uncovered code path; add a default label");
}
} // namespace hicpp
} // namespace tidy
} // namespace clang

View File

@ -0,0 +1,51 @@
//===--- MultiwayPathsCoveredCheck.h - clang-tidy----------------*- C++ -*-===//
//
// The LLVM Compiler Infrastructure
//
// This file is distributed under the University of Illinois Open Source
// License. See LICENSE.TXT for details.
//
//===----------------------------------------------------------------------===//
#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_HICPP_MULTIWAY_PATHS_COVERED_H
#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_HICPP_MULTIWAY_PATHS_COVERED_H
#include "../ClangTidy.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include <iostream>
namespace clang {
namespace tidy {
namespace hicpp {
/// Find occasions where not all codepaths are explicitly covered in code.
/// This includes 'switch' without a 'default'-branch and 'if'-'else if'-chains
/// without a final 'else'-branch.
///
/// For the user-facing documentation see:
/// http://clang.llvm.org/extra/clang-tidy/checks/hicpp-multiway-paths-covered.html
class MultiwayPathsCoveredCheck : public ClangTidyCheck {
public:
MultiwayPathsCoveredCheck(StringRef Name, ClangTidyContext *Context)
: ClangTidyCheck(Name, Context),
WarnOnMissingElse(Options.get("WarnOnMissingElse", 0)) {}
void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
private:
void handleSwitchWithDefault(const SwitchStmt *Switch);
void handleSwitchWithoutDefault(
const SwitchStmt *Switch,
const ast_matchers::MatchFinder::MatchResult &Result);
/// This option can be configured to warn on missing 'else' branches in an
/// 'if-else if' chain. The default is false because this option might be
/// noisy on some code bases.
const bool WarnOnMissingElse;
};
} // namespace hicpp
} // namespace tidy
} // namespace clang
#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_HICPP_MULTIWAY_PATHS_COVERED_H

View File

@ -158,6 +158,11 @@ Improvements to clang-tidy
Ensures that all exception will be instances of ``std::exception`` and classes
that are derived from it.
- New `hicpp-multiway-paths-covered
<http://clang.llvm.org/extra/clang-tidy/checks/hicpp-multiway-paths-covered.html>`_ check
Checks on ``switch`` and ``if`` - ``else if`` constructs that do not cover all possible code paths.
- New `hicpp-signed-bitwise
<http://clang.llvm.org/extra/clang-tidy/checks/hicpp-signed-bitwise.html>`_ check

View File

@ -0,0 +1,95 @@
.. title:: clang-tidy - hicpp-multiway-paths-covered
hicpp-multiway-paths-covered
============================
This check discovers situations where code paths are not fully-covered.
It furthermore suggests using ``if`` instead of ``switch`` if the code will be more clear.
The `rule 6.1.2 <http://www.codingstandard.com/rule/6-1-2-explicitly-cover-all-paths-through-multi-way-selection-statements/>`_
and `rule 6.1.4 <http://www.codingstandard.com/rule/6-1-4-ensure-that-a-switch-statement-has-at-least-two-case-labels-distinct-from-the-default-label/>`_
of the High Integrity C++ Coding Standard are enforced.
``if-else if`` chains that miss a final ``else`` branch might lead to unexpected
program execution and be the result of a logical error.
If the missing ``else`` branch is intended you can leave it empty with a clarifying
comment.
This warning can be noisy on some code bases, so it is disabled by default.
.. code-block:: c++
void f1() {
int i = determineTheNumber();
if(i > 0) {
// Some Calculation
} else if (i < 0) {
// Precondition violated or something else.
}
// ...
}
Similar arguments hold for ``switch`` statements which do not cover all possible code paths.
.. code-block:: c++
// The missing default branch might be a logical error. It can be kept empty
// if there is nothing to do, making it explicit.
void f2(int i) {
switch (i) {
case 0: // something
break;
case 1: // something else
break;
}
// All other numbers?
}
// Violates this rule as well, but already emits a compiler warning (-Wswitch).
enum Color { Red, Green, Blue, Yellow };
void f3(enum Color c) {
switch (c) {
case Red: // We can't drive for now.
break;
case Green: // We are allowed to drive.
break;
}
// Other cases missing
}
The `rule 6.1.4 <http://www.codingstandard.com/rule/6-1-4-ensure-that-a-switch-statement-has-at-least-two-case-labels-distinct-from-the-default-label/>`_
requires every ``switch`` statement to have at least two ``case`` labels other than a `default` label.
Otherwise, the ``switch`` could be better expressed with an ``if`` statement.
Degenerated ``switch`` statements without any labels are caught as well.
.. code-block:: c++
// Degenerated switch that could be better written as `if`
int i = 42;
switch(i) {
case 1: // do something here
default: // do somethe else here
}
// Should rather be the following:
if (i == 1) {
// do something here
}
else {
// do something here
}
.. code-block:: c++
// A completly degenerated switch will be diagnosed.
int i = 42;
switch(i) {}
Options
-------
.. option:: WarnOnMissingElse
Activates warning for missing``else`` branches. Default is `0`.

View File

@ -81,6 +81,7 @@ Clang-Tidy Checks
hicpp-invalid-access-moved (redirects to misc-use-after-move) <hicpp-invalid-access-moved>
hicpp-member-init (redirects to cppcoreguidelines-pro-type-member-init) <hicpp-member-init>
hicpp-move-const-arg (redirects to misc-move-const-arg) <hicpp-move-const-arg>
hicpp-multiway-paths-covered
hicpp-named-parameter (redirects to readability-named-parameter) <hicpp-named-parameter>
hicpp-new-delete-operators (redirects to misc-new-delete-overloads) <hicpp-new-delete-operators>
hicpp-no-array-decay (redirects to cppcoreguidelines-pro-bounds-array-to-pointer-decay) <hicpp-no-array-decay>

View File

@ -0,0 +1,57 @@
// RUN: %check_clang_tidy %s hicpp-multiway-paths-covered %t \
// RUN: -config='{CheckOptions: \
// RUN: [{key: hicpp-multiway-paths-covered.WarnOnMissingElse, value: 1}]}'\
// RUN: --
enum OS { Mac,
Windows,
Linux };
void problematic_if(int i, enum OS os) {
if (i > 0) {
return;
} else if (i < 0) {
// CHECK-MESSAGES: [[@LINE-1]]:10: warning: potentially uncovered codepath; add an ending else statement
return;
}
// Could be considered as false positive because all paths are covered logically.
// I still think this is valid since the possibility of a final 'everything else'
// codepath is expected from if-else if.
if (i > 0) {
return;
} else if (i <= 0) {
// CHECK-MESSAGES: [[@LINE-1]]:10: warning: potentially uncovered codepath; add an ending else statement
return;
}
// Test if nesting of if-else chains does get caught as well.
if (os == Mac) {
return;
} else if (os == Linux) {
// These checks are kind of degenerated, but the check will not try to solve
// if logically all paths are covered, which is more the area of the static analyzer.
if (true) {
return;
} else if (false) {
// CHECK-MESSAGES: [[@LINE-1]]:12: warning: potentially uncovered codepath; add an ending else statement
return;
}
return;
} else {
/* unreachable */
if (true) // check if the parent would match here as well
return;
// No warning for simple if statements, since it is common to just test one condition
// and ignore the opposite.
}
// Ok, because all paths are covered
if (i > 0) {
return;
} else if (i < 0) {
return;
} else {
/* error, maybe precondition failed */
}
}

View File

@ -0,0 +1,467 @@
// RUN: %check_clang_tidy %s hicpp-multiway-paths-covered %t
enum OS { Mac,
Windows,
Linux };
struct Bitfields {
unsigned UInt : 3;
int SInt : 1;
};
int return_integer() { return 42; }
void bad_switch(int i) {
switch (i) {
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: switch with only one case; use an if statement
case 0:
break;
}
// No default in this switch
switch (i) {
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered code path; add a default label
case 0:
break;
case 1:
break;
case 2:
break;
}
// degenerate, maybe even warning
switch (i) {
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: degenerated switch without labels
}
switch (int j = return_integer()) {
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered code path; add a default label
case 0:
case 1:
case 2:
break;
}
// Degenerated, only default case.
switch (i) {
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: degenerated switch with default label only
default:
break;
}
// Degenerated, only one case label and default case -> Better as if-stmt.
switch (i) {
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: switch could be better written as an if/else statement
case 0:
break;
default:
break;
}
unsigned long long BigNumber = 0;
switch (BigNumber) {
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered code path; add a default label
case 0:
case 1:
break;
}
const int &IntRef = i;
switch (IntRef) {
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered code path; add a default label
case 0:
case 1:
break;
}
char C = 'A';
switch (C) {
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered code path; add a default label
case 'A':
break;
case 'B':
break;
}
Bitfields Bf;
// UInt has 3 bits size.
switch (Bf.UInt) {
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered code path; add a default label
case 0:
case 1:
break;
}
// All paths explicitly covered.
switch (Bf.UInt) {
case 0:
case 1:
case 2:
case 3:
case 4:
case 5:
case 6:
case 7:
break;
}
// SInt has 1 bit size, so this is somewhat degenerated.
switch (Bf.SInt) {
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: switch with only one case; use an if statement
case 0:
break;
}
// All paths explicitly covered.
switch (Bf.SInt) {
case 0:
case 1:
break;
}
bool Flag = false;
switch (Flag) {
// CHECK-MESSAGES:[[@LINE-1]]:3: warning: switch with only one case; use an if statement
case true:
break;
}
switch (Flag) {
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: degenerated switch with default label only
default:
break;
}
// This `switch` will create a frontend warning from '-Wswitch-bool' but is
// ok for this check.
switch (Flag) {
case true:
break;
case false:
break;
}
}
void unproblematic_switch(unsigned char c) {
switch (c) {
case 0:
case 1:
case 2:
case 3:
case 4:
case 5:
case 6:
case 7:
case 8:
case 9:
case 10:
case 11:
case 12:
case 13:
case 14:
case 15:
case 16:
case 17:
case 18:
case 19:
case 20:
case 21:
case 22:
case 23:
case 24:
case 25:
case 26:
case 27:
case 28:
case 29:
case 30:
case 31:
case 32:
case 33:
case 34:
case 35:
case 36:
case 37:
case 38:
case 39:
case 40:
case 41:
case 42:
case 43:
case 44:
case 45:
case 46:
case 47:
case 48:
case 49:
case 50:
case 51:
case 52:
case 53:
case 54:
case 55:
case 56:
case 57:
case 58:
case 59:
case 60:
case 61:
case 62:
case 63:
case 64:
case 65:
case 66:
case 67:
case 68:
case 69:
case 70:
case 71:
case 72:
case 73:
case 74:
case 75:
case 76:
case 77:
case 78:
case 79:
case 80:
case 81:
case 82:
case 83:
case 84:
case 85:
case 86:
case 87:
case 88:
case 89:
case 90:
case 91:
case 92:
case 93:
case 94:
case 95:
case 96:
case 97:
case 98:
case 99:
case 100:
case 101:
case 102:
case 103:
case 104:
case 105:
case 106:
case 107:
case 108:
case 109:
case 110:
case 111:
case 112:
case 113:
case 114:
case 115:
case 116:
case 117:
case 118:
case 119:
case 120:
case 121:
case 122:
case 123:
case 124:
case 125:
case 126:
case 127:
case 128:
case 129:
case 130:
case 131:
case 132:
case 133:
case 134:
case 135:
case 136:
case 137:
case 138:
case 139:
case 140:
case 141:
case 142:
case 143:
case 144:
case 145:
case 146:
case 147:
case 148:
case 149:
case 150:
case 151:
case 152:
case 153:
case 154:
case 155:
case 156:
case 157:
case 158:
case 159:
case 160:
case 161:
case 162:
case 163:
case 164:
case 165:
case 166:
case 167:
case 168:
case 169:
case 170:
case 171:
case 172:
case 173:
case 174:
case 175:
case 176:
case 177:
case 178:
case 179:
case 180:
case 181:
case 182:
case 183:
case 184:
case 185:
case 186:
case 187:
case 188:
case 189:
case 190:
case 191:
case 192:
case 193:
case 194:
case 195:
case 196:
case 197:
case 198:
case 199:
case 200:
case 201:
case 202:
case 203:
case 204:
case 205:
case 206:
case 207:
case 208:
case 209:
case 210:
case 211:
case 212:
case 213:
case 214:
case 215:
case 216:
case 217:
case 218:
case 219:
case 220:
case 221:
case 222:
case 223:
case 224:
case 225:
case 226:
case 227:
case 228:
case 229:
case 230:
case 231:
case 232:
case 233:
case 234:
case 235:
case 236:
case 237:
case 238:
case 239:
case 240:
case 241:
case 242:
case 243:
case 244:
case 245:
case 246:
case 247:
case 248:
case 249:
case 250:
case 251:
case 252:
case 253:
case 254:
case 255:
break;
}
// Some paths are covered by the switch and a default case is present.
switch (c) {
case 1:
case 2:
case 3:
default:
break;
}
}
OS return_enumerator() {
return Linux;
}
// Enumpaths are already covered by a warning, this is just to ensure, that there is
// no interference or false positives.
// -Wswitch warns about uncovered enum paths and each here described case is already
// covered.
void switch_enums(OS os) {
switch (os) {
case Linux:
break;
}
switch (OS another_os = return_enumerator()) {
case Linux:
break;
}
switch (os) {
}
}
/// All of these cases will not emit a warning per default, but with explicit activation.
/// Covered in extra test file.
void problematic_if(int i, enum OS os) {
if (i > 0) {
return;
} else if (i < 0) {
return;
}
if (os == Mac) {
return;
} else if (os == Linux) {
if (true) {
return;
} else if (false) {
return;
}
return;
} else {
/* unreachable */
if (true) // check if the parent would match here as well
return;
}
// Ok, because all paths are covered
if (i > 0) {
return;
} else if (i < 0) {
return;
} else {
/* error, maybe precondition failed */
}
}