[alpha.webkit.UnretainedLambdaCapturesChecker] Add the support for protectedSelf (llvm#132518)

This PR adds the support for treating capturing of "self" as safe if the
lambda simultaneously captures "protectedSelf", which is a RetainPtr of
"self".

This PR also fixes a bug that the checker wasn't generating a warning
when "self" is implicitly captured. Note when "self" is implicitly
captured, we use the lambda's getBeginLoc as a fallback source location.

Author: rniwa

clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp

@@ -147,7 +147,7 @@ bool isCtorOfCheckedPtr(const clang::FunctionDecl *F) {
 bool isCtorOfRetainPtr(const clang::FunctionDecl *F) {
   const std::string &FunctionName = safeGetName(F);
   return FunctionName == "RetainPtr" || FunctionName == "adoptNS" ||
-         FunctionName == "adoptCF";
+         FunctionName == "adoptCF" || FunctionName == "retainPtr";
 }
 
 bool isCtorOfSafePtr(const clang::FunctionDecl *F) {

clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLambdaCapturesChecker.cpp

@@ -36,6 +36,7 @@ class RawPtrRefLambdaCapturesChecker
       : Bug(this, description, "WebKit coding guidelines") {}
 
   virtual std::optional<bool> isUnsafePtr(QualType) const = 0;
+  virtual bool isPtrType(const std::string &) const = 0;
   virtual const char *ptrKind(QualType QT) const = 0;
 
   void checkASTDecl(const TranslationUnitDecl *TUD, AnalysisManager &MGR,
@@ -71,6 +72,15 @@ class RawPtrRefLambdaCapturesChecker
         return Base::TraverseCXXMethodDecl(CXXMD);
       }
 
+      bool TraverseObjCMethodDecl(ObjCMethodDecl *OCMD) {
+        llvm::SaveAndRestore SavedDecl(ClsType);
+        if (OCMD && OCMD->isInstanceMethod()) {
+          if (auto *ImplParamDecl = OCMD->getSelfDecl())
+            ClsType = ImplParamDecl->getType();
+        }
+        return Base::TraverseObjCMethodDecl(OCMD);
+      }
+
       bool VisitTypedefDecl(TypedefDecl *TD) {
         if (Checker->RTC)
           Checker->RTC->visitTypedef(TD);
@@ -278,10 +288,10 @@ class RawPtrRefLambdaCapturesChecker
         auto *VD = dyn_cast<VarDecl>(ValueDecl);
         if (!VD)
           return false;
-        auto *Init = VD->getInit()->IgnoreParenCasts();
+        auto *Init = VD->getInit();
         if (!Init)
           return false;
-        const Expr *Arg = Init;
+        const Expr *Arg = Init->IgnoreParenCasts();
         do {
           if (auto *BTE = dyn_cast<CXXBindTemporaryExpr>(Arg))
             Arg = BTE->getSubExpr()->IgnoreParenCasts();
@@ -290,7 +300,7 @@ class RawPtrRefLambdaCapturesChecker
             if (!Ctor)
               return false;
             auto clsName = safeGetName(Ctor->getParent());
-            if (isRefType(clsName) && CE->getNumArgs()) {
+            if (Checker->isPtrType(clsName) && CE->getNumArgs()) {
               Arg = CE->getArg(0)->IgnoreParenCasts();
               continue;
             }
@@ -310,6 +320,12 @@ class RawPtrRefLambdaCapturesChecker
               Arg = CE->getArg(0)->IgnoreParenCasts();
               continue;
             }
+            if (auto *Callee = CE->getDirectCallee()) {
+              if (isCtorOfSafePtr(Callee) && CE->getNumArgs() == 1) {
+                Arg = CE->getArg(0)->IgnoreParenCasts();
+                continue;
+              }
+            }
           }
           if (auto *OpCE = dyn_cast<CXXOperatorCallExpr>(Arg)) {
             auto OpCode = OpCE->getOperator();
@@ -318,7 +334,7 @@ class RawPtrRefLambdaCapturesChecker
               if (!Callee)
                 return false;
               auto clsName = safeGetName(Callee->getParent());
-              if (!isRefType(clsName) || !OpCE->getNumArgs())
+              if (!Checker->isPtrType(clsName) || !OpCE->getNumArgs())
                 return false;
               Arg = OpCE->getArg(0)->IgnoreParenCasts();
               continue;
@@ -333,8 +349,15 @@ class RawPtrRefLambdaCapturesChecker
           }
           break;
         } while (Arg);
-        if (auto *DRE = dyn_cast<DeclRefExpr>(Arg))
-          return ProtectedThisDecls.contains(DRE->getDecl());
+        if (auto *DRE = dyn_cast<DeclRefExpr>(Arg)) {
+          auto *Decl = DRE->getDecl();
+          if (auto *ImplicitParam = dyn_cast<ImplicitParamDecl>(Decl)) {
+            auto kind = ImplicitParam->getParameterKind();
+            return kind == ImplicitParamKind::ObjCSelf ||
+                   kind == ImplicitParamKind::CXXThis;
+          }
+          return ProtectedThisDecls.contains(Decl);
+        }
         return isa<CXXThisExpr>(Arg);
       }
     };
@@ -354,10 +377,17 @@ class RawPtrRefLambdaCapturesChecker
         ValueDecl *CapturedVar = C.getCapturedVar();
         if (ignoreParamVarDecl && isa<ParmVarDecl>(CapturedVar))
           continue;
+        if (auto *ImplicitParam = dyn_cast<ImplicitParamDecl>(CapturedVar)) {
+          auto kind = ImplicitParam->getParameterKind();
+          if ((kind == ImplicitParamKind::ObjCSelf ||
+               kind == ImplicitParamKind::CXXThis) &&
+              !shouldCheckThis)
+            continue;
+        }
         QualType CapturedVarQualType = CapturedVar->getType();
         auto IsUncountedPtr = isUnsafePtr(CapturedVar->getType());
         if (IsUncountedPtr && *IsUncountedPtr)
-          reportBug(C, CapturedVar, CapturedVarQualType);
+          reportBug(C, CapturedVar, CapturedVarQualType, L);
       } else if (C.capturesThis() && shouldCheckThis) {
         if (ignoreParamVarDecl) // this is always a parameter to this function.
           continue;
@@ -367,11 +397,12 @@ class RawPtrRefLambdaCapturesChecker
   }
 
   void reportBug(const LambdaCapture &Capture, ValueDecl *CapturedVar,
-                 const QualType T) const {
+                 const QualType T, LambdaExpr *L) const {
     assert(CapturedVar);
 
-    if (isa<ImplicitParamDecl>(CapturedVar) && !Capture.getLocation().isValid())
-      return; // Ignore implicit captruing of self.
+    auto Location = Capture.getLocation();
+    if (isa<ImplicitParamDecl>(CapturedVar) && !Location.isValid())
+      Location = L->getBeginLoc();
 
     SmallString<100> Buf;
     llvm::raw_svector_ostream Os(Buf);
@@ -390,7 +421,7 @@ class RawPtrRefLambdaCapturesChecker
     printQuotedQualifiedName(Os, CapturedVar);
     Os << " to " << ptrKind(T) << " type is unsafe.";
 
-    PathDiagnosticLocation BSLoc(Capture.getLocation(), BR->getSourceManager());
+    PathDiagnosticLocation BSLoc(Location, BR->getSourceManager());
     auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc);
     BR->emitReport(std::move(Report));
   }
@@ -432,6 +463,10 @@ class UncountedLambdaCapturesChecker : public RawPtrRefLambdaCapturesChecker {
     return result2;
   }
 
+  virtual bool isPtrType(const std::string &Name) const final {
+    return isRefType(Name) || isCheckedPtr(Name);
+  }
+
   const char *ptrKind(QualType QT) const final {
     if (isUncounted(QT))
       return "uncounted";
@@ -451,6 +486,10 @@ class UnretainedLambdaCapturesChecker : public RawPtrRefLambdaCapturesChecker {
     return RTC->isUnretained(QT);
   }
 
+  virtual bool isPtrType(const std::string &Name) const final {
+    return isRetainPtr(Name);
+  }
+
   const char *ptrKind(QualType QT) const final { return "unretained"; }
 };
 

clang/test/Analysis/Checkers/WebKit/unretained-lambda-captures-arc.mm

@@ -256,6 +256,7 @@ @interface ObjWithSelf : NSObject {
   RetainPtr<id> delegate;
 }
 -(void)doWork;
+-(void)doMoreWork;
 -(void)run;
 @end
 
@@ -265,9 +266,28 @@ -(void)doWork {
     someFunction();
     [delegate doWork];
   };
+  auto doMoreWork = [=] {
+    someFunction();
+    [delegate doWork];
+  };
+  auto doExtraWork = [&, protectedSelf = retainPtr(self)] {
+    someFunction();
+    [delegate doWork];
+  };
   callFunctionOpaque(doWork);
+  callFunctionOpaque(doMoreWork);
+  callFunctionOpaque(doExtraWork);
 }
+
+-(void)doMoreWork {
+  auto doWork = [self, protectedSelf = retainPtr(self)] {
+    someFunction();
+    [self doWork];
+  };
+  callFunctionOpaque(doWork);
+}
+
 -(void)run {
   someFunction();
 }
-@end
+@end

clang/test/Analysis/Checkers/WebKit/unretained-lambda-captures.mm

@@ -279,18 +279,40 @@ @interface ObjWithSelf : NSObject {
   RetainPtr<id> delegate;
 }
 -(void)doWork;
+-(void)doMoreWork;
 -(void)run;
 @end
 
 @implementation ObjWithSelf
 -(void)doWork {
   auto doWork = [&] {
+    // expected-warning@-1{{Implicitly captured raw-pointer 'self' to unretained type is unsafe [alpha.webkit.UnretainedLambdaCapturesChecker]}}
+    someFunction();
+    [delegate doWork];
+  };
+  auto doMoreWork = [=] {
+    // expected-warning@-1{{Implicitly captured raw-pointer 'self' to unretained type is unsafe [alpha.webkit.UnretainedLambdaCapturesChecker]}}
+    someFunction();
+    [delegate doWork];
+  };
+  auto doExtraWork = [&, protectedSelf = retainPtr(self)] {
     someFunction();
     [delegate doWork];
   };
   callFunctionOpaque(doWork);
+  callFunctionOpaque(doMoreWork);
+  callFunctionOpaque(doExtraWork);
 }
+
+-(void)doMoreWork {
+  auto doWork = [self, protectedSelf = retainPtr(self)] {
+    someFunction();
+    [self doWork];
+  };
+  callFunctionOpaque(doWork);
+}
+
 -(void)run {
   someFunction();
 }
-@end
+@end