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

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.

5 files changed, 211 insertions, 20 deletions

diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index e3dda3e..c947f262 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -3522,6 +3522,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 for CF types regardless of whether ARC is enabled or disabled. Only RetainPtr is allowed for NS types when ARC is disabled.
+
+.. 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.UnretainedLambdaCapturesChecker
 """"""""""""""""""""""""""""""""""""""""""""
 Raw pointers and references to NS or CF types can't be captured in lambdas. Only RetainPtr is allowed for CF types regardless of whether ARC is enabled or disabled, and only RetainPtr is allowed for NS types when ARC is disabled.
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index 5d2b70b..21b5851 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -1765,6 +1765,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 UnretainedLambdaCapturesChecker : Checker<"UnretainedLambdaCapturesChecker">,
   HelpText<"Check unretained lambda captures.">,
   Documentation<HasDocumentation>;
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp
index 963f598..593e6e3 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp
@@ -31,12 +31,16 @@ private:
   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 @@ public:
         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 @@ public:
     };
 
     LocalVisitor visitor(this);
+    if (RTC)
+      RTC->visitTranslationUnitDecl(TUD);
     visitor.TraverseDecl(TUD);
   }
 
@@ -77,16 +89,22 @@ public:
       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 @@ public:
 
   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 @@ public:
     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 @@ public:
     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,15 @@ public:
     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 +221,9 @@ public:
                                "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 +244,9 @@ public:
                                "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 +261,40 @@ public:
   }
 };
 
+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 +313,11 @@ bool ento::shouldRegisterNoUncheckedPtrMemberChecker(
     const CheckerManager &Mgr) {
   return true;
 }
+
+void ento::registerNoUnretainedMemberChecker(CheckerManager &Mgr) {
+  Mgr.registerChecker<NoUnretainedMemberChecker>();
+}
+
+bool ento::shouldRegisterNoUnretainedMemberChecker(const CheckerManager &Mgr) {
+  return true;
+}
diff --git a/clang/test/Analysis/Checkers/WebKit/unretained-members-arc.mm b/clang/test/Analysis/Checkers/WebKit/unretained-members-arc.mm
new file mode 100644
index 0000000..9820c87
--- /dev/null
+++ b/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;
+  };
+
+}
diff --git a/clang/test/Analysis/Checkers/WebKit/unretained-members.mm b/clang/test/Analysis/Checkers/WebKit/unretained-members.mm
new file mode 100644
index 0000000..e068a58
--- /dev/null
+++ b/clang/test/Analysis/Checkers/WebKit/unretained-members.mm
@@ -0,0 +1,60 @@
+// 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;
+// No warning.
+
+  protected:
+    RetainPtr<SomeObj> b;
+// No warning.
+
+  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;
+// No warning.
+  };
+
+}
+
+namespace ignore_unions {
+  union Foo {
+    SomeObj* a;
+    RetainPtr<SomeObj> b;
+    CFMutableArrayRef c;
+  };
+
+  template<class T>
+  union RefPtr {
+    T* a;
+  };
+
+  void forceTmplToInstantiate(RefPtr<SomeObj>) {}
+}