[webkit.UncountedLambdaCapturesChecker] Recognize nested protectedThis pattern #126443
+108
−48
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…s pattern In WebKit, it's pretty common to capture "this" and "protectedThis" where "protectedThis" is a guardian variable of type Ref or RefPtr for "this". Furthermore, it's common for this "protectedThis" variable from being passed to an inner lambda by std::move. Recognize this pattern so that we don't emit warnings for nested inner lambdas. To recognize this pattern, we introduce a new DenseSet, ProtectedThisDecls, which contains every "protectedThis" we've recognized to our subclass of DynamicRecursiveASTVisitor. This set is now populated in "hasProtectedThis" and "declProtectsThis" uses this DenseSet to determine a given value declaration constitutes a "protectedThis" pattern or not. Because hasProtectedThis and declProtectsThis had to be moved from the checker class to the visitor class, it's now a responsibility of each caller of visitLambdaExpr to check whether a given lambda captures "this" without a "protectedThis" or not. Finally, this PR improves the code to recognize "protectedThis" pattern by allowing more nested CXXBindTemporaryExpr, CXXOperatorCallExpr, and UnaryOperator expressions.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-static-analyzer-1 Author: Ryosuke Niwa (rniwa) ChangesIn WebKit, it's pretty common to capture "this" and "protectedThis" where "protectedThis" is a guardian variable of type Ref or RefPtr for "this". Furthermore, it's common for this "protectedThis" variable from being passed to an inner lambda by std::move. Recognize this pattern so that we don't emit warnings for nested inner lambdas. To recognize this pattern, we introduce a new DenseSet, ProtectedThisDecls, which contains every "protectedThis" we've recognized to our subclass of DynamicRecursiveASTVisitor. This set is now populated in "hasProtectedThis" and "declProtectsThis" uses this DenseSet to determine a given value declaration constitutes a "protectedThis" pattern or not. Because hasProtectedThis and declProtectsThis had to be moved from the checker class to the visitor class, it's now a responsibility of each caller of visitLambdaExpr to check whether a given lambda captures "this" without a "protectedThis" or not. Finally, this PR improves the code to recognize "protectedThis" pattern by allowing more nested CXXBindTemporaryExpr, CXXOperatorCallExpr, and UnaryOperator expressions. Full diff: https://github.com/llvm/llvm-project/pull/126443.diff 2 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
index a56f48c83c660a9..c0a9e38b6aab41b 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
@@ -41,6 +41,7 @@ class UncountedLambdaCapturesChecker
const UncountedLambdaCapturesChecker *Checker;
llvm::DenseSet<const DeclRefExpr *> DeclRefExprsToIgnore;
llvm::DenseSet<const LambdaExpr *> LambdasToIgnore;
+ llvm::DenseSet<const ValueDecl *> ProtectedThisDecls;
QualType ClsType;
explicit LocalVisitor(const UncountedLambdaCapturesChecker *Checker)
@@ -65,7 +66,7 @@ class UncountedLambdaCapturesChecker
bool VisitLambdaExpr(LambdaExpr *L) override {
if (LambdasToIgnore.contains(L))
return true;
- Checker->visitLambdaExpr(L, shouldCheckThis());
+ Checker->visitLambdaExpr(L, shouldCheckThis() && !hasProtectedThis(L));
return true;
}
@@ -93,7 +94,7 @@ class UncountedLambdaCapturesChecker
if (!L)
return true;
LambdasToIgnore.insert(L);
- Checker->visitLambdaExpr(L, shouldCheckThis());
+ Checker->visitLambdaExpr(L, shouldCheckThis() && !hasProtectedThis(L));
return true;
}
@@ -118,7 +119,8 @@ class UncountedLambdaCapturesChecker
if (auto *L = findLambdaInArg(Arg)) {
LambdasToIgnore.insert(L);
if (!Param->hasAttr<NoEscapeAttr>() && !TreatAllArgsAsNoEscape)
- Checker->visitLambdaExpr(L, shouldCheckThis());
+ Checker->visitLambdaExpr(L, shouldCheckThis() &&
+ !hasProtectedThis(L));
}
++ArgIndex;
}
@@ -145,6 +147,11 @@ class UncountedLambdaCapturesChecker
return nullptr;
if (auto *Lambda = dyn_cast<LambdaExpr>(CtorArg))
return Lambda;
+ if (auto *TempExpr = dyn_cast<CXXBindTemporaryExpr>(CtorArg)) {
+ E = TempExpr->getSubExpr()->IgnoreParenCasts();
+ if (auto *Lambda = dyn_cast<LambdaExpr>(E))
+ return Lambda;
+ }
auto *DRE = dyn_cast<DeclRefExpr>(CtorArg);
if (!DRE)
return nullptr;
@@ -189,9 +196,68 @@ class UncountedLambdaCapturesChecker
return;
DeclRefExprsToIgnore.insert(ArgRef);
LambdasToIgnore.insert(L);
- Checker->visitLambdaExpr(L, shouldCheckThis(),
+ Checker->visitLambdaExpr(L, shouldCheckThis() && !hasProtectedThis(L),
/* ignoreParamVarDecl */ true);
}
+
+ bool hasProtectedThis(LambdaExpr *L) {
+ for (const LambdaCapture &OtherCapture : L->captures()) {
+ if (!OtherCapture.capturesVariable())
+ continue;
+ if (auto *ValueDecl = OtherCapture.getCapturedVar()) {
+ if (declProtectsThis(ValueDecl)) {
+ ProtectedThisDecls.insert(ValueDecl);
+ return true;
+ }
+ }
+ }
+ return false;
+ }
+
+ bool declProtectsThis(const ValueDecl *ValueDecl) const {
+ auto *VD = dyn_cast<VarDecl>(ValueDecl);
+ if (!VD)
+ return false;
+ auto *Init = VD->getInit()->IgnoreParenCasts();
+ if (!Init)
+ return false;
+ const Expr *Arg = Init;
+ do {
+ if (auto *BTE = dyn_cast<CXXBindTemporaryExpr>(Arg))
+ Arg = BTE->getSubExpr()->IgnoreParenCasts();
+ if (auto *CE = dyn_cast_or_null<CXXConstructExpr>(Arg)) {
+ auto *Ctor = CE->getConstructor();
+ if (!Ctor)
+ return false;
+ auto clsName = safeGetName(Ctor->getParent());
+ if (!isRefType(clsName) || !CE->getNumArgs())
+ return false;
+ Arg = CE->getArg(0)->IgnoreParenCasts();
+ continue;
+ }
+ if (auto *OpCE = dyn_cast<CXXOperatorCallExpr>(Arg)) {
+ auto OpCode = OpCE->getOperator();
+ if (OpCode == OO_Star || OpCode == OO_Amp) {
+ auto *Callee = OpCE->getDirectCallee();
+ auto clsName = safeGetName(Callee->getParent());
+ if (!isRefType(clsName) || !OpCE->getNumArgs())
+ return false;
+ Arg = OpCE->getArg(0)->IgnoreParenCasts();
+ continue;
+ }
+ }
+ if (auto *UO = dyn_cast<UnaryOperator>(Arg)) {
+ auto OpCode = UO->getOpcode();
+ if (OpCode == UO_Deref || OpCode == UO_AddrOf)
+ Arg = UO->getSubExpr()->IgnoreParenCasts();
+ continue;
+ }
+ break;
+ } while (Arg);
+ if (auto *DRE = dyn_cast<DeclRefExpr>(Arg))
+ return ProtectedThisDecls.contains(DRE->getDecl());
+ return isa<CXXThisExpr>(Arg);
+ }
};
LocalVisitor visitor(this);
@@ -214,53 +280,11 @@ class UncountedLambdaCapturesChecker
} else if (C.capturesThis() && shouldCheckThis) {
if (ignoreParamVarDecl) // this is always a parameter to this function.
continue;
- bool hasProtectThis = false;
- for (const LambdaCapture &OtherCapture : L->captures()) {
- if (!OtherCapture.capturesVariable())
- continue;
- if (auto *ValueDecl = OtherCapture.getCapturedVar()) {
- if (protectThis(ValueDecl)) {
- hasProtectThis = true;
- break;
- }
- }
- }
- if (!hasProtectThis)
- reportBugOnThisPtr(C);
+ reportBugOnThisPtr(C);
}
}
}
- bool protectThis(const ValueDecl *ValueDecl) const {
- auto *VD = dyn_cast<VarDecl>(ValueDecl);
- if (!VD)
- return false;
- auto *Init = VD->getInit()->IgnoreParenCasts();
- if (!Init)
- return false;
- auto *BTE = dyn_cast<CXXBindTemporaryExpr>(Init);
- if (!BTE)
- return false;
- auto *CE = dyn_cast_or_null<CXXConstructExpr>(BTE->getSubExpr());
- if (!CE)
- return false;
- auto *Ctor = CE->getConstructor();
- if (!Ctor)
- return false;
- auto clsName = safeGetName(Ctor->getParent());
- if (!isRefType(clsName) || !CE->getNumArgs())
- return false;
- auto *Arg = CE->getArg(0)->IgnoreParenCasts();
- while (auto *UO = dyn_cast<UnaryOperator>(Arg)) {
- auto OpCode = UO->getOpcode();
- if (OpCode == UO_Deref || OpCode == UO_AddrOf)
- Arg = UO->getSubExpr();
- else
- break;
- }
- return isa<CXXThisExpr>(Arg);
- }
-
void reportBug(const LambdaCapture &Capture, ValueDecl *CapturedVar,
const QualType T) const {
assert(CapturedVar);
diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp
index 4f4a96028225303..4895879c4a385db 100644
--- a/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp
@@ -89,6 +89,7 @@ template <typename Callback> void call(Callback callback) {
someFunction();
callback();
}
+void callAsync(const WTF::Function<void()>&);
void raw_ptr() {
RefCountable* ref_countable = make_obj();
@@ -299,6 +300,29 @@ struct RefCountableWithLambdaCapturingThis {
return obj->next();
});
}
+
+ void callAsyncNoescape([[clang::noescape]] WTF::Function<bool(RefCountable&)>&&);
+ void method_temp_lambda(RefCountable* obj) {
+ callAsyncNoescape([this, otherObj = RefPtr { obj }](auto& obj) {
+ return otherObj == &obj;
+ });
+ }
+
+ void method_nested_lambda() {
+ callAsync([this, protectedThis = Ref { *this }] {
+ callAsync([this, protectedThis = static_cast<const Ref<RefCountableWithLambdaCapturingThis>&&>(protectedThis)] {
+ nonTrivial();
+ });
+ });
+ }
+
+ void method_nested_lambda2() {
+ callAsync([this, protectedThis = RefPtr { this }] {
+ callAsync([this, protectedThis = static_cast<const Ref<RefCountableWithLambdaCapturingThis>&&>(*protectedThis)] {
+ nonTrivial();
+ });
+ });
+ }
};
struct NonRefCountableWithLambdaCapturingThis {
|
|
@llvm/pr-subscribers-clang Author: Ryosuke Niwa (rniwa) ChangesIn WebKit, it's pretty common to capture "this" and "protectedThis" where "protectedThis" is a guardian variable of type Ref or RefPtr for "this". Furthermore, it's common for this "protectedThis" variable from being passed to an inner lambda by std::move. Recognize this pattern so that we don't emit warnings for nested inner lambdas. To recognize this pattern, we introduce a new DenseSet, ProtectedThisDecls, which contains every "protectedThis" we've recognized to our subclass of DynamicRecursiveASTVisitor. This set is now populated in "hasProtectedThis" and "declProtectsThis" uses this DenseSet to determine a given value declaration constitutes a "protectedThis" pattern or not. Because hasProtectedThis and declProtectsThis had to be moved from the checker class to the visitor class, it's now a responsibility of each caller of visitLambdaExpr to check whether a given lambda captures "this" without a "protectedThis" or not. Finally, this PR improves the code to recognize "protectedThis" pattern by allowing more nested CXXBindTemporaryExpr, CXXOperatorCallExpr, and UnaryOperator expressions. Full diff: https://github.com/llvm/llvm-project/pull/126443.diff 2 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
index a56f48c83c660a9..c0a9e38b6aab41b 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
@@ -41,6 +41,7 @@ class UncountedLambdaCapturesChecker
const UncountedLambdaCapturesChecker *Checker;
llvm::DenseSet<const DeclRefExpr *> DeclRefExprsToIgnore;
llvm::DenseSet<const LambdaExpr *> LambdasToIgnore;
+ llvm::DenseSet<const ValueDecl *> ProtectedThisDecls;
QualType ClsType;
explicit LocalVisitor(const UncountedLambdaCapturesChecker *Checker)
@@ -65,7 +66,7 @@ class UncountedLambdaCapturesChecker
bool VisitLambdaExpr(LambdaExpr *L) override {
if (LambdasToIgnore.contains(L))
return true;
- Checker->visitLambdaExpr(L, shouldCheckThis());
+ Checker->visitLambdaExpr(L, shouldCheckThis() && !hasProtectedThis(L));
return true;
}
@@ -93,7 +94,7 @@ class UncountedLambdaCapturesChecker
if (!L)
return true;
LambdasToIgnore.insert(L);
- Checker->visitLambdaExpr(L, shouldCheckThis());
+ Checker->visitLambdaExpr(L, shouldCheckThis() && !hasProtectedThis(L));
return true;
}
@@ -118,7 +119,8 @@ class UncountedLambdaCapturesChecker
if (auto *L = findLambdaInArg(Arg)) {
LambdasToIgnore.insert(L);
if (!Param->hasAttr<NoEscapeAttr>() && !TreatAllArgsAsNoEscape)
- Checker->visitLambdaExpr(L, shouldCheckThis());
+ Checker->visitLambdaExpr(L, shouldCheckThis() &&
+ !hasProtectedThis(L));
}
++ArgIndex;
}
@@ -145,6 +147,11 @@ class UncountedLambdaCapturesChecker
return nullptr;
if (auto *Lambda = dyn_cast<LambdaExpr>(CtorArg))
return Lambda;
+ if (auto *TempExpr = dyn_cast<CXXBindTemporaryExpr>(CtorArg)) {
+ E = TempExpr->getSubExpr()->IgnoreParenCasts();
+ if (auto *Lambda = dyn_cast<LambdaExpr>(E))
+ return Lambda;
+ }
auto *DRE = dyn_cast<DeclRefExpr>(CtorArg);
if (!DRE)
return nullptr;
@@ -189,9 +196,68 @@ class UncountedLambdaCapturesChecker
return;
DeclRefExprsToIgnore.insert(ArgRef);
LambdasToIgnore.insert(L);
- Checker->visitLambdaExpr(L, shouldCheckThis(),
+ Checker->visitLambdaExpr(L, shouldCheckThis() && !hasProtectedThis(L),
/* ignoreParamVarDecl */ true);
}
+
+ bool hasProtectedThis(LambdaExpr *L) {
+ for (const LambdaCapture &OtherCapture : L->captures()) {
+ if (!OtherCapture.capturesVariable())
+ continue;
+ if (auto *ValueDecl = OtherCapture.getCapturedVar()) {
+ if (declProtectsThis(ValueDecl)) {
+ ProtectedThisDecls.insert(ValueDecl);
+ return true;
+ }
+ }
+ }
+ return false;
+ }
+
+ bool declProtectsThis(const ValueDecl *ValueDecl) const {
+ auto *VD = dyn_cast<VarDecl>(ValueDecl);
+ if (!VD)
+ return false;
+ auto *Init = VD->getInit()->IgnoreParenCasts();
+ if (!Init)
+ return false;
+ const Expr *Arg = Init;
+ do {
+ if (auto *BTE = dyn_cast<CXXBindTemporaryExpr>(Arg))
+ Arg = BTE->getSubExpr()->IgnoreParenCasts();
+ if (auto *CE = dyn_cast_or_null<CXXConstructExpr>(Arg)) {
+ auto *Ctor = CE->getConstructor();
+ if (!Ctor)
+ return false;
+ auto clsName = safeGetName(Ctor->getParent());
+ if (!isRefType(clsName) || !CE->getNumArgs())
+ return false;
+ Arg = CE->getArg(0)->IgnoreParenCasts();
+ continue;
+ }
+ if (auto *OpCE = dyn_cast<CXXOperatorCallExpr>(Arg)) {
+ auto OpCode = OpCE->getOperator();
+ if (OpCode == OO_Star || OpCode == OO_Amp) {
+ auto *Callee = OpCE->getDirectCallee();
+ auto clsName = safeGetName(Callee->getParent());
+ if (!isRefType(clsName) || !OpCE->getNumArgs())
+ return false;
+ Arg = OpCE->getArg(0)->IgnoreParenCasts();
+ continue;
+ }
+ }
+ if (auto *UO = dyn_cast<UnaryOperator>(Arg)) {
+ auto OpCode = UO->getOpcode();
+ if (OpCode == UO_Deref || OpCode == UO_AddrOf)
+ Arg = UO->getSubExpr()->IgnoreParenCasts();
+ continue;
+ }
+ break;
+ } while (Arg);
+ if (auto *DRE = dyn_cast<DeclRefExpr>(Arg))
+ return ProtectedThisDecls.contains(DRE->getDecl());
+ return isa<CXXThisExpr>(Arg);
+ }
};
LocalVisitor visitor(this);
@@ -214,53 +280,11 @@ class UncountedLambdaCapturesChecker
} else if (C.capturesThis() && shouldCheckThis) {
if (ignoreParamVarDecl) // this is always a parameter to this function.
continue;
- bool hasProtectThis = false;
- for (const LambdaCapture &OtherCapture : L->captures()) {
- if (!OtherCapture.capturesVariable())
- continue;
- if (auto *ValueDecl = OtherCapture.getCapturedVar()) {
- if (protectThis(ValueDecl)) {
- hasProtectThis = true;
- break;
- }
- }
- }
- if (!hasProtectThis)
- reportBugOnThisPtr(C);
+ reportBugOnThisPtr(C);
}
}
}
- bool protectThis(const ValueDecl *ValueDecl) const {
- auto *VD = dyn_cast<VarDecl>(ValueDecl);
- if (!VD)
- return false;
- auto *Init = VD->getInit()->IgnoreParenCasts();
- if (!Init)
- return false;
- auto *BTE = dyn_cast<CXXBindTemporaryExpr>(Init);
- if (!BTE)
- return false;
- auto *CE = dyn_cast_or_null<CXXConstructExpr>(BTE->getSubExpr());
- if (!CE)
- return false;
- auto *Ctor = CE->getConstructor();
- if (!Ctor)
- return false;
- auto clsName = safeGetName(Ctor->getParent());
- if (!isRefType(clsName) || !CE->getNumArgs())
- return false;
- auto *Arg = CE->getArg(0)->IgnoreParenCasts();
- while (auto *UO = dyn_cast<UnaryOperator>(Arg)) {
- auto OpCode = UO->getOpcode();
- if (OpCode == UO_Deref || OpCode == UO_AddrOf)
- Arg = UO->getSubExpr();
- else
- break;
- }
- return isa<CXXThisExpr>(Arg);
- }
-
void reportBug(const LambdaCapture &Capture, ValueDecl *CapturedVar,
const QualType T) const {
assert(CapturedVar);
diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp
index 4f4a96028225303..4895879c4a385db 100644
--- a/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp
@@ -89,6 +89,7 @@ template <typename Callback> void call(Callback callback) {
someFunction();
callback();
}
+void callAsync(const WTF::Function<void()>&);
void raw_ptr() {
RefCountable* ref_countable = make_obj();
@@ -299,6 +300,29 @@ struct RefCountableWithLambdaCapturingThis {
return obj->next();
});
}
+
+ void callAsyncNoescape([[clang::noescape]] WTF::Function<bool(RefCountable&)>&&);
+ void method_temp_lambda(RefCountable* obj) {
+ callAsyncNoescape([this, otherObj = RefPtr { obj }](auto& obj) {
+ return otherObj == &obj;
+ });
+ }
+
+ void method_nested_lambda() {
+ callAsync([this, protectedThis = Ref { *this }] {
+ callAsync([this, protectedThis = static_cast<const Ref<RefCountableWithLambdaCapturingThis>&&>(protectedThis)] {
+ nonTrivial();
+ });
+ });
+ }
+
+ void method_nested_lambda2() {
+ callAsync([this, protectedThis = RefPtr { this }] {
+ callAsync([this, protectedThis = static_cast<const Ref<RefCountableWithLambdaCapturingThis>&&>(*protectedThis)] {
+ nonTrivial();
+ });
+ });
+ }
};
struct NonRefCountableWithLambdaCapturingThis {
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
t-rasmud
reviewed
Feb 14, 2025
| nonTrivial(); | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Do we need a test for a lambda capture with a this and an unprotected this?
There was a problem hiding this comment.
Oh, we have such a test case. See method_captures_this_unsafe around line 217 :)
There was a problem hiding this comment.
I guess we can add a test case for nested lambdas though.
t-rasmud
approved these changes
Feb 14, 2025
|
Thanks for the review! |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/134/builds/13474 Here is the relevant piece of the build log for the reference |
rniwa
added a commit
to rniwa/llvm-project
that referenced
this pull request
Feb 15, 2025
…s pattern (llvm#126443) In WebKit, it's pretty common to capture "this" and "protectedThis" where "protectedThis" is a guardian variable of type Ref or RefPtr for "this". Furthermore, it's common for this "protectedThis" variable from being passed to an inner lambda by std::move. Recognize this pattern so that we don't emit warnings for nested inner lambdas. To recognize this pattern, we introduce a new DenseSet, ProtectedThisDecls, which contains every "protectedThis" we've recognized to our subclass of DynamicRecursiveASTVisitor. This set is now populated in "hasProtectedThis" and "declProtectsThis" uses this DenseSet to determine a given value declaration constitutes a "protectedThis" pattern or not. Because hasProtectedThis and declProtectsThis had to be moved from the checker class to the visitor class, it's now a responsibility of each caller of visitLambdaExpr to check whether a given lambda captures "this" without a "protectedThis" or not. Finally, this PR improves the code to recognize "protectedThis" pattern by allowing more nested CXXBindTemporaryExpr, CXXOperatorCallExpr, and UnaryOperator expressions.
sivan-shani
pushed a commit
to sivan-shani/llvm-project
that referenced
this pull request
Feb 24, 2025
…s pattern (llvm#126443) In WebKit, it's pretty common to capture "this" and "protectedThis" where "protectedThis" is a guardian variable of type Ref or RefPtr for "this". Furthermore, it's common for this "protectedThis" variable from being passed to an inner lambda by std::move. Recognize this pattern so that we don't emit warnings for nested inner lambdas. To recognize this pattern, we introduce a new DenseSet, ProtectedThisDecls, which contains every "protectedThis" we've recognized to our subclass of DynamicRecursiveASTVisitor. This set is now populated in "hasProtectedThis" and "declProtectsThis" uses this DenseSet to determine a given value declaration constitutes a "protectedThis" pattern or not. Because hasProtectedThis and declProtectsThis had to be moved from the checker class to the visitor class, it's now a responsibility of each caller of visitLambdaExpr to check whether a given lambda captures "this" without a "protectedThis" or not. Finally, this PR improves the code to recognize "protectedThis" pattern by allowing more nested CXXBindTemporaryExpr, CXXOperatorCallExpr, and UnaryOperator expressions.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In WebKit, it's pretty common to capture "this" and "protectedThis" where "protectedThis" is a guardian variable of type Ref or RefPtr for "this". Furthermore, it's common for this "protectedThis" variable from being passed to an inner lambda by std::move. Recognize this pattern so that we don't emit warnings for nested inner lambdas.
To recognize this pattern, we introduce a new DenseSet, ProtectedThisDecls, which contains every "protectedThis" we've recognized to our subclass of DynamicRecursiveASTVisitor. This set is now populated in "hasProtectedThis" and "declProtectsThis" uses this DenseSet to determine a given value declaration constitutes a "protectedThis" pattern or not.
Because hasProtectedThis and declProtectsThis had to be moved from the checker class to the visitor class, it's now a responsibility of each caller of visitLambdaExpr to check whether a given lambda captures "this" without a "protectedThis" or not.
Finally, this PR improves the code to recognize "protectedThis" pattern by allowing more nested CXXBindTemporaryExpr, CXXOperatorCallExpr, and UnaryOperator expressions.