https://github.com/PiotrZSL updated https://github.com/llvm/llvm-project/pull/77246
>From f7534c0d9ce6d2c8ce8a075a36a801549287edb9 Mon Sep 17 00:00:00 2001 From: Piotr Zegar <m...@piotrzegar.pl> Date: Sun, 7 Jan 2024 18:52:05 +0000 Subject: [PATCH 1/5] [clang-tidy] Add support for lambdas in cppcoreguidelines-owning-memory Implement proper support for lambdas and sub-functions/classes. Moved from https://reviews.llvm.org/D157285 Fixes: #59389 --- .../cppcoreguidelines/OwningMemoryCheck.cpp | 71 ++++++++++++-- clang-tools-extra/docs/ReleaseNotes.rst | 4 + .../cppcoreguidelines/owning-memory.cpp | 95 +++++++++++++++++++ 3 files changed, 163 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..89450149820f30 100644 --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp @@ -19,6 +19,18 @@ using namespace clang::ast_matchers::internal; namespace clang::tidy::cppcoreguidelines { +namespace { +AST_MATCHER_P(LambdaExpr, hasCallOperator, + ast_matchers::internal::Matcher<CXXMethodDecl>, InnerMatcher) { + return InnerMatcher.matches(*Node.getCallOperator(), Finder, Builder); +} + +AST_MATCHER_P(LambdaExpr, hasLambdaBody, ast_matchers::internal::Matcher<Stmt>, + 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 @@ 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( @@ -147,10 +161,52 @@ void OwningMemoryCheck::registerMatchers(MatchFinder *Finder) { // 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 @@ 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<ReturnStmt>("bad_owner_return"); - const auto *Function = Nodes.getNodeAs<FunctionDecl>("function_decl"); + const auto *ResultType = Nodes.getNodeAs<QualType>("result"); // Function return values, that should be owners but aren't. if (BadReturnType) { @@ -338,8 +394,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<Expr>("lambda") != nullptr) + << BadReturnType->getSourceRange(); // FIXME: Rewrite the return type as 'gsl::owner<OriginalType>' return true; diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 143ae230fc443c..b6bfd58a9637e5 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -158,6 +158,10 @@ Changes in existing checks <clang-tidy/checks/cppcoreguidelines/missing-std-forward>` check by no longer giving false positives for deleted functions. +- Improved :doc:`cppcoreguidelines-owning-memory + <clang-tidy/checks/cppcoreguidelines/owning-memory>` check to properly handle + return type in lambdas and in nested functions. + - Cleaned up :doc:`cppcoreguidelines-prefer-member-initializer <clang-tidy/checks/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..d308f1dcc4f0e2 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,98 @@ 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<S*> { + return ::gsl::owner<S*>{new S{}}; + }; + } + + void testLambdaInFunctionPositive() { + const auto MakeS = []() -> S* { + return ::gsl::owner<S*>{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<S*> test() { + return ::gsl::owner<S*>{new S{}}; + } + }; + } + + void testFunctionInFunctionPositive() { + struct C { + S* test() { + return ::gsl::owner<S*>{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<S*> testReverseLambdaNegative() { + const auto MakeI = [] -> int { return 5; }; + return ::gsl::owner<S*>{new S(MakeI())}; + } + + S* testReverseLambdaPositive() { + const auto MakeI = [] -> int { return 5; }; + return ::gsl::owner<S*>{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<S*> testReverseFunctionNegative() { + struct C { + int test() { return 5; } + }; + return ::gsl::owner<S*>{new S(C().test())}; + } + + S* testReverseFunctionPositive() { + struct C { + int test() { return 5; } + }; + return ::gsl::owner<S*>{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<S*> { + const auto MakeI = []() -> int { return 5; }; + return ::gsl::owner<S*>{new S(MakeI())}; + }; + } + + void testLambdaInLambdaPositive() { + const auto MakeS = []() -> S* { + const auto MakeI = []() -> int { return 5; }; + return ::gsl::owner<S*>{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<S*> { 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; + }; + } +} >From d4a6d1897906e6c2a9fa8baa78475af8c2d86582 Mon Sep 17 00:00:00 2001 From: Piotr Zegar <m...@piotrzegar.pl> Date: Tue, 23 Jan 2024 16:21:49 +0000 Subject: [PATCH 2/5] Remove redundant qualType --- .../clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp index 89450149820f30..2203f4aacb9ba2 100644 --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp @@ -179,8 +179,7 @@ void OwningMemoryCheck::registerMatchers(MatchFinder *Finder) { equalsBoundNode("context"), equalsBoundNode("function_decl")))) .bind("bad_owner_return")))), - returns(qualType(qualType().bind("result"), - unless(hasDeclaration(OwnerDecl))))), + returns(qualType(unless(hasDeclaration(OwnerDecl))).bind("result"))), this); // Matching on lambdas, that return an owner/resource, but don't declare @@ -204,8 +203,8 @@ void OwningMemoryCheck::registerMatchers(MatchFinder *Finder) { hasAncestor(decl(equalsBoundNode("context"), equalsBoundNode("scope-decl")))) .bind("bad_owner_return")))), - hasCallOperator(returns(qualType(qualType().bind("result"), - unless(hasDeclaration(OwnerDecl)))))) + hasCallOperator(returns( + qualType(unless(hasDeclaration(OwnerDecl))).bind("result")))) .bind("lambda"), this); >From 2bd83d0b5ba6cbe921dc0d61d00ab9ea6e13f95a Mon Sep 17 00:00:00 2001 From: Piotr Zegar <m...@piotrzegar.pl> Date: Mon, 4 Mar 2024 22:19:14 +0000 Subject: [PATCH 3/5] Remove ast_matchers::internal:: --- .../clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp index 2203f4aacb9ba2..faa153c14314d9 100644 --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp @@ -20,13 +20,12 @@ using namespace clang::ast_matchers::internal; namespace clang::tidy::cppcoreguidelines { namespace { -AST_MATCHER_P(LambdaExpr, hasCallOperator, - ast_matchers::internal::Matcher<CXXMethodDecl>, InnerMatcher) { +AST_MATCHER_P(LambdaExpr, hasCallOperator, Matcher<CXXMethodDecl>, + InnerMatcher) { return InnerMatcher.matches(*Node.getCallOperator(), Finder, Builder); } -AST_MATCHER_P(LambdaExpr, hasLambdaBody, ast_matchers::internal::Matcher<Stmt>, - InnerMatcher) { +AST_MATCHER_P(LambdaExpr, hasLambdaBody, Matcher<Stmt>, InnerMatcher) { return InnerMatcher.matches(*Node.getBody(), Finder, Builder); } } // namespace >From 2d6bb71e050f398b004587d5ae81c92f3c649bd3 Mon Sep 17 00:00:00 2001 From: Piotr Zegar <m...@piotrzegar.pl> Date: Tue, 5 Mar 2024 21:32:40 +0000 Subject: [PATCH 4/5] test --- .../checkers/cppcoreguidelines/owning-memory.cpp | 11 +++++++++++ 1 file changed, 11 insertions(+) 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 d308f1dcc4f0e2..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 @@ -475,6 +475,17 @@ namespace PR59389 { }; } + void testLambdaInLambdaWithDoubleReturns() { + const auto MakeS = []() -> S* { + const auto MakeS2 = []() -> S* { + return ::gsl::owner<S*>{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<S*>{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<S*> { return new S(); }; >From fc7d37451613029a914d2e4c9894464cbaea6258 Mon Sep 17 00:00:00 2001 From: Piotr Zegar <m...@piotrzegar.pl> Date: Tue, 5 Mar 2024 21:46:25 +0000 Subject: [PATCH 5/5] Remove duplicate --- .../cppcoreguidelines/OwningMemoryCheck.cpp | 61 +++++++++---------- 1 file changed, 29 insertions(+), 32 deletions(-) diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp index faa153c14314d9..9b4d2ef99e5bf1 100644 --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp @@ -157,27 +157,28 @@ 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( 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")))), + 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); @@ -186,22 +187,18 @@ void OwningMemoryCheck::registerMatchers(MatchFinder *Finder) { 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")))), + 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"), _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
