Skip to content

Fix ticket 8442: detect unselectable switch cases #8815

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
JeewoongKim wants to merge 1 commit into
base: main
Choose a base branch
from
+122 −0
Open

Fix ticket 8442: detect unselectable switch cases #8815

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 62 additions & 0 deletions lib/checkother.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -968,6 +968,66 @@ void CheckOtherImpl::suspiciousCaseInSwitchError(const Token* tok, const std::st
"Using an operator like '" + operatorString + "' in a case label is suspicious. Did you intend to use a bitwise operator, multiple case labels or if/else instead?", CWE398, Certainty::inconclusive);
}

void CheckOtherImpl::checkUnreachableSwitchCase()
{
if (!mSettings.severity.isEnabled(Severity::style))
return;

logChecker("CheckOther::checkUnreachableSwitchCase"); // style

const SymbolDatabase* symbolDatabase = mTokenizer->getSymbolDatabase();

for (const Scope& scope : symbolDatabase->scopeList) {
if (scope.type != ScopeType::eSwitch || !scope.bodyStart)
continue;
const Token* rpar = scope.bodyStart->previous();
if (!Token::simpleMatch(rpar, ")"))
continue;
const Token* lpar = rpar->link();
if (!lpar)
continue;
const Token* condition = lpar->astOperand2();
if (!condition)
continue;
const ValueFlow::Value* switchValue =
condition->getKnownValue(ValueFlow::Value::ValueType::INT);
if (!switchValue)
continue;

for (const Token* tok = scope.bodyStart->next();
tok && tok != scope.bodyEnd;
tok = tok->next()) {

// Do not inspect cases belonging to a nested switch.
if (Token::simpleMatch(tok, "{") &&
tok->scope()->type == ScopeType::eSwitch) {
tok = tok->link();
continue;
}
if (!Token::simpleMatch(tok, "case"))
continue;
const Token* caseExpression = tok->astOperand1();
if (!caseExpression)
continue;
const ValueFlow::Value* caseValue =
caseExpression->getKnownValue(ValueFlow::Value::ValueType::INT);
if (!caseValue)
continue;
if (switchValue->intvalue == caseValue->intvalue)
continue;
unreachableSwitchCaseError(tok, caseExpression->expressionString());
}
}
}

void CheckOtherImpl::unreachableSwitchCaseError(const Token* tok, const std::string& caseExpression)
{
reportError(tok, Severity::style, "unreachableSwitchCase",
"Switch case '" + caseExpression +
"' can never be selected because the switch condition has a known value.",
CWE561, Certainty::normal);
}

static bool isNestedInSwitch(const Scope* scope)
{
while (scope) {
Expand Down Expand Up @@ -4820,6 +4880,7 @@ void CheckOther::runChecks(const Tokenizer &tokenizer, ErrorLogger& errorLogger)
checkOther.checkCharVariable();
checkOther.redundantBitwiseOperationInSwitchError();
checkOther.checkSuspiciousCaseInSwitch();
checkOther.checkUnreachableSwitchCase();
checkOther.checkDuplicateBranch();
checkOther.checkDuplicateExpression();
checkOther.checkRedundantAssignment();
Expand Down Expand Up @@ -4907,6 +4968,7 @@ void CheckOther::getErrorMessages(ErrorLogger& errorLogger, const Settings &sett
c.duplicateExpressionTernaryError(nullptr, ErrorPath{});
c.duplicateBreakError(nullptr, false);
c.unreachableCodeError(nullptr, nullptr, false);
c.unreachableSwitchCaseError(nullptr, "case");
c.unsignedLessThanZeroError(nullptr, nullptr, "varname");
c.unsignedPositiveError(nullptr, nullptr, "varname");
c.pointerLessThanZeroError(nullptr, nullptr);
Expand Down
4 changes: 4 additions & 0 deletions lib/checkother.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,9 @@ class CPPCHECKLIB CheckOtherImpl : public CheckImpl {
/** @brief %Check for code like 'case A||B:'*/
void checkSuspiciousCaseInSwitch();

/** @brief %Check for case labels that cannot be selected */
void checkUnreachableSwitchCase();

/** @brief %Check for objects that are destroyed immediately */
void checkMisusedScopedObject();

Expand Down Expand Up @@ -290,6 +293,7 @@ class CPPCHECKLIB CheckOtherImpl : public CheckImpl {
void redundantCopyError(const Token *tok1, const Token* tok2, const std::string& var);
void redundantBitwiseOperationInSwitchError(const Token *tok, const std::string &varname);
void suspiciousCaseInSwitchError(const Token* tok, const std::string& operatorString);
void unreachableSwitchCaseError(const Token* tok, const std::string& caseExpression);
void selfAssignmentError(const Token *tok, const std::string &varname);
void misusedScopeObjectError(const Token *tok, const std::string &varname, bool isAssignment = false);
void duplicateBranchError(const Token *tok1, const Token *tok2, ErrorPath errors);
Expand Down
56 changes: 56 additions & 0 deletions test/testother.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,7 @@ class TestOther : public TestFixture {
TEST_CASE(switchRedundantOperationTest);
TEST_CASE(switchRedundantBitwiseOperationTest);
TEST_CASE(unreachableCode);
TEST_CASE(unreachableSwitchCase); // #8442
TEST_CASE(redundantContinue);

TEST_CASE(suspiciousCase);
Expand Down Expand Up @@ -6348,6 +6349,61 @@ class TestOther : public TestFixture {
ASSERT_EQUALS("", errout_str());
}

void unreachableSwitchCase() {
check("enum T { A, B};\n"
"void f(const T &t) {\n"
" if (t == A) {\n"
" switch (t) {\n"
" case A:\n"
" break;\n"
" case B:\n"
" break;\n"
" }\n"
" }\n"
"}\n");
ASSERT_EQUALS("[test.cpp:7:9]: (style) Switch case 'B' can never be selected because the switch condition has a known value. [unreachableSwitchCase]\n", errout_str());

check("void f(int t) {\n"
" if (t == 0) {\n"
" switch (t) {\n"
" case 0:\n"
" break;\n"
" case 1:\n"
" break;\n"
" }\n"
" }\n"
"}\n");
ASSERT_EQUALS("[test.cpp:6:9]: (style) Switch case '1' can never be selected because the switch condition has a known value. [unreachableSwitchCase]\n", errout_str());

check("void f(int t) {\n"
" switch (t) {\n"
" case 0:\n"
" break;\n"
" case 1:\n"
" break;\n"
" }\n"
"}\n");
ASSERT_EQUALS("", errout_str());

check("void f(int x, int y) {\n"
" if (x == 0) {\n"
" switch (x) {\n"
" case 0:\n"
" switch (y) {\n"
" case 1:\n"
" break;\n"
" case 2:\n"
" break;\n"
" }\n"
" break;\n"
" case 1:\n"
" break;\n"
" }\n"
" }\n"
"}\n");
ASSERT_EQUALS("[test.cpp:12:9]: (style) Switch case '1' can never be selected because the switch condition has a known value. [unreachableSwitchCase]\n", errout_str());
}

void redundantContinue() {
check("void f() {\n" // #11195
" for (int i = 0; i < 10; ++i) {\n"
Expand Down