[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 Jan 23 08:23:51 PST 2024
https://github.com/PiotrZSL updated https://github.com/llvm/llvm-project/pull/77246
>From b2e230f90f97c0fb3385ab05d0217371b72b9a88 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/2] [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 | 6 +-
.../cppcoreguidelines/owning-memory.cpp | 95 +++++++++++++++++++
3 files changed, 164 insertions(+), 8 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp
index 9215b833573afdd..89450149820f301 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 1bd5a72126c10be..e9abcae8902c5c3 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 eb8ad1b8b879250..d308f1dcc4f0e2c 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 e89af3dbcab2b39b4a06b411d360a268b232ba13 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/2] 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 89450149820f301..2203f4aacb9ba20 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);
More information about the cfe-commits
mailing list