diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp @@ -19,6 +19,18 @@ namespace clang::tidy::cppcoreguidelines { +namespace { +AST_MATCHER_P(LambdaExpr, hasCallOperator, + ast_matchers::internal::Matcher, InnerMatcher) { + return InnerMatcher.matches(*Node.getCallOperator(), Finder, Builder); +} + +AST_MATCHER_P(LambdaExpr, hasLambdaBody, ast_matchers::internal::Matcher, + InnerMatcher) { + return InnerMatcher.matches(*Node.getBody(), Finder, Builder); +} +} // namespace + void OwningMemoryCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { Options.store(Opts, "LegacyResourceProducers", LegacyResourceProducers); Options.store(Opts, "LegacyResourceConsumers", LegacyResourceConsumers); @@ -55,6 +67,8 @@ CreatesLegacyOwner, LegacyOwnerCast); const auto ConsideredOwner = eachOf(IsOwnerType, CreatesOwner); + const auto ScopeDeclaration = anyOf(translationUnitDecl(), namespaceDecl(), + recordDecl(), functionDecl()); // Find delete expressions that delete non-owners. Finder->addMatcher( @@ -147,10 +161,52 @@ // Matching on functions, that return an owner/resource, but don't declare // their return type as owner. Finder->addMatcher( - functionDecl(hasDescendant(returnStmt(hasReturnValue(ConsideredOwner)) - .bind("bad_owner_return")), - unless(returns(qualType(hasDeclaration(OwnerDecl))))) - .bind("function_decl"), + functionDecl( + decl().bind("function_decl"), + hasBody(stmt( + stmt().bind("body"), + hasDescendant( + returnStmt(hasReturnValue(ConsideredOwner), + // Ignore sub-lambda expressions + hasAncestor(stmt(anyOf(equalsBoundNode("body"), + lambdaExpr())) + .bind("scope")), + hasAncestor(stmt(equalsBoundNode("scope"), + equalsBoundNode("body"))), + // Ignore sub-functions + hasAncestor(functionDecl().bind("context")), + hasAncestor(functionDecl( + equalsBoundNode("context"), + equalsBoundNode("function_decl")))) + .bind("bad_owner_return")))), + returns(qualType(qualType().bind("result"), + unless(hasDeclaration(OwnerDecl))))), + this); + + // Matching on lambdas, that return an owner/resource, but don't declare + // their return type as owner. + Finder->addMatcher( + lambdaExpr( + hasAncestor(decl(ScopeDeclaration).bind("scope-decl")), + hasLambdaBody(stmt( + stmt().bind("body"), + hasDescendant( + returnStmt( + hasReturnValue(ConsideredOwner), + // Ignore sub-lambdas + hasAncestor( + stmt(anyOf(equalsBoundNode("body"), lambdaExpr())) + .bind("scope")), + hasAncestor(stmt(equalsBoundNode("scope"), + equalsBoundNode("body"))), + // Ignore sub-functions + hasAncestor(decl(ScopeDeclaration).bind("context")), + hasAncestor(decl(equalsBoundNode("context"), + equalsBoundNode("scope-decl")))) + .bind("bad_owner_return")))), + hasCallOperator(returns(qualType(qualType().bind("result"), + unless(hasDeclaration(OwnerDecl)))))) + .bind("lambda"), this); // Match on classes that have an owner as member, but don't declare a @@ -329,7 +385,7 @@ // Function return statements, that are owners/resources, but the function // declaration does not declare its return value as owner. const auto *BadReturnType = Nodes.getNodeAs("bad_owner_return"); - const auto *Function = Nodes.getNodeAs("function_decl"); + const auto *ResultType = Nodes.getNodeAs("result"); // Function return values, that should be owners but aren't. if (BadReturnType) { @@ -338,8 +394,9 @@ diag(BadReturnType->getBeginLoc(), "returning a newly created resource of " "type %0 or 'gsl::owner<>' from a " - "function whose return type is not 'gsl::owner<>'") - << Function->getReturnType() << BadReturnType->getSourceRange(); + "%select{function|lambda}1 whose return type is not 'gsl::owner<>'") + << *ResultType << (Nodes.getNodeAs("lambda") != nullptr) + << BadReturnType->getSourceRange(); // FIXME: Rewrite the return type as 'gsl::owner' return true; diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -172,6 +172,10 @@ to ignore ``static`` variables declared within the scope of ``class``/``struct``. +- Improved :doc:`cppcoreguidelines-owning-memory + ` check to properly handle + return type in lambdas and in nested functions. + - Improved :doc:`cppcoreguidelines-prefer-member-initializer ` check to ignore delegate constructors. diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/owning-memory.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/owning-memory.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/owning-memory.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/owning-memory.cpp @@ -395,3 +395,98 @@ // CHECK-NOTES: [[@LINE-1]]:5: warning: returning a newly created resource of type 'A *' or 'gsl::owner<>' from a function whose return type is not 'gsl::owner<>' } } + +namespace PR59389 { + struct S { + S(); + S(int); + + int value = 1; + }; + + void testLambdaInFunctionNegative() { + const auto MakeS = []() -> ::gsl::owner { + return ::gsl::owner{new S{}}; + }; + } + + void testLambdaInFunctionPositive() { + const auto MakeS = []() -> S* { + return ::gsl::owner{new S{}}; + // CHECK-NOTES: [[@LINE-1]]:7: warning: returning a newly created resource of type 'S *' or 'gsl::owner<>' from a lambda whose return type is not 'gsl::owner<>' + }; + } + + void testFunctionInFunctionNegative() { + struct C { + ::gsl::owner test() { + return ::gsl::owner{new S{}}; + } + }; + } + + void testFunctionInFunctionPositive() { + struct C { + S* test() { + return ::gsl::owner{new S{}}; + // CHECK-NOTES: [[@LINE-1]]:9: warning: returning a newly created resource of type 'S *' or 'gsl::owner<>' from a function whose return type is not 'gsl::owner<>' + } + }; + } + + ::gsl::owner testReverseLambdaNegative() { + const auto MakeI = [] -> int { return 5; }; + return ::gsl::owner{new S(MakeI())}; + } + + S* testReverseLambdaPositive() { + const auto MakeI = [] -> int { return 5; }; + return ::gsl::owner{new S(MakeI())}; + // CHECK-NOTES: [[@LINE-1]]:5: warning: returning a newly created resource of type 'S *' or 'gsl::owner<>' from a function whose return type is not 'gsl::owner<>' + } + + ::gsl::owner testReverseFunctionNegative() { + struct C { + int test() { return 5; } + }; + return ::gsl::owner{new S(C().test())}; + } + + S* testReverseFunctionPositive() { + struct C { + int test() { return 5; } + }; + return ::gsl::owner{new S(C().test())}; + // CHECK-NOTES: [[@LINE-1]]:5: warning: returning a newly created resource of type 'S *' or 'gsl::owner<>' from a function whose return type is not 'gsl::owner<>' + } + + void testLambdaInLambdaNegative() { + const auto MakeS = []() -> ::gsl::owner { + const auto MakeI = []() -> int { return 5; }; + return ::gsl::owner{new S(MakeI())}; + }; + } + + void testLambdaInLambdaPositive() { + const auto MakeS = []() -> S* { + const auto MakeI = []() -> int { return 5; }; + return ::gsl::owner{new S(MakeI())}; + // CHECK-NOTES: [[@LINE-1]]:7: warning: returning a newly created resource of type 'S *' or 'gsl::owner<>' from a lambda whose return type is not 'gsl::owner<>' + }; + } + + void testReverseLambdaInLambdaNegative() { + const auto MakeI = []() -> int { + const auto MakeS = []() -> ::gsl::owner { return new S(); }; + return 5; + }; + } + + void testReverseLambdaInLambdaPositive() { + const auto MakeI = []() -> int { + const auto MakeS = []() -> S* { return new S(); }; + // CHECK-NOTES: [[@LINE-1]]:39: warning: returning a newly created resource of type 'S *' or 'gsl::owner<>' from a lambda whose return type is not 'gsl::owner<>' + return 5; + }; + } +}