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

[NoQ]

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.

[dcoughlin]

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]

• Address comments
• Richer warning message (is it in good shape?)
• Add tests for lack of implementation

[NoQ]

lib/StaticAnalyzer/Checkers/ObjCPropertyChecker.cpp

68 ↗

(On Diff #80644)

ObjCContainerDecl::HasUserDeclaredSetterMethod()

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

[NoQ]

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

[NoQ]

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

[dcoughlin]

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]

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]

• Fix crashes on checking properties in categories.
• Address both comments.

cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td

 
 //===----------------------------------------------------------------------===//
 // Mac OS X, Cocoa, and Core Foundation checkers.
 //===----------------------------------------------------------------------===//
 
 let ParentPackage = OSX in {
 
 def NumberObjectConversionChecker : Checker<"NumberObjectConversion">,
-  InPackage<OSX>,
   HelpText<"Check for erroneous conversions of objects representing numbers into numbers">,
   DescFile<"NumberObjectConversionChecker.cpp">;
 
 def MacOSXAPIChecker : Checker<"API">,
-  InPackage<OSX>,
   HelpText<"Check for proper uses of various Apple APIs">,
   DescFile<"MacOSXAPIChecker.cpp">;
 
 def MacOSKeychainAPIChecker : Checker<"SecKeychainAPI">,
-  InPackage<OSX>,
   HelpText<"Check for proper uses of Secure Keychain APIs">,
   DescFile<"MacOSKeychainAPIChecker.cpp">;
 
+def ObjCPropertyChecker : Checker<"ObjCProperty">,
+  HelpText<"Check for proper uses of Objective-C properties">,
+  DescFile<"ObjCPropertyChecker.cpp">;
+
 } // end "osx"
 
 let ParentPackage = Cocoa in {
 
 def ObjCAtSyncChecker : Checker<"AtSync">,
   HelpText<"Check for nil pointers used as mutexes for @synchronized">,
   DescFile<"ObjCAtSyncChecker.cpp">;
 

cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt

add_clang_library(clangStaticAnalyzerCheckers
   NoReturnFunctionChecker.cpp
   NonNullParamChecker.cpp
   NullabilityChecker.cpp
   NumberObjectConversionChecker.cpp
   ObjCAtSyncChecker.cpp
   ObjCContainersASTChecker.cpp
   ObjCContainersChecker.cpp
   ObjCMissingSuperCallChecker.cpp
+  ObjCPropertyChecker.cpp
   ObjCSelfInitChecker.cpp
   ObjCSuperDeallocChecker.cpp
   ObjCUnusedIVarsChecker.cpp
   PaddingChecker.cpp
   PointerArithChecker.cpp
   PointerSubChecker.cpp
   PthreadLockChecker.cpp
   RetainCountChecker.cpp

cfe/trunk/lib/StaticAnalyzer/Checkers/ObjCPropertyChecker.cpp

+//==- ObjCPropertyChecker.cpp - Check ObjC properties ------------*- C++ -*-==//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+//  This checker finds issues with Objective-C properties.
+//  Currently finds only one kind of issue:
+//  - Find synthesized properties with copy attribute of mutable NS collection
+//    types. Calling -copy on such collections produces an immutable copy,
+//    which contradicts the type of the property.
+//
+//===----------------------------------------------------------------------===//
+
+#include "ClangSACheckers.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+class ObjCPropertyChecker
+    : public Checker<check::ASTDecl<ObjCPropertyDecl>> {
+  void checkCopyMutable(const ObjCPropertyDecl *D, BugReporter &BR) const;
+
+public:
+  void checkASTDecl(const ObjCPropertyDecl *D, AnalysisManager &Mgr,
+                    BugReporter &BR) const;
+};
+} // end anonymous namespace.
+
+void ObjCPropertyChecker::checkASTDecl(const ObjCPropertyDecl *D,
+                                       AnalysisManager &Mgr,
+                                       BugReporter &BR) const {
+  checkCopyMutable(D, BR);
+}
+
+void ObjCPropertyChecker::checkCopyMutable(const ObjCPropertyDecl *D,
+                                           BugReporter &BR) const {
+  if (D->isReadOnly() || D->getSetterKind() != ObjCPropertyDecl::Copy)
+    return;
+
+  QualType T = D->getType();
+  if (!T->isObjCObjectPointerType())
+    return;
+
+  const std::string &PropTypeName(T->getPointeeType().getCanonicalType()
+                                                     .getUnqualifiedType()
+                                                     .getAsString());
+  if (!StringRef(PropTypeName).startswith("NSMutable"))
+    return;
+
+  const ObjCImplDecl *ImplD = nullptr;
+  if (const ObjCInterfaceDecl *IntD =
+          dyn_cast<ObjCInterfaceDecl>(D->getDeclContext())) {
+    ImplD = IntD->getImplementation();
+  } else {
+    const ObjCCategoryDecl *CatD = cast<ObjCCategoryDecl>(D->getDeclContext());
+    ImplD = CatD->getClassInterface()->getImplementation();
+  }
+
+  if (!ImplD || ImplD->HasUserDeclaredSetterMethod(D))
+    return;
+
+  SmallString<128> Str;
+  llvm::raw_svector_ostream OS(Str);
+  OS << "Property of mutable type '" << PropTypeName
+     << "' has 'copy' attribute; an immutable object will be stored instead";
+
+  BR.EmitBasicReport(
+      D, this, "Objective-C property misuse", "Logic error", OS.str(),
+      PathDiagnosticLocation::createBegin(D, BR.getSourceManager()),
+      D->getSourceRange());
+}
+
+void ento::registerObjCPropertyChecker(CheckerManager &Mgr) {
+  Mgr.registerChecker<ObjCPropertyChecker>();
+}

cfe/trunk/test/Analysis/ObjCPropertiesSyntaxChecks.m

+// RUN: %clang_cc1 -w -fblocks -analyze -analyzer-checker=osx.ObjCProperty %s -verify
+
+#include "Inputs/system-header-simulator-objc.h"
+
+@interface I : NSObject {
+  NSMutableString *_mutableExplicitStr;
+  NSMutableString *_trulyMutableStr;
+  NSMutableString *_trulyMutableExplicitStr;
+}
+@property(copy) NSString *str; // no-warning
+@property(copy) NSMutableString *mutableStr; // expected-warning{{Property of mutable type 'NSMutableString' has 'copy' attribute; an immutable object will be stored instead}}
+@property(copy) NSMutableString *mutableExplicitStr; // expected-warning{{Property of mutable type 'NSMutableString' has 'copy' attribute; an immutable object will be stored instead}}
+@property(copy, readonly) NSMutableString *mutableReadonlyStr; // no-warning
+@property(copy, readonly) NSMutableString *mutableReadonlyStrOverriddenInChild; // no-warning
+@property(copy, readonly) NSMutableString *mutableReadonlyStrOverriddenInCategory; // no-warning
+@property(copy) NSMutableString *trulyMutableStr; // no-warning
+@property(copy) NSMutableString *trulyMutableExplicitStr; // no-warning
+@property(copy) NSMutableString *trulyMutableStrWithSynthesizedStorage; // no-warning
+@end
+
+@interface I () {}
+@property(copy) NSMutableString *mutableStrInCategory; // expected-warning{{Property of mutable type 'NSMutableString' has 'copy' attribute; an immutable object will be stored instead}}
+@property (copy, readwrite) NSMutableString *mutableReadonlyStrOverriddenInCategory; // expected-warning{{Property of mutable type 'NSMutableString' has 'copy' attribute; an immutable object will be stored instead}}
+@end
+
+@implementation I
+@synthesize mutableExplicitStr = _mutableExplicitStr;
+- (NSMutableString *)trulyMutableStr {
+  return _trulyMutableStr;
+}
+- (void)setTrulyMutableStr: (NSMutableString *) S {
+  _trulyMutableStr = [S mutableCopy];
+}
+@dynamic trulyMutableExplicitStr;
+- (NSMutableString *)trulyMutableExplicitStr {
+  return _trulyMutableExplicitStr;
+}
+- (void)setTrulyMutableExplicitStr: (NSMutableString *) S {
+  _trulyMutableExplicitStr = [S mutableCopy];
+}
+@synthesize trulyMutableStrWithSynthesizedStorage;
+- (NSMutableString *)trulyMutableStrWithSynthesizedStorage {
+  return trulyMutableStrWithSynthesizedStorage;
+}
+- (void)setTrulyMutableStrWithSynthesizedStorage: (NSMutableString *) S {
+  trulyMutableStrWithSynthesizedStorage = [S mutableCopy];
+}
+@end
+
+@interface J : I {}
+@property (copy, readwrite) NSMutableString *mutableReadonlyStrOverriddenInChild; // expected-warning{{Property of mutable type 'NSMutableString' has 'copy' attribute; an immutable object will be stored instead}}
+@end
+
+@implementation J
+@end
+
+// If we do not see the implementation then we do not want to warn,
+// because we may miss a user-defined setter that works correctly.
+@interface IWithoutImpl : NSObject {}
+@property(copy) NSMutableString *mutableStr; // no-warning
+@end