add prefix with '_' support for property name. Corresponding apple dev doc: https://developer.apple.com/library/content/qa/qa1908/_index.html

[benhamilton]

clang-tidy/objc/PropertyDeclarationCheck.cpp

102–116

Instead of bool isCategory, how about an enum NamingStyle:

enum NamingStyle {
  StandardProperty = 1,
  CategoryProperty = 2,
}

138

I think this is an off-by-one error, right? Change:

i < PropertyName.size() - 1

to

i < PropertyName.size()

139–141

Do we really want a leading _ to count? I think this might need to be a regular expression instead, something like:

^[a-zA-Z][a-zA-Z0-9]+_[a-zA-Z0-9][a-zA-Z0-9_]*$

156

I don't understand what this is doing. Why are we replacing things in a regex?

I don't think this is very maintainable. Can you rewrite it for legibility, please?

198

Is a class extension treated as a category? If so, we probably need to treat it specially.

e.g.:

Foo.h:

@interface Foo
@end

Foo.m:

@interface Foo ()
@property (nonatomic, readonly) int foo_bar;
@end

should not be allowed.

Can you handle this and add tests, please?

[Wizard]

clang-tidy/objc/PropertyDeclarationCheck.cpp

138

I was thinking of not letting go anything that ends with "_" otherwise I have to do more sanity check later.

139–141

Yes it is better to use a regex instead. I would use ^[a-z]+_[a-zA-Z0-9][a-zA-Z0-9_]+$ to make sure we are not having anything like foo_

156

It is a little awkward here because in the matcher, the regex use "::" to indicate matching start, but in llvm::regex it is the conventional "^". I will add some comments here.

[Wizard]

check for class extentsion

[Wizard]

clang-tidy/objc/PropertyDeclarationCheck.cpp

156

End up using a bool since it is not about naming style but whether we are using it in matcher or normal regex match.

[hokein]

clang-tidy/objc/PropertyDeclarationCheck.cpp

29

Please add documentation describing what these properties are.

97–98

Do we need to update the documentation here? it seems stale now?

120–122

Also add documentation for the method as well as parameters. It is a bit hard to follow the logic.

123

Does the comment still make sense? Seems you changed the regex below.

145

no need to pass const reference of llvm::StringRef. StringRef is cheap.

146

nit: use llvm::ArrayRef<std::string> would save a lot of keystroke.

148

we can use find_first_not_of or something similar.

200

Consider using const auto* CategoryDecl = dyn_cast<ObjCCategoryDecl*>(DeclContext), we can get rid of this cast, and NodeKind variable.

[Wizard]

update some documents and comments

clang-tidy/objc/PropertyDeclarationCheck.cpp

123

Yes they are actually the same. I am just replacing the prefix "::" with "^" according to the UsedInMatcher param.

145

The original property name I directly get from ast is a const. If I remove the const here, I will have to make a copy of the property name before calling this.
I prefer keep this const to save that copy :-)

200

Tried that before but I encountered 2 issues:

1. 'clang::DeclContext' is not polymorphic
2. cannot use dynamic_cast with -fno-rtti

which are preventing me from using dynamic_cast

[hokein]

clang-tidy/objc/PropertyDeclarationCheck.cpp

107

I think we save this code by using find_first_of and llvm::StringRef::tolower() function.

127

The same, ArrayRef.

140

The same: llvm::StringRef

145

Why? MatchedDecl->getName() returns llvm::StringRef. As the name indicates, StringRef is a const reference of String, using StringRef is sufficient.

The same to ArrayRef. So the function is like bool prefixedPropertyNameMatches(llvm::StringRef PropertyName, llvm::ArrayRef<std::string> Acronyms).

200

Sorry, I mean llvm::dyn_cast here, it should work.

203

Do we need it? I think you can pass SpecialAcronyms directly.

[Wizard]

clang-tidy/objc/PropertyDeclarationCheck.cpp

140

Cannot remove the const for the same reason.

145

If I remove the const in the method, compiler will have error of "candidate function not viable: expects an l-value for 1st argument". I believe it cannot accept a const reference as arg if the definition is var.

200

It is not working either. It says "error: static_cast from 'clang::Decl *' to 'const clang::ObjCCategoryDecl *const *' is not allowed" though I have no idea why it is regarded as a static cast.
I am using it like this:
const auto *CategoryDecl = llvm::dyn_cast<const ObjCCategoryDecl *>(DeclContext);

[Wizard]

resolve comments

[benhamilton]

clang-tidy/objc/PropertyDeclarationCheck.cpp

145

I think hokein@ is saying you should change:

const llvm::StringRef &

to:

llvm::StringRef

Note removing both const and &, not just removing const. This is because there is no need to pass these by reference, they are already a reference.

Same with const llvm::ArrayRef &, it should just be llvm::ArrayRef.

151

No need to construct another llvm::StringRef(), StringRef::substr() returns a StringRef already.

200

You definitely don't want to use a C-style cast. My guess is adding const is not part of what dyn_cast<> does, so you can probably remove that from the dyn_cast<>. (You can of course assign to a const pointer if you want.)

[Wizard]

resolve comments and fix some logic

[benhamilton]

clang-tidy/objc/PropertyDeclarationCheck.cpp

148

Can you also check that Start + 1 < PropertyName.length() (e.g., the string doesn't end with a _)?

I know the regular expression shouldn't match this, so an assert is probably good. (There is logic later which tries to access Start + 1 without checking.)

154

Don't need to explicitly wrap in llvm::StringRef, that class has an implicit constructor from std::string.

https://github.com/llvm-mirror/llvm/blob/master/include/llvm/ADT/StringRef.h#L95

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

19–35

Can we add checks for properties whose names end with _?

[Wizard]

add more tests

[benhamilton]

Thanks!

[Wizard]

clang-tidy/objc/PropertyDeclarationCheck.cpp

200

Finally figured out that I cannot put pointer type in dyn_cast<>. It needs to be "llvm::dyn_cast<ObjCCategoryDecl>(DeclContext)" even though DeclContext is a pointer. I have to say it is weird in C++...

203

It is not about acronyms but determine how to generate the fix. It will save me from checking the naming format again when generating the fix hint.

[Wizard]

merge changes and add EscapedAcronyms to store the actual acronyms during runtime.

[hokein]

clang-tidy/objc/PropertyDeclarationCheck.cpp

145

Yeah, this is what I mean, thanks for the clarifying.

200

Yeah, llvm::dyn_cast doesn't need to specify the pointer, sorry for my beginning comment...

clang-tidy/objc/PropertyDeclarationCheck.h

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;
+  std::vector<std::string> EscapedAcronyms;
 };
 
 } // 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 <algorithm>
 #include "../utils/OptionsUtils.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "llvm/ADT/STLExtras.h"
+#include "clang/Basic/CharInfo.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 {
+
+// For StandardProperty the naming style is 'lowerCamelCase'.
+// For CategoryProperty especially in categories of system class,
+// to avoid naming conflict, the suggested naming style is
+// 'abc_lowerCamelCase' (adding lowercase prefix followed by '_').
+enum NamingStyle {
+  StandardProperty = 1,
+  CategoryProperty = 2,
+};
+
 /// 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 llvm::StringLiteral DefaultSpecialAcronyms[] = {
     "ACL",
     "API",
     "ARGB",
constexpr llvm::StringLiteral DefaultSpecialAcronyms[] = {
     "URL",
     "VC",
     "VOIP",
     "VPN",
     "VR",
     "WAN",
     "XML",
 };
 
-/// For now we will only fix 'CamelCase' property to
-/// 'camelCase'. For other cases the users need to
+/// 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.
 /// 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]);
+FixItHint generateFixItHint(const ObjCPropertyDecl *Decl, NamingStyle Style) {
+  auto Name = Decl->getName();
+  size_t Index = 0;
+  if (Style == CategoryProperty) {
+    Index = Name.find_first_of('_') + 1;
+    NewName.replace(0, Index - 1, Name.substr(0, Index - 1).lower());
+  }
+  if (Index < Name.size()) {
+    NewName[Index] = tolower(NewName[Index]);
+    if (NewName != Name) {
       return FixItHint::CreateReplacement(
           CharSourceRange::getTokenRange(SourceRange(Decl->getLocation())),
           llvm::StringRef(NewName));
     }
+  }
   return FixItHint();
 }
 
-std::string validPropertyNameRegex(const std::vector<std::string> &EscapedAcronyms) {
+std::string validPropertyNameRegex(llvm::ArrayRef<std::string> EscapedAcronyms,
+                                   bool UsedInMatcher) {
   // Allow any of these names:
   // foo
   // fooBar
   // url
   // urlString
   // URL
   // URLString
   // bundleID
-  return std::string("::((") +
+  std::string StartMatcher = UsedInMatcher ? "::" : "^";
+
+  return StartMatcher + "((" +
          llvm::join(EscapedAcronyms.begin(), EscapedAcronyms.end(), "|") +
          ")[A-Z]?)?[a-z]+[a-z0-9]*([A-Z][a-z0-9]+)*" + "(" +
-      llvm::join(EscapedAcronyms.begin(), EscapedAcronyms.end(), "|") + ")?$";
+         llvm::join(EscapedAcronyms.begin(), EscapedAcronyms.end(), "|") +
+         ")?$";
+}
+
+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) {
+  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)));
+  return RegexExp.match(PropertyName.substr(Start + 1));
 }
 }  // namespace
 
 PropertyDeclarationCheck::PropertyDeclarationCheck(StringRef Name,
                                                    ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       SpecialAcronyms(
           utils::options::parseStringList(Options.get("Acronyms", ""))),
-      IncludeDefaultAcronyms(Options.get("IncludeDefaultAcronyms", true)) {}
+      IncludeDefaultAcronyms(Options.get("IncludeDefaultAcronyms", true)),
+      EscapedAcronyms() {}
 
 void PropertyDeclarationCheck::registerMatchers(MatchFinder *Finder) {
-  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(EscapedAcronyms))))
+          unless(matchesName(validPropertyNameRegex(EscapedAcronyms, true))))
           .bind("property"),
       this);
 }
 
 void PropertyDeclarationCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *MatchedDecl =
       Result.Nodes.getNodeAs<ObjCPropertyDecl>("property");
   assert(MatchedDecl->getName().size() > 0);
+  auto *DeclContext = MatchedDecl->getDeclContext();
+  auto *CategoryDecl = llvm::dyn_cast<ObjCCategoryDecl>(DeclContext);
+  if (CategoryDecl != nullptr &&
+      hasCategoryPropertyPrefix(MatchedDecl->getName())) {
+    if (!prefixedPropertyNameValid(MatchedDecl->getName(), EscapedAcronyms) ||
+        CategoryDecl->IsClassExtension()) {
+      NamingStyle Style = CategoryDecl->IsClassExtension() ? StandardProperty
+                                                           : CategoryProperty;
+      diag(MatchedDecl->getLocation(),
+           "property name '%0' not using lowerCamelCase style or not prefixed "
+           "in a category, according to the Apple Coding Guidelines")
+          << MatchedDecl->getName() << generateFixItHint(MatchedDecl, Style);
+    }
+    return;
+  }
   diag(MatchedDecl->getLocation(),
-       "property name '%0' should use lowerCamelCase style, according to "
-       "the Apple Coding Guidelines")
-      << MatchedDecl->getName() << generateFixItHint(MatchedDecl);
+       "property name '%0' not using lowerCamelCase style or not prefixed in "
+       "a category, according to the Apple Coding Guidelines")
+      << MatchedDecl->getName()
+      << generateFixItHint(MatchedDecl, StandardProperty);
 }
 
 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

 will suppress the Lower Camel Case check according to the guide:
 https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/CodingGuidelines/Articles/NamingBasics.html#//apple_ref/doc/uid/20001281-1002931-BBCFHEAB
 
 For a full list of well-known acronyms:
 https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/CodingGuidelines/Articles/APIAbbreviations.html#//apple_ref/doc/uid/20001285-BCIHCGAE
 
 The corresponding style rule: https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/CodingGuidelines/Articles/NamingIvarsAndTypes.html#//apple_ref/doc/uid/20001284-1001757
 
+The check will also accept property declared in category with a prefix of
+lowercase letters followed by a '_' to avoid naming conflict. For example:
+
+.. code-block:: objc
+@property(nonatomic, assign) int abc_lowerCamelCase;
+
+The corresponding style rule: https://developer.apple.com/library/content/qa/qa1908/_index.html
+
+
 Options
 -------
 
 .. option:: Acronyms
 
    Semicolon-separated list of custom acronyms that can be used as a prefix
    or a suffix of property names.
 

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-MESSAGES: :[[@LINE-1]]:34: warning: property name 'AbcNotRealPrefix' not using lowerCamelCase style or not prefixed in a category, 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]
+// CHECK-MESSAGES: :[[@LINE-1]]:40: warning: property name 'ABC_custom_prefix' not using lowerCamelCase style or not prefixed in a category, 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.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-MESSAGES: :[[@LINE-1]]:34: warning: property name 'AbcNotRealPrefix' not using lowerCamelCase style or not prefixed in a category, 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]
+// CHECK-MESSAGES: :[[@LINE-1]]:40: warning: property name 'ABC_custom_prefix' not using lowerCamelCase style or not prefixed in a category, 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]
+// CHECK-MESSAGES: :[[@LINE-1]]:34: warning: property name 'GIFIgnoreStandardAcronym' not using lowerCamelCase style or not prefixed in a category, according to the Apple Coding Guidelines [objc-property-declaration]
 @end

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

 // RUN: %check_clang_tidy %s objc-property-declaration %t
 @class NSData;
 @class NSString;
 @class UIViewController;
 
 @interface Foo
 @property(assign, nonatomic) int NotCamelCase;
-// CHECK-MESSAGES: :[[@LINE-1]]:34: warning: property name 'NotCamelCase' should use lowerCamelCase style, according to the Apple Coding Guidelines [objc-property-declaration]
+// 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]
 // CHECK-FIXES: @property(assign, nonatomic) int notCamelCase;
 @property(assign, nonatomic) int camelCase;
 @property(strong, nonatomic) NSString *URLString;
 @property(strong, nonatomic) NSString *bundleID;
 @property(strong, nonatomic) NSData *RGBABytes;
 @property(strong, nonatomic) UIViewController *notificationsVC;
 @property(strong, nonatomic) NSString *URL_string;
-// CHECK-MESSAGES: :[[@LINE-1]]:40: warning: property name 'URL_string' should use lowerCamelCase style, according to the Apple Coding Guidelines [objc-property-declaration]
+// CHECK-MESSAGES: :[[@LINE-1]]:40: warning: property name 'URL_string' not using lowerCamelCase style or not prefixed in a category, according to the Apple Coding Guidelines [objc-property-declaration]
 @end
+
+@interface Foo (Bar)
+@property(assign, nonatomic) int abc_NotCamelCase;
+// CHECK-MESSAGES: :[[@LINE-1]]:34: warning: property name 'abc_NotCamelCase' not using lowerCamelCase style or not prefixed in a category, according to the Apple Coding Guidelines [objc-property-declaration]
+@property(assign, nonatomic) int abCD_camelCase;
+// CHECK-MESSAGES: :[[@LINE-1]]:34: warning: property name 'abCD_camelCase' not using lowerCamelCase style or not prefixed in a category, according to the Apple Coding Guidelines [objc-property-declaration]
+// CHECK-FIXES: @property(assign, nonatomic) int abcd_camelCase;
+@property(assign, nonatomic) int abCD_NotCamelCase;
+// CHECK-MESSAGES: :[[@LINE-1]]:34: warning: property name 'abCD_NotCamelCase' not using lowerCamelCase style or not prefixed in a category, according to the Apple Coding Guidelines [objc-property-declaration]
+// CHECK-FIXES: @property(assign, nonatomic) int abcd_notCamelCase;
+@property(assign, nonatomic) int wrongFormat_;
+// CHECK-MESSAGES: :[[@LINE-1]]:34: warning: property name 'wrongFormat_' not using lowerCamelCase style or not prefixed in a category, according to the Apple Coding Guidelines [objc-property-declaration]
+@property(strong, nonatomic) NSString *URLStr;
+@property(assign, nonatomic) int abc_camelCase;
+@end
+
+@interface Foo ()
+@property(assign, nonatomic) int abc_inClassExtension;
+// CHECK-MESSAGES: :[[@LINE-1]]:34: warning: property name 'abc_inClassExtension' not using lowerCamelCase style or not prefixed in a category, according to the Apple Coding Guidelines [objc-property-declaration]
+@end
 No newline at end of file