Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rniwa merged 4 commits into from
Sep 25, 2025
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e369e9c
[alpha.webkit.NoUnretainedMemberChecker] Only check @property when @i…
rniwa Sep 20, 2025
ffa520f
Merge branch 'main' into fix-webkit-ivar-impl-only
rniwa Sep 23, 2025
6d70f15
Add test cases for inherited interfaces
rniwa Sep 24, 2025
c4bd06b
Add a test case for a subclass interface with implementation
rniwa Sep 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 9 additions & 10 deletions clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,17 +130,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())
Expand Down
15 changes: 15 additions & 0 deletions clang/test/Analysis/Checkers/WebKit/unretained-members-arc.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,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;
Copy link
Contributor

@t-rasmud t-rasmud Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Contributor Author

@rniwa rniwa Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@rniwa rniwa Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@rniwa rniwa Sep 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@t-rasmud t-rasmud Sep 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the explanation and the additional test cases.

@property(nonatomic, readonly) dispatch_queue_t prop_string5;
@end

Expand Down
18 changes: 17 additions & 1 deletion clang/test/Analysis/Checkers/WebKit/unretained-members.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,23 @@ @interface AnotherObject : NSObject {
// expected-warning@-1{{Instance variable 'dispatch' in 'AnotherObject' is a retainable type 'dispatch_queue_t'}}
}
@property(nonatomic, strong) NSString *prop_string;
// expected-warning@-1{{Property 'prop_string' in 'AnotherObject' is a raw pointer to retainable type 'NSString'}}
// 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;

Copy link
Contributor

@t-rasmud t-rasmud Sep 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@interface InterfaceOnlyObjectInherited : InterfaceOnlyObject
@end

@implementation InterfaceOnlyObjectInherited
@end

Copy link
Contributor Author

@rniwa rniwa Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@rniwa rniwa Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some test cases for this.

@end

NS_REQUIRES_PROPERTY_DEFINITIONS
Expand Down
Loading