Bug 1336510 - Part 1: Correct the RefCountedInsideLambda Check to complain about capturing and using `this` without a backing strong reference, r=ehsan
authorMichael Layzell <michael@thelayzells.com>
Fri, 03 Feb 2017 16:53:03 -0500
changeset 394492 ed9b6ec1a0e8789b793bccf1426113e1c78f9ad5
parent 394335 f3b989e0a7b90950c47dd72425c7f669f027c685
child 394493 c92dc508b3b50cbf159b6470254d7b4291daf111
push id1468
push userasasaki@mozilla.com
push dateMon, 05 Jun 2017 19:31:07 +0000
treeherdermozilla-release@0641fc6ee9d1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersehsan
bugs1336510
milestone54.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1336510 - Part 1: Correct the RefCountedInsideLambda Check to complain about capturing and using `this` without a backing strong reference, r=ehsan MozReview-Commit-ID: 19jiqArKgxo
build/clang-plugin/RefCountedInsideLambdaChecker.cpp
build/clang-plugin/RefCountedInsideLambdaChecker.h
build/clang-plugin/tests/TestNoRefcountedInsideLambdas.cpp
--- a/build/clang-plugin/RefCountedInsideLambdaChecker.cpp
+++ b/build/clang-plugin/RefCountedInsideLambdaChecker.cpp
@@ -23,25 +23,29 @@ void RefCountedInsideLambdaChecker::regi
       this);
   AstMatcher->addMatcher(
       classTemplateSpecializationDecl(hasAnyTemplateArgument(refersToType(
         recordType(hasDeclaration(cxxRecordDecl(
           isLambdaDecl()).bind("decl")))))),
       this);
 }
 
+void RefCountedInsideLambdaChecker::emitDiagnostics(SourceLocation Loc,
+                                                    StringRef Name,
+                                                    QualType Type) {
+  diag(Loc, "Refcounted variable '%0' of type %1 cannot be captured by a lambda",
+       DiagnosticIDs::Error) << Name << Type;
+  diag(Loc, "Please consider using a smart pointer",
+       DiagnosticIDs::Note);
+}
+
 void RefCountedInsideLambdaChecker::check(
     const MatchFinder::MatchResult &Result) {
   static DenseSet<const CXXRecordDecl*> CheckedDecls;
 
-  const char* Error =
-      "Refcounted variable %0 of type %1 cannot be captured by a lambda";
-  const char* Note =
-      "Please consider using a smart pointer";
-
   const CXXRecordDecl *Lambda = Result.Nodes.getNodeAs<CXXRecordDecl>("decl");
 
   if (const LambdaExpr *OuterLambda =
     Result.Nodes.getNodeAs<LambdaExpr>("lambdaExpr")) {
     const CXXMethodDecl *OpCall = OuterLambda->getCallOperator();
     QualType ReturnTy = OpCall->getReturnType();
     if (const CXXRecordDecl *Record = ReturnTy->getAsCXXRecordDecl()) {
       Lambda = Record;
@@ -53,23 +57,93 @@ void RefCountedInsideLambdaChecker::chec
   }
 
   // Don't report errors on the same declarations more than once.
   if (CheckedDecls.count(Lambda)) {
     return;
   }
   CheckedDecls.insert(Lambda);
 
-  for (const LambdaCapture Capture : Lambda->captures()) {
-    if (Capture.capturesVariable() && Capture.getCaptureKind() != LCK_ByRef) {
+  bool StrongRefToThisCaptured = false;
+
+  for (const LambdaCapture& Capture : Lambda->captures()) {
+    // Check if any of the captures are ByRef. If they are, we have nothing to
+    // report, as it's OK to capture raw pointers to refcounted objects so long as
+    // the Lambda doesn't escape the current scope, which is required by ByRef
+    // captures already.
+    if (Capture.getCaptureKind() == LCK_ByRef) {
+      return;
+    }
+
+    // Check if this capture is byvalue, and captures a strong reference to this.
+    // XXX: Do we want to make sure that this type which we are capturing is a "Smart Pointer" somehow?
+    if (!StrongRefToThisCaptured &&
+        Capture.capturesVariable() &&
+        Capture.getCaptureKind() == LCK_ByCopy) {
+      const VarDecl *Var = Capture.getCapturedVar();
+      if (Var->hasInit()) {
+        const Stmt *Init = Var->getInit();
+
+        // Ignore single argument constructors, and trivial nodes.
+        while (true) {
+          auto NewInit = IgnoreTrivials(Init);
+          if (auto ConstructExpr = dyn_cast<CXXConstructExpr>(NewInit)) {
+            if (ConstructExpr->getNumArgs() == 1) {
+              NewInit = ConstructExpr->getArg(0);
+            }
+          }
+          if (Init == NewInit) {
+            break;
+          }
+          Init = NewInit;
+        }
+
+        if (isa<CXXThisExpr>(Init)) {
+          StrongRefToThisCaptured = true;
+        }
+      }
+    }
+  }
+
+  // Now we can go through and produce errors for any captured variables or this pointers.
+  for (const LambdaCapture& Capture : Lambda->captures()) {
+    if (Capture.capturesVariable()) {
       QualType Pointee = Capture.getCapturedVar()->getType()->getPointeeType();
 
       if (!Pointee.isNull() && isClassRefCounted(Pointee)) {
-        diag(Capture.getLocation(), Error,
-             DiagnosticIDs::Error) << Capture.getCapturedVar()
-                                   << Pointee;
-        diag(Capture.getLocation(), Note,
-             DiagnosticIDs::Note);
+        emitDiagnostics(Capture.getLocation(), Capture.getCapturedVar()->getName(), Pointee);
+        return;
+      }
+    }
+
+    // The situation with captures of `this` is more complex. All captures of
+    // `this` look the same-ish (they are LCK_This). We want to complain about
+    // captures of `this` where `this` is a refcounted type, and the capture is
+    // actually used in the body of the lambda (if the capture isn't used, then
+    // we don't care, because it's only being captured in order to give access
+    // to private methods).
+    //
+    // In addition, we don't complain about this, even if it is used, if it was
+    // captured implicitly when the LambdaCaptureDefault was LCD_ByRef, as that
+    // expresses the intent that the lambda won't leave the enclosing scope.
+    bool ImplicitByRefDefaultedCapture =
+      Capture.isImplicit() && Lambda->getLambdaCaptureDefault() == LCD_ByRef;
+    if (Capture.capturesThis() &&
+        !ImplicitByRefDefaultedCapture &&
+        !StrongRefToThisCaptured) {
+      ThisVisitor V(*this);
+      bool NotAborted = V.TraverseDecl(const_cast<CXXMethodDecl *>(Lambda->getLambdaCallOperator()));
+      if (!NotAborted) {
         return;
       }
     }
   }
 }
+
+bool RefCountedInsideLambdaChecker::ThisVisitor::VisitCXXThisExpr(CXXThisExpr *This) {
+  QualType Pointee = This->getType()->getPointeeType();
+  if (!Pointee.isNull() && isClassRefCounted(Pointee)) {
+    Checker.emitDiagnostics(This->getLocStart(), "this", Pointee);
+    return false;
+  }
+
+  return true;
+}
--- a/build/clang-plugin/RefCountedInsideLambdaChecker.h
+++ b/build/clang-plugin/RefCountedInsideLambdaChecker.h
@@ -9,11 +9,24 @@
 
 class RefCountedInsideLambdaChecker : public BaseCheck {
 public:
   RefCountedInsideLambdaChecker(StringRef CheckName,
                                 ContextType *Context = nullptr)
     : BaseCheck(CheckName, Context) {}
   void registerMatchers(MatchFinder* AstMatcher) override;
   void check(const MatchFinder::MatchResult &Result) override;
+
+  void emitDiagnostics(SourceLocation Loc, StringRef Name, QualType Type);
+
+private:
+  class ThisVisitor : public RecursiveASTVisitor<ThisVisitor> {
+  public:
+    explicit ThisVisitor(RefCountedInsideLambdaChecker& Checker)
+      : Checker(Checker) {}
+
+    bool VisitCXXThisExpr(CXXThisExpr *This);
+  private:
+    RefCountedInsideLambdaChecker& Checker;
+  };
 };
 
 #endif
--- a/build/clang-plugin/tests/TestNoRefcountedInsideLambdas.cpp
+++ b/build/clang-plugin/tests/TestNoRefcountedInsideLambdas.cpp
@@ -1,24 +1,29 @@
 #include <functional>
 #define MOZ_STRONG_REF __attribute__((annotate("moz_strong_ref")))
+#define MOZ_IMPLICIT __attribute__((annotate("moz_implicit")))
 
 struct RefCountedBase {
   void AddRef();
   void Release();
 };
 
 template <class T>
 struct SmartPtr {
+  SmartPtr();
+  MOZ_IMPLICIT SmartPtr(T*);
   T* MOZ_STRONG_REF t;
   T* operator->() const;
 };
 
 struct R : RefCountedBase {
   void method();
+private:
+  void privateMethod();
 };
 
 void take(...);
 void foo() {
   R* ptr;
   SmartPtr<R> sp;
   take([&](R* argptr) {
     R* localptr;
@@ -542,8 +547,75 @@ void e() {
     return ([&sp](SmartPtr<R> argsp) {
       SmartPtr<R> localsp;
       take(sp);
       take(argsp);
       take(localsp);
     });
   };
 }
+
+void
+R::privateMethod() {
+  SmartPtr<R> self = this;
+  std::function<void()>([&]() {
+    self->method();
+  });
+  std::function<void()>([&]() {
+    self->privateMethod();
+  });
+  std::function<void()>([&]() {
+    this->method();
+  });
+  std::function<void()>([&]() {
+    this->privateMethod();
+  });
+  std::function<void()>([=]() {
+    self->method();
+  });
+  std::function<void()>([=]() {
+    self->privateMethod();
+  });
+  std::function<void()>([=]() {
+    this->method(); // expected-error{{Refcounted variable 'this' of type 'R' cannot be captured by a lambda}} expected-note{{Please consider using a smart pointer}}
+  });
+  std::function<void()>([=]() {
+    this->privateMethod(); // expected-error{{Refcounted variable 'this' of type 'R' cannot be captured by a lambda}} expected-note{{Please consider using a smart pointer}}
+  });
+  std::function<void()>([self]() {
+    self->method();
+  });
+  std::function<void()>([self]() {
+    self->privateMethod();
+  });
+  std::function<void()>([this]() {
+    this->method(); // expected-error{{Refcounted variable 'this' of type 'R' cannot be captured by a lambda}} expected-note{{Please consider using a smart pointer}}
+  });
+  std::function<void()>([this]() {
+    this->privateMethod(); // expected-error{{Refcounted variable 'this' of type 'R' cannot be captured by a lambda}} expected-note{{Please consider using a smart pointer}}
+  });
+  std::function<void()>([this]() {
+    method(); // expected-error{{Refcounted variable 'this' of type 'R' cannot be captured by a lambda}} expected-note{{Please consider using a smart pointer}}
+  });
+  std::function<void()>([this]() {
+    privateMethod(); // expected-error{{Refcounted variable 'this' of type 'R' cannot be captured by a lambda}} expected-note{{Please consider using a smart pointer}}
+  });
+  std::function<void()>([=]() {
+    method(); // expected-error{{Refcounted variable 'this' of type 'R' cannot be captured by a lambda}} expected-note{{Please consider using a smart pointer}}
+  });
+  std::function<void()>([=]() {
+    privateMethod(); // expected-error{{Refcounted variable 'this' of type 'R' cannot be captured by a lambda}} expected-note{{Please consider using a smart pointer}}
+  });
+  std::function<void()>([&]() {
+    method();
+  });
+  std::function<void()>([&]() {
+    privateMethod();
+  });
+
+  // It should be OK to go through `this` if we have captured a reference to it.
+  std::function<void()>([this, self]() {
+    this->method();
+    this->privateMethod();
+    method();
+    privateMethod();
+  });
+}