-
Notifications
You must be signed in to change notification settings - Fork 11.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[clang-tidy] Add support for lambdas in cppcoreguidelines-owning-memory #77246
Conversation
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clang-tidy Author: Piotr Zegar (PiotrZSL) ChangesImplement proper support for lambdas and sub-functions/classes. Fixes: #59389 Full diff: https://github.com/llvm/llvm-project/pull/77246.diff 3 Files Affected:
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;
+ };
+ }
+}
|
clang-tools-extra/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only have two nits
clang-tools-extra/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had an idea for the added matchers that improves their readability.
clang-tools-extra/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp
Outdated
Show resolved
Hide resolved
Implement proper support for lambdas and sub-functions/classes. Moved from https://reviews.llvm.org/D157285 Fixes: llvm#59389
e89af3d
to
fc7d374
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
But wait for @HerrCai0907, you commented earlier as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…ry (llvm#77246) Implement proper support for lambdas and sub-functions/classes. Moved from https://reviews.llvm.org/D157285 Fixes: llvm#59389
Implement proper support for lambdas and sub-functions/classes.
Moved from https://reviews.llvm.org/D157285
Fixes: #59389