Bug 1336510 - Part 1: Correct the RefCountedInsideLambda Check to complain about capturing and using `this` without a backing strong reference, r=ehsan, a=abillings
authorMichael Layzell <michael@thelayzells.com>
Fri, 03 Feb 2017 16:53:03 -0500
changeset 376468 101239f350b5bd19db60d97c14d55d07cf9c658d
parent 376467 c269adcb4e01ae92e215efe6232a5ab28a9c3949
child 376469 38d32d83202ead3d878468ed2f673ad648b2c36d
push id6996
push userjlorenzo@mozilla.com
push dateMon, 06 Mar 2017 20:48:21 +0000
treeherdermozilla-beta@d89512dab048 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersehsan, abillings
bugs1336510
milestone53.0a2
Bug 1336510 - Part 1: Correct the RefCountedInsideLambda Check to complain about capturing and using `this` without a backing strong reference, r=ehsan, a=abillings 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();
+  });
+}