[Analyzer][WebKit] UncountedLambdaCaptureChecker

[NoQ]

Normally this isn't a big deal but in case of this checker i'm veeery curious about the highlighted columns (not just lines) and source ranges. Especially for the implicit case: like, how obvious is it which specific captured variable is problematic? I don't see a better way to test that than making a few FileCheck tests with those ^~~~~~ thingies but i think it's worth at least a couple of such test cases.

You can also add extra source ranges manually with BugReport::addRange.

It's probably also worth it to include the name of the captured variable in the warning message.

[jkorous]

I know what you mean - actionability of these warnings was my major concern too.

[jkorous]

• There's actually no reason to not print the name of the variable in either case.
• I added a test for each explicit/implicit capture and made tests more specific overall.

[jkorous]

• added documentation

[NoQ]

That looks much better, thanks! I've a few more tiny annoying nits.

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

27

I recommend re-using this string because it's used by multiple checkers already.

We usually keep those in CommonBugCategories.h and CommonBugCategories.cpp.

82

I think it should be fairly easy to automatically figure out whether it's a raw pointer or a reference (?) I think we should get this polished in this patch if possible, because we're instantly turning the checker on by default.

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

9–10

Oh my, i see what you did here!

I wonder if the second line actually tests the exact position of ^. Because by default {{xxx}} checks that xxx is a substring of the actual warning (in this case, notice). Might need regex matching to actually match the beginning and the end of the line.

16

So it points to the opening brace? I guess that's good enough with the name of the variable present in the warning text but there's room for experimentation - for instance, with some effort we could probably point to the first use of the variable; i'm not sure it's actually going to be much better but at least it'd point to the actual offending code.

35

Maybe let's put this comment inside so that nobody thought that it applies to the rest of the file :)

[jkorous]

Oh no - nits are welcome as always!

I agree with you on all of account and will work on it in the morning.

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

9–10

Oh my, now I see it too :(

More in the response below.

16

Oh no ... [facepalm]

You just proved your point that I am matching just a substring here - in reality (all the handful of cases I've seen) the caret point to the start of the identifier.

I'd be inclined to say that now that we're printing the name of the variable the snippet isn't critical anymore and we might not necessarily have tests for it. But to be honest I am actually not sure if the current behavior (the caret points to the first occurrence of the implicitly captured variable) is defined or not.

I'll think about it - would prefer to not use multi-line regex magic if possible. Maybe I could somehow use serialized diagnostics here?

[NoQ]

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

16

multi-line regex magic

Mmm, i'm thinking of [[ https://clang.llvm.org/doxygen/classclang_1_1VerifyDiagnosticConsumer.html | the usual -verify regex machinery ]], something like // expected-notice-re{{{{^}} ^{{$}}}}.

[jkorous]

I didn't figure out how to match the code snippet with the caret by using -verify. IIUC it supports only {error,warning,remark,note} diagnostics while the code snippet is neither of these.
I took inspiration from clang/test/Misc/caret-diags-multiline.cpp and just use FileCheck %s --strict-whitespace.

[jkorous]

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

27

I'll do this in a separate patch as I need to touch all the other WebKit checkers too.

[jkorous]

distinguish between raw-ptr & reference

[jkorous]

Friendly ping

[NoQ]

Whoops! Looks great now, i have no further comments, let's land :)

clang/docs/analyzer/checkers.rst

  };
 
  struct Foo {
    RefCntbl * ptr; // warn
    RefCntbl & ptr; // warn
    // ...
  };
 
+.. _webkit-UncountedLambdaCapturesChecker:
+
+webkit.UncountedLambdaCapturesChecker
+"""""""""""""""""""""""""""""""""""""
+Raw pointers and references to uncounted types can't be captured in lambdas. Only ref-counted types are allowed.
+
+.. code-block:: cpp
+
+ struct RefCntbl {
+   void ref() {}
+   void deref() {}
+ };
+
+ void foo(RefCntbl* a, RefCntbl& b) {
+   [&, a](){ // warn about 'a'
+     do_something(b); // warn about 'b'
+   };
+ };
+
 .. _alpha-checkers:
 
 Experimental Checkers
 ---------------------
 
 *These are checkers with known issues or limitations that keep them from being on by default. They are likely to have false positives. Bug reports and especially patches are welcome.*
 
 alpha.clone

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

 def RefCntblBaseVirtualDtorChecker : Checker<"RefCntblBaseVirtualDtor">,
   HelpText<"Check for any ref-countable base class having virtual destructor.">,
   Documentation<HasDocumentation>;
 
 def NoUncountedMemberChecker : Checker<"NoUncountedMemberChecker">,
   HelpText<"Check for no uncounted member variables.">,
   Documentation<HasDocumentation>;
 
+def UncountedLambdaCapturesChecker : Checker<"UncountedLambdaCapturesChecker">,
+  HelpText<"Check uncounted lambda captures.">,
+  Documentation<HasDocumentation>;
+
 } // end webkit
 
 let ParentPackage = WebKitAlpha in {
 
 def UncountedCallArgsChecker : Checker<"UncountedCallArgsChecker">,
   HelpText<"Check uncounted call arguments.">,
   Documentation<HasDocumentation>;
 
 } // end alpha.webkit

clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt

add_clang_library(clangStaticAnalyzerCheckers
   VLASizeChecker.cpp
   ValistChecker.cpp
   VirtualCallChecker.cpp
   WebKit/NoUncountedMembersChecker.cpp
   WebKit/ASTUtils.cpp
   WebKit/PtrTypesSemantics.cpp
   WebKit/RefCntblBaseVirtualDtorChecker.cpp
   WebKit/UncountedCallArgsChecker.cpp
+  WebKit/UncountedLambdaCapturesChecker.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangAnalysis
   clangBasic
   clangLex
   clangStaticAnalyzerCore
 
   DEPENDS
   omp_gen
   )

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

+//=======- UncountedLambdaCapturesChecker.cpp --------------------*- C++ -*-==//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "DiagOutputUtils.h"
+#include "PtrTypesSemantics.h"
+#include "clang/AST/CXXInheritance.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+class UncountedLambdaCapturesChecker
+    : public Checker<check::ASTDecl<TranslationUnitDecl>> {
+private:
+  BugType Bug{this, "Lambda capture of uncounted variable",
+              "WebKit coding guidelines"};
+  mutable BugReporter *BR;
+
+public:
+  void checkASTDecl(const TranslationUnitDecl *TUD, AnalysisManager &MGR,
+                    BugReporter &BRArg) const {
+    BR = &BRArg;
+
+    // The calls to checkAST* from AnalysisConsumer don't
+    // visit template instantiations or lambda classes. We
+    // want to visit those, so we make our own RecursiveASTVisitor.
+    struct LocalVisitor : public RecursiveASTVisitor<LocalVisitor> {
+      const UncountedLambdaCapturesChecker *Checker;
+      explicit LocalVisitor(const UncountedLambdaCapturesChecker *Checker)
+          : Checker(Checker) {
+        assert(Checker);
+      }
+
+      bool shouldVisitTemplateInstantiations() const { return true; }
+      bool shouldVisitImplicitCode() const { return false; }
+
+      bool VisitLambdaExpr(LambdaExpr *L) {
+        Checker->visitLambdaExpr(L);
+        return true;
+      }
+    };
+
+    LocalVisitor visitor(this);
+    visitor.TraverseDecl(const_cast<TranslationUnitDecl *>(TUD));
+  }
+
+  void visitLambdaExpr(LambdaExpr *L) const {
+    for (const LambdaCapture &C : L->captures()) {
+      if (C.capturesVariable()) {
+        VarDecl *CapturedVar = C.getCapturedVar();
+        if (auto *CapturedVarType = CapturedVar->getType().getTypePtrOrNull()) {
+          if (isUncountedPtr(CapturedVarType)) {
+            reportBug(C, CapturedVar, CapturedVarType);
+          }
+        }
+      }
+    }
+  }
+
+  void reportBug(const LambdaCapture &Capture, VarDecl *CapturedVar,
+                 const Type *T) const {
+    assert(CapturedVar);
+
+    SmallString<100> Buf;
+    llvm::raw_svector_ostream Os(Buf);
+
+    if (Capture.isExplicit()) {
+      Os << "Captured ";
+    } else {
+      Os << "Implicitly captured ";
+    }
+    if (T->isPointerType()) {
+      Os << "raw-pointer ";
+    } else {
+      assert(T->isReferenceType());
+      Os << "reference ";
+    }
+
+    printQuotedQualifiedName(Os, Capture.getCapturedVar());
+    Os << " to uncounted type is unsafe.";
+
+    PathDiagnosticLocation BSLoc(Capture.getLocation(), BR->getSourceManager());
+    auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc);
+    BR->emitReport(std::move(Report));
+  }
+};
+} // namespace
+
+void ento::registerUncountedLambdaCapturesChecker(CheckerManager &Mgr) {
+  Mgr.registerChecker<UncountedLambdaCapturesChecker>();
+}
+
+bool ento::shouldRegisterUncountedLambdaCapturesChecker(
+    const CheckerManager &mgr) {
+  return true;
+}

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

+// RUN: %clang_analyze_cc1 -analyzer-checker=webkit.UncountedLambdaCapturesChecker %s 2>&1 | FileCheck %s --strict-whitespace
+#include "mock-types.h"
+
+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:{{^}}  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:{{^}}  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]
+}
+
+void references() {
+  RefCountable automatic;
+  RefCountable& ref_countable_ref = automatic;
+
+  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]
+}
+
+void quiet() {
+// This code is not expected to trigger any warnings.
+  {
+    RefCountable automatic;
+    RefCountable &ref_countable_ref = automatic;
+  }
+
+  auto foo3 = [&]() {};
+  auto foo4 = [=]() {};
+  RefCountable *ref_countable = nullptr;
+}