From cc2bc7408418eddd626f36010b1e86a1f1a3c7f8 Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Sat, 5 Sep 2020 00:56:01 -0500 Subject: [PATCH] Track lifetime for lambdas with explicit capture (#2776) --- lib/astutils.cpp | 4 +++ lib/tokenlist.cpp | 10 ++++++ lib/valueflow.cpp | 69 +++++++++++++++++++++++++++++++------- test/testautovariables.cpp | 44 ++++++++++++++++++++++++ test/testtokenize.cpp | 19 ++++++----- 5 files changed, 124 insertions(+), 22 deletions(-) diff --git a/lib/astutils.cpp b/lib/astutils.cpp index 71ca69f4b..06728799e 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -299,6 +299,10 @@ static T* nextAfterAstRightmostLeafGeneric(T* tok) if (!rightmostLeaf || !rightmostLeaf->astOperand1()) return nullptr; do { + if (const Token* lam = findLambdaEndToken(rightmostLeaf)) { + rightmostLeaf = lam; + break; + } if (rightmostLeaf->astOperand2()) rightmostLeaf = rightmostLeaf->astOperand2(); else diff --git a/lib/tokenlist.cpp b/lib/tokenlist.cpp index abe898bd4..efd17ed50 100644 --- a/lib/tokenlist.cpp +++ b/lib/tokenlist.cpp @@ -911,6 +911,16 @@ static void compilePrecedence2(Token *&tok, AST_state& state) // - Compile the content of the lambda function as separate tree (this is done later) // this must be consistent with isLambdaCaptureList Token* const squareBracket = tok; + // Parse arguments in the capture list + if (tok->strAt(1) != "]") { + Token* tok2 = tok->next(); + AST_state state2(state.cpp); + compileExpression(tok2, state2); + if (!state2.op.empty()) { + squareBracket->astOperand2(state2.op.top()); + } + } + if (Token::simpleMatch(squareBracket->link(), "] (")) { Token* const roundBracket = squareBracket->link()->next(); Token* curlyBracket = roundBracket->link()->next(); diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index bec2614aa..d51c0ca0b 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -3706,8 +3706,12 @@ static void valueFlowLifetimeConstructor(Token* tok, TokenList* tokenlist, Error } struct Lambda { + enum class Capture { + ByValue, + ByReference + }; explicit Lambda(const Token * tok) - : capture(nullptr), arguments(nullptr), returnTok(nullptr), bodyTok(nullptr) { + : capture(nullptr), arguments(nullptr), returnTok(nullptr), bodyTok(nullptr), explicitCaptures() { if (!Token::simpleMatch(tok, "[") || !tok->link()) return; capture = tok; @@ -3722,12 +3726,31 @@ struct Lambda { } else if (Token::simpleMatch(afterArguments, "{")) { bodyTok = afterArguments; } + for(const Token* c:getCaptures()) { + if (c->variable()) { + explicitCaptures[c->variable()] = std::make_pair(c, Capture::ByValue); + } else if (c->isUnaryOp("&") && Token::Match(c->astOperand1(), "%var%")) { + explicitCaptures[c->astOperand1()->variable()] = std::make_pair(c->astOperand1(), Capture::ByReference); + } else { + const std::string& s = c->expressionString(); + if (s == "=") + implicitCapture = Capture::ByValue; + else if (s == "&") + implicitCapture = Capture::ByReference; + } + } } const Token * capture; const Token * arguments; const Token * returnTok; const Token * bodyTok; + std::unordered_map> explicitCaptures; + Capture implicitCapture; + + std::vector getCaptures() { + return getArguments(capture); + } bool isLambda() const { return capture && bodyTok; @@ -3762,11 +3785,15 @@ static void valueFlowLifetime(TokenList *tokenlist, SymbolDatabase*, ErrorLogger const Scope * bodyScope = lam.bodyTok->scope(); std::set scopes; + // Avoid capturing a variable twice + std::set varids; - auto isCapturingVariable = [&](const Token *varTok) { + auto isImplicitCapturingVariable = [&](const Token *varTok) { const Variable *var = varTok->variable(); if (!var) return false; + if (varids.count(var->declarationId()) > 0) + return false; if (!var->isLocal() && !var->isArgument()) return false; const Scope *scope = var->scope(); @@ -3777,23 +3804,39 @@ static void valueFlowLifetime(TokenList *tokenlist, SymbolDatabase*, ErrorLogger if (scope->isNestedIn(bodyScope)) return false; scopes.insert(scope); + varids.insert(var->declarationId()); return true; }; - // TODO: Handle explicit capture - bool captureByRef = Token::Match(lam.capture, "[ & ]"); - bool captureByValue = Token::Match(lam.capture, "[ = ]"); + auto captureVariable = [&](const Token* tok2, Lambda::Capture c, std::function pred) { + if (varids.count(tok->varId()) > 0) + return; + ErrorPath errorPath; + if (c == Lambda::Capture::ByReference) { + LifetimeStore{tok2, "Lambda captures variable by reference here.", ValueFlow::Value::LifetimeKind::Lambda} .byRef( + tok, tokenlist, errorLogger, settings, pred); + } else if (c == Lambda::Capture::ByValue) { + LifetimeStore{tok2, "Lambda captures variable by value here.", ValueFlow::Value::LifetimeKind::Lambda} .byVal( + tok, tokenlist, errorLogger, settings, pred); + } + }; + + // Handle explicit capture + for(const auto& p:lam.explicitCaptures) { + const Variable* var = p.first; + if (!var) + continue; + const Token* tok2 = p.second.first; + Lambda::Capture c = p.second.second; + captureVariable(tok2, c, [](const Token*) { return true; }); + varids.insert(var->declarationId()); + } for (const Token * tok2 = lam.bodyTok; tok2 != lam.bodyTok->link(); tok2 = tok2->next()) { - ErrorPath errorPath; - if (captureByRef) { - LifetimeStore{tok2, "Lambda captures variable by reference here.", ValueFlow::Value::LifetimeKind::Lambda} .byRef( - tok, tokenlist, errorLogger, settings, isCapturingVariable); - } else if (captureByValue) { - LifetimeStore{tok2, "Lambda captures variable by value here.", ValueFlow::Value::LifetimeKind::Lambda} .byVal( - tok, tokenlist, errorLogger, settings, isCapturingVariable); - } + captureVariable(tok2, lam.implicitCapture, isImplicitCapturingVariable); } + + valueFlowForwardLifetime(tok, tokenlist, errorLogger, settings); } // address of else if (tok->isUnaryOp("&")) { diff --git a/test/testautovariables.cpp b/test/testautovariables.cpp index 059d15064..2e77340e9 100644 --- a/test/testautovariables.cpp +++ b/test/testautovariables.cpp @@ -1825,6 +1825,50 @@ private: " return g([&]() { return x; });\n" "}\n"); ASSERT_EQUALS("", errout.str()); + + check("auto f() {\n" + " int i = 0;\n" + " return [&i] {};\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:2] -> [test.cpp:3]: (error) Returning lambda that captures local variable 'i' that will be invalid when returning.\n", errout.str()); + + check("auto f() {\n" + " int i = 0;\n" + " return [i] {};\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("auto f() {\n" + " int i = 0;\n" + " return [=, &i] {};\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:2] -> [test.cpp:3]: (error) Returning lambda that captures local variable 'i' that will be invalid when returning.\n", errout.str()); + + check("auto f() {\n" + " int i = 0;\n" + " int j = 0;\n" + " return [=, &i] { return j; };\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:2] -> [test.cpp:4]: (error) Returning lambda that captures local variable 'i' that will be invalid when returning.\n", errout.str()); + + check("auto f() {\n" + " int i = 0;\n" + " return [&, i] {};\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("auto f() {\n" + " int i = 0;\n" + " int j = 0;\n" + " return [&, i] { return j; };\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:3] -> [test.cpp:4]: (error) Returning lambda that captures local variable 'j' that will be invalid when returning.\n", errout.str()); + + check("auto f(int& i) {\n" + " int j = 0;\n" + " return [=, &i] { return j; };\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); } void danglingLifetimeContainer() { diff --git a/test/testtokenize.cpp b/test/testtokenize.cpp index fa78062cc..45b264b4b 100644 --- a/test/testtokenize.cpp +++ b/test/testtokenize.cpp @@ -7895,7 +7895,7 @@ private: // `-( // `-{ - ASSERT_EQUALS("x{([( ai=", testAst("x([&a](int i){a=i;});")); + ASSERT_EQUALS("x{(a&[( ai=", testAst("x([&a](int i){a=i;});")); ASSERT_EQUALS("{([(return 0return", testAst("return [](){ return 0; }();")); // noexcept @@ -7903,40 +7903,40 @@ private: // -> ASSERT_EQUALS("{([(return 0return", testAst("return []() -> int { return 0; }();")); - ASSERT_EQUALS("{([(return 0return", testAst("return [something]() -> int { return 0; }();")); + ASSERT_EQUALS("{(something[(return 0return", testAst("return [something]() -> int { return 0; }();")); ASSERT_EQUALS("{([cd,(return 0return", testAst("return [](int a, int b) -> int { return 0; }(c, d);")); ASSERT_EQUALS("{([return", testAst("return []() -> decltype(0) {};")); - ASSERT_EQUALS("x{([=", testAst("x = [&]()->std::string const & {};")); + ASSERT_EQUALS("x{(&[=", testAst("x = [&]()->std::string const & {};")); ASSERT_EQUALS("f{([=", testAst("f = []() -> foo* {};")); ASSERT_EQUALS("f{([=", testAst("f = [](void) mutable -> foo* {};")); ASSERT_EQUALS("f{([=", testAst("f = []() mutable {};")); ASSERT_EQUALS("x{([= 0return", testAst("x = [](){return 0; };")); - ASSERT_EQUALS("ab{[(= cd=", testAst("a = b([&]{c=d;});")); + ASSERT_EQUALS("ab{&[(= cd=", testAst("a = b([&]{c=d;});")); // 8628 ASSERT_EQUALS("f{([( switchx( 1case y++", testAst("f([](){switch(x){case 1:{++y;}}});")); - ASSERT_EQUALS("{([{return ab=", + ASSERT_EQUALS("{(=[{return ab=", testAst("return {\n" " [=]() {\n" " a = b;\n" " }\n" "};\n")); - ASSERT_EQUALS("{[{return ab=", + ASSERT_EQUALS("{=[{return ab=", testAst("return {\n" " [=] {\n" " a = b;\n" " }\n" "};\n")); - ASSERT_EQUALS("{([{return ab=", + ASSERT_EQUALS("{(=[{return ab=", testAst("return {\n" " [=]() -> int {\n" " a=b;\n" " }\n" "}")); - ASSERT_EQUALS("{([{return ab=", + ASSERT_EQUALS("{(=[{return ab=", testAst("return {\n" " [=]() mutable -> int {\n" " a=b;\n" @@ -7944,12 +7944,13 @@ private: "}")); // daca@home hang - ASSERT_EQUALS("a{([= 0return b{([= fori0=i10!=i++;;(", + ASSERT_EQUALS("a{(&[= 0return b{(=[= fori0=i10!=i++;;(", testAst("a = [&]() -> std::pair { return 0; };\n" "b = [=]() { for (i = 0; i != 10; ++i); };")); // #9662 ASSERT_EQUALS("b{[{ stdunique_ptr::0nullptrnullptr:?{", testAst("auto b{[] { std::unique_ptr{0 ? nullptr : nullptr}; }};")); + ASSERT_EQUALS("a( b{[=", testAst("void a() { [b = [] { ; }] {}; }")); }