[clang-tools-extra] [clang-tidy] Add support for lambdas in cppcoreguidelines-owning-memory (PR #77246)

Piotr Zegar via cfe-commits cfe-commits at lists.llvm.org
Tue Mar 5 13:54:03 PST 2024


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 <me at 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 <me at 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 <me at 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 <me at 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 <me at 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"),



More information about the cfe-commits mailing list