Fix Ticket #8442: detect unselectable switch cases

Author: JeewoongKim

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);

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);

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"