[clang-tidy/checks] Update objc-property-declaration check to allow a…

…rbitrary acronyms and initialisms 🔧 Summary: §1 Description This changes the objc-property-declaration check to allow arbitrary acronyms and initialisms instead of using whitelisted acronyms. In Objective-C it is relatively common to use project prefixes in property names for the purposes of disambiguation. For example, the CIColor¹ and CGColor² properties on UIColor both represent symbol prefixes being used in proeprty names outside of Apple's accepted acronyms³. The union of Apple's accepted acronyms and all symbol prefixes that might be used for disambiguation in property declarations effectively allows for any arbitrary sequence of capital alphanumeric characters to be acceptable in property declarations. This change updates the check accordingly. The test variants with custom configurations are deleted as part of this change because their configurations no longer impact behavior. The acronym configurations are currently preserved for backwards compatibility of check configuration. [1] https://developer.apple.com/documentation/uikit/uicolor/1621951-cicolor?language=objc [2] https://developer.apple.com/documentation/uikit/uicolor/1621954-cgcolor?language=objc [3] https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/CodingGuidelines/Articles/APIAbbreviations.html#//apple_ref/doc/uid/20001285-BCIHCGAE §2 Test Notes Changes verified by: • Running clang-tidy unit tests. • Used check_clang_tidy.py to verify expected output of processing objc-property-declaration.m Reviewers: benhamilton, Wizard Reviewed By: benhamilton Subscribers: jfb, cfe-commits Differential Revision: https://reviews.llvm.org/D51832 llvm-svn: 348331
llvm · Dec 5, 2018 · af4755a · af4755a
1 parent 6934202
commit af4755a
Show file tree

Hide file tree

Showing 6 changed files with 29 additions and 179 deletions.
diff --git a/clang-tools-extra/clang-tidy/objc/PropertyDeclarationCheck.cpp b/clang-tools-extra/clang-tidy/objc/PropertyDeclarationCheck.cpp
@@ -29,104 +29,12 @@ namespace {
 // For CategoryProperty especially in categories of system class,
 // to avoid naming conflict, the suggested naming style is
 // 'abc_lowerCamelCase' (adding lowercase prefix followed by '_').
+// Regardless of the style, all acronyms and initialisms should be capitalized.
 enum NamingStyle {
   StandardProperty = 1,
   CategoryProperty = 2,
 };
 
-/// The acronyms are aggregated from multiple sources including
-/// https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/CodingGuidelines/Articles/APIAbbreviations.html#//apple_ref/doc/uid/20001285-BCIHCGAE
-///
-/// Keep this list sorted.
-constexpr llvm::StringLiteral DefaultSpecialAcronyms[] = {
-    "[2-9]G",
-    "ACL",
-    "API",
-    "APN",
-    "APNS",
-    "AR",
-    "ARGB",
-    "ASCII",
-    "AV",
-    "BGRA",
-    "CA",
-    "CDN",
-    "CF",
-    "CG",
-    "CI",
-    "CRC",
-    "CV",
-    "CMYK",
-    "DNS",
-    "FPS",
-    "FTP",
-    "GIF",
-    "GL",
-    "GPS",
-    "GUID",
-    "HD",
-    "HDR",
-    "HMAC",
-    "HTML",
-    "HTTP",
-    "HTTPS",
-    "HUD",
-    "ID",
-    "JPG",
-    "JS",
-    "JSON",
-    "LAN",
-    "LZW",
-    "LTR",
-    "MAC",
-    "MD",
-    "MDNS",
-    "MIDI",
-    "NS",
-    "OS",
-    "P2P",
-    "PDF",
-    "PIN",
-    "PNG",
-    "POI",
-    "PSTN",
-    "PTR",
-    "QA",
-    "QOS",
-    "RGB",
-    "RGBA",
-    "RGBX",
-    "RIPEMD",
-    "ROM",
-    "RPC",
-    "RTF",
-    "RTL",
-    "SC",
-    "SDK",
-    "SHA",
-    "SQL",
-    "SSO",
-    "TCP",
-    "TIFF",
-    "TOS",
-    "TTS",
-    "UI",
-    "URI",
-    "URL",
-    "UUID",
-    "VC",
-    "VO",
-    "VOIP",
-    "VPN",
-    "VR",
-    "W",
-    "WAN",
-    "X",
-    "XML",
-    "Y",
-    "Z",
-};
-
 /// For now we will only fix 'CamelCase' or 'abc_CamelCase' property to
 /// 'camelCase' or 'abc_camelCase'. For other cases the users need to
 /// come up with a proper name by their own.
@@ -150,43 +58,42 @@ FixItHint generateFixItHint(const ObjCPropertyDecl *Decl, NamingStyle Style) {
   return FixItHint();
 }
 
-std::string AcronymsGroupRegex(llvm::ArrayRef<std::string> EscapedAcronyms) {
-  return "(" +
-         llvm::join(EscapedAcronyms.begin(), EscapedAcronyms.end(), "s?|") +
-         "s?)";
-}
-
-std::string validPropertyNameRegex(llvm::ArrayRef<std::string> EscapedAcronyms,
-                                   bool UsedInMatcher) {
+std::string validPropertyNameRegex(bool UsedInMatcher) {
   // Allow any of these names:
   // foo
   // fooBar
   // url
   // urlString
+  // ID
+  // IDs
   // URL
   // URLString
   // bundleID
+  // CIColor
+  //
+  // Disallow names of this form:
+  // LongString
+  //
+  // aRbITRaRyCapS is allowed to avoid generating false positives for names
+  // like isVitaminBSupplement, CProgrammingLanguage, and isBeforeM.
   std::string StartMatcher = UsedInMatcher ? "::" : "^";
-  std::string AcronymsMatcher = AcronymsGroupRegex(EscapedAcronyms);
-  return StartMatcher + "(" + AcronymsMatcher + "[A-Z]?)?[a-z]+[a-z0-9]*(" +
-         AcronymsMatcher + "|([A-Z][a-z0-9]+)|A|I)*$";
+  return StartMatcher + "([a-z]|[A-Z][A-Z0-9])[a-z0-9A-Z]*$";
 }
 
 bool hasCategoryPropertyPrefix(llvm::StringRef PropertyName) {
   auto RegexExp = llvm::Regex("^[a-zA-Z]+_[a-zA-Z0-9][a-zA-Z0-9_]+$");
   return RegexExp.match(PropertyName);
 }
 
-bool prefixedPropertyNameValid(llvm::StringRef PropertyName,
-                               llvm::ArrayRef<std::string> Acronyms) {
+bool prefixedPropertyNameValid(llvm::StringRef PropertyName) {
   size_t Start = PropertyName.find_first_of('_');
   assert(Start != llvm::StringRef::npos && Start + 1 < PropertyName.size());
   auto Prefix = PropertyName.substr(0, Start);
   if (Prefix.lower() != Prefix) {
     return false;
   }
   auto RegexExp =
-      llvm::Regex(llvm::StringRef(validPropertyNameRegex(Acronyms, false)));
+      llvm::Regex(llvm::StringRef(validPropertyNameRegex(false)));
   return RegexExp.match(PropertyName.substr(Start + 1));
 }
 }  // namespace
@@ -203,26 +110,11 @@ void PropertyDeclarationCheck::registerMatchers(MatchFinder *Finder) {
   // this check should only be applied to ObjC sources.
   if (!getLangOpts().ObjC) return;
 
-  if (IncludeDefaultAcronyms) {
-    EscapedAcronyms.reserve(llvm::array_lengthof(DefaultSpecialAcronyms) +
-                            SpecialAcronyms.size());
-    // No need to regex-escape the default acronyms.
-    EscapedAcronyms.insert(EscapedAcronyms.end(),
-                           std::begin(DefaultSpecialAcronyms),
-                           std::end(DefaultSpecialAcronyms));
-  } else {
-    EscapedAcronyms.reserve(SpecialAcronyms.size());
-  }
-  // In case someone defines a prefix which includes a regex
-  // special character, regex-escape all the user-defined prefixes.
-  std::transform(SpecialAcronyms.begin(), SpecialAcronyms.end(),
-                 std::back_inserter(EscapedAcronyms),
-                 [](const std::string &s) { return llvm::Regex::escape(s); });
   Finder->addMatcher(
       objcPropertyDecl(
           // the property name should be in Lower Camel Case like
           // 'lowerCamelCase'
-          unless(matchesName(validPropertyNameRegex(EscapedAcronyms, true))))
+          unless(matchesName(validPropertyNameRegex(true))))
           .bind("property"),
       this);
 }
@@ -234,14 +126,9 @@ void PropertyDeclarationCheck::check(const MatchFinder::MatchResult &Result) {
   auto *DeclContext = MatchedDecl->getDeclContext();
   auto *CategoryDecl = llvm::dyn_cast<ObjCCategoryDecl>(DeclContext);
 
-  auto SingleAcronymRegex =
-      llvm::Regex("^([a-zA-Z]+_)?" + AcronymsGroupRegex(EscapedAcronyms) + "$");
-  if (SingleAcronymRegex.match(MatchedDecl->getName())) {
-    return;
-  }
   if (CategoryDecl != nullptr &&
       hasCategoryPropertyPrefix(MatchedDecl->getName())) {
-    if (!prefixedPropertyNameValid(MatchedDecl->getName(), EscapedAcronyms) ||
+    if (!prefixedPropertyNameValid(MatchedDecl->getName()) ||
         CategoryDecl->IsClassExtension()) {
       NamingStyle Style = CategoryDecl->IsClassExtension() ? StandardProperty
                                                            : CategoryProperty;

diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -231,6 +231,10 @@ Improvements to clang-tidy
   - floating point - integer narrowing conversions,
   - constants with narrowing conversions (even in ternary operator).
 
+- The :doc:`objc-property-declaration
+  <clang-tidy/checks/objc-property-declaration>` check now ignores the
+  `Acronyms` and `IncludeDefaultAcronyms` options.
+
 Improvements to include-fixer
 -----------------------------
 

diff --git a/clang-tools-extra/docs/clang-tidy/checks/objc-property-declaration.rst b/clang-tools-extra/docs/clang-tidy/checks/objc-property-declaration.rst
@@ -47,23 +47,8 @@ Options
 
 .. option:: Acronyms
 
-   Semicolon-separated list of custom acronyms that can be used as a prefix
-   or a suffix of property names.
-
-   By default, appends to the list of default acronyms (
-   ``IncludeDefaultAcronyms`` set to ``1``).
-   If ``IncludeDefaultAcronyms`` is set to ``0``, instead replaces the
-   default list of acronyms.
+   This option is deprecated and ignored.
 
 .. option:: IncludeDefaultAcronyms
 
-   Integer value (defaults to ``1``) to control whether the default
-   acronyms are included in the list of acronyms.
-
-   If set to ``1``, the value in ``Acronyms`` is appended to the
-   default list of acronyms:
-
-   ``[2-9]G;ACL;API;APN;APNS;AR;ARGB;ASCII;AV;BGRA;CA;CDN;CF;CG;CI;CRC;CV;CMYK;DNS;FPS;FTP;GIF;GL;GPS;GUID;HD;HDR;HMAC;HTML;HTTP;HTTPS;HUD;ID;JPG;JS;JSON;LAN;LZW;LTR;MAC;MD;MDNS;MIDI;NS;OS;P2P;PDF;PIN;PNG;POI;PSTN;PTR;QA;QOS;RGB;RGBA;RGBX;RIPEMD;ROM;RPC;RTF;RTL;SC;SDK;SHA;SQL;SSO;TCP;TIFF;TOS;TTS;UI;URI;URL;UUID;VC;VO;VOIP;VPN;VR;W;WAN;X;XML;Y;Z``.
-
-   If set to ``0``, the value in ``Acronyms`` replaces the default list
-   of acronyms.
+   This option is deprecated and ignored.
diff --git a/clang-tools-extra/test/clang-tidy/objc-property-declaration-additional.m b/clang-tools-extra/test/clang-tidy/objc-property-declaration-additional.m
diff --git a/clang-tools-extra/test/clang-tidy/objc-property-declaration-custom.m b/clang-tools-extra/test/clang-tidy/objc-property-declaration-custom.m
diff --git a/clang-tools-extra/test/clang-tidy/objc-property-declaration.m b/clang-tools-extra/test/clang-tidy/objc-property-declaration.m
@@ -1,8 +1,12 @@
 // RUN: %check_clang_tidy %s objc-property-declaration %t
+@class CIColor;
+@class NSArray;
 @class NSData;
 @class NSString;
 @class UIViewController;
 
+typedef void *CGColorRef;
+
 @interface Foo
 @property(assign, nonatomic) int NotCamelCase;
 // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: property name 'NotCamelCase' not using lowerCamelCase style or not prefixed in a category, according to the Apple Coding Guidelines [objc-property-declaration]
@@ -23,6 +27,9 @@ @interface Foo
 @property(assign, nonatomic) int enableGLAcceleration;
 @property(assign, nonatomic) int ID;
 @property(assign, nonatomic) int hasADog;
+@property(nonatomic, readonly) CGColorRef CGColor;
+@property(nonatomic, readonly) CIColor *CIColor;
+@property(nonatomic, copy) NSArray *IDs;
 @end
 
 @interface Foo (Bar)