diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 7f22e531230..9bf4512add9 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -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) { @@ -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(); @@ -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); diff --git a/lib/checkother.h b/lib/checkother.h index caf9f4a9e57..3de7f046089 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -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(); @@ -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); diff --git a/test/testother.cpp b/test/testother.cpp index 3c301a3fc3c..ebcab503565 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -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); @@ -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"