Remove valueFlowFwdAnalysis and update valueFlowAfterAssign to handle expressions (#3074)

lib/forwardanalyzer.cpp

@@ -13,16 +13,18 @@ struct ForwardTraversal {
     enum class Progress { Continue, Break, Skip };
     enum class Terminate { None, Bail, Escape, Modified, Inconclusive, Conditional };
     ForwardTraversal(const ValuePtr<Analyzer>& analyzer, const Settings* settings)
-        : analyzer(analyzer), settings(settings), actions(Analyzer::Action::None), analyzeOnly(false)
+        : analyzer(analyzer), settings(settings), actions(Analyzer::Action::None), analyzeOnly(false), analyzeTerminate(false)
     {}
     ValuePtr<Analyzer> analyzer;
     const Settings* settings;
     Analyzer::Action actions;
     bool analyzeOnly;
+    bool analyzeTerminate;
     Terminate terminate = Terminate::None;
 
-    Progress Break(Terminate t = Terminate::None) {
+    Progress Break(Terminate t = Terminate::None)
-        if (!analyzeOnly && t != Terminate::None)
+    {
+        if ((!analyzeOnly || analyzeTerminate) && t != Terminate::None)
             terminate = t;
         return Progress::Break;
     }
@@ -224,11 +226,21 @@ struct ForwardTraversal {
         ft.updateRange(start, end);
     }
 
-    std::vector<ForwardTraversal> forkScope(Token* endBlock, bool isModified = false) {
+    ForwardTraversal forkScope(Token* endBlock, bool analyze = false) const {
+        ForwardTraversal ft = *this;
+        ft.analyzer->forkScope(endBlock);
+        if (analyze) {
+            ft.analyzeOnly = true;
+            ft.analyzeTerminate = true;
+        }
+        ft.updateRange(endBlock->link(), endBlock);
+        return ft;
+    }
+
+    std::vector<ForwardTraversal> tryForkScope(Token* endBlock, bool isModified = false)
+    {
         if (analyzer->updateScope(endBlock, isModified)) {
-            ForwardTraversal ft = *this;
+            ForwardTraversal ft = forkScope(endBlock);
-            ft.analyzer->forkScope(endBlock);
-            ft.updateRange(endBlock->link(), endBlock);
             return {ft};
         }
         return std::vector<ForwardTraversal> {};
@@ -238,6 +250,18 @@ struct ForwardTraversal {
         return Token::findsimplematch(endBlock->link(), "goto", endBlock);
     }
 
+    bool hasInnerReturnScope(const Token* start, const Token* end) const {
+        for(const Token* tok=start;tok != end;tok = tok->previous()) {
+            if (Token::simpleMatch(tok, "}")) {
+                const Token* ftok = nullptr;
+                bool r = isReturnScope(tok, &settings->library, &ftok);
+                if (r)
+                    return true;
+            }
+        }
+        return false;
+    }
+
     bool isEscapeScope(const Token* endBlock, bool& unknown) {
         const Token* ftok = nullptr;
         bool r = isReturnScope(endBlock, &settings->library, &ftok);
@@ -259,7 +283,7 @@ struct ForwardTraversal {
 
     Analyzer::Action checkScope(Token* endBlock) {
         Analyzer::Action a = analyzeScope(endBlock);
-        forkScope(endBlock, a.isModified());
+        tryForkScope(endBlock, a.isModified());
         return a;
     }
 
@@ -268,6 +292,31 @@ struct ForwardTraversal {
         return a;
     }
 
+    bool checkBranch(Branch& branch, Token* endBlock) {
+        Analyzer::Action a = analyzeScope(endBlock);
+        branch.action = a;
+        std::vector<ForwardTraversal> ft1 = tryForkScope(endBlock, a.isModified());
+        bool bail = hasGoto(endBlock);
+        if (!a.isModified() && !bail) {
+            if (ft1.empty()) {
+                // Traverse into the branch to see if there is a conditional escape
+                if (!branch.escape && hasInnerReturnScope(endBlock->previous(), endBlock->link())) {
+                    ForwardTraversal ft2 = forkScope(endBlock, true);
+                    if (ft2.terminate == Terminate::Escape) {
+                        branch.escape = true;
+                        branch.escapeUnknown = false;
+                    }
+                }
+            } else {
+                if (ft1.front().terminate == Terminate::Escape) {
+                    branch.escape = true;
+                    branch.escapeUnknown = false;
+                }
+            }
+        }
+        return bail;
+    }
+
     void continueUpdateRangeAfterLoop(std::vector<ForwardTraversal>& ftv, Token* start, const Token* endToken) {
         for (ForwardTraversal& ft : ftv) {
             // If analysis has terminated normally, then continue analysis
@@ -311,7 +360,7 @@ struct ForwardTraversal {
                 return Progress::Continue;
         }
 
-        std::vector<ForwardTraversal> ftv = forkScope(endBlock, allAnalysis.isModified());
+        std::vector<ForwardTraversal> ftv = tryForkScope(endBlock, allAnalysis.isModified());
         if (bodyAnalysis.isModified()) {
             Token* writeTok = findRange(endBlock->link(), endBlock, std::mem_fn(&Analyzer::Action::isModified));
             const Token* nextStatement = Token::findmatch(writeTok, ";|}", endBlock);
@@ -451,8 +500,7 @@ struct ForwardTraversal {
                         if (updateRange(endCond->next(), endBlock, depth - 1) == Progress::Break)
                             return Break();
                     } else if (!elseBranch.check) {
-                        thenBranch.action = checkScope(endBlock);
+                        if (checkBranch(thenBranch, endBlock))
-                        if (hasGoto(endBlock))
                             bail = true;
                     }
                     // Traverse else block
@@ -463,8 +511,7 @@ struct ForwardTraversal {
                             if (result == Progress::Break)
                                 return Break();
                         } else if (!thenBranch.check) {
-                            elseBranch.action = checkScope(endBlock->linkAt(2));
+                            if (checkBranch(elseBranch, endBlock->linkAt(2)))
-                            if (hasGoto(endBlock))
                                 bail = true;
                         }
                         tok = endBlock->linkAt(2);

lib/valueflow.cpp

@@ -2833,7 +2833,7 @@ static const Token* getEndOfVarScope(const Token* tok, const std::vector<const V
 {
     const Token* endOfVarScope = nullptr;
     for (const Variable* var : vars) {
-        if (var && var->isLocal())
+        if (var && (var->isLocal() || var->isArgument()))
             endOfVarScope = var->typeStartToken()->scope()->bodyEnd;
         else if (!endOfVarScope)
             endOfVarScope = tok->scope()->bodyEnd;
@@ -3812,25 +3812,22 @@ static void valueFlowConditionExpressions(TokenList *tokenlist, SymbolDatabase*
     }
 }
 
-static void valueFlowForwardAssign(Token * const               tok,
+static void valueFlowForwardAssign(Token* const tok,
-                                   const Variable * const      var,
+                                   const Token* expr,
+                                   std::vector<const Variable*> vars,
                                    std::list<ValueFlow::Value> values,
-                                   const bool                  constValue,
+                                   const bool init,
-                                   const bool                  init,
+                                   TokenList* const tokenlist,
-                                   TokenList * const           tokenlist,
+                                   ErrorLogger* const errorLogger,
-                                   ErrorLogger * const         errorLogger,
+                                   const Settings* const settings)
-                                   const Settings * const      settings)
 {
-    const Token * endOfVarScope = nullptr;
+    const Token* endOfVarScope = getEndOfVarScope(tok, vars);
-    if (var->isLocal())
-        endOfVarScope = var->scope()->bodyEnd;
-    if (!endOfVarScope)
-        endOfVarScope = tok->scope()->bodyEnd;
     if (std::any_of(values.begin(), values.end(), std::mem_fn(&ValueFlow::Value::isLifetimeValue))) {
         valueFlowForwardLifetime(tok, tokenlist, errorLogger, settings);
         values.remove_if(std::mem_fn(&ValueFlow::Value::isLifetimeValue));
     }
-    if (!var->isPointer() && !var->isSmartPointer())
+    if (std::all_of(
+            vars.begin(), vars.end(), [&](const Variable* var) { return !var->isPointer() && !var->isSmartPointer(); }))
         values.remove_if(std::mem_fn(&ValueFlow::Value::isTokValue));
     if (tok->astParent()) {
         for (ValueFlow::Value& value : values) {
@@ -3839,7 +3836,7 @@ static void valueFlowForwardAssign(Token * const               tok,
         }
     }
 
-    if (tokenlist->isCPP() && Token::Match(var->typeStartToken(), "bool|_Bool")) {
+    if (tokenlist->isCPP() && vars.size() == 1 && Token::Match(vars.front()->typeStartToken(), "bool|_Bool")) {
         std::list<ValueFlow::Value>::iterator it;
         for (it = values.begin(); it != values.end(); ++it) {
             if (it->isIntValue())
@@ -3850,7 +3847,7 @@ static void valueFlowForwardAssign(Token * const               tok,
     }
 
     // Static variable initialisation?
-    if (var->isStatic() && init)
+    if (vars.size() == 1 && vars.front()->isStatic() && init)
         lowerToPossible(values);
 
     // Skip RHS
@@ -3862,30 +3859,24 @@ static void valueFlowForwardAssign(Token * const               tok,
                      values.end(),
                      std::back_inserter(tokvalues),
                      std::mem_fn(&ValueFlow::Value::isTokValue));
-        valueFlowForwardVariable(const_cast<Token*>(nextExpression),
+        valueFlowForward(const_cast<Token*>(nextExpression), endOfVarScope, expr, values, tokenlist, settings);
-                                 endOfVarScope,
-                                 var,
-                                 var->declarationId(),
-                                 tokvalues,
-                                 constValue,
-                                 false,
-                                 tokenlist,
-                                 errorLogger,
-                                 settings);
         values.remove_if(std::mem_fn(&ValueFlow::Value::isTokValue));
     }
     for (ValueFlow::Value& value:values)
         value.tokvalue = tok;
-    valueFlowForwardVariable(const_cast<Token*>(nextExpression),
+    valueFlowForward(const_cast<Token*>(nextExpression), endOfVarScope, expr, values, tokenlist, settings);
-                             endOfVarScope,
+}
-                             var,
+
-                             var->declarationId(),
+static void valueFlowForwardAssign(Token* const tok,
-                             values,
+                                   const Variable* const var,
-                             constValue,
+                                   const std::list<ValueFlow::Value>& values,
-                             false,
+                                   const bool,
-                             tokenlist,
+                                   const bool init,
-                             errorLogger,
+                                   TokenList* const tokenlist,
-                             settings);
+                                   ErrorLogger* const errorLogger,
+                                   const Settings* const settings)
+{
+    valueFlowForwardAssign(tok, var->nameToken(), {var}, values, init, tokenlist, errorLogger, settings);
 }
 
 static std::list<ValueFlow::Value> truncateValues(std::list<ValueFlow::Value> values, const ValueType *valueType, const Settings *settings)
@@ -3935,7 +3926,7 @@ static void valueFlowAfterAssign(TokenList *tokenlist, SymbolDatabase* symboldat
         for (Token* tok = const_cast<Token*>(scope->bodyStart); tok != scope->bodyEnd; tok = tok->next()) {
             // Alias
             if (tok->isUnaryOp("&")) {
-                aliased.insert(tok->astOperand1()->varId());
+                aliased.insert(tok->astOperand1()->exprId());
                 continue;
             }
 
@@ -3944,14 +3935,12 @@ static void valueFlowAfterAssign(TokenList *tokenlist, SymbolDatabase* symboldat
                 continue;
 
             // Lhs should be a variable
-            if (!tok->astOperand1() || !tok->astOperand1()->varId())
+            if (!tok->astOperand1() || !tok->astOperand1()->exprId())
                 continue;
-            const int varid = tok->astOperand1()->varId();
+            const int exprid = tok->astOperand1()->exprId();
-            if (aliased.find(varid) != aliased.end())
+            if (aliased.find(exprid) != aliased.end())
-                continue;
-            const Variable *var = tok->astOperand1()->variable();
-            if (!var || (!var->isLocal() && !var->isGlobal() && !var->isArgument()))
                 continue;
+            std::vector<const Variable*> vars = getLHSVariables(tok);
 
             // Rhs values..
             if (!tok->astOperand2() || tok->astOperand2()->values().empty())
@@ -3976,9 +3965,9 @@ static void valueFlowAfterAssign(TokenList *tokenlist, SymbolDatabase* symboldat
             });
             if (values.empty())
                 continue;
-            const bool constValue = isLiteralNumber(tok->astOperand2(), tokenlist->isCPP());
+            const bool init = vars.size() == 1 && vars.front()->nameToken() == tok->astOperand1();
-            const bool init = var->nameToken() == tok->astOperand1();
+            valueFlowForwardAssign(
-            valueFlowForwardAssign(tok->astOperand2(), var, values, constValue, init, tokenlist, errorLogger, settings);
+                tok->astOperand2(), tok->astOperand1(), vars, values, init, tokenlist, errorLogger, settings);
         }
     }
 }
@@ -6261,31 +6250,6 @@ struct ContainerConditionHandler : ConditionHandler {
     }
 };
 
-static void valueFlowFwdAnalysis(const TokenList *tokenlist, const Settings *settings)
-{
-    for (const Token *tok = tokenlist->front(); tok; tok = tok->next()) {
-        if (Token::simpleMatch(tok, "for ("))
-            tok = tok->linkAt(1);
-        if (tok->str() != "=" || !tok->astOperand1() || !tok->astOperand2())
-            continue;
-        // Skip variables
-        if (tok->astOperand1()->variable())
-            continue;
-        if (!tok->scope()->isExecutable())
-            continue;
-        if (!tok->astOperand2()->hasKnownIntValue())
-            continue;
-        ValueFlow::Value v(tok->astOperand2()->values().front());
-        v.errorPath.emplace_back(tok, tok->astOperand1()->expressionString() + " is assigned value " + MathLib::toString(v.intvalue));
-        const Token *startToken = tok->findExpressionStartEndTokens().second->next();
-        const Scope *functionScope = tok->scope();
-        while (functionScope->nestedIn && functionScope->nestedIn->isExecutable())
-            functionScope = functionScope->nestedIn;
-        const Token *endToken = functionScope->bodyEnd;
-        valueFlowForwardExpression(const_cast<Token*>(startToken), endToken, tok->astOperand1(), {v}, tokenlist, settings);
-    }
-}
-
 static void valueFlowDynamicBufferSize(TokenList *tokenlist, SymbolDatabase *symboldatabase, ErrorLogger *errorLogger, const Settings *settings)
 {
     for (const Scope *functionScope : symboldatabase->functionScopes) {
@@ -6663,7 +6627,6 @@ void ValueFlow::setValues(TokenList *tokenlist, SymbolDatabase* symboldatabase,
     valueFlowBitAnd(tokenlist);
     valueFlowSameExpressions(tokenlist);
     valueFlowConditionExpressions(tokenlist, symboldatabase, errorLogger, settings);
-    valueFlowFwdAnalysis(tokenlist, settings);
 
     std::size_t values = 0;
     std::size_t n = 4;

test/testnullpointer.cpp

@@ -110,6 +110,7 @@ private:
         TEST_CASE(nullpointer67); // #10062
         TEST_CASE(nullpointer68);
         TEST_CASE(nullpointer69);         // #8143
+        TEST_CASE(nullpointer70);
         TEST_CASE(nullpointer_addressOf); // address of
         TEST_CASE(nullpointerSwitch); // #2626
         TEST_CASE(nullpointer_cast); // #4692
@@ -2177,6 +2178,41 @@ private:
         ASSERT_EQUALS("", errout.str());
     }
 
+    void nullpointer70() {
+        check("struct Token {\n"
+              "    const Token* nextArgument() const;\n"
+              "    const Token* next() const;\n"
+              "    int varId() const;\n"
+              "};\n"
+              "int f(const Token *first, const Token* second) const {\n"
+              "    first = first->nextArgument();\n"
+              "    if (first)\n"
+              "        first = first->next();\n"
+              "    if (second->next()->varId() == 0) {\n"
+              "        second = second->nextArgument();\n"
+              "        if (!first || !second)\n"
+              "            return 0;\n"
+              "    } else if (!first) {\n"
+              "        return 0;\n"
+              "    }\n"
+              "    return first->varId();\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+
+        check("struct Token {\n"
+              "    const Token* nextArgument() const;\n"
+              "    const Token* next() const;\n"
+              "    int varId() const;\n"
+              "};\n"
+              "int f(const Token *first) const {\n"
+              "    first = first->nextArgument();\n"
+              "    if (first)\n"
+              "        first = first->next();\n"
+              "    first->str();\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:8] -> [test.cpp:10]: (warning) Either the condition 'first' is redundant or there is possible null pointer dereference: first.\n", errout.str());
+    }
+    
     void nullpointer_addressOf() { // address of
         check("void f() {\n"
               "  struct X *x = 0;\n"