[clang-tidy objc-property-declaration] New option IncludeDefaultAcronyms

[benhamilton]

Summary

The existing option objc-property-declaration.Acronyms
replaces the built-in set of acronyms.

While this behavior is OK for clients that don't want the default
behavior, many clients may just want to add their own custom acronyms
to the default list.

This revision introduces a new option,
objc-property-declaration.IncludeDefaultAcronyms, which controls
whether the acronyms in objc-property-declaration.Acronyms are
appended to the default list (the default behavior) or whether they
replace.

I also updated the documentation.

Test Plan: make -j12 check-clang-tools

[Eugene.Zelenko]

docs/clang-tidy/checks/objc-property-declaration.rst

45

Please limit string length to 80 symbols.

[hokein]

clang-tidy/objc/PropertyDeclarationCheck.h

38

nit: code indent

docs/clang-tidy/checks/objc-property-declaration.rst

47

It seems to me the Acronyms and AdditionalAcronyms play most same role here.

If we want to keep the long default list, how about using a bool flag IncludeDefaultList + the existing Acronyms option?

• if IncludeDefaultList is on, the acronyms will be "default" + "Acronyms".
• if IncludeDefaultList is off, the acronyms will be only "Acronyms".

[benhamilton]

• Switch to IncludeDefaultAcronyms option (defaults to 1).
• Use array for default acronyms, since we no longer need to parse it.
• Don't regex-escape default acronyms, since we control them.

[benhamilton]

Thanks, fixed.

clang-tidy/objc/PropertyDeclarationCheck.h

38

Ah, the previous one was wrong, I see. Fixed both.

docs/clang-tidy/checks/objc-property-declaration.rst

45

Thanks, fixed.

47

I think most people will want the "include default + add more" option. My goal was to make that possible, which is why I added the new AdditionalAcronyms option.

I agree the idea of a setting to control including the default list is nice, but I feel that should be on by default, since most users will want that.

If we add IncludeDefaultList, it should need to be on by default. That would break backwards compatibility for existing users. Do we think that's okay?

I'm assuming not a lot of people use this check yet, but I have no way of knowing that.

[benhamilton]

• Remove debugging code

[hokein]

LGTM.

docs/clang-tidy/checks/objc-property-declaration.rst

47

Yeah, IncludeDefaultList should be on by default.

clang-tidy/objc/PropertyDeclarationCheck.h

 class PropertyDeclarationCheck : public ClangTidyCheck {
 public:
   PropertyDeclarationCheck(StringRef Name, ClangTidyContext *Context);
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
   void storeOptions(ClangTidyOptions::OptionMap &Options) override;
 
 private:
-    const std::vector<std::string> SpecialAcronyms;
+  const bool IncludeDefaultAcronyms;
 };
 
 } // namespace objc
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_OBJC_PROPERTY_DECLARATION_H

clang-tidy/objc/PropertyDeclarationCheck.cpp

 //===--- PropertyDeclarationCheck.cpp - clang-tidy-------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "PropertyDeclarationCheck.h"
 #include "../utils/OptionsUtils.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/Regex.h"
 #include <algorithm>
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace objc {
 
 namespace {
 /// The acronyms are from
 /// https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/CodingGuidelines/Articles/APIAbbreviations.html#//apple_ref/doc/uid/20001285-BCIHCGAE
 ///
 /// Keep this list sorted.
-constexpr char DefaultSpecialAcronyms[] =
-    "ACL;"
-    "API;"
-    "ARGB;"
-    "ASCII;"
-    "BGRA;"
-    "CMYK;"
-    "DNS;"
-    "FPS;"
-    "FTP;"
-    "GIF;"
-    "GPS;"
-    "HD;"
-    "HDR;"
-    "HTML;"
-    "HTTP;"
-    "HTTPS;"
-    "HUD;"
-    "ID;"
-    "JPG;"
-    "JS;"
-    "LAN;"
-    "LZW;"
-    "MDNS;"
-    "MIDI;"
-    "OS;"
-    "PDF;"
-    "PIN;"
-    "PNG;"
-    "POI;"
-    "PSTN;"
-    "PTR;"
-    "QA;"
-    "QOS;"
-    "RGB;"
-    "RGBA;"
-    "RGBX;"
-    "ROM;"
-    "RPC;"
-    "RTF;"
-    "RTL;"
-    "SDK;"
-    "SSO;"
-    "TCP;"
-    "TIFF;"
-    "TTS;"
-    "UI;"
-    "URI;"
-    "URL;"
-    "VC;"
-    "VOIP;"
-    "VPN;"
-    "VR;"
-    "WAN;"
-    "XML";
+constexpr llvm::StringLiteral DefaultSpecialAcronyms[] = {
+    "ACL",
+    "API",
+    "ARGB",
+    "ASCII",
+    "BGRA",
+    "CMYK",
+    "DNS",
+    "FPS",
+    "FTP",
+    "GIF",
+    "GPS",
+    "HD",
+    "HDR",
+    "HTML",
+    "HTTP",
+    "HTTPS",
+    "HUD",
+    "ID",
+    "JPG",
+    "JS",
+    "LAN",
+    "LZW",
+    "MDNS",
+    "MIDI",
+    "OS",
+    "PDF",
+    "PIN",
+    "PNG",
+    "POI",
+    "PSTN",
+    "PTR",
+    "QA",
+    "QOS",
+    "RGB",
+    "RGBA",
+    "RGBX",
+    "ROM",
+    "RPC",
+    "RTF",
+    "RTL",
+    "SDK",
+    "SSO",
+    "TCP",
+    "TIFF",
+    "TTS",
+    "UI",
+    "URI",
+    "URL",
+    "VC",
+    "VOIP",
+    "VPN",
+    "VR",
+    "WAN",
+    "XML",
+};
 
 /// For now we will only fix 'CamelCase' property to
 /// 'camelCase'. For other cases the users need to
 /// come up with a proper name by their own.
 /// FIXME: provide fix for snake_case to snakeCase
 FixItHint generateFixItHint(const ObjCPropertyDecl *Decl) {
   if (isupper(Decl->getName()[0])) {
     auto NewName = Decl->getName().str();
     NewName[0] = tolower(NewName[0]);
     return FixItHint::CreateReplacement(
         CharSourceRange::getTokenRange(SourceRange(Decl->getLocation())),
         llvm::StringRef(NewName));
   }
   return FixItHint();
 }
 
-std::string validPropertyNameRegex(const std::vector<std::string> &Acronyms) {
-  std::vector<std::string> EscapedAcronyms;
-  EscapedAcronyms.reserve(Acronyms.size());
-  // In case someone defines a custom prefix which includes a regex
-  // special character, escape all the prefixes.
-  std::transform(Acronyms.begin(), Acronyms.end(),
-                 std::back_inserter(EscapedAcronyms), [](const std::string& s) {
-                   return llvm::Regex::escape(s); });
+std::string validPropertyNameRegex(const std::vector<std::string> &EscapedAcronyms) {
   // Allow any of these names:
   // foo
   // fooBar
   // url
   // urlString
   // URL
   // URLString
   // bundleID
-  return std::string("::((") +
+  auto result = std::string("::((") +
       llvm::join(EscapedAcronyms.begin(), EscapedAcronyms.end(), "|") +
       ")[A-Z]?)?[a-z]+[a-z0-9]*([A-Z][a-z0-9]+)*" + "(" +
       llvm::join(EscapedAcronyms.begin(), EscapedAcronyms.end(), "|") + ")?$";
+  fprintf(stderr, "Got regex: [%s]\n", result.c_str());
+  return result;
 }
 }  // namespace
 
 PropertyDeclarationCheck::PropertyDeclarationCheck(StringRef Name,
                                                    ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
-      SpecialAcronyms(utils::options::parseStringList(
-          Options.get("Acronyms", DefaultSpecialAcronyms))) {}
+      SpecialAcronyms(
+          utils::options::parseStringList(Options.get("Acronyms", ""))),
+      IncludeDefaultAcronyms(Options.get("IncludeDefaultAcronyms", true)) {}
 
 void PropertyDeclarationCheck::registerMatchers(MatchFinder *Finder) {
+  fprintf(stderr, "Include default: %d\n", IncludeDefaultAcronyms);
+  std::vector<std::string> EscapedAcronyms;
+  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(SpecialAcronyms))))
+          unless(matchesName(validPropertyNameRegex(EscapedAcronyms))))
           .bind("property"),
       this);
 }
 
 void PropertyDeclarationCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *MatchedDecl =
       Result.Nodes.getNodeAs<ObjCPropertyDecl>("property");
   assert(MatchedDecl->getName().size() > 0);
   diag(MatchedDecl->getLocation(),
        "property name '%0' should use lowerCamelCase style, according to "
        "the Apple Coding Guidelines")
       << MatchedDecl->getName() << generateFixItHint(MatchedDecl);
 }
 
 void PropertyDeclarationCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "Acronyms",
                 utils::options::serializeStringList(SpecialAcronyms));
+  Options.store(Opts, "IncludeDefaultAcronyms", IncludeDefaultAcronyms);
 }
 
 }  // namespace objc
 }  // namespace tidy
 }  // namespace clang

docs/clang-tidy/checks/objc-property-declaration.rst

 
 The corresponding style rule: https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/CodingGuidelines/Articles/NamingIvarsAndTypes.html#//apple_ref/doc/uid/20001284-1001757
 
 Options
 -------
 
 .. option:: Acronyms
 
-   Semicolon-separated list of acronyms that can be used as a prefix
+   Semicolon-separated list of custom acronyms that can be used as a prefix
    or a suffix of property names.
 
-   If unset, defaults to "ACL;API;ARGB;ASCII;BGRA;CMYK;DNS;FPS;FTP;GIF;GPS;HD;HDR;HTML;HTTP;HTTPS;HUD;ID;JPG;JS;LAN;LZW;MDNS;MIDI;OS;PDF;PIN;PNG;POI;PSTN;PTR;QA;QOS;RGB;RGBA;RGBX;ROM;RPC;RTF;RTL;SDK;SSO;TCP;TIFF;TTS;UI;URI;URL;VC;VOIP;VPN;VR;WAN;XML".
+   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.
+
+.. 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:
+
+   ``ACL;API;ARGB;ASCII;BGRA;CMYK;DNS;FPS;FTP;GIF;GPS;HD;HDR;HTML;HTTP;HTTPS;
+HUD;ID;JPG;JS;LAN;LZW;MDNS;MIDI;OS;PDF;PIN;PNG;POI;PSTN;PTR;QA;QOS;RGB;RGBA;
+RGBX;ROM;RPC;RTF;RTL;SDK;SSO;TCP;TIFF;TTS;UI;URI;URL;VC;VOIP;VPN;VR;WAN;XML``.
+
+   If set to ``0``, the value in ``Acronyms`` replaces the default list
+   of acronyms.

test/clang-tidy/objc-property-declaration-additional.m

 // RUN: %check_clang_tidy %s objc-property-declaration %t \
 // RUN: -config='{CheckOptions: \
 // RUN:  [{key: objc-property-declaration.Acronyms, value: "ABC;TGIF"}]}' \
 // RUN: --
 @class NSString;
 
 @interface Foo
 @property(assign, nonatomic) int AbcNotRealPrefix;
 // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: property name 'AbcNotRealPrefix' should use lowerCamelCase style, according to the Apple Coding Guidelines [objc-property-declaration]
 // CHECK-FIXES: @property(assign, nonatomic) int abcNotRealPrefix;
 @property(assign, nonatomic) int ABCCustomPrefix;
 @property(strong, nonatomic) NSString *ABC_custom_prefix;
 // CHECK-MESSAGES: :[[@LINE-1]]:40: warning: property name 'ABC_custom_prefix' should use lowerCamelCase style, according to the Apple Coding Guidelines [objc-property-declaration]
+@property(assign, nonatomic) int GIFShouldIncludeStandardAcronym;
 @end

test/clang-tidy/objc-property-declaration-custom.m

 // RUN: %check_clang_tidy %s objc-property-declaration %t \
 // RUN: -config='{CheckOptions: \
-// RUN:  [{key: objc-property-declaration.Acronyms, value: "ABC;TGIF"}]}' \
+// RUN:  [{key: objc-property-declaration.Acronyms, value: "ABC;TGIF"}, \
+// RUN:   {key: objc-property-declaration.IncludeDefaultAcronyms, value: 0}]}' \
 // RUN: --
 @class NSString;
 
 @interface Foo
 @property(assign, nonatomic) int AbcNotRealPrefix;
 // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: property name 'AbcNotRealPrefix' should use lowerCamelCase style, according to the Apple Coding Guidelines [objc-property-declaration]
 // CHECK-FIXES: @property(assign, nonatomic) int abcNotRealPrefix;
 @property(assign, nonatomic) int ABCCustomPrefix;
 @property(strong, nonatomic) NSString *ABC_custom_prefix;
 // CHECK-MESSAGES: :[[@LINE-1]]:40: warning: property name 'ABC_custom_prefix' should use lowerCamelCase style, according to the Apple Coding Guidelines [objc-property-declaration]
+@property(assign, nonatomic) int GIFIgnoreStandardAcronym;
+// CHECK-MESSAGES: :[[@LINE-1]]:34: warning: property name 'GIFIgnoreStandardAcronym' should use lowerCamelCase style, according to the Apple Coding Guidelines [objc-property-declaration]
 @end