add new check for property declaration

[Wizard]

Summary

This check finds property declarations in Objective-C files that do not follow the pattern of property names in Apple's programming guide. The property name should be in the format of Lower Camel Case or with some particular acronyms as prefix.

Example:
@property(nonatomic, assign) int lowerCamelCase;

@property(nonatomic, strong) NSString *URLString;

Test plan: ninja check-clang-tools

[hokein]

Next time, please remember to add cfe-commits to the subscriber.

clang-tidy/objc/PropertyDeclarationCheck.cpp

15

Do we need this header?

26

I might miss some background context.

The fix of the check seems to me that it does more things it should. It removes all the non-alphabetical prefix characters, I'd be conservative of the fix here (just fix the case "CamelCase", and only give a warning for other cases).

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

8

Google style? but the link you provided is Apple.

44

nit: add a newline.

[benhamilton]

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

8

Google's Objective-C style guide is a list of additions on top of Apple's Objective-C style guide.

Property naming standards are defined in Apple's style guide, and not changed by Google's.

[benhamilton]

clang-tidy/objc/PropertyDeclarationCheck.cpp

26

I agree, removing a prefix is not a good idea. Warning is fine.

We could probably also change snake_case variables to CamelCase automatically. Not sure if it's worth doing in this review, but @Wizard can file a bug to follow up and add a TODO comment here mentioning the bug.

[Wizard]

address comments

clang-tidy/objc/PropertyDeclarationCheck.cpp

26

Will do

[hokein]

clang-tidy/objc/PropertyDeclarationCheck.cpp

23

I think we need to update the comment according to the new change (it is a bit ambiguous to users). We only fix "camelCase" currently.

26

nit: llvm uses FIXME.

28

nit: I would add an assert to make sure the name is not empty.

docs/ReleaseNotes.rst

63

nit: this can be removed.

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

8

I see, thanks for the clarification, I think the "Apple" would be clearer.

23

The same.

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

9

Why does the check catch this case? Isn't camelCase a correct name?

[Wizard]

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

9

This is CHECK-MESSAGES-NOT

[benhamilton]

clang-tidy/objc/PropertyDeclarationCheck.cpp

42

There are some exceptions we should special case. Acronyms like URL and HTTP and HTML are allowed at the beginning of method names (and property names—although that is not explicitly mentioned, property names follow the same guidelines as method names):

https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/CodingGuidelines/Articles/NamingBasics.html#//apple_ref/doc/uid/20001281-1002931-BBCFHEAB

For method names, start with a lowercase letter and capitalize the first letter of embedded words. Don’t use prefixes.
fileExistsAtPath:isDirectory:

An exception to this guideline is method names that start with a well-known acronym, for example, TIFFRepresentation (NSImage).

There is a list of well-known acronyms listed here:

https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/CodingGuidelines/Articles/APIAbbreviations.html#//apple_ref/doc/uid/20001285-BCIHCGAE

ASCII
PDF
XML
HTML
URL
RTF
HTTP
TIFF
JPG
PNG
GIF
LZW
ROM
RGB
CMYK
MIDI
FTP

docs/clang-tidy/checks/google-objc-global-variable-declaration.rst

1

Let's remove "google-" everywhere and mention only Apple's style guide.

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

8

Yeah, we don't want to mention Google here. Apple is good.

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

9

I think in that case you don't need an explicit CHECK (there is implicitly a "CHECK-MESSAGES-NOT" on every line for all warnings which fails the test if a warning is raised).

[benhamilton]

clang-tidy/objc/PropertyDeclarationCheck.cpp

42

Probably the right thing to do is make this a configuration option, the same way I did with objc-forbidden-subclassing, and default to the above list.

https://github.com/llvm-mirror/clang-tools-extra/blob/master/clang-tidy/objc/ForbiddenSubclassingCheck.cpp#L25
https://github.com/llvm-mirror/clang-tools-extra/blob/master/clang-tidy/objc/ForbiddenSubclassingCheck.cpp#L74
https://github.com/llvm-mirror/clang-tools-extra/blob/master/clang-tidy/objc/ForbiddenSubclassingCheck.cpp#L110

[Wizard]

add custom options

[Wizard]

clang-tidy/objc/PropertyDeclarationCheck.cpp

28

Added it to the line before generating the warning message.

docs/clang-tidy/checks/google-objc-global-variable-declaration.rst

1

Discussed offline. The change here is for another doc. Irrelevant to the new check.

[hokein]

Looks good to me. I (or @benhamilton) will commit the patch for you if @benhamilton is fine with it.

clang-tidy/objc/PropertyDeclarationCheck.cpp

23

nit: add a comment documenting these are from https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/CodingGuidelines/Articles/APIAbbreviations.html#//apple_ref/doc/uid/20001285-BCIHCGAE.

94

nit: clang-tidy message is not a complete sentence. just convertion; it should.

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

41

s/acronys/acronyms

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

9

Sorry, I misread it as CHECK-MESSAGES. As Ben pointed out, we don't need explicit CHECK here (the default behavior is what we expected).

[Wizard]

address nits

[benhamilton]

clang-tidy/objc/PropertyDeclarationCheck.cpp

89

For the acronyms, this means we will allow properties with names like:

URLfoo_bar_blech

when we do not want that to be legal.

Instead, let's generate the regular expression we pass to matchesName() from the options. I will provide a suggestion shortly.

[benhamilton]

• Use regex to match acronym prefixes, update tests

[benhamilton]

OK, I updated this diff with the suggested changes. @Wizard, want to take a look before I land?

[Wizard]

LGTM

[hokein]

clang-tidy/objc/PropertyDeclarationCheck.cpp

89

Good catch.

clang-tidy/objc/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyObjCModule
   ForbiddenSubclassingCheck.cpp
   ObjCTidyModule.cpp
+  PropertyDeclarationCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   )

clang-tidy/objc/ObjCTidyModule.cpp

 //===--- ObjCTidyModule.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 "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "ForbiddenSubclassingCheck.h"
+#include "PropertyDeclarationCheck.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace objc {
 
 class ObjCModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<ForbiddenSubclassingCheck>(
         "objc-forbidden-subclassing");
+    CheckFactories.registerCheck<PropertyDeclarationCheck>(
+        "objc-property-declaration");
   }
 };
 
 // Register the ObjCTidyModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<ObjCModule> X(
     "objc-module",
     "Adds Objective-C lint checks.");
 
 } // namespace objc
 
 // This anchor is used to force the linker to link in the generated object file
 // and thus register the ObjCModule.
 volatile int ObjCModuleAnchorSource = 0;
 
 } // namespace tidy
 } // namespace clang

clang-tidy/objc/PropertyDeclarationCheck.h

+//===--- PropertyDeclarationCheck.h - clang-tidy-----------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_OBJC_PROPERTY_DECLARATION_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_OBJC_PROPERTY_DECLARATION_H
+
+#include "../ClangTidy.h"
+#include <string>
+#include <vector>
+
+namespace clang {
+namespace tidy {
+namespace objc {
+
+/// Finds Objective-C property declarations which
+/// are not in Lower Camel Case.
+///
+/// The format of property should look like:
+/// @property(nonatomic) NSString *lowerCamelCase;
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/objc-property-declaration.html
+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;
+};
+
+} // 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"
+
+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
+constexpr char DefaultSpecialAcronyms[] =
+    "ASCII;"
+    "PDF;"
+    "XML;"
+    "HTML;"
+    "URL;"
+    "RTF;"
+    "HTTP;"
+    "TIFF;"
+    "JPG;"
+    "PNG;"
+    "GIF;"
+    "LZW;"
+    "ROM;"
+    "RGB;"
+    "CMYK;"
+    "MIDI;"
+    "FTP";
+
+/// 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();
+}
+
+bool startsWithSpecialAcronyms(StringRef Name,
+                               const std::vector<std::string> &Prefixes) {
+  for (auto const &Prefix : Prefixes) {
+    if (Name.startswith(Prefix)) {
+      return true;
+    }
+  }
+  return false;
+}
+}  // namespace
+
+PropertyDeclarationCheck::PropertyDeclarationCheck(StringRef Name,
+                                                   ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      SpecialAcronyms(utils::options::parseStringList(
+          Options.get("Acronyms", DefaultSpecialAcronyms))) {}
+
+void PropertyDeclarationCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      objcPropertyDecl(
+          // the property name should be in Lower Camel Case like
+          // 'lowerCamelCase'
+          unless(matchesName("::[a-z]+[a-z0-9]*([A-Z][a-z0-9]+)*$")))
+          .bind("property"),
+      this);
+}
+
+void PropertyDeclarationCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *MatchedDecl =
+      Result.Nodes.getNodeAs<ObjCPropertyDecl>("property");
+  assert(MatchedDecl->getName().size() > 0);
+  // Skip the check of lowerCamelCase if the name has prefix of special acronyms
+  if (startsWithSpecialAcronyms(MatchedDecl->getName(), SpecialAcronyms)) {
+    return;
+  }
+  diag(MatchedDecl->getLocation(),
+       "property '%0' is not in proper format according to property naming "
+       "convention")
+      << MatchedDecl->getName() << generateFixItHint(MatchedDecl);
+}
+
+void PropertyDeclarationCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "Acronyms",
+                utils::options::serializeStringList(SpecialAcronyms));
+}
+
+}  // namespace objc
+}  // namespace tidy
+}  // namespace clang

docs/ReleaseNotes.rst

 Improvements to clang-rename
 ----------------------------
 
 The improvements are...
 
 Improvements to clang-tidy
 --------------------------
 
+- New `objc-property-declaration
+  <http://clang.llvm.org/extra/clang-tidy/checks/objc-property-declaration.html>`_ check
+
+  Add new check for Objective-C code to ensure property
+  names follow the naming convention of Apple's programming
+  guide.
+
 - New `google-objc-global-variable-declaration
   <http://clang.llvm.org/extra/clang-tidy/checks/google-global-variable-declaration.html>`_ check
 
   Add new check for Objective-C code to ensure global 
   variables follow the naming convention of 'k[A-Z].*' (for constants) 
   or 'g[A-Z].*' (for variables).
 
 - New module `objc` for Objective-C style checks.

docs/clang-tidy/checks/google-objc-global-variable-declaration.rst

 .. title:: clang-tidy - google-objc-global-variable-declaration
 
 google-objc-global-variable-declaration
 =======================================
 
-Finds global variable declarations in Objective-C files that are not follow the pattern
+Finds global variable declarations in Objective-C files that do not follow the pattern
 of variable names in Google's Objective-C Style Guide.
 
 The corresponding style guide rule:
 http://google.github.io/styleguide/objcguide.html#variable-names
 
 All the global variables should follow the pattern of `g[A-Z].*` (variables) or
 `k[A-Z].*` (constants). The check will suggest a variable name that follows the pattern
 if it can be inferred from the original name.
 
 For code:
 
 .. code-block:: objc
+
   static NSString* myString = @"hello";
 
 The fix will be:
 
 .. code-block:: objc
+
   static NSString* gMyString = @"hello";
 
 Another example of constant:
 
 .. code-block:: objc
+
   static NSString* const myConstString = @"hello";
 
 The fix will be:
 
 .. code-block:: objc
+
   static NSString* const kMyConstString = @"hello";
 
 However for code that prefixed with non-alphabetical characters like:
 
 .. code-block:: objc
+
   static NSString* __anotherString = @"world";
 
 The check will give a warning message but will not be able to suggest a fix. The user
 need to fix it on his own.

docs/clang-tidy/checks/list.rst

.. toctree::
    modernize-use-equals-delete
    modernize-use-noexcept
    modernize-use-nullptr
    modernize-use-override
    modernize-use-transparent-functors
    modernize-use-using
    mpi-buffer-deref
    mpi-type-mismatch
+   objc-forbidden-subclassing
+   objc-property-declaration
    performance-faster-string-find
    performance-for-range-copy
    performance-implicit-conversion-in-loop
    performance-inefficient-string-concatenation
    performance-inefficient-vector-operation
    performance-type-promotion-in-math-fn
    performance-unnecessary-copy-initialization
    performance-unnecessary-value-param
-   objc-forbidden-subclassing
    readability-avoid-const-params-in-decls
    readability-braces-around-statements
    readability-container-size-empty
    readability-delete-null-pointer
    readability-deleted-default
    readability-else-after-return
    readability-function-size
    readability-identifier-naming

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

+.. title:: clang-tidy - objc-property-declaration
+
+objc-property-declaration
+=========================
+
+Finds property declarations in Objective-C files that do not follow the pattern
+of property names in Apple's programming guide. The property name should be
+in the format of Lower Camel Case.
+
+For code:
+
+.. code-block:: objc
+
+@property(nonatomic, assign) int LowerCamelCase;
+
+The fix will be:
+
+.. code-block:: objc
+
+@property(nonatomic, assign) int lowerCamelCase;
+
+The check will only fix 'CamelCase' to 'camelCase'. In some other cases we will
+only provide warning messages since the property name could be complicated.
+Users will need to come up with a proper name by their own.
+
+This check also accepts special acronyms as prefix. Such prefix will suppress
+the check of Lower Camel Case 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
+
+Options
+-------
+
+.. option:: Acronyms
+
+   Semicolon-separated list of acronyms that can be used as prefix
+   of property names.
+
+   Defaults to ``ASCII;PDF;XML;HTML;URL;RTF;HTTP;TIFF;JPG;PNG;GIF;LZW;ROM;RGB;CMYK;MIDI;FTP``.

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: --
+
+@interface Foo
+@property(assign, nonatomic) int AbcNotRealPrefix;
+// CHECK-MESSAGES: :[[@LINE-1]]:34: warning: property 'AbcNotRealPrefix' is not in proper format according to property naming convention [objc-property-declaration]
+// CHECK-FIXES: @property(assign, nonatomic) int abcNotRealPrefix;
+@property(assign, nonatomic) int ABCCustomPrefix;
+@end

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

+// RUN: %check_clang_tidy %s objc-property-declaration %t
+@class NSString;
+
+@interface Foo
+@property(assign, nonatomic) int NotCamelCase;
+// CHECK-MESSAGES: :[[@LINE-1]]:34: warning: property 'NotCamelCase' is not in proper format according to property naming convention [objc-property-declaration]
+// CHECK-FIXES: @property(assign, nonatomic) int notCamelCase;
+@property(assign, nonatomic) int camelCase;
+@property(strong, nonatomic) NSString *URLString;
+@end