[clang-tidy] add new check to find out objc ivars which do not have prefix '_'

[Wizard]

Summary

For code of ivar declaration:

int barWithoutPrefix;

The fix will be:

int _barWithoutPrefix;

[Wizard]

fix doc

[Eugene.Zelenko]

If this is Apple guideline, check name should reflect this. I think will be good idea to have general check for Apple naming conventions instead of separate checks for specific situations like objc-ivar-declaration and objc-property-declaration.

clang-tidy/objc/IvarDeclarationCheck.cpp

23

Please use static instead of anonymous namespace.

24

Please don't use auto when type could not be deduced from statement. Same in other places.

docs/ReleaseNotes.rst

60

Please place in new check list in alphabetical order.

63

Please remove New check that.

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

6

Please synchronize first statement with Release Notes.

[Wizard]

In D45392#1060485, @Eugene.Zelenko wrote:

If this is Apple guideline, check name should reflect this. I think will be good idea to have general check for Apple naming conventions instead of separate checks for specific situations like objc-ivar-declaration and objc-property-declaration.

Thanks for the suggestion. I understand your point that they are both naming convention, however, they are about different components and using totally different naming rules. PropertyDeclarationCheck is already a very complicated check (the most complicated one for ObjC), I would rather not make it more heavy and try my best to split independent logic to different checks.

clang-tidy/objc/IvarDeclarationCheck.cpp

23

Using anonymous namespace was suggested by others for private methods that are only used within the check (e.g. ForbiddenSubclassingCheck, PropertyDeclarationCheck, etc). I would rather keep this pattern consistent with other checks.

docs/ReleaseNotes.rst

60

I did not see a "new check list" in alphabetical order in this file. I believe they are ordered by commit time.If you mean the list.rst, they are already ordered alphabetically.

[Wizard]

resolve comments

[Eugene.Zelenko]

In D45392#1060845, @Wizard wrote:

In D45392#1060485, @Eugene.Zelenko wrote:

If this is Apple guideline, check name should reflect this. I think will be good idea to have general check for Apple naming conventions instead of separate checks for specific situations like objc-ivar-declaration and objc-property-declaration.

Thanks for the suggestion. I understand your point that they are both naming convention, however, they are about different components and using totally different naming rules. PropertyDeclarationCheck is already a very complicated check (the most complicated one for ObjC), I would rather not make it more heavy and try my best to split independent logic to different checks.

See readability-identifier-naming as example of multiple rules in one check.

docs/ReleaseNotes.rst

60

Please read starting words in list of changes.

[Wizard]

In D45392#1060854, @Eugene.Zelenko wrote:

In D45392#1060845, @Wizard wrote:

In D45392#1060485, @Eugene.Zelenko wrote:

If this is Apple guideline, check name should reflect this. I think will be good idea to have general check for Apple naming conventions instead of separate checks for specific situations like objc-ivar-declaration and objc-property-declaration.

Thanks for the suggestion. I understand your point that they are both naming convention, however, they are about different components and using totally different naming rules. PropertyDeclarationCheck is already a very complicated check (the most complicated one for ObjC), I would rather not make it more heavy and try my best to split independent logic to different checks.

See readability-identifier-naming as example of multiple rules in one check.

I took a look at IdentifierNamingCheck. Here's my thought:

1. IdentifierNamingCheck is trying to apply configurable naming convention to C++ identifiers, and all the identifiers will share the same style set. That is not the case of ObjC, where we follow Apple's programming guide, and different types of identifiers are using different style.
2. Such pattern can handle complicated requirements but to me it is not simple enough to read and maintain. I would rather keep things simple and clear as long as we have choice.

However, this check provides a good example of refactoring if in the future we have the needs of organizing complicated naming styles. Moving from simplicity to complexity is always easier. Thanks for pointing this out for us.

docs/ReleaseNotes.rst

60

Hmm line 96 is "- New :doc:fuchsia-multiple-inheritance <clang-tidy/checks/fuchsia-multiple-inheritance> check", while line 101 is "- New :doc:abseil-string-find-startswith <clang-tidy/checks/abseil-string-find-startswith> check". And similar thing happened to line 138. I thought it wasn't alphabetical. But that's probably mistakes from others. I will fix this one.

[Eugene.Zelenko]

In D45392#1060912, @Wizard wrote:

In D45392#1060854, @Eugene.Zelenko wrote:

In D45392#1060845, @Wizard wrote:

In D45392#1060485, @Eugene.Zelenko wrote:

If this is Apple guideline, check name should reflect this. I think will be good idea to have general check for Apple naming conventions instead of separate checks for specific situations like objc-ivar-declaration and objc-property-declaration.

Thanks for the suggestion. I understand your point that they are both naming convention, however, they are about different components and using totally different naming rules. PropertyDeclarationCheck is already a very complicated check (the most complicated one for ObjC), I would rather not make it more heavy and try my best to split independent logic to different checks.

See readability-identifier-naming as example of multiple rules in one check.

I took a look at IdentifierNamingCheck. Here's my thought:

1. IdentifierNamingCheck is trying to apply configurable naming convention to C++ identifiers, and all the identifiers will share the same style set. That is not the case of ObjC, where we follow Apple's programming guide, and different types of identifiers are using different style.
2. Such pattern can handle complicated requirements but to me it is not simple enough to read and maintain. I would rather keep things simple and clear as long as we have choice.

However, this check provides a good example of refactoring if in the future we have the needs of organizing complicated naming styles. Moving from simplicity to complexity is always easier. Thanks for pointing this out for us.

My point is not flexibility of configuration, but handling of various types of identifiers in same check, even if conventions are different.

[Wizard]

In D45392#1060971, @Eugene.Zelenko wrote:

In D45392#1060912, @Wizard wrote:

In D45392#1060854, @Eugene.Zelenko wrote:

In D45392#1060845, @Wizard wrote:

In D45392#1060485, @Eugene.Zelenko wrote:

If this is Apple guideline, check name should reflect this. I think will be good idea to have general check for Apple naming conventions instead of separate checks for specific situations like objc-ivar-declaration and objc-property-declaration.

Thanks for the suggestion. I understand your point that they are both naming convention, however, they are about different components and using totally different naming rules. PropertyDeclarationCheck is already a very complicated check (the most complicated one for ObjC), I would rather not make it more heavy and try my best to split independent logic to different checks.

See readability-identifier-naming as example of multiple rules in one check.

I took a look at IdentifierNamingCheck. Here's my thought:

1. IdentifierNamingCheck is trying to apply configurable naming convention to C++ identifiers, and all the identifiers will share the same style set. That is not the case of ObjC, where we follow Apple's programming guide, and different types of identifiers are using different style.
2. Such pattern can handle complicated requirements but to me it is not simple enough to read and maintain. I would rather keep things simple and clear as long as we have choice.

However, this check provides a good example of refactoring if in the future we have the needs of organizing complicated naming styles. Moving from simplicity to complexity is always easier. Thanks for pointing this out for us.

My point is not flexibility of configuration, but handling of various types of identifiers in same check, even if conventions are different.

Yes I understand but I mean "flexibility of configuration" is one of the reasons of handling of various types of identifiers in same check, but we don't need it here.

[Eugene.Zelenko]

In D45392#1061064, @Wizard wrote:

In D45392#1060971, @Eugene.Zelenko wrote:

In D45392#1060912, @Wizard wrote:

In D45392#1060854, @Eugene.Zelenko wrote:

In D45392#1060845, @Wizard wrote:

In D45392#1060485, @Eugene.Zelenko wrote:

If this is Apple guideline, check name should reflect this. I think will be good idea to have general check for Apple naming conventions instead of separate checks for specific situations like objc-ivar-declaration and objc-property-declaration.

Thanks for the suggestion. I understand your point that they are both naming convention, however, they are about different components and using totally different naming rules. PropertyDeclarationCheck is already a very complicated check (the most complicated one for ObjC), I would rather not make it more heavy and try my best to split independent logic to different checks.

See readability-identifier-naming as example of multiple rules in one check.

I took a look at IdentifierNamingCheck. Here's my thought:

1. IdentifierNamingCheck is trying to apply configurable naming convention to C++ identifiers, and all the identifiers will share the same style set. That is not the case of ObjC, where we follow Apple's programming guide, and different types of identifiers are using different style.
2. Such pattern can handle complicated requirements but to me it is not simple enough to read and maintain. I would rather keep things simple and clear as long as we have choice.

However, this check provides a good example of refactoring if in the future we have the needs of organizing complicated naming styles. Moving from simplicity to complexity is always easier. Thanks for pointing this out for us.

My point is not flexibility of configuration, but handling of various types of identifiers in same check, even if conventions are different.

Yes I understand but I mean "flexibility of configuration" is one of the reasons of handling of various types of identifiers in same check, but we don't need it here.

From user point of view, it's much easy to have one check which will check all possible types of identifiers, then set of not so obviously related checks.

[Wizard]

reorder release note for alphabetical order

[alexfh]

I wonder whether the readability-identifier-naming check could be extended to support this use case instead of adding a new check specifically for underscores in ivar names?

[Wizard]

In D45392#1061433, @alexfh wrote:

I wonder whether the readability-identifier-naming check could be extended to support this use case instead of adding a new check specifically for underscores in ivar names?

Hmm readability-identifier-naming is a C++ check but this one is only for ObjC. I prefer putting them in separate places unless they work for both languages.
Moreover, readability-identifier-naming always runs all matchers and apply the same checks on all identifiers. We have to change at least part of the structure to make it applicable for this check. And readability-identifier-naming is not in google default clang-tidy check list yet. We will even need more work to import it.
So I think creating a new simple ObjC-specific check is a better way here.

[alexfh]

In D45392#1061960, @Wizard wrote:

In D45392#1061433, @alexfh wrote:

I wonder whether the readability-identifier-naming check could be extended to support this use case instead of adding a new check specifically for underscores in ivar names?

Hmm readability-identifier-naming is a C++ check but this one is only for ObjC. I prefer putting them in separate places unless they work for both languages.

I see no reasons why this check can't work for ObjC, if the handling of ObjC-specific AST nodes is added to it.

Moreover, readability-identifier-naming always runs all matchers and apply the same checks on all identifiers.

It has some sort of a hierarchical structure of rules that allow it to only touch a certain subset of identifiers. E.g. configure different naming styles for local variables and local constants. What it's lacking is a good documentation for all of these options =\

We have to change at least part of the structure to make it applicable for this check.

Yes, the existing check may need some modifications to do what this check needs to do, but I'd expect these modifications to be quite small, and we would potentially get a much more useful and generic tool instead of "one check per type of named entity per naming style" situation.

And readability-identifier-naming is not in google default clang-tidy check list yet. We will even need more work to import it.

IIUC, it can be configured to only verify certain kinds of named entities. Thus there's no requirement to make it work with every aspect of our naming rules.

So I think creating a new simple ObjC-specific check is a better way here.

I'll respectfully disagree here. I would prefer to have a generic solution, if it's feasible. And so far, it looks like it is.

[Wizard]

move check to readability-identifier-naming

[Eugene.Zelenko]

How about doing same for objc-property-declaration?

[Wizard]

In D45392#1063164, @alexfh wrote:

In D45392#1061960, @Wizard wrote:

In D45392#1061433, @alexfh wrote:

I wonder whether the readability-identifier-naming check could be extended to support this use case instead of adding a new check specifically for underscores in ivar names?

Hmm readability-identifier-naming is a C++ check but this one is only for ObjC. I prefer putting them in separate places unless they work for both languages.

I see no reasons why this check can't work for ObjC, if the handling of ObjC-specific AST nodes is added to it.

Moreover, readability-identifier-naming always runs all matchers and apply the same checks on all identifiers.

It has some sort of a hierarchical structure of rules that allow it to only touch a certain subset of identifiers. E.g. configure different naming styles for local variables and local constants. What it's lacking is a good documentation for all of these options =\

We have to change at least part of the structure to make it applicable for this check.

Yes, the existing check may need some modifications to do what this check needs to do, but I'd expect these modifications to be quite small, and we would potentially get a much more useful and generic tool instead of "one check per type of named entity per naming style" situation.

And readability-identifier-naming is not in google default clang-tidy check list yet. We will even need more work to import it.

IIUC, it can be configured to only verify certain kinds of named entities. Thus there's no requirement to make it work with every aspect of our naming rules.

So I think creating a new simple ObjC-specific check is a better way here.

I'll respectfully disagree here. I would prefer to have a generic solution, if it's feasible. And so far, it looks like it is.

Done. Yes it is very small changes to integrate this into it. I like it. The only annoying part was the doc is not that good and I spent a lot of time understanding the whole structure. Just one question, when we enable it in google by default, do we need to take care of the flags in the tests like -I%S/Inputs/readability-identifier-naming and -isystem %S/Inputs/readability-identifier-naming/system?

[alexfh]

Thank you, that's much better!

I'd also appreciate, if you could document the options this check supports in its .rst document (separately from this patch).

A few more comments inline.

test/clang-tidy/readability-identifier-naming-objc.m

1–3 ↗

(On Diff #141938)

I'd try committing the patch without disabling the test for powerpc64le and see whether the relevant buildbot complains. Looking at the description of r288563, I don't see anything that could be relevant to this test.

7 ↗

(On Diff #141938)

I suspect the -fno-delayed-template-parsing is not needed, since no templates are used in this test.

8–9 ↗

(On Diff #141938)

It looks like these flags are not needed, since there are no #include directives in this test.

[Wizard]

remove unnecessary flags

[Wizard]

test/clang-tidy/readability-identifier-naming-objc.m

8–9 ↗

(On Diff #141938)

So we won't worry about those flags when using this check in google codebase right?

[alexfh]

test/clang-tidy/readability-identifier-naming-objc.m

4 ↗

(On Diff #142057)

The -- and the trailing backslash above can be removed as well.

8–9 ↗

(On Diff #141938)

These flags are used by the other test to specify directories used to search for #included headers. Since the test is copied to a temporary directory by the check_clang_tidy.py script and the headers stay in the source tree (see the test/clang-tidy/Inputs/readability-identifier-naming/ directory), the test can't just use relative include paths, instead we specify the full path using the -I and -isystem compiler options. In real use cases all compilation arguments (including the required -I and -isystem options) are taken from a compilation database. No need to specify them in clang-tidy configuration.

[alexfh]

BTW, it looks like the http://clang.llvm.org/extra/clang-tidy/checks/objc-property-declaration.html check could also be replaced with readability-identifier-naming (not in this patch).

[Wizard]

test/clang-tidy/readability-identifier-naming-objc.m

4 ↗

(On Diff #142057)

I tried but when I run the test there some error output:
Running ['clang-tidy', '/Users/ynzhang/clang-llvm/build/tools/clang/tools/extra/test/clang-tidy/Output/readability-identifier-naming-objc.m.tmp.m', '-fix', '--checks=-*,readability-identifier-naming', '-config={CheckOptions: [:+readability-identifier-naming.ObjcIvarPrefix,+value:+_]}', '-nostdinc++']...
clang-tidy failed:
LLVM ERROR: CommonOptionsParser: failed to parse command-line arguments. [CommonOptionsParser]: clang-tidy: Unknown command line argument '-nostdinc++'. Try: 'clang-tidy -help'
clang-tidy: Did you mean '-gpsize'?

No idea where '-nostdinc++' came from, but there is no such problem with these stuff added. I actually followed the command pattern in objc-forbidden-subclassing-custom.m

[alexfh]

LG. Thanks!

clang-tidy/objc/CMakeLists.txt

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

clang-tidy/objc/IvarDeclarationCheck.h

+//===--- IvarDeclarationCheck.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_IVARDECLARATIONCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_OBJC_IVARDECLARATIONCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace objc {
+
+/// Finds Objective-C ivars which do not have '_' prefix.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/objc-ivar-declaration.html
+class IvarDeclarationCheck : public ClangTidyCheck {
+public:
+  IvarDeclarationCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace objc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_OBJC_IVARDECLARATIONCHECK_H

clang-tidy/objc/IvarDeclarationCheck.cpp

+//===--- IvarDeclarationCheck.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 "IvarDeclarationCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "llvm/Support/Regex.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace objc {
+
+namespace {
+
+FixItHint generateFixItHint(const ObjCIvarDecl *Decl) {
+  auto IvarName = Decl->getName();
+  if (IvarName[0] != '_') {
+    auto NewName = Decl->getName().str();
+    NewName.insert(0, 1, '_');
+    return FixItHint::CreateReplacement(
+        CharSourceRange::getTokenRange(SourceRange(Decl->getLocation())),
+        llvm::StringRef(NewName));
+  }
+  return FixItHint();
+}
+}  // namespace
+
+void IvarDeclarationCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      objcIvarDecl(unless(matchesName("::_[a-zA-Z]+[a-zA-Z0-9_]*")))
+          .bind("ivar"),
+      this);
+}
+
+void IvarDeclarationCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *MatchedDecl = Result.Nodes.getNodeAs<ObjCIvarDecl>("ivar");
+  diag(MatchedDecl->getLocation(),
+       "instance variable %0 not using '_' as prefix")
+      << MatchedDecl << generateFixItHint(MatchedDecl);
+}
+
+}  // namespace objc
+}  // namespace tidy
+}  // namespace clang

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 "AvoidNSErrorInitCheck.h"
 #include "AvoidSpinlockCheck.h"
 #include "ForbiddenSubclassingCheck.h"
+#include "IvarDeclarationCheck.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<AvoidNSErrorInitCheck>(
         "objc-avoid-nserror-init");
     CheckFactories.registerCheck<AvoidSpinlockCheck>(
         "objc-avoid-spinlock");
     CheckFactories.registerCheck<ForbiddenSubclassingCheck>(
         "objc-forbidden-subclassing");
+    CheckFactories.registerCheck<IvarDeclarationCheck>(
+        "objc-ivar-declaration");
     CheckFactories.registerCheck<PropertyDeclarationCheck>(
         "objc-property-declaration");
   }
 };
 
 // Register the ObjCTidyModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<ObjCModule> X(
     "objc-module",

docs/ReleaseNotes.rst

 Improvements to clang-rename
 ----------------------------
 
 The improvements are...
 
 Improvements to clang-tidy
 --------------------------
 
+- New :doc:`objc-ivar-declaration
+  <clang-tidy/checks/objc-ivar-declaration>` check
+
+  New check that finds Objective-C ivars that do not have a '_' prefix.
+
 - New module `abseil` for checks related to the `Abseil <https://abseil.io>`_
   library.
 
 - New module ``portability``.
 
 - New module ``zircon`` for checks related to Fuchsia's Zircon kernel.
 
 - New :doc:`bugprone-parent-virtual-call

docs/clang-tidy/checks/list.rst

.. toctree::
    modernize-use-transparent-functors
    modernize-use-uncaught-exceptions
    modernize-use-using
    mpi-buffer-deref
    mpi-type-mismatch
    objc-avoid-nserror-init
    objc-avoid-spinlock
    objc-forbidden-subclassing
+   objc-ivar-declaration
    objc-property-declaration
    performance-faster-string-find
    performance-for-range-copy
    performance-implicit-conversion-in-loop
    performance-inefficient-algorithm
    performance-inefficient-string-concatenation
    performance-inefficient-vector-operation
    performance-move-const-arg

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

+.. title:: clang-tidy - objc-ivar-declaration
+
+objc-ivar-declaration
+=====================
+
+Finds ivar declarations in Objective-C files that do not follow the pattern
+in Apple's programming guide. The ivar name should be fixed with '_'.
+
+For code of ivar declaration:
+
+.. code-block:: objc
+
+   int barWithoutPrefix;
+
+The fix will be:
+
+.. code-block:: objc
+
+   int _barWithoutPrefix;
+
+The corresponding style rule: https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/CodingGuidelines/Articles/NamingIvarsAndTypes.html#//apple_ref/doc/uid/20001284-1001757

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

+// RUN: %check_clang_tidy %s objc-ivar-declaration %t
+@interface Foo
+@end 
+
+@interface Foo () {
+    int _bar;
+    int barWithoutPrefix;
+    // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: instance variable 'barWithoutPrefix' not using '_' as prefix [objc-ivar-declaration]
+    // CHECK-FIXES: int _barWithoutPrefix;
+}
+@end