Fix issue 10141: Errors with ref assignment (duplicateValueTenary and knownEmptyContainer) (#3093)

lib/astutils.cpp

@@ -30,6 +30,7 @@
 #include "valueflow.h"
 
 #include <algorithm>
+#include <deque>
 #include <functional>
 #include <iterator>
 #include <list>
@@ -751,73 +752,105 @@ static void followVariableExpressionError(const Token *tok1, const Token *tok2,
     errors->push_back(item);
 }
 
-static const Token* followReferences(const Token* tok, ErrorPath* errors, int depth = 20)
+std::vector<ReferenceToken> followAllReferences(const Token* tok, bool inconclusive, ErrorPath errors, int depth)
 {
+    struct ReferenceTokenLess {
+        bool operator()(const ReferenceToken& x, const ReferenceToken& y) const
+        {
+            return x.token < y.token;
+        }
+    };
     if (!tok)
-        return nullptr;
+        return std::vector<ReferenceToken>{};
     if (depth < 0)
-        return tok;
+        return {{tok, std::move(errors)}};
     const Variable *var = tok->variable();
     if (var && var->declarationId() == tok->varId()) {
         if (var->nameToken() == tok) {
-            return tok;
+            return {{tok, std::move(errors)}};
         } else if (var->isReference() || var->isRValueReference()) {
             if (!var->declEndToken())
-                return tok;
+                return {{tok, std::move(errors)}};
             if (var->isArgument()) {
-                if (errors)
+                errors.emplace_back(var->declEndToken(), "Passed to reference.");
-                    errors->emplace_back(var->declEndToken(), "Passed to reference.");
+                return {{tok, std::move(errors)}};
-                return tok;
             } else if (Token::simpleMatch(var->declEndToken(), "=")) {
-                if (errors)
+                errors.emplace_back(var->declEndToken(), "Assigned to reference.");
-                    errors->emplace_back(var->declEndToken(), "Assigned to reference.");
                 const Token *vartok = var->declEndToken()->astOperand2();
                 if (vartok == tok)
-                    return tok;
+                    return {{tok, std::move(errors)}};
                 if (vartok)
-                    return followReferences(vartok, errors, depth - 1);
+                    return followAllReferences(vartok, inconclusive, std::move(errors), depth - 1);
             } else {
-                return tok;
+                return {{tok, std::move(errors)}};
             }
         }
+    } else if (Token::simpleMatch(tok, "?") && Token::simpleMatch(tok->astOperand2(), ":")) {
+        std::set<ReferenceToken, ReferenceTokenLess> result;
+        const Token* tok2 = tok->astOperand2();
+
+        std::vector<ReferenceToken> refs;
+        refs = followAllReferences(tok2->astOperand1(), inconclusive, errors, depth - 1);
+        result.insert(refs.begin(), refs.end());
+        refs = followAllReferences(tok2->astOperand2(), inconclusive, errors, depth - 1);
+        result.insert(refs.begin(), refs.end());
+
+        if (!inconclusive && result.size() != 1)
+            return {{tok, std::move(errors)}};
+
+        if (!result.empty())
+            return std::vector<ReferenceToken>(result.begin(), result.end());
+
     } else if (Token::Match(tok->previous(), "%name% (")) {
         const Function *f = tok->previous()->function();
         if (f) {
             if (!Function::returnsReference(f))
-                return tok;
+                return {{tok, std::move(errors)}};
-            std::set<const Token*> result;
+            std::set<ReferenceToken, ReferenceTokenLess> result;
-            ErrorPath errorPath;
             for (const Token* returnTok : Function::findReturns(f)) {
                 if (returnTok == tok)
                     continue;
-                const Token* argvarTok = followReferences(returnTok, errors, depth - 1);
+                std::vector<ReferenceToken> argvarRt = followAllReferences(returnTok, inconclusive, errors, depth - 1);
-                const Variable* argvar = argvarTok->variable();
+                for(const ReferenceToken& rt:followAllReferences(returnTok, inconclusive, errors, depth - 1)) {
-                if (!argvar)
+                    const Variable* argvar = rt.token->variable();
-                    return tok;
+                    if (!argvar)
-                if (argvar->isArgument() && (argvar->isReference() || argvar->isRValueReference())) {
+                        return {{tok, std::move(errors)}};
-                    int n = getArgumentPos(argvar, f);
+                    if (argvar->isArgument() && (argvar->isReference() || argvar->isRValueReference())) {
-                    if (n < 0)
+                        int n = getArgumentPos(argvar, f);
-                        return tok;
+                        if (n < 0)
-                    std::vector<const Token*> args = getArguments(tok->previous());
+                            return {{tok, std::move(errors)}};
-                    if (n >= args.size())
+                        std::vector<const Token*> args = getArguments(tok->previous());
-                        return tok;
+                        if (n >= args.size())
-                    const Token* argTok = args[n];
+                            return {{tok, std::move(errors)}};
-                    ErrorPath er;
+                        const Token* argTok = args[n];
-                    er.emplace_back(returnTok, "Return reference.");
+                        ErrorPath er = errors;
-                    er.emplace_back(tok->previous(), "Called function passing '" + argTok->expressionString() + "'.");
+                        er.emplace_back(returnTok, "Return reference.");
-                    const Token* tok2 = followReferences(argTok, &er, depth - 1);
+                        er.emplace_back(tok->previous(), "Called function passing '" + argTok->expressionString() + "'.");
-                    errorPath = er;
+                        std::vector<ReferenceToken> refs = followAllReferences(argTok, inconclusive, std::move(er), depth - 1);
-                    result.insert(tok2);
+                        result.insert(refs.begin(), refs.end());
+                        if (!inconclusive && result.size() > 1)
+                            return {{tok, std::move(errors)}};
+                    }   
                 }
             }
-            if (result.size() == 1) {
+            if (!result.empty())
-                if (errors)
+                return std::vector<ReferenceToken>(result.begin(), result.end());
-                    errors->insert(errors->end(), errorPath.begin(), errorPath.end());
-                return *result.begin();
-            }
         }
     }
-    return tok;
+    return {{tok, std::move(errors)}};
+}
+
+const Token* followReferences(const Token* tok, ErrorPath* errors)
+{
+    if (!tok)
+        return nullptr;
+    std::vector<ReferenceToken> refs = followAllReferences(tok, false);
+    if (refs.size() == 1) {
+        if (errors)
+            *errors = refs.front().errors;
+        return refs.front().token;
+    }
+    return nullptr;
 }
 
 static bool isSameLifetime(const Token * const tok1, const Token * const tok2)

lib/astutils.h

@@ -140,6 +140,14 @@ bool precedes(const Token * tok1, const Token * tok2);
 
 bool exprDependsOnThis(const Token* expr, nonneg int depth = 0);
 
+struct ReferenceToken {
+    const Token* token;
+    ErrorPath errors;
+};
+
+std::vector<ReferenceToken> followAllReferences(const Token* tok, bool inconclusive = true, ErrorPath errors = ErrorPath{}, int depth = 20);
+const Token* followReferences(const Token* tok, ErrorPath* errors = nullptr);
+
 bool isSameExpression(bool cpp, bool macro, const Token *tok1, const Token *tok2, const Library& library, bool pure, bool followVar, ErrorPath* errors=nullptr);
 
 bool isEqualKnownValue(const Token * const tok1, const Token * const tok2);

lib/valueflow.cpp

@@ -1915,25 +1915,37 @@ struct ValueFlowAnalyzer : Analyzer {
         }
     }
 
-    virtual Action analyze(const Token* tok, Direction d) const OVERRIDE {
+    Action analyzeMatch(const Token* tok, Direction d) const {
-        if (invalid())
+        const Token* parent = tok->astParent();
-            return Action::Invalid;
+        if (astIsPointer(tok) && (Token::Match(parent, "*|[") || (parent && parent->originalName() == "->")) && getIndirect(tok) <= 0)
+            return Action::Read;
+
+        Action w = isWritable(tok, d);
+        if (w != Action::None)
+            return w;
+
+        // Check for modifications by function calls
+        return isModified(tok);
+    }
+
+    Action analyzeToken(const Token* ref, const Token* tok, Direction d, bool inconclusiveRef) const
+    {
+        if (!ref)
+            return Action::None;
+        // If its an inconclusiveRef then ref != tok
+        assert(!inconclusiveRef || ref != tok);
         bool inconclusive = false;
-        if (match(tok)) {
+        if (match(ref)) {
-            const Token* parent = tok->astParent();
+            if (inconclusiveRef) {
-            if (astIsPointer(tok) && (Token::Match(parent, "*|[") || (parent && parent->originalName() == "->")) && getIndirect(tok) <= 0)
+                Action a = isModified(tok);
-                return Action::Read | Action::Match;
+                if (a.isModified() || a.isInconclusive())
-
+                    return Action::Inconclusive;
-            // Action read = Action::Read;
+            } else {
-            Action w = isWritable(tok, d);
+                return analyzeMatch(tok, d) | Action::Match;
-            if (w != Action::None)
+            }
-                return w | Action::Match;
+        } else if (ref->isUnaryOp("*")) {
-
-            // Check for modifications by function calls
-            return isModified(tok) | Action::Match;
-        } else if (tok->isUnaryOp("*")) {
             const Token* lifeTok = nullptr;
-            for (const ValueFlow::Value& v:tok->astOperand1()->values()) {
+            for (const ValueFlow::Value& v:ref->astOperand1()->values()) {
                 if (!v.isLocalLifetimeValue())
                     continue;
                 if (lifeTok)
@@ -1946,17 +1958,37 @@ struct ValueFlowAnalyzer : Analyzer {
                     a = Action::Invalid;
                 if (Token::Match(tok->astParent(), "%assign%") && astIsLHS(tok))
                     a |= Action::Invalid;
+                if (inconclusiveRef && a.isModified())
+                    return Action::Inconclusive;
                 return a;
             }
             return Action::None;
 
-        } else if (isAlias(tok, inconclusive)) {
+        } else if (isAlias(ref, inconclusive)) {
+            inconclusive |= inconclusiveRef;
             Action a = isAliasModified(tok);
             if (inconclusive && a.isModified())
                 return Action::Inconclusive;
             else
                 return a;
-        } else if (Token::Match(tok, "%name% (") && !Token::simpleMatch(tok->linkAt(1), ") {")) {
+        }
+        return Action::None;
+    }
+
+    virtual Action analyze(const Token* tok, Direction d) const OVERRIDE {
+        if (invalid())
+            return Action::Invalid;
+        // Follow references
+        std::vector<ReferenceToken> refs = followAllReferences(tok);
+        const bool inconclusiveRefs = refs.size() != 1;
+        for(const ReferenceToken& ref:refs) {
+            Action a = analyzeToken(ref.token, tok, d, inconclusiveRefs);
+            if (a != Action::None)
+                return a;
+        }
+
+        // TODO: Check if function is pure
+        if (Token::Match(tok, "%name% (") && !Token::simpleMatch(tok->linkAt(1), ") {")) {
             // bailout: global non-const variables
             if (isGlobal()) {
                 return Action::Invalid;
@@ -4216,7 +4248,14 @@ struct ConditionHandler {
                 }
             }
 
-            Token* startTok = tok->astParent() ? tok->astParent() : tok->previous();
+            Token* startTok = nullptr;
+            if (astIsRHS(tok))
+                startTok = tok->astParent();
+            else if (astIsLHS(tok))
+                startTok = previousBeforeAstLeftmostLeaf(tok->astParent());
+            if (!startTok)
+                startTok = tok->previous();
+
             reverse(startTok, nullptr, cond.vartok, values, tokenlist, settings);
         });
     }
@@ -5789,7 +5828,7 @@ struct ContainerVariableAnalyzer : VariableAnalyzer {
             return Action::Invalid;
         if (isLikelyStreamRead(isCPP(), tok->astParent()))
             return Action::Invalid;
-        if (isContainerSizeChanged(tok))
+        if (astIsContainer(tok) && isContainerSizeChanged(tok))
             return Action::Invalid;
         return read;
     }

test/testvalueflow.cpp

@@ -4472,6 +4472,18 @@ private:
         return "";
     }
 
+    static std::string isInconclusiveContainerSizeValue(const std::list<ValueFlow::Value>& values, MathLib::bigint i) {
+        if (values.size() != 1)
+            return "values.size():" + std::to_string(values.size());
+        if (!values.front().isContainerSizeValue())
+            return "ContainerSizeValue";
+        if (!values.front().isInconclusive())
+            return "Inconclusive";
+        if (values.front().intvalue != i)
+            return "intvalue:" + std::to_string(values.front().intvalue);
+        return "";
+    }
+
     static std::string isKnownContainerSizeValue(const std::list<ValueFlow::Value> &values, MathLib::bigint i) {
         if (values.size() != 1)
             return "values.size():" + std::to_string(values.size());
@@ -5026,17 +5038,29 @@ private:
                "}\n";
         ASSERT_EQUALS(true, testValueOfXKnown(code, 4U, 0));
 
-        code = "bool f(std::string s) {\n"
+      code = "bool f(std::string s) {\n"
-               "    if (!s.empty()) {\n"
+             "    if (!s.empty()) {\n"
-               "        bool x = s == \"0\";\n"
+             "        bool x = s == \"0\";\n"
-               "        return x;\n"
+             "        return x;\n"
+             "    }\n"
+             "    return false;\n"
+             "}\n";
+      ASSERT_EQUALS(false, testValueOfXKnown(code, 4U, 0));
+      ASSERT_EQUALS(false, testValueOfXKnown(code, 4U, 1));
+      ASSERT_EQUALS(false, testValueOfXImpossible(code, 4U, 0));
+      
+      code = "bool f() {\n"
+               "    std::list<int> x1;\n"
+               "    std::list<int> x2;\n"
+               "    for (int i = 0; i < 10; ++i) {\n"
+               "        std::list<int>& x = (i < 5) ? x1 : x2;\n"
+               "        x.push_back(i);\n"
                "    }\n"
-               "    return false;\n"
+               "    return x1.empty() || x2.empty();\n"
                "}\n";
-        ASSERT_EQUALS(false, testValueOfXKnown(code, 4U, 0));
+        ASSERT_EQUALS("", isInconclusiveContainerSizeValue(tokenValues(code, "x1 . empty", ValueFlow::Value::ValueType::CONTAINER_SIZE), 0));
-        ASSERT_EQUALS(false, testValueOfXKnown(code, 4U, 1));
+        ASSERT_EQUALS("", isInconclusiveContainerSizeValue(tokenValues(code, "x2 . empty", ValueFlow::Value::ValueType::CONTAINER_SIZE), 0));
-        ASSERT_EQUALS(false, testValueOfXImpossible(code, 4U, 0));
+        
-
         code = "std::vector<int> g();\n"
                "int f(bool b) {\n"
                "    std::set<int> a;\n"