[alpha.webkit.NoUnretainedMemberChecker] Add a new WebKit checker for unretained member variables and ivars.

Add a new WebKit checker for member variables and instance variables of NS and CF types.
A member variable or instance variable to a CF type should be RetainPtr regardless of whether
ARC is enabled or disabled, and that of a NS type should be RetainPtr when ARC is disabled.

Author: rniwa

clang/docs/analyzer/checkers.rst

@@ -3487,6 +3487,19 @@ Raw pointers and references to an object which supports CheckedPtr or CheckedRef
 
 See `WebKit Guidelines for Safer C++ Programming <https://github.com/WebKit/WebKit/wiki/Safer-CPP-Guidelines>`_ for details.
 
+alpha.webkit.NoUnretainedMemberChecker
+""""""""""""""""""""""""""""""""""""""""
+Raw pointers and references to a NS or CF object can't be used as class members or ivars. Only RetainPtr is allowed.
+
+.. code-block:: cpp
+
+ struct Foo {
+   NSObject *ptr; // warn
+   // ...
+ };
+
+See `WebKit Guidelines for Safer C++ Programming <https://github.com/WebKit/WebKit/wiki/Safer-CPP-Guidelines>`_ for details.
+
 .. _alpha-webkit-UncountedCallArgsChecker:
 
 alpha.webkit.UncountedCallArgsChecker

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

@@ -1766,6 +1766,10 @@ def NoUncheckedPtrMemberChecker : Checker<"NoUncheckedPtrMemberChecker">,
   HelpText<"Check for no unchecked member variables.">,
   Documentation<HasDocumentation>;
 
+def NoUnretainedMemberChecker : Checker<"NoUnretainedMemberChecker">,
+  HelpText<"Check for no unretained member variables.">,
+  Documentation<HasDocumentation>;
+
 def UncountedCallArgsChecker : Checker<"UncountedCallArgsChecker">,
   HelpText<"Check uncounted call arguments.">,
   Documentation<HasDocumentation>;

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

@@ -31,12 +31,16 @@ class RawPtrRefMemberChecker
   BugType Bug;
   mutable BugReporter *BR;
 
+protected:
+  mutable std::optional<RetainTypeChecker> RTC;
+
 public:
   RawPtrRefMemberChecker(const char *description)
       : Bug(this, description, "WebKit coding guidelines") {}
 
   virtual std::optional<bool>
-  isPtrCompatible(const clang::CXXRecordDecl *) const = 0;
+  isPtrCompatible(const clang::QualType,
+                  const clang::CXXRecordDecl *R) const = 0;
   virtual bool isPtrCls(const clang::CXXRecordDecl *) const = 0;
   virtual const char *typeName() const = 0;
   virtual const char *invariant() const = 0;
@@ -57,6 +61,12 @@ class RawPtrRefMemberChecker
         ShouldVisitImplicitCode = false;
       }
 
+      bool VisitTypedefDecl(const TypedefDecl *TD) override {
+        if (Checker->RTC)
+          Checker->RTC->visitTypedef(TD);
+        return true;
+      }
+
       bool VisitRecordDecl(const RecordDecl *RD) override {
         Checker->visitRecordDecl(RD);
         return true;
@@ -69,6 +79,8 @@ class RawPtrRefMemberChecker
     };
 
     LocalVisitor visitor(this);
+    if (RTC)
+      RTC->visitTranslationUnitDecl(TUD);
     visitor.TraverseDecl(TUD);
   }
 
@@ -77,16 +89,22 @@ class RawPtrRefMemberChecker
       return;
 
     for (auto *Member : RD->fields()) {
-      const Type *MemberType = Member->getType().getTypePtrOrNull();
+      auto QT = Member->getType();
+      const Type *MemberType = QT.getTypePtrOrNull();
       if (!MemberType)
         continue;
 
       if (auto *MemberCXXRD = MemberType->getPointeeCXXRecordDecl()) {
-        // If we don't see the definition we just don't know.
-        if (MemberCXXRD->hasDefinition()) {
-          std::optional<bool> isRCAble = isPtrCompatible(MemberCXXRD);
-          if (isRCAble && *isRCAble)
-            reportBug(Member, MemberType, MemberCXXRD, RD);
+        std::optional<bool> IsCompatible = isPtrCompatible(QT, MemberCXXRD);
+        if (IsCompatible && *IsCompatible)
+          reportBug(Member, MemberType, MemberCXXRD, RD);
+      } else {
+        std::optional<bool> IsCompatible = isPtrCompatible(QT, nullptr);
+        auto* PointeeType = MemberType->getPointeeType().getTypePtrOrNull();
+        if (IsCompatible && *IsCompatible) {
+          auto *Desugared = PointeeType->getUnqualifiedDesugaredType();
+          if (auto *ObjCType = dyn_cast_or_null<ObjCInterfaceType>(Desugared))
+            reportBug(Member, MemberType, ObjCType->getDecl(), RD);
         }
       }
     }
@@ -107,11 +125,12 @@ class RawPtrRefMemberChecker
 
   void visitIvarDecl(const ObjCContainerDecl *CD,
                      const ObjCIvarDecl *Ivar) const {
-    const Type *IvarType = Ivar->getType().getTypePtrOrNull();
+    auto QT = Ivar->getType();
+    const Type *IvarType = QT.getTypePtrOrNull();
     if (!IvarType)
       return;
     if (auto *IvarCXXRD = IvarType->getPointeeCXXRecordDecl()) {
-      std::optional<bool> IsCompatible = isPtrCompatible(IvarCXXRD);
+      std::optional<bool> IsCompatible = isPtrCompatible(QT, IvarCXXRD);
       if (IsCompatible && *IsCompatible)
         reportBug(Ivar, IvarType, IvarCXXRD, CD);
     }
@@ -151,13 +170,13 @@ class RawPtrRefMemberChecker
     return false;
   }
 
-  template <typename DeclType, typename ParentDeclType>
+  template <typename DeclType, typename PointeeType, typename ParentDeclType>
   void reportBug(const DeclType *Member, const Type *MemberType,
-                 const CXXRecordDecl *MemberCXXRD,
+                 const PointeeType *Pointee,
                  const ParentDeclType *ClassCXXRD) const {
     assert(Member);
     assert(MemberType);
-    assert(MemberCXXRD);
+    assert(Pointee);
 
     SmallString<100> Buf;
     llvm::raw_svector_ostream Os(Buf);
@@ -169,10 +188,13 @@ class RawPtrRefMemberChecker
     printQuotedName(Os, Member);
     Os << " in ";
     printQuotedQualifiedName(Os, ClassCXXRD);
-    Os << " is a "
-       << (isa<PointerType>(MemberType) ? "raw pointer" : "reference") << " to "
-       << typeName() << " ";
-    printQuotedQualifiedName(Os, MemberCXXRD);
+    Os << " is a ";
+    if (printPointer(Os, MemberType) == PrintDeclKind::Pointer) {
+      auto Typedef = MemberType->getAs<TypedefType>();
+      assert(Typedef);
+      printQuotedQualifiedName(Os, Typedef->getDecl());
+    } else
+      printQuotedQualifiedName(Os, Pointee);
     Os << "; " << invariant() << ".";
 
     PathDiagnosticLocation BSLoc(Member->getSourceRange().getBegin(),
@@ -181,6 +203,16 @@ class RawPtrRefMemberChecker
     Report->addRange(Member->getSourceRange());
     BR->emitReport(std::move(Report));
   }
+
+  enum class PrintDeclKind { Pointee, Pointer };
+  virtual PrintDeclKind printPointer(llvm::raw_svector_ostream& Os,
+                            const Type *T) const {
+    T = T->getUnqualifiedDesugaredType();
+    bool IsPtr = isa<PointerType>(T) || isa<ObjCObjectPointerType>(T);
+    Os << (IsPtr ? "raw pointer" : "reference") << " to "
+       << typeName() << " ";
+    return PrintDeclKind::Pointee;
+  }
 };
 
 class NoUncountedMemberChecker final : public RawPtrRefMemberChecker {
@@ -190,8 +222,9 @@ class NoUncountedMemberChecker final : public RawPtrRefMemberChecker {
                                "reference-countable type") {}
 
   std::optional<bool>
-  isPtrCompatible(const clang::CXXRecordDecl *R) const final {
-    return isRefCountable(R);
+  isPtrCompatible(const clang::QualType,
+                  const clang::CXXRecordDecl *R) const final {
+    return R && isRefCountable(R);
   }
 
   bool isPtrCls(const clang::CXXRecordDecl *R) const final {
@@ -212,8 +245,9 @@ class NoUncheckedPtrMemberChecker final : public RawPtrRefMemberChecker {
                                "checked-pointer capable type") {}
 
   std::optional<bool>
-  isPtrCompatible(const clang::CXXRecordDecl *R) const final {
-    return isCheckedPtrCapable(R);
+  isPtrCompatible(const clang::QualType,
+                  const clang::CXXRecordDecl *R) const final {
+    return R && isCheckedPtrCapable(R);
   }
 
   bool isPtrCls(const clang::CXXRecordDecl *R) const final {
@@ -228,6 +262,40 @@ class NoUncheckedPtrMemberChecker final : public RawPtrRefMemberChecker {
   }
 };
 
+class NoUnretainedMemberChecker final : public RawPtrRefMemberChecker {
+public:
+  NoUnretainedMemberChecker()
+      : RawPtrRefMemberChecker("Member variable is a raw-pointer/reference to "
+                               "retainable type") {
+    RTC = RetainTypeChecker();
+  }
+
+  std::optional<bool>
+  isPtrCompatible(const clang::QualType QT,
+                  const clang::CXXRecordDecl *) const final {
+    return RTC->isUnretained(QT);
+  }
+
+  bool isPtrCls(const clang::CXXRecordDecl *R) const final {
+    return isRetainPtr(R);
+  }
+
+  const char *typeName() const final { return "retainable type"; }
+
+  const char *invariant() const final {
+    return "member variables must be a RetainPtr";
+  }
+
+  PrintDeclKind printPointer(llvm::raw_svector_ostream& Os,
+                             const Type *T) const final {
+    if (!isa<ObjCObjectPointerType>(T) && T->getAs<TypedefType>()) {
+      Os << typeName() << " ";
+      return PrintDeclKind::Pointer;
+    }
+    return RawPtrRefMemberChecker::printPointer(Os, T);
+  }
+};
+
 } // namespace
 
 void ento::registerNoUncountedMemberChecker(CheckerManager &Mgr) {
@@ -246,3 +314,12 @@ bool ento::shouldRegisterNoUncheckedPtrMemberChecker(
     const CheckerManager &Mgr) {
   return true;
 }
+
+void ento::registerNoUnretainedMemberChecker(CheckerManager &Mgr) {
+  Mgr.registerChecker<NoUnretainedMemberChecker>();
+}
+
+bool ento::shouldRegisterNoUnretainedMemberChecker(
+    const CheckerManager &Mgr) {
+  return true;
+}

clang/test/Analysis/Checkers/WebKit/unretained-members-arc.mm

@@ -0,0 +1,39 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.webkit.NoUnretainedMemberChecker -fobjc-arc -verify %s
+
+#include "objc-mock-types.h"
+
+namespace members {
+
+  struct Foo {
+  private:
+    SomeObj* a = nullptr;
+
+    [[clang::suppress]]
+    SomeObj* a_suppressed = nullptr;
+
+  protected:
+    RetainPtr<SomeObj> b;
+
+  public:
+    SomeObj* c = nullptr;
+    RetainPtr<SomeObj> d;
+
+    CFMutableArrayRef e = nullptr;
+// expected-warning@-1{{Member variable 'e' in 'members::Foo' is a retainable type 'CFMutableArrayRef'}}
+  };
+
+  template<class T, class S>
+  struct FooTmpl {
+    T* x;
+    S y;
+// expected-warning@-1{{Member variable 'y' in 'members::FooTmpl<SomeObj, __CFArray *>' is a raw pointer to retainable type}}
+  };
+
+  void forceTmplToInstantiate(FooTmpl<SomeObj, CFMutableArrayRef>) {}
+
+  struct [[clang::suppress]] FooSuppressed {
+  private:
+    SomeObj* a = nullptr;
+  };
+
+}

clang/test/Analysis/Checkers/WebKit/unretained-members.mm

@@ -0,0 +1,57 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.webkit.NoUnretainedMemberChecker -verify %s
+
+#include "objc-mock-types.h"
+
+namespace members {
+
+  struct Foo {
+  private:
+    SomeObj* a = nullptr;
+// expected-warning@-1{{Member variable 'a' in 'members::Foo' is a raw pointer to retainable type}}
+
+    [[clang::suppress]]
+    SomeObj* a_suppressed = nullptr;
+
+  protected:
+    RetainPtr<SomeObj> b;
+
+  public:
+    SomeObj* c = nullptr;
+// expected-warning@-1{{Member variable 'c' in 'members::Foo' is a raw pointer to retainable type}}
+    RetainPtr<SomeObj> d;
+
+    CFMutableArrayRef e = nullptr;
+// expected-warning@-1{{Member variable 'e' in 'members::Foo' is a retainable type 'CFMutableArrayRef'}}
+  };
+
+  template<class T, class S>
+  struct FooTmpl {
+    T* a;
+// expected-warning@-1{{Member variable 'a' in 'members::FooTmpl<SomeObj, __CFArray *>' is a raw pointer to retainable type}}
+    S b;
+// expected-warning@-1{{Member variable 'b' in 'members::FooTmpl<SomeObj, __CFArray *>' is a raw pointer to retainable type}}
+  };
+
+  void forceTmplToInstantiate(FooTmpl<SomeObj, CFMutableArrayRef>) {}
+
+  struct [[clang::suppress]] FooSuppressed {
+  private:
+    SomeObj* a = nullptr;
+  };
+
+}
+
+namespace ignore_unions {
+  union Foo {
+    SomeObj* a;
+    RetainPtr<SomeObj> b;
+    CFMutableArrayRef c;
+  };
+
+  template<class T>
+  union RefPtr {
+    T* a;
+  };
+
+  void forceTmplToInstantiate(RefPtr<SomeObj>) {}
+}