[alpha.webkit.NoUnretainedMemberChecker] Only check @property when @implementation is seen

[rniwa]

A @interface declaration with a raw pointer @Property does not necessarily mean it synthesizes ivar of that type. To determine whether such a synthesis happens or not, we must wait for @implementation to appear. So this PR makes the checker only validate @Property then.

[llvmbot]

@llvm/pr-subscribers-clang

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

Author: Ryosuke Niwa (rniwa)

Changes

A @interface declaration with a raw pointer @property does not necessarily mean it synthesizes ivar of that type. To determine whether such a synthesis happens or not, we must wait for @implementation to appear. So this PR makes the checker only validate @property then.

---

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

3 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp (+9-10)
• (modified) clang/test/Analysis/Checkers/WebKit/unretained-members-arc.mm (+15)
• (modified) clang/test/Analysis/Checkers/WebKit/unretained-members.mm (+15)

diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp
index 8faf6a219450a..baafc1bc61c15 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp
@@ -129,17 +129,16 @@ class RawPtrRefMemberChecker
     if (BR->getSourceManager().isInSystemHeader(CD->getLocation()))
       return;
 
-    ObjCContainerDecl::PropertyMap map;
-    CD->collectPropertiesToImplement(map);
-    for (auto it : map)
-      visitObjCPropertyDecl(CD, it.second);
-
-    if (auto *ID = dyn_cast<ObjCInterfaceDecl>(CD)) {
-      for (auto *Ivar : ID->ivars())
-        visitIvarDecl(CD, Ivar);
-      return;
-    }
     if (auto *ID = dyn_cast<ObjCImplementationDecl>(CD)) {
+      ObjCContainerDecl::PropertyMap map;
+      CD->collectPropertiesToImplement(map);
+      for (auto it : map)
+        visitObjCPropertyDecl(CD, it.second);
+
+      if (auto *Interface = ID->getClassInterface()) {
+        for (auto *Ivar : Interface->ivars())
+          visitIvarDecl(CD, Ivar);
+      }
       for (auto *PropImpl : ID->property_impls())
         visitPropImpl(CD, PropImpl);
       for (auto *Ivar : ID->ivars())
diff --git a/clang/test/Analysis/Checkers/WebKit/unretained-members-arc.mm b/clang/test/Analysis/Checkers/WebKit/unretained-members-arc.mm
index 00e6e6ec1dcfa..daa0c222251d9 100644
--- a/clang/test/Analysis/Checkers/WebKit/unretained-members-arc.mm
+++ b/clang/test/Analysis/Checkers/WebKit/unretained-members-arc.mm
@@ -76,6 +76,21 @@ @interface AnotherObject : NSObject {
 @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;
+@property(nonatomic, readonly) NSString *prop_safe;
+@end
+
+@implementation AnotherObject
+- (NSString *)prop_safe {
+  return nil;
+}
+@end
+
+// No warnings for @interface declaration itself. 
+@interface InterfaceOnlyObject : NSObject
+@property(nonatomic, strong) NSString *prop_string1;
+@property(nonatomic, assign) NSString *prop_string2;
+@property(nonatomic, unsafe_unretained) NSString *prop_string3;
+@property(nonatomic, readonly) NSString *prop_string4;
 @end
 
 NS_REQUIRES_PROPERTY_DEFINITIONS
diff --git a/clang/test/Analysis/Checkers/WebKit/unretained-members.mm b/clang/test/Analysis/Checkers/WebKit/unretained-members.mm
index 46f65dfa603ad..800d882f79696 100644
--- a/clang/test/Analysis/Checkers/WebKit/unretained-members.mm
+++ b/clang/test/Analysis/Checkers/WebKit/unretained-members.mm
@@ -98,6 +98,21 @@ @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}}
+@property(nonatomic, readonly) NSString *prop_safe;
+@end
+
+@implementation AnotherObject
+- (NSString *)prop_safe {
+  return nil;
+}
+@end
+
+// No warnings for @interface declaration itself. 
+@interface InterfaceOnlyObject : NSObject
+@property(nonatomic, strong) NSString *prop_string1;
+@property(nonatomic, assign) NSString *prop_string2;
+@property(nonatomic, unsafe_unretained) NSString *prop_string3;
+@property(nonatomic, readonly) NSString *prop_string4;
 @end
 
 NS_REQUIRES_PROPERTY_DEFINITIONS

[t-rasmud]

LGTM!

[t-rasmud]

What is the expected behavior for inherited interfaces that have an implementation? For example:

@interface InterfaceOnlyObjectInherited : InterfaceOnlyObject
@end

@implementation InterfaceOnlyObjectInherited
@end

[rniwa]

They should behave no different from non-inherited interfaces. We'd check @property synthesis only if @implementation appears.

[rniwa]

Added some test cases for this.

[t-rasmud]

Hmmm, what if we have a derived interface (with an implementation) that derives from InterfaceOnlyObject? Is the expectation then that the checker reports warnings on these properties (because the derived interface's implementation can access these properties via self)?

[rniwa]

In that case, we should be checking each property, yes.

[rniwa]

Oh I think we have a bug there. We're not checking superclass's properties.

[rniwa]

Actually, that's not really a bug since we don't auto-synthesize a property on a superclass. I added one more test case to test this exact scenario.

[t-rasmud]

Thank you for the explanation and the additional test cases.

[t-rasmud]

Thank you for the explanation and the additional test cases.