[alpha.webkit.NoUnretainedMemberChecker] Recognize NS_REQUIRES_PROPERTY_DEFINITIONS #143408

rniwa · 2025-06-09T16:36:06Z

Allow @Property of a raw pointer when NS_REQUIRES_PROPERTY_DEFINITIONS is specified on the interface since such an interface does not automatically synthesize raw pointer ivars.

Also emit a warning for @Property(assign) and @Property(unsafe_unretained) under ARC as well as when explicitly synthesizing a unsafe raw pointer property.

…TY_DEFINITIONS Allow @Property of a raw pointer when NS_REQUIRES_PROPERTY_DEFINITIONS is specified on the interface since such an interface does not automatically synthesize raw pointer ivars. Also emit a warning for @Property(assign) and @Property(unsafe_unretained) under ARC as well as when explicitly synthesizing a unsafe raw pointer property.

llvmbot · 2025-06-09T16:36:43Z

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-static-analyzer-1

Author: Ryosuke Niwa (rniwa)

Changes

Allow @property of a raw pointer when NS_REQUIRES_PROPERTY_DEFINITIONS is specified on the interface since such an interface does not automatically synthesize raw pointer ivars.

Also emit a warning for @property(assign) and @property(unsafe_unretained) under ARC as well as when explicitly synthesizing a unsafe raw pointer property.

Full diff: https://github.com/llvm/llvm-project/pull/143408.diff

6 Files Affected:

(modified) clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp (+1)
(modified) clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h (+2)
(modified) clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp (+64-11)
(modified) clang/test/Analysis/Checkers/WebKit/objc-mock-types.h (+1)
(modified) clang/test/Analysis/Checkers/WebKit/unretained-members-arc.mm (+36)
(modified) clang/test/Analysis/Checkers/WebKit/unretained-members.mm (+25)

diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
index cd33476344a34..72199af2f80a5 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
@@ -236,6 +236,7 @@ std::optional<bool> isUnchecked(const QualType T) {
 void RetainTypeChecker::visitTranslationUnitDecl(
     const TranslationUnitDecl *TUD) {
   IsARCEnabled = TUD->getLangOpts().ObjCAutoRefCount;
+  DefaultSynthProperties = TUD->getLangOpts().ObjCDefaultSynthProperties;
 }
 
 void RetainTypeChecker::visitTypedef(const TypedefDecl *TD) {
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
index f9fcfe9878d54..3c9560cb8059b 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
@@ -76,12 +76,14 @@ class RetainTypeChecker {
   llvm::DenseSet<const RecordType *> CFPointees;
   llvm::DenseSet<const Type *> RecordlessTypes;
   bool IsARCEnabled{false};
+  bool DefaultSynthProperties{true};
 
 public:
   void visitTranslationUnitDecl(const TranslationUnitDecl *);
   void visitTypedef(const TypedefDecl *);
   bool isUnretained(const QualType, bool ignoreARC = false);
   bool isARCEnabled() const { return IsARCEnabled; }
+  bool defaultSynthProperties() const { return DefaultSynthProperties; }
 };
 
 /// \returns true if \p Class is NS or CF objects AND not retained, false if
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp
index b1350b9093021..8faf6a219450a 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp
@@ -28,6 +28,7 @@ class RawPtrRefMemberChecker
 private:
   BugType Bug;
   mutable BugReporter *BR;
+  mutable llvm::DenseSet<const ObjCIvarDecl *> IvarDeclsToIgnore;
 
 protected:
   mutable std::optional<RetainTypeChecker> RTC;
@@ -36,7 +37,8 @@ class RawPtrRefMemberChecker
   RawPtrRefMemberChecker(const char *description)
       : Bug(this, description, "WebKit coding guidelines") {}
 
-  virtual std::optional<bool> isUnsafePtr(QualType) const = 0;
+  virtual std::optional<bool> isUnsafePtr(QualType,
+                                          bool ignoreARC = false) const = 0;
   virtual const char *typeName() const = 0;
   virtual const char *invariant() const = 0;
 
@@ -138,6 +140,8 @@ class RawPtrRefMemberChecker
       return;
     }
     if (auto *ID = dyn_cast<ObjCImplementationDecl>(CD)) {
+      for (auto *PropImpl : ID->property_impls())
+        visitPropImpl(CD, PropImpl);
       for (auto *Ivar : ID->ivars())
         visitIvarDecl(CD, Ivar);
       return;
@@ -148,6 +152,10 @@ class RawPtrRefMemberChecker
                      const ObjCIvarDecl *Ivar) const {
     if (BR->getSourceManager().isInSystemHeader(Ivar->getLocation()))
       return;
+
+    if (IvarDeclsToIgnore.contains(Ivar))
+      return;
+
     auto QT = Ivar->getType();
     const Type *IvarType = QT.getTypePtrOrNull();
     if (!IvarType)
@@ -157,6 +165,8 @@ class RawPtrRefMemberChecker
     if (!IsUnsafePtr || !*IsUnsafePtr)
       return;
 
+    IvarDeclsToIgnore.insert(Ivar);
+
     if (auto *MemberCXXRD = IvarType->getPointeeCXXRecordDecl())
       reportBug(Ivar, IvarType, MemberCXXRD, CD);
     else if (auto *ObjCDecl = getObjCDecl(IvarType))
@@ -167,13 +177,15 @@ class RawPtrRefMemberChecker
                              const ObjCPropertyDecl *PD) const {
     if (BR->getSourceManager().isInSystemHeader(PD->getLocation()))
       return;
-    auto QT = PD->getType();
-    const Type *PropType = QT.getTypePtrOrNull();
-    if (!PropType)
-      return;
 
-    auto IsUnsafePtr = isUnsafePtr(QT);
-    if (!IsUnsafePtr || !*IsUnsafePtr)
+    if (const ObjCInterfaceDecl *ID = dyn_cast<ObjCInterfaceDecl>(CD)) {
+      if (!RTC || !RTC->defaultSynthProperties() ||
+          ID->isObjCRequiresPropertyDefs())
+        return;
+    }
+
+    auto [IsUnsafe, PropType] = isPropImplUnsafePtr(PD);
+    if (!IsUnsafe)
       return;
 
     if (auto *MemberCXXRD = PropType->getPointeeCXXRecordDecl())
@@ -182,6 +194,47 @@ class RawPtrRefMemberChecker
       reportBug(PD, PropType, ObjCDecl, CD);
   }
 
+  void visitPropImpl(const ObjCContainerDecl *CD,
+                     const ObjCPropertyImplDecl *PID) const {
+    if (BR->getSourceManager().isInSystemHeader(PID->getLocation()))
+      return;
+
+    if (PID->getPropertyImplementation() != ObjCPropertyImplDecl::Synthesize)
+      return;
+
+    auto *PropDecl = PID->getPropertyDecl();
+    if (auto *IvarDecl = PID->getPropertyIvarDecl()) {
+      if (IvarDeclsToIgnore.contains(IvarDecl))
+        return;
+      IvarDeclsToIgnore.insert(IvarDecl);
+    }
+    auto [IsUnsafe, PropType] = isPropImplUnsafePtr(PropDecl);
+    if (!IsUnsafe)
+      return;
+
+    if (auto *MemberCXXRD = PropType->getPointeeCXXRecordDecl())
+      reportBug(PropDecl, PropType, MemberCXXRD, CD);
+    else if (auto *ObjCDecl = getObjCDecl(PropType))
+      reportBug(PropDecl, PropType, ObjCDecl, CD);
+  }
+
+  std::pair<bool, const Type *>
+  isPropImplUnsafePtr(const ObjCPropertyDecl *PD) const {
+    if (!PD)
+      return {false, nullptr};
+
+    auto QT = PD->getType();
+    const Type *PropType = QT.getTypePtrOrNull();
+    if (!PropType)
+      return {false, nullptr};
+
+    // "assign" property doesn't retain even under ARC so treat it as unsafe.
+    bool ignoreARC =
+        !PD->isReadOnly() && PD->getSetterKind() == ObjCPropertyDecl::Assign;
+    auto IsUnsafePtr = isUnsafePtr(QT, ignoreARC);
+    return {IsUnsafePtr && *IsUnsafePtr, PropType};
+  }
+
   bool shouldSkipDecl(const RecordDecl *RD) const {
     if (!RD->isThisDeclarationADefinition())
       return true;
@@ -272,7 +325,7 @@ class NoUncountedMemberChecker final : public RawPtrRefMemberChecker {
       : RawPtrRefMemberChecker("Member variable is a raw-pointer/reference to "
                                "reference-countable type") {}
 
-  std::optional<bool> isUnsafePtr(QualType QT) const final {
+  std::optional<bool> isUnsafePtr(QualType QT, bool) const final {
     return isUncountedPtr(QT.getCanonicalType());
   }
 
@@ -289,7 +342,7 @@ class NoUncheckedPtrMemberChecker final : public RawPtrRefMemberChecker {
       : RawPtrRefMemberChecker("Member variable is a raw-pointer/reference to "
                                "checked-pointer capable type") {}
 
-  std::optional<bool> isUnsafePtr(QualType QT) const final {
+  std::optional<bool> isUnsafePtr(QualType QT, bool) const final {
     return isUncheckedPtr(QT.getCanonicalType());
   }
 
@@ -309,8 +362,8 @@ class NoUnretainedMemberChecker final : public RawPtrRefMemberChecker {
     RTC = RetainTypeChecker();
   }
 
-  std::optional<bool> isUnsafePtr(QualType QT) const final {
-    return RTC->isUnretained(QT);
+  std::optional<bool> isUnsafePtr(QualType QT, bool ignoreARC) const final {
+    return RTC->isUnretained(QT, ignoreARC);
   }
 
   const char *typeName() const final { return "retainable type"; }
diff --git a/clang/test/Analysis/Checkers/WebKit/objc-mock-types.h b/clang/test/Analysis/Checkers/WebKit/objc-mock-types.h
index 93e7dfd77b9e9..9e4356a71f1b5 100644
--- a/clang/test/Analysis/Checkers/WebKit/objc-mock-types.h
+++ b/clang/test/Analysis/Checkers/WebKit/objc-mock-types.h
@@ -22,6 +22,7 @@ typedef struct CF_BRIDGED_TYPE(id) CGImage *CGImageRef;
 #define NS_RETURNS_RETAINED __attribute__((ns_returns_retained))
 #define CF_CONSUMED __attribute__((cf_consumed))
 #define CF_RETURNS_RETAINED __attribute__((cf_returns_retained))
+#define NS_REQUIRES_PROPERTY_DEFINITIONS __attribute__((objc_requires_property_definitions))
 
 extern const CFAllocatorRef kCFAllocatorDefault;
 typedef struct _NSZone NSZone;
diff --git a/clang/test/Analysis/Checkers/WebKit/unretained-members-arc.mm b/clang/test/Analysis/Checkers/WebKit/unretained-members-arc.mm
index 3491bc93ed98a..00e6e6ec1dcfa 100644
--- a/clang/test/Analysis/Checkers/WebKit/unretained-members-arc.mm
+++ b/clang/test/Analysis/Checkers/WebKit/unretained-members-arc.mm
@@ -64,3 +64,39 @@ void forceTmplToInstantiate(FooTmpl<SomeObj, CFMutableArrayRef>) {}
   };
 
 } // namespace ptr_to_ptr_to_retained
+
+@interface AnotherObject : NSObject {
+  NSString *ns_string;
+  CFStringRef cf_string;
+  // expected-warning@-1{{Instance variable 'cf_string' in 'AnotherObject' is a retainable type 'CFStringRef'; member variables must be a RetainPtr}}
+}
+@property(nonatomic, strong) NSString *prop_string1;
+@property(nonatomic, assign) NSString *prop_string2;
+// expected-warning@-1{{Property 'prop_string2' in 'AnotherObject' is a raw pointer to retainable type 'NSString'; member variables must be a RetainPtr}}
+@property(nonatomic, unsafe_unretained) NSString *prop_string3;
+// expected-warning@-1{{Property 'prop_string3' in 'AnotherObject' is a raw pointer to retainable type 'NSString'; member variables must be a RetainPtr}}
+@property(nonatomic, readonly) NSString *prop_string4;
+@end
+
+NS_REQUIRES_PROPERTY_DEFINITIONS
+@interface NoSynthObject : NSObject {
+  NSString *ns_string;
+  CFStringRef cf_string;
+  // expected-warning@-1{{Instance variable 'cf_string' in 'NoSynthObject' is a retainable type 'CFStringRef'; member variables must be a RetainPtr}}
+}
+@property(nonatomic, readonly, strong) NSString *prop_string1;
+@property(nonatomic, readonly, strong) NSString *prop_string2;
+@property(nonatomic, assign) NSString *prop_string3;
+// expected-warning@-1{{Property 'prop_string3' in 'NoSynthObject' is a raw pointer to retainable type 'NSString'; member variables must be a RetainPtr}}
+@property(nonatomic, unsafe_unretained) NSString *prop_string4;
+// expected-warning@-1{{Property 'prop_string4' in 'NoSynthObject' is a raw pointer to retainable type 'NSString'; member variables must be a RetainPtr}}
+@end
+
+@implementation NoSynthObject
+- (NSString *)prop_string1 {
+  return nil;
+}
+@synthesize prop_string2;
+@synthesize prop_string3;
+@synthesize prop_string4;
+@end
diff --git a/clang/test/Analysis/Checkers/WebKit/unretained-members.mm b/clang/test/Analysis/Checkers/WebKit/unretained-members.mm
index 0cb4c4ac0f6a0..46f65dfa603ad 100644
--- a/clang/test/Analysis/Checkers/WebKit/unretained-members.mm
+++ b/clang/test/Analysis/Checkers/WebKit/unretained-members.mm
@@ -99,3 +99,28 @@ @interface AnotherObject : NSObject {
 @property(nonatomic, strong) NSString *prop_string;
 // expected-warning@-1{{Property 'prop_string' in 'AnotherObject' is a raw pointer to retainable type 'NSString'; member variables must be a RetainPtr}}
 @end
+
+NS_REQUIRES_PROPERTY_DEFINITIONS
+@interface NoSynthObject : NSObject {
+  NSString *ns_string;
+  // expected-warning@-1{{Instance variable 'ns_string' in 'NoSynthObject' is a raw pointer to retainable type 'NSString'; member variables must be a RetainPtr}}
+  CFStringRef cf_string;
+  // expected-warning@-1{{Instance variable 'cf_string' in 'NoSynthObject' is a retainable type 'CFStringRef'; member variables must be a RetainPtr}}
+}
+@property(nonatomic, readonly, strong) NSString *prop_string1;
+@property(nonatomic, readonly, strong) NSString *prop_string2;
+// expected-warning@-1{{Property 'prop_string2' in 'NoSynthObject' is a raw pointer to retainable type 'NSString'}}
+@property(nonatomic, assign) NSString *prop_string3;
+// expected-warning@-1{{Property 'prop_string3' in 'NoSynthObject' is a raw pointer to retainable type 'NSString'; member variables must be a RetainPtr}}
+@property(nonatomic, unsafe_unretained) NSString *prop_string4;
+// expected-warning@-1{{Property 'prop_string4' in 'NoSynthObject' is a raw pointer to retainable type 'NSString'; member variables must be a RetainPtr}}
+@end
+
+@implementation NoSynthObject
+- (NSString *)prop_string1 {
+  return nil;
+}
+@synthesize prop_string2;
+@synthesize prop_string3;
+@synthesize prop_string4;
+@end

ziqingluo-90

LGTM now!

rniwa · 2025-06-10T03:55:45Z

Thank for you all the reviews!

…TY_DEFINITIONS (llvm#143408) Allow @Property of a raw pointer when NS_REQUIRES_PROPERTY_DEFINITIONS is specified on the interface since such an interface does not automatically synthesize raw pointer ivars. Also emit a warning for @Property(assign) and @Property(unsafe_unretained) under ARC as well as when explicitly synthesizing a unsafe raw pointer property.

rniwa mentioned this pull request Jun 9, 2025

[alpha.webkit.NoUnretainedMemberChecker] Recognize NS_REQUIRES_PROPERTY_DEFINITIONS #141293

Closed

llvmbot added clang Clang issues not falling into any other category clang:static analyzer labels Jun 9, 2025

rniwa requested review from t-rasmud and ziqingluo-90 June 9, 2025 16:36

ziqingluo-90 approved these changes Jun 10, 2025

View reviewed changes

rniwa merged commit 0123ee5 into llvm:main Jun 10, 2025
10 checks passed

rniwa deleted the allow-objc-property-requires-definitions branch June 10, 2025 03:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[alpha.webkit.NoUnretainedMemberChecker] Recognize NS_REQUIRES_PROPERTY_DEFINITIONS #143408

[alpha.webkit.NoUnretainedMemberChecker] Recognize NS_REQUIRES_PROPERTY_DEFINITIONS #143408

Uh oh!

rniwa commented Jun 9, 2025

Uh oh!

llvmbot commented Jun 9, 2025 •

edited

Loading

Uh oh!

ziqingluo-90 left a comment

Uh oh!

rniwa commented Jun 10, 2025

Uh oh!

Uh oh!

Uh oh!

[alpha.webkit.NoUnretainedMemberChecker] Recognize NS_REQUIRES_PROPERTY_DEFINITIONS #143408

[alpha.webkit.NoUnretainedMemberChecker] Recognize NS_REQUIRES_PROPERTY_DEFINITIONS #143408

Uh oh!

Conversation

rniwa commented Jun 9, 2025

Uh oh!

llvmbot commented Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ziqingluo-90 left a comment

Choose a reason for hiding this comment

Uh oh!

rniwa commented Jun 10, 2025

Uh oh!

Uh oh!

Uh oh!

llvmbot commented Jun 9, 2025 •

edited

Loading