Fix ticket 8442: detect unselectable switch cases (#8815)

ValueFlow already knows the switch condition value in this case,
but the case labels were not checked against it.

This change uses that value to report case labels that cannot be
selected when the switch condition has a known integer value.

Added regression tests for enum and integer cases, unknown
switch values, and nested switches.

Author: JeewoongKim

AUTHORS

@@ -192,6 +192,7 @@ János Maros
 Jay Sigbrandt
 Jean-François Deverge
 Jedrzej Klocek
+Jeewoong Kim
 Jens Bäckman
 Jens Yllman
 Jérémy Lefaure

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(), MathLib::toString(switchValue->intvalue));
+        }
+    }
+}
+
+void CheckOtherImpl::unreachableSwitchCaseError(const Token* tok, const std::string& caseExpression, const std::string& switchValue)
+{
+    reportError(tok, Severity::style, "unreachableSwitchCase",
+                "Switch case '" + caseExpression +
+                "' can never be selected because the switch condition is known to be " + switchValue + ".",
+                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", "0");
     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, const std::string& switchValue);
     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);

releasenotes.txt

@@ -8,6 +8,7 @@ New checks:
 - Warn when feof() is used as a while loop condition (wrongfeofUsage).
 - ftell() result is unspecified when file is opened in mode "t".
 - Detect when an STL algorithm such as std::copy, std::equal, std::transform, etc. accesses more elements through an iterator than are available in the container (algorithmOutOfBounds).
+- Detect switch cases that cannot be selected when the switch condition has a known value (unreachableSwitchCase).
 
 C/C++ support:
 -

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 is known to be 0. [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 is known to be 0. [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 is known to be 0. [unreachableSwitchCase]\n", errout_str());
+    }
+
     void redundantContinue() {
         check("void f() {\n" // #11195
               "    for (int i = 0; i < 10; ++i) {\n"