From 45de9a7d08359c87151d2d26c9ccf651fe361b4f Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Wed, 23 Feb 2022 23:50:34 -0600 Subject: [PATCH] Refactor: Use getEndOfExprScope instead of getEndOfVarScope (#3855) --- lib/astutils.cpp | 70 ++++++++++++++++++++++++++++++-- lib/astutils.h | 1 + lib/checkautovariables.cpp | 64 ----------------------------- lib/valueflow.cpp | 82 ++++++++++++++++---------------------- 4 files changed, 102 insertions(+), 115 deletions(-) diff --git a/lib/astutils.cpp b/lib/astutils.cpp index cfd3c3947..f7df99e9b 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -514,6 +514,70 @@ const Token* getParentLifetime(const Token* tok) return tok; } +static std::vector getParentMembers(const Token* tok) +{ + if (!tok) + return {}; + if (!Token::simpleMatch(tok->astParent(), ".")) + return {tok}; + const Token* parent = tok; + while (Token::simpleMatch(parent->astParent(), ".")) + parent = parent->astParent(); + std::vector result; + for (const Token* tok2 : astFlatten(parent, ".")) { + if (Token::simpleMatch(tok2, "(") && Token::simpleMatch(tok2->astOperand1(), ".")) { + std::vector sub = getParentMembers(tok2->astOperand1()); + result.insert(result.end(), sub.begin(), sub.end()); + } + result.push_back(tok2); + } + return result; +} + +const Token* getParentLifetime(bool cpp, const Token* tok, const Library* library) +{ + std::vector members = getParentMembers(tok); + if (members.size() < 2) + return tok; + // Find the first local variable or temporary + auto it = std::find_if(members.rbegin(), members.rend(), [&](const Token* tok2) { + const Variable* var = tok2->variable(); + if (var) { + return var->isLocal() || var->isArgument(); + } else { + return isTemporary(cpp, tok2, library); + } + }); + if (it == members.rend()) + return tok; + // If any of the submembers are borrowed types then stop + if (std::any_of(it.base() - 1, members.end() - 1, [&](const Token* tok2) { + if (astIsPointer(tok2) || astIsContainerView(tok2) || astIsIterator(tok2)) + return true; + if (!astIsUniqueSmartPointer(tok2)) { + if (astIsSmartPointer(tok2)) + return true; + const Token* dotTok = tok2->next(); + if (!Token::simpleMatch(dotTok, ".")) { + const Token* endTok = nextAfterAstRightmostLeaf(tok2); + if (!endTok) + dotTok = tok2->next(); + else if (Token::simpleMatch(endTok, ".")) + dotTok = endTok; + else if (Token::simpleMatch(endTok->next(), ".")) + dotTok = endTok->next(); + } + // If we are dereferencing the member variable then treat it as borrowed + if (Token::simpleMatch(dotTok, ".") && dotTok->originalName() == "->") + return true; + } + const Variable* var = tok2->variable(); + return var && var->isReference(); + })) + return nullptr; + return *it; +} + bool astIsLHS(const Token* tok) { if (!tok) @@ -2708,9 +2772,9 @@ const Token* getLHSVariableToken(const Token* tok) if (tok->astOperand1()->varId() > 0) return tok->astOperand1(); const Token* vartok = getLHSVariableRecursive(tok->astOperand1()); - if (!vartok) - return tok->astOperand1(); - return vartok; + if (vartok && vartok->variable() && vartok->variable()->nameToken() == vartok) + return vartok; + return tok->astOperand1(); } const Token* findAllocFuncCallToken(const Token *expr, const Library &library) diff --git a/lib/astutils.h b/lib/astutils.h index 0c7515aa5..14ff75603 100644 --- a/lib/astutils.h +++ b/lib/astutils.h @@ -149,6 +149,7 @@ const Token* astParentSkipParens(const Token* tok); const Token* getParentMember(const Token * tok); const Token* getParentLifetime(const Token* tok); +const Token* getParentLifetime(bool cpp, const Token* tok, const Library* library); bool astIsLHS(const Token* tok); bool astIsRHS(const Token* tok); diff --git a/lib/checkautovariables.cpp b/lib/checkautovariables.cpp index 1642abea0..a33090d08 100644 --- a/lib/checkautovariables.cpp +++ b/lib/checkautovariables.cpp @@ -520,70 +520,6 @@ static bool isAssignedToNonLocal(const Token* tok) return !var->isLocal() || var->isStatic(); } -static std::vector getParentMembers(const Token* tok) -{ - if (!tok) - return {}; - if (!Token::simpleMatch(tok->astParent(), ".")) - return {tok}; - const Token* parent = tok; - while (Token::simpleMatch(parent->astParent(), ".")) - parent = parent->astParent(); - std::vector result; - for (const Token* tok2 : astFlatten(parent, ".")) { - if (Token::simpleMatch(tok2, "(") && Token::simpleMatch(tok2->astOperand1(), ".")) { - std::vector sub = getParentMembers(tok2->astOperand1()); - result.insert(result.end(), sub.begin(), sub.end()); - } - result.push_back(tok2); - } - return result; -} - -static const Token* getParentLifetime(bool cpp, const Token* tok, const Library* library) -{ - std::vector members = getParentMembers(tok); - if (members.size() < 2) - return tok; - // Find the first local variable or temporary - auto it = std::find_if(members.rbegin(), members.rend(), [&](const Token* tok2) { - const Variable* var = tok2->variable(); - if (var) { - return var->isLocal() || var->isArgument(); - } else { - return isTemporary(cpp, tok2, library); - } - }); - if (it == members.rend()) - return tok; - // If any of the submembers are borrowed types then stop - if (std::any_of(it.base() - 1, members.end() - 1, [&](const Token* tok2) { - if (astIsPointer(tok2) || astIsContainerView(tok2) || astIsIterator(tok2)) - return true; - if (!astIsUniqueSmartPointer(tok2)) { - if (astIsSmartPointer(tok2)) - return true; - const Token* dotTok = tok2->next(); - if (!Token::simpleMatch(dotTok, ".")) { - const Token* endTok = nextAfterAstRightmostLeaf(tok2); - if (!endTok) - dotTok = tok2->next(); - else if (Token::simpleMatch(endTok, ".")) - dotTok = endTok; - else if (Token::simpleMatch(endTok->next(), ".")) - dotTok = endTok->next(); - } - // If we are dereferencing the member variable then treat it as borrowed - if (Token::simpleMatch(dotTok, ".") && dotTok->originalName() == "->") - return true; - } - const Variable* var = tok2->variable(); - return var && var->isReference(); - })) - return nullptr; - return *it; -} - void CheckAutoVariables::checkVarLifetimeScope(const Token * start, const Token * end) { const bool printInconclusive = mSettings->certainty.isEnabled(Certainty::inconclusive); diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index fad16a45e..b18e0d933 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -2168,13 +2168,19 @@ struct ValueFlowAnalyzer : Analyzer { const ValueFlow::Value* value = getValue(tok); if (!value) return Action::None; - if (!(value->isIntValue() || value->isFloatValue() || value->isSymbolicValue())) + if (!(value->isIntValue() || value->isFloatValue() || value->isSymbolicValue() || value->isLifetimeValue())) return Action::None; const Token* parent = tok->astParent(); // Only if its invertible if (value->isImpossible() && !Token::Match(parent, "+=|-=|*=|++|--")) return Action::None; - + if (value->isLifetimeValue()) { + if (value->lifetimeKind != ValueFlow::Value::LifetimeKind::Iterator) + return Action::None; + if (!Token::Match(parent, "++|--|+=")) + return Action::None; + return Action::Read | Action::Write; + } if (parent && parent->isAssignmentOp() && astIsLHS(tok) && parent->astOperand2()->hasKnownValue()) { const Token* rhs = parent->astOperand2(); @@ -2206,6 +2212,9 @@ struct ValueFlowAnalyzer : Analyzer { return; if (!tok->astParent()) return; + // Lifetime value doesn't change + if (value->isLifetimeValue()) + return; if (tok->astParent()->isAssignmentOp()) { const ValueFlow::Value* rhsValue = tok->astParent()->astOperand2()->getKnownValue(ValueFlow::Value::ValueType::INT); @@ -3459,43 +3468,13 @@ const Token* getEndOfExprScope(const Token* tok, const Scope* defaultScope, bool return end; } -static const Token* getEndOfVarScope(const Token* tok, const std::vector& vars) -{ - const Token* endOfVarScope = nullptr; - for (const Variable* var : vars) { - const Scope *varScope = nullptr; - if (var && (var->isLocal() || var->isArgument()) && var->typeStartToken()->scope()->type != Scope::eNamespace) - varScope = var->typeStartToken()->scope(); - else if (!endOfVarScope) { - varScope = tok->scope(); - // A "local member" will be a expression like foo.x where foo is a local variable. - // A "global member" will be a member that belongs to a global object. - const bool globalMember = vars.size() == 1; // <- could check if it's a member here also but it seems redundant - if (var && (var->isGlobal() || var->isNamespace() || globalMember)) { - // Global variable => end of function - while (varScope->isLocal()) - varScope = varScope->nestedIn; - } - } - if (varScope && (!endOfVarScope || precedes(endOfVarScope, varScope->bodyEnd))) { - endOfVarScope = varScope->bodyEnd; - } - } - return endOfVarScope; -} - static void valueFlowForwardLifetime(Token * tok, TokenList *tokenlist, ErrorLogger *errorLogger, const Settings *settings) { // Forward lifetimes to constructed variable if (Token::Match(tok->previous(), "%var% {")) { std::list values = tok->values(); values.remove_if(&isNotLifetimeValue); - valueFlowForward(nextAfterAstRightmostLeaf(tok), - getEndOfVarScope(tok, {tok->variable()}), - tok->previous(), - values, - tokenlist, - settings); + valueFlowForward(nextAfterAstRightmostLeaf(tok), getEndOfExprScope(tok), tok->previous(), values, tokenlist, settings); return; } Token *parent = tok->astParent(); @@ -3512,9 +3491,9 @@ static void valueFlowForwardLifetime(Token * tok, TokenList *tokenlist, ErrorLog if (!isLifetimeBorrowed(parent->astOperand2(), settings)) return; - std::vector vars = getLHSVariables(parent); + const Token* expr = getLHSVariableToken(parent); - const Token* endOfVarScope = getEndOfVarScope(tok, vars); + const Token* endOfVarScope = getEndOfExprScope(expr); // Only forward lifetime values std::list values = parent->astOperand2()->values(); @@ -3523,23 +3502,30 @@ static void valueFlowForwardLifetime(Token * tok, TokenList *tokenlist, ErrorLog // Skip RHS const Token *nextExpression = nextAfterAstRightmostLeaf(parent); - if (Token::Match(parent->astOperand1(), ".|[|(") && parent->astOperand1()->exprId() > 0) { - valueFlowForwardExpression( - const_cast(nextExpression), endOfVarScope, parent->astOperand1(), values, tokenlist, settings); + if (expr->exprId() > 0) { + valueFlowForwardExpression(const_cast(nextExpression), + endOfVarScope->next(), + expr, + values, + tokenlist, + settings); for (ValueFlow::Value& val : values) { if (val.lifetimeKind == ValueFlow::Value::LifetimeKind::Address) val.lifetimeKind = ValueFlow::Value::LifetimeKind::SubObject; } - } - for (const Variable* var : vars) { - valueFlowForward( - const_cast(nextExpression), endOfVarScope, var->nameToken(), values, tokenlist, settings); - - if (tok->astTop() && Token::simpleMatch(tok->astTop()->previous(), "for (") && - Token::simpleMatch(tok->astTop()->link(), ") {")) { - Token* start = tok->astTop()->link()->next(); - valueFlowForward(start, start->link(), var->nameToken(), values, tokenlist, settings); + // TODO: handle `[` + if (Token::simpleMatch(parent->astOperand1(), ".")) { + const Token* parentLifetime = + getParentLifetime(tokenlist->isCPP(), parent->astOperand1()->astOperand2(), &settings->library); + if (parentLifetime && parentLifetime->exprId() > 0) { + valueFlowForward(const_cast(nextExpression), + endOfVarScope, + parentLifetime, + values, + tokenlist, + settings); + } } } // Constructor @@ -4915,7 +4901,7 @@ static void valueFlowForwardAssign(Token* const tok, { if (Token::simpleMatch(tok->astParent(), "return")) return; - const Token* endOfVarScope = getEndOfVarScope(tok, vars); + const Token* endOfVarScope = getEndOfExprScope(expr); 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));