llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tidy Author: Piotr Zegar (PiotrZSL) <details> <summary>Changes</summary> Implement proper support for lambdas and sub-functions/classes. Moved from https://reviews.llvm.org/D157285 Fixes: #<!-- -->59389 --- Full diff: https://github.com/llvm/llvm-project/pull/77246.diff 3 Files Affected: - (modified) clang-tools-extra/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp (+64-7) - (modified) clang-tools-extra/docs/ReleaseNotes.rst (+5-1) - (modified) clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/owning-memory.cpp (+95) ``````````diff 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 1bd5a72126c10b..e9abcae8902c5c 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -119,7 +119,7 @@ Improvements to clang-tidy - Improved `--dump-config` to print check options in alphabetical order. -- Improved :program:`clang-tidy-diff.py` script. +- Improved :program:`clang-tidy-diff.py` script. * Return exit code `1` if any :program:`clang-tidy` subprocess exits with a non-zero code or if exporting fixes fails. @@ -310,6 +310,10 @@ Changes in existing checks extending the `IgnoreConversionFromTypes` option to include types without a declaration, such as built-in types. +- Improved :doc:`cppcoreguidelines-owning-memory + <clang-tidy/checks/cppcoreguidelines/owning-memory>` check to properly handle + return type in lambdas and in nested functions. + - Improved :doc:`cppcoreguidelines-prefer-member-initializer <clang-tidy/checks/cppcoreguidelines/prefer-member-initializer>` check to ignore delegate constructors and ignore re-assignment for reference or when 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; + }; + } +} `````````` </details> https://github.com/llvm/llvm-project/pull/77246 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
