[webkit.UncountedLambdaCapturesChecker] Ignore trivial functions and [[clang::noescape]].

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

@@ -63,6 +63,10 @@ std::optional<bool> isUncounted(const clang::CXXRecordDecl* Class);
 /// class, false if not, std::nullopt if inconclusive.
 std::optional<bool> isUncountedPtr(const clang::QualType T);
 
+/// \returns true if \p T is either a raw pointer or reference to an uncounted
+/// or unchecked class, false if not, std::nullopt if inconclusive.
+std::optional<bool> isUnsafePtr(const QualType T);
+
 /// \returns true if \p T is a RefPtr, Ref, CheckedPtr, CheckedRef, or its
 /// variant, false if not.
 bool isSafePtrType(const clang::QualType T);

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

@@ -6,6 +6,7 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "ASTUtils.h"
 #include "DiagOutputUtils.h"
 #include "PtrTypesSemantics.h"
 #include "clang/AST/CXXInheritance.h"
@@ -26,6 +27,7 @@ class UncountedLambdaCapturesChecker
   BugType Bug{this, "Lambda capture of uncounted variable",
               "WebKit coding guidelines"};
   mutable BugReporter *BR = nullptr;
+  TrivialFunctionAnalysis TFA;
 
 public:
   void checkASTDecl(const TranslationUnitDecl *TUD, AnalysisManager &MGR,
@@ -37,6 +39,8 @@ class UncountedLambdaCapturesChecker
     // want to visit those, so we make our own RecursiveASTVisitor.
     struct LocalVisitor : public RecursiveASTVisitor<LocalVisitor> {
       const UncountedLambdaCapturesChecker *Checker;
+      llvm::DenseSet<const DeclRefExpr *> DeclRefExprsToIgnore;
+
       explicit LocalVisitor(const UncountedLambdaCapturesChecker *Checker)
           : Checker(Checker) {
         assert(Checker);
@@ -45,8 +49,62 @@ class UncountedLambdaCapturesChecker
       bool shouldVisitTemplateInstantiations() const { return true; }
       bool shouldVisitImplicitCode() const { return false; }
 
-      bool VisitLambdaExpr(LambdaExpr *L) {
-        Checker->visitLambdaExpr(L);
+      bool VisitDeclRefExpr(DeclRefExpr *DRE) {
+        if (DeclRefExprsToIgnore.contains(DRE))
+          return true;
+        if (auto *VD = dyn_cast_or_null<VarDecl>(DRE->getDecl())) {
+          auto *Init = VD->getInit()->IgnoreParenCasts();
+          if (auto *L = dyn_cast_or_null<LambdaExpr>(Init)) {
+            Checker->visitLambdaExpr(L);
+            return true;
+          }
+        }
+        return true;
+      }
+
+      // WTF::switchOn(T, F... f) is a variadic template function and couldn't
+      // be annotated with NOESCAPE. We hard code it here to workaround that.
+      bool shouldTreatAllArgAsNoEscape(FunctionDecl *Decl) {
+        auto *NsDecl = Decl->getParent();
+        if (!NsDecl || !isa<NamespaceDecl>(NsDecl))
+          return false;
+        return safeGetName(NsDecl) == "WTF" && safeGetName(Decl) == "switchOn";
+      }
+
+      bool VisitCallExpr(CallExpr *CE) {
+        if (auto *Callee = CE->getCallee()) {
+          if (auto *DRE = dyn_cast<DeclRefExpr>(Callee->IgnoreParenCasts())) {
+            if (auto *MD = dyn_cast_or_null<CXXMethodDecl>(DRE->getDecl())) {
+              if (CE->getNumArgs() == 1) {
+                auto *Arg = CE->getArg(0)->IgnoreParenCasts();
+                if (auto *DRE = dyn_cast<DeclRefExpr>(Arg)) {
+                  if (auto *VD = dyn_cast_or_null<VarDecl>(DRE->getDecl())) {
+                    auto *Init = VD->getInit()->IgnoreParenCasts();
+                    if (auto *L = dyn_cast_or_null<LambdaExpr>(Init)) {
+                      DeclRefExprsToIgnore.insert(DRE);
+                      Checker->visitLambdaExpr(L,
+                                               /* ignoreParamVarDecl */ true);
+                    }
+                  }
+                }
+              }
+            }
+          }
+        }
+        if (auto *Callee = CE->getDirectCallee()) {
+          bool TreatAllArgsAsNoEscape = shouldTreatAllArgAsNoEscape(Callee);
+          unsigned ArgIndex = 0;
+          for (auto *Param : Callee->parameters()) {
+            if (ArgIndex >= CE->getNumArgs())
+              break;
+            auto *Arg = CE->getArg(ArgIndex)->IgnoreParenCasts();
+            if (!Param->hasAttr<NoEscapeAttr>() && !TreatAllArgsAsNoEscape) {
+              if (auto *L = dyn_cast_or_null<LambdaExpr>(Arg))
+                Checker->visitLambdaExpr(L);
+            }
+            ++ArgIndex;
+          }
+        }
         return true;
       }
     };
@@ -55,22 +113,28 @@ class UncountedLambdaCapturesChecker
     visitor.TraverseDecl(const_cast<TranslationUnitDecl *>(TUD));
   }
 
-  void visitLambdaExpr(LambdaExpr *L) const {
+  void visitLambdaExpr(LambdaExpr *L, bool ignoreParamVarDecl = false) const {
+    if (TFA.isTrivial(L->getBody()))
+      return;
     for (const LambdaCapture &C : L->captures()) {
       if (C.capturesVariable()) {
         ValueDecl *CapturedVar = C.getCapturedVar();
+        if (ignoreParamVarDecl && isa<ParmVarDecl>(CapturedVar))
+          continue;
         QualType CapturedVarQualType = CapturedVar->getType();
-        if (auto *CapturedVarType = CapturedVarQualType.getTypePtrOrNull()) {
-          auto IsUncountedPtr = isUncountedPtr(CapturedVarQualType);
-          if (IsUncountedPtr && *IsUncountedPtr)
-            reportBug(C, CapturedVar, CapturedVarType);
-        }
+        auto IsUncountedPtr = isUnsafePtr(CapturedVar->getType());
+        if (IsUncountedPtr && *IsUncountedPtr)
+          reportBug(C, CapturedVar, CapturedVarQualType);
+      } else if (C.capturesThis()) {
+        if (ignoreParamVarDecl) // this is always a parameter to this function.
+          continue;
+        reportBugOnThisPtr(C);
       }
     }
   }
 
   void reportBug(const LambdaCapture &Capture, ValueDecl *CapturedVar,
-                 const Type *T) const {
+                 const QualType T) const {
     assert(CapturedVar);
 
     SmallString<100> Buf;
@@ -89,7 +153,27 @@ class UncountedLambdaCapturesChecker
     }
 
     printQuotedQualifiedName(Os, Capture.getCapturedVar());
-    Os << " to uncounted type is unsafe.";
+    Os << " to ref-counted / CheckedPtr capable type is unsafe.";
+
+    PathDiagnosticLocation BSLoc(Capture.getLocation(), BR->getSourceManager());
+    auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc);
+    BR->emitReport(std::move(Report));
+  }
+
+  void reportBugOnThisPtr(const LambdaCapture &Capture) const {
+    assert(CapturedVar);
+
+    SmallString<100> Buf;
+    llvm::raw_svector_ostream Os(Buf);
+
+    if (Capture.isExplicit()) {
+      Os << "Captured ";
+    } else {
+      Os << "Implicitly captured ";
+    }
+
+    Os << "raw-pointer 'this' to ref-counted / CheckedPtr capable type is "
+          "unsafe.";
 
     PathDiagnosticLocation BSLoc(Capture.getLocation(), BR->getSourceManager());
     auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc);

clang/test/Analysis/Checkers/WebKit/mock-types.h

@@ -103,7 +103,9 @@ struct RefCountable {
   void ref() {}
   void deref() {}
   void method();
+  void constMethod() const;
   int trivial() { return 123; }
+  RefCountable* next();
 };
 
 template <typename T> T *downcast(T *t) { return t; }

clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp

@@ -1,39 +1,73 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=webkit.UncountedLambdaCapturesChecker %s 2>&1 | FileCheck %s --strict-whitespace
-#include "mock-types.h"
+// RUN: %clang_analyze_cc1 -analyzer-checker=webkit.UncountedLambdaCapturesChecker -verify %s
+//#include "mock-types.h"
+
+struct RefCountable {
+//  static Ref<RefCountable> create();
+  void ref() {}
+  void deref() {}
+  void method();
+  void constMethod() const;
+  int trivial() { return 123; }
+  RefCountable* next();
+};
+
+RefCountable* make_obj();
+
+void someFunction();
+template <typename Callback> void call(Callback callback) {
+  someFunction();
+  callback();
+}
 
 void raw_ptr() {
-  RefCountable* ref_countable = nullptr;
-  auto foo1 = [ref_countable](){};
-  // CHECK: warning: Captured raw-pointer 'ref_countable' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker]
-  // CHECK-NEXT:{{^   6 | }}  auto foo1 = [ref_countable](){};
-  // CHECK-NEXT:{{^     | }}               ^
-  auto foo2 = [&ref_countable](){};
-  // CHECK: warning: Captured raw-pointer 'ref_countable' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker]
-  auto foo3 = [&](){ ref_countable = nullptr; };
-  // CHECK: warning: Implicitly captured raw-pointer 'ref_countable' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker]
-  // CHECK-NEXT:{{^  12 | }}  auto foo3 = [&](){ ref_countable = nullptr; };
-  // CHECK-NEXT:{{^     | }}                     ^
-  auto foo4 = [=](){ (void) ref_countable; };
-  // CHECK: warning: Implicitly captured raw-pointer 'ref_countable' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker]
+  RefCountable* ref_countable = make_obj();
+  auto foo1 = [ref_countable](){
+    // expected-warning@-1{{Captured raw-pointer 'ref_countable' to ref-counted / CheckedPtr capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}}
+    ref_countable->method();
+  };
+  auto foo2 = [&ref_countable](){
+    // expected-warning@-1{{Captured raw-pointer 'ref_countable' to ref-counted / CheckedPtr capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}}
+    ref_countable->method();
+  };
+  auto foo3 = [&](){
+    ref_countable->method();
+    // expected-warning@-1{{Implicitly captured raw-pointer 'ref_countable' to ref-counted / CheckedPtr capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}}
+    ref_countable = nullptr;
+  };
+
+  auto foo4 = [=](){
+    ref_countable->method();
+    // expected-warning@-1{{Implicitly captured raw-pointer 'ref_countable' to ref-counted / CheckedPtr capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}}
+  };
+
+  call(foo1);
+  call(foo2);
+  call(foo3);
+  call(foo4);
 
   // Confirm that the checker respects [[clang::suppress]].
   RefCountable* suppressed_ref_countable = nullptr;
   [[clang::suppress]] auto foo5 = [suppressed_ref_countable](){};
-  // CHECK-NOT: warning: Captured raw-pointer 'suppressed_ref_countable' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker]
+  // no warning.
+  call(foo5);
 }
 
 void references() {
   RefCountable automatic;
   RefCountable& ref_countable_ref = automatic;
+  auto foo1 = [ref_countable_ref](){ ref_countable_ref.constMethod(); };
+  // expected-warning@-1{{Captured reference 'ref_countable_ref' to ref-counted / CheckedPtr capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}}
+  auto foo2 = [&ref_countable_ref](){ ref_countable_ref.method(); };
+  // expected-warning@-1{{Captured reference 'ref_countable_ref' to ref-counted / CheckedPtr capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}}
+  auto foo3 = [&](){ ref_countable_ref.method(); };
+  // expected-warning@-1{{Implicitly captured reference 'ref_countable_ref' to ref-counted / CheckedPtr capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}}
+  auto foo4 = [=](){ ref_countable_ref.constMethod(); };
+  // expected-warning@-1{{Implicitly captured reference 'ref_countable_ref' to ref-counted / CheckedPtr capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}}
 
-  auto foo1 = [ref_countable_ref](){};
-  // CHECK: warning: Captured reference 'ref_countable_ref' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker]
-  auto foo2 = [&ref_countable_ref](){};
-  // CHECK: warning: Captured reference 'ref_countable_ref' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker]
-  auto foo3 = [&](){ (void) ref_countable_ref; };
-  // CHECK: warning: Implicitly captured reference 'ref_countable_ref' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker]
-  auto foo4 = [=](){ (void) ref_countable_ref; };
-  // CHECK: warning: Implicitly captured reference 'ref_countable_ref' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker]
+  call(foo1);
+  call(foo2);
+  call(foo3);
+  call(foo4);
 }
 
 void quiet() {
@@ -45,5 +79,82 @@ void quiet() {
 
   auto foo3 = [&]() {};
   auto foo4 = [=]() {};
+
+  call(foo3);
+  call(foo4);
+
   RefCountable *ref_countable = nullptr;
 }
+
+template <typename Callback>
+void map(RefCountable* start, [[clang::noescape]] Callback&& callback)
+{
+  while (start) {
+    callback(*start);
+    start = start->next();
+  }
+}
+
+template <typename Callback1, typename Callback2>
+void doubleMap(RefCountable* start, [[clang::noescape]] Callback1&& callback1, Callback2&& callback2)
+{
+  while (start) {
+    callback1(*start);
+    callback2(*start);
+    start = start->next();
+  }
+}
+
+void noescape_lambda() {
+  RefCountable* someObj = make_obj();
+  RefCountable* otherObj = make_obj();
+  map(make_obj(), [&](RefCountable& obj) {
+    otherObj->method();
+  });
+  doubleMap(make_obj(), [&](RefCountable& obj) {
+    otherObj->method();
+  }, [&](RefCountable& obj) {
+    otherObj->method();
+    // expected-warning@-1{{Implicitly captured raw-pointer 'otherObj' to ref-counted / CheckedPtr capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}}
+  });
+  ([&] {
+    someObj->method();
+  })();
+}
+
+void lambda_capture_param(RefCountable* obj) {
+  auto someLambda = [&] {
+    obj->method();
+  };
+  someLambda();
+  someLambda();
+}
+
+struct RefCountableWithLambdaCapturingThis {
+  void ref() const;
+  void deref() const;
+  void nonTrivial();
+
+  void method_captures_this_safe() {
+    auto lambda = [&]() {
+      nonTrivial();
+    };
+    lambda();
+  }
+
+  void method_captures_this_unsafe() {
+    auto lambda = [&]() {
+      nonTrivial();
+      // expected-warning@-1{{Implicitly captured raw-pointer 'this' to ref-counted / CheckedPtr capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}}
+    };
+    call(lambda);
+  }
+};
+
+void trivial_lambda() {
+  RefCountable* ref_countable = make_obj();
+  auto trivial_lambda = [&]() {
+    return ref_countable->trivial();
+  };
+  trivial_lambda();
+}