This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Add ObjCPropertyChecker - check for autosynthesized copy-properties of mutable types.
ClosedPublic

Authored by NoQ on Dec 7 2016, 1:04 PM.

Details

Summary

When an ObjC property is declared as (copy), and the type of the property is mutable (eg. NSMutableSomething), the autosynthesized code for the setter, which as usual copies the object by calling -copy, produces an immutable object (eg. NSSomething; cf. -mutableCopy which would actually produce NSMutableSomething), which would lead to unexpected results.

The checker is named to be a bit more generic, even though i have no particular plans for other checks at the moment.

Testing in progress.

Diff Detail

Repository
rL LLVM

Event Timeline

NoQ updated this revision to Diff 80644.Dec 7 2016, 1:04 PM
NoQ retitled this revision from to [analyzer] Add ObjCPropertyChecker - check for autosynthesized copy-properties of mutable types..
NoQ updated this object.
NoQ added reviewers: zaks.anna, dcoughlin.
NoQ added a subscriber: cfe-commits.
dcoughlin edited edge metadata.Dec 7 2016, 7:35 PM

This looks great to me other than the idiom I mentioned inline (which is common, as I have found to my chagrin).

lib/StaticAnalyzer/Checkers/ObjCPropertyChecker.cpp
12 ↗(On Diff #80644)

Super pedantic nit: 'autosynthesized' properties are properties for which the programmer didn't even write @synthesize but the compiler decided to synthesize storage and accessors for them anyway (it didn't always do this; it was a new feature a while back). If I understand correctly, I think you warn for both synthesized and autosynthesized properties.

68 ↗(On Diff #80644)

You'll also want to make sure to not warn on the following idiom, in which programmers use @synthesize to generate the storage but still provide their own accessors:

@interface Foo
@property(copy) NSMutableString *foo;
@end

@implementation Foo
@synthesize foo;
-(NSMutableString *)foo {
  return foo;
}
- (void)setFoo:(NSMutableString *)newValue {
  foo = [newValue mutableCopy];
}
@end

I *think* a call to ObjCContainerDecl::HasUserDeclaredSetterMethod() should be sufficient to detect and early exit in this case.

NoQ updated this revision to Diff 80751.Dec 8 2016, 6:21 AM
NoQ edited edge metadata.
NoQ marked an inline comment as done.
  • Address comments
  • Richer warning message (is it in good shape?)
  • Add tests for lack of implementation
NoQ added inline comments.Dec 8 2016, 6:23 AM
lib/StaticAnalyzer/Checkers/ObjCPropertyChecker.cpp
68 ↗(On Diff #80644)

ObjCContainerDecl::HasUserDeclaredSetterMethod()

Didn't notice this one, that's much easier!

NoQ updated this revision to Diff 80756.Dec 8 2016, 6:27 AM

Don't construct a StringRef to a temporary std::string.

NoQ updated this revision to Diff 80811.Dec 8 2016, 1:03 PM

Don't yell at read-only properties (no -copy is performed).
Single-quote 'copy' in the error messages.

dcoughlin added inline comments.Dec 9 2016, 8:58 AM
lib/StaticAnalyzer/Checkers/ObjCPropertyChecker.cpp
44 ↗(On Diff #80811)

It is probably worth adding a test that the checker still issues a diagnostic when a readonly property in the primary interface is 'shadowed' by a readwrite property in a class extension. This is a very common idiom which lets the programmer expose the property as readonly in public but readwrite in private.

zaks.anna added inline comments.Dec 9 2016, 11:07 PM
include/clang/StaticAnalyzer/Checkers/Checkers.td
494 ↗(On Diff #80811)

Nit: maybe use "Check for proper uses of Objective-C properties."? "various issues" seems like filler words.

NoQ updated this revision to Diff 81083.Dec 12 2016, 6:59 AM
  • Fix crashes on checking properties in categories.
  • Address both comments.
NoQ marked 3 inline comments as done.Dec 12 2016, 6:59 AM
This revision was automatically updated to reflect the committed changes.