From 2fc31e9cab0b13a47c8116fe972d91f0adc5f85d Mon Sep 17 00:00:00 2001 From: Piotr Zegar Date: Tue, 19 Mar 2024 20:13:20 +0100 Subject: [PATCH] [clang-tidy] Add support for lambdas in cppcoreguidelines-owning-memory (#77246) Implement proper support for lambdas and sub-functions/classes. Moved from https://reviews.llvm.org/D157285 Fixes: #59389 --- .../cppcoreguidelines/OwningMemoryCheck.cpp | 66 +++++++++-- clang-tools-extra/docs/ReleaseNotes.rst | 4 + .../cppcoreguidelines/owning-memory.cpp | 106 ++++++++++++++++++ 3 files changed, 169 insertions(+), 7 deletions(-) diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp index 9215b833573afd..9b4d2ef99e5bf1 100644 --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp @@ -19,6 +19,17 @@ using namespace clang::ast_matchers::internal; namespace clang::tidy::cppcoreguidelines { +namespace { +AST_MATCHER_P(LambdaExpr, hasCallOperator, Matcher, + InnerMatcher) { + return InnerMatcher.matches(*Node.getCallOperator(), Finder, Builder); +} + +AST_MATCHER_P(LambdaExpr, hasLambdaBody, 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 +66,8 @@ void OwningMemoryCheck::registerMatchers(MatchFinder *Finder) { 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( @@ -144,13 +157,51 @@ void OwningMemoryCheck::registerMatchers(MatchFinder *Finder) { .bind("bad_owner_creation_parameter"))), this); + auto IsNotInSubLambda = stmt( + hasAncestor( + stmt(anyOf(equalsBoundNode("body"), lambdaExpr())).bind("scope")), + hasAncestor(stmt(equalsBoundNode("scope"), equalsBoundNode("body")))); + // 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 + IsNotInSubLambda, + // Ignore sub-functions + hasAncestor(functionDecl().bind("context")), + hasAncestor(functionDecl( + equalsBoundNode("context"), + equalsBoundNode("function_decl")))) + .bind("bad_owner_return")))), + returns(qualType(unless(hasDeclaration(OwnerDecl))).bind("result"))), + 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 + IsNotInSubLambda, + // Ignore sub-functions + hasAncestor(decl(ScopeDeclaration).bind("context")), + hasAncestor(decl(equalsBoundNode("context"), + equalsBoundNode("scope-decl")))) + .bind("bad_owner_return")))), + hasCallOperator(returns( + qualType(unless(hasDeclaration(OwnerDecl))).bind("result")))) + .bind("lambda"), this); // Match on classes that have an owner as member, but don't declare a @@ -329,7 +380,7 @@ bool OwningMemoryCheck::handleReturnValues(const BoundNodes &Nodes) { // 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 +389,9 @@ bool OwningMemoryCheck::handleReturnValues(const BoundNodes &Nodes) { 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 index 44680f79de6f54..3e69d2eec94279 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -165,6 +165,10 @@ Changes in existing checks giving false positives for deleted functions and fix false negative when some parameters are forwarded, but other aren't. +- Improved :doc:`cppcoreguidelines-owning-memory + ` check to properly handle + return type in lambdas and in nested functions. + - Cleaned up :doc:`cppcoreguidelines-prefer-member-initializer ` by removing enforcement of rule `C.48 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 index eb8ad1b8b87925..574efe7bd91478 100644 --- 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,109 @@ namespace PR63994 { // 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 testLambdaInLambdaWithDoubleReturns() { + const auto MakeS = []() -> S* { + const auto MakeS2 = []() -> S* { + return ::gsl::owner{new S(1)}; + // CHECK-NOTES: [[@LINE-1]]:9: warning: returning a newly created resource of type 'S *' or 'gsl::owner<>' from a lambda whose return type is not 'gsl::owner<>' [cppcoreguidelines-owning-memory] + }; + return ::gsl::owner{new S(2)}; + // 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; + }; + } +}