[clang-tidy/google] Improve the Objective-C global variable declaration check 🔧

[stephanemoore]

Summary

The current Objective-C global variable declaration check restricts naming that is permitted by the Google Objective-C style guide.

The Objective-C style guide states the following:
"Global and file scope constants should have an appropriate prefix. [...] Constants may use a lowercase k prefix when appropriate"
http://google.github.io/styleguide/objcguide#constants

This change fixes the check to allow two or more capital letters as an appropriate prefix. This change intentionally avoids making a decision regarding whether to flag constants that use a two letter prefix (two letter prefixes are reserved by Apple¹ but many projects seem to violate this guideline).

This change eliminates an important category of false positives (constants prefixed with '[A-Z]{2,}') at the cost of introducing a less important category of false negatives (constants prefixed with only '[A-Z]'). The false positives are observed in standard recommended code while the false negatives occur in non-standard unrecommended code. The number of eliminated false positives is expected to be significantly larger than the number of exposed false negatives.

❧

(1)
"Two-letter prefixes like these are reserved by Apple for use in framework classes."
https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/ProgrammingWithObjectiveC/Conventions/Conventions.html

[aaron.ballman]

LGTM with a few additional test cases.

It'd be nice if the style guide linked actually described what a "good" prefix is rather than make the reader guess.

test/clang-tidy/google-objc-global-variable-declaration.m

33

Can you also add the code from the style guide as a test case?

extern NSString *const GTLServiceErrorDomain;

typedef NS_ENUM(NSInteger, GTLServiceError) {
  GTLServiceErrorQueryResultMissing = -3000,
  GTLServiceErrorWaitTimedOut       = -3001,
};

[stephanemoore]

Added excerpts from the Google Objective-C style guide section on constant naming (http://google.github.io/styleguide/objcguide#constants) as additional test cases.

[stephanemoore]

In D43581#1014664, @aaron.ballman wrote:

LGTM with a few additional test cases.

It'd be nice if the style guide linked actually described what a "good" prefix is rather than make the reader guess.

I will work on updating the Google Objective-C style guide to provide more clarity regarding what constitutes an appropriate prefix.

[aaron.ballman]

In D43581#1014903, @stephanemoore wrote:

In D43581#1014664, @aaron.ballman wrote:

LGTM with a few additional test cases.

It'd be nice if the style guide linked actually described what a "good" prefix is rather than make the reader guess.

I will work on updating the Google Objective-C style guide to provide more clarity regarding what constitutes an appropriate prefix.

Fantastic, thank you! This patch LGTM.

[stephanemoore]

I forgot to update the diagnostic message per the change.

[Wizard]

Please update the warning info to indicate that prefix 'k' is not the only option for constants. Something like:
"const global variable '%0' must have an appropriate prefix or a name which starts with 'k[A-Z]'"

[stephanemoore]

In D43581#1016300, @Wizard wrote:

Please update the warning info to indicate that prefix 'k' is not the only option for constants. Something like:
"const global variable '%0' must have an appropriate prefix or a name which starts with 'k[A-Z]'"

Is the latest warning info satisfactory? I believe I updated it after adding you as a reviewer.

[Wizard]

In D43581#1016318, @stephanemoore wrote:

In D43581#1016300, @Wizard wrote:

Please update the warning info to indicate that prefix 'k' is not the only option for constants. Something like:
"const global variable '%0' must have an appropriate prefix or a name which starts with 'k[A-Z]'"

Is the latest warning info satisfactory? I believe I updated it after adding you as a reviewer.

Hmm I feel it is a bit unfriendly to show users a rather complicated regex in the info, but I will leave it to you. Not a big problem.

[stephanemoore]

Update the diagnostic information to link to the Google Objective-C style guide for guidance on an appropriate prefix.

[stephanemoore]

In D43581#1016327, @Wizard wrote:

In D43581#1016318, @stephanemoore wrote:

In D43581#1016300, @Wizard wrote:

Please update the warning info to indicate that prefix 'k' is not the only option for constants. Something like:
"const global variable '%0' must have an appropriate prefix or a name which starts with 'k[A-Z]'"

Is the latest warning info satisfactory? I believe I updated it after adding you as a reviewer.

Hmm I feel it is a bit unfriendly to show users a rather complicated regex in the info, but I will leave it to you. Not a big problem.

At a glance this seems unconventional but could we have the warning link to the Google Objective-C style guide for guidance on an appropriate prefix? Is that allowed for diagnostic messages? I can revert if that is considered inappropriate.

[aaron.ballman]

clang-tidy/google/GlobalVariableDeclarationCheck.cpp

92

We don't usually put hyperlinks in the diagnostic messages, so please remove this.

My suggestion about describing what constitutes an appropriate prefix was with regards to the style guide wording itself. For instance, that document doesn't mention that two capital letters is good. That's not on you to fix before this patch goes in, of course.

[Wizard]

clang-tidy/google/GlobalVariableDeclarationCheck.cpp

92

Btw it is actually "2 or more" characters for prefix. I think it makes sense because we need at least 2 or more characters to call it a "prefix" :-)

[stephanemoore]

Revert the inclusion of the link to the Google Objective-C style guide in the diagnostic message.

[stephanemoore]

I had to make two more fixes to get the tests working:
• Updated the CHECK-MESSAGES in test/clang-tidy/google-objc-global-variable-declaration.m to the revised diagnostic message.
• Removed NS_ENUM and NSInteger from the additions to test/clang-tidy/google-objc-global-variable-declaration.m because their declarations were missing and I assumed that replication of the Foundation macro and type alias was not meaningfully important to this test case.

I verified the changes with the following commands:

$ ${LLVM_ROOT}/tools/clang/tools/extra/test/clang-tidy/check_clang_tidy.py ${LLVM_ROOT}/tools/clang/tools/extra/test/clang-tidy/google-objc-global-variable-declaration.m google-objc-global-variable-declaration /tmp/test && make check-all

Is that satisfactory verification of these changes?

[stephanemoore]

clang-tidy/google/GlobalVariableDeclarationCheck.cpp

92

Reverted the inclusion of the hyperlink in the message. I agree that embedding the hyperlink in the message is indirect and inconsistent with other diagnostic messages in LLVM projecta.

92

§1 Regarding Prefixes And Character Count

Strictly speaking, a single character prefix (e.g., extern const int ALimit; ❌) is possible but not allowed for constants in Google Objective-C style except for lowercase 'k' as a prefix for file scope constants (e.g., static const int kLimit = 3; ✔️). As hinted in the commit description, Google currently enforces a minimum two character prefix (e.g., extern const int ABPrefix; ✔️) or exceptionally 'k' where appropriate.

Technically this modified regex allows authored code to include inadvisable constant names with a single character prefix (e.g., extern const int APrefix; ❌). In this respect I would say that this change eliminates an important category of false positives (constants prefixed with '[A-Z]{2,}') while introducing a less important category of false negatives (constants prefixed with only '[A-Z]'). The false positives are observed in standard recommended code while the false negatives occur in non-standard unrecommended code. Without a reliable mechanism to separate the constant prefix from the constant name (e.g., splitting "FOOURL" into "FOO" and "URL" or "HTTP2Header" into "HTTP2" and "Header"), I cannot immediately think of a way to eliminate the introduced category of false negatives without introducing other false positives. I intend to continue thinking about alternative methods that might eliminate these false negatives without introducing other compromises. If it is acceptable to the reviewers I would propose to move forward with this proposed change to eliminate the observed false positives and then consider addressing the false negatives in a followup.

§2 Regarding the Google Objective-C Style Guide's Description of Appropriate Prefixes

I agree that the Google Objective-C style guide should be clearer on this topic and will pursue clarifying this promptly.

[stephanemoore]

test/clang-tidy/google-objc-global-variable-declaration.m

33

NS_ENUM and NSInteger are not defined in this implementation file.

I can either (1) Omit the macro and the type alias; or (2) reasonably replicate the macro and type alias in this source file.

Which option is preferred? For the time being, I have pursued option (1) on the assumption that NS_ENUM and NSInteger are not critical to this test case.

[aaron.ballman]

clang-tidy/google/GlobalVariableDeclarationCheck.cpp

92

I don't think we should have the regex as part of the diagnostic either -- it's a distraction and it's not something we typically do. While it's nice for the diagnostic to explain how to fix the issue, the critical bit is diagnosing what's wrong with the code. The diagnostic without the regex does that and users have a place to find that information (the style guide docs).

test/clang-tidy/google-objc-global-variable-declaration.m

33

I think (1) is totally fine. It's the identifiers we're worried about, not the macros or types.

[stephanemoore]

Removed the regex from the diagnostic ✂️

[stephanemoore]

clang-tidy/google/GlobalVariableDeclarationCheck.cpp

92

Sounds good to me 👌 Removed the regex from the diagnostic.

[aaron.ballman]

This LGTM! Do you need someone to commit on your behalf?

[stephanemoore]

In D43581#1018499, @aaron.ballman wrote:

This LGTM! Do you need someone to commit on your behalf?

I would be happy to commit assuming that I am able to and can meet submission requirements.

I see a "Submit" button in Phabricator that I assume will land the commit if I press it?

I found some submission guidelines in the LLVM Developer Policy. Are there any other submission guidelines I should follow?

I ran the LLVM and Clang regression tests (by running make check-all from my LLVM build directory) and I encountered a failure in "Bindings/Go/go.test":

******************** TEST 'LLVM :: Bindings/Go/go.test' FAILED ********************
Script:
--
/Users/mog/projects/llvm-build/bin/llvm-go go=/usr/local/go/bin/go test llvm.org/llvm/bindings/go/llvm
--
Exit Code: 1

Command Output (stdout):
--
FAIL	llvm.org/llvm/bindings/go/llvm [build failed]

--
Command Output (stderr):
--
# llvm.org/llvm/bindings/go/llvm
In file included from /var/folders/qh/4y215hgd4zqg30v9q04czw58005k3k/T/lit_tmp_7sZYWR/gopath735545420/src/llvm.org/llvm/bindings/go/llvm/analysis.go:17:
In file included from /Users/mog/projects/llvm/include/llvm-c/Analysis.h:22:
In file included from /Users/mog/projects/llvm/include/llvm-c/Types.h:17:
/Users/mog/projects/llvm-build/include/llvm/Support/DataTypes.h:35:10: fatal error: 'math.h' file not found
#include <math.h>
         ^~~~~~~~
1 error generated.

--

********************

I reran these tests on a clean checkout and I still encountered the failure so I presume that this failure is unrelated?

I have also been trying to run the test-suite but I have been encountering Python exceptions while trying to run lnt 😕.

If someone wants to submit on my behalf that works for me. If not, I can also continue trying to drive this myself (though verification of submission requirements would be helpful in that case).

[Wizard]

In D43581#1018584, @stephanemoore wrote:

In D43581#1018499, @aaron.ballman wrote:

This LGTM! Do you need someone to commit on your behalf?

I would be happy to commit assuming that I am able to and can meet submission requirements.

I see a "Submit" button in Phabricator that I assume will land the commit if I press it?

I found some submission guidelines in the LLVM Developer Policy. Are there any other submission guidelines I should follow?

I ran the LLVM and Clang regression tests (by running make check-all from my LLVM build directory) and I encountered a failure in "Bindings/Go/go.test":

******************** TEST 'LLVM :: Bindings/Go/go.test' FAILED ********************
Script:
--
/Users/mog/projects/llvm-build/bin/llvm-go go=/usr/local/go/bin/go test llvm.org/llvm/bindings/go/llvm
--
Exit Code: 1

Command Output (stdout):
--
FAIL	llvm.org/llvm/bindings/go/llvm [build failed]

--
Command Output (stderr):
--
# llvm.org/llvm/bindings/go/llvm
In file included from /var/folders/qh/4y215hgd4zqg30v9q04czw58005k3k/T/lit_tmp_7sZYWR/gopath735545420/src/llvm.org/llvm/bindings/go/llvm/analysis.go:17:
In file included from /Users/mog/projects/llvm/include/llvm-c/Analysis.h:22:
In file included from /Users/mog/projects/llvm/include/llvm-c/Types.h:17:
/Users/mog/projects/llvm-build/include/llvm/Support/DataTypes.h:35:10: fatal error: 'math.h' file not found
#include <math.h>
         ^~~~~~~~
1 error generated.

--

********************

I reran these tests on a clean checkout and I still encountered the failure so I presume that this failure is unrelated?

I have also been trying to run the test-suite but I have been encountering Python exceptions while trying to run lnt 😕.

If someone wants to submit on my behalf that works for me. If not, I can also continue trying to drive this myself (though verification of submission requirements would be helpful in that case).

You may wanna follow this http://llvm.org/docs/Phabricator.html#git-svn-and-arcanist to submit your changes. But as aaron.ballman@ mentioned, you may need someone who has acl to commit it for you (I have committed it for you)
For the error you see it is likely because you only updated the tools/extra repo. Usually when we sync this repo, we will have to pull parent repos as well (i.e. clang repo and llvm repo).

clang-tidy/google/GlobalVariableDeclarationCheck.cpp

   // Note that hasGlobalStorage() matches static variables declared locally
   // inside a function or method, so we need to exclude those with
   // isLocalVariable().
   Finder->addMatcher(
       varDecl(hasGlobalStorage(), unless(hasType(isConstQualified())),
               unless(isLocalVariable()), unless(matchesName("::g[A-Z]")))
           .bind("global_var"),
       this);
   Finder->addMatcher(varDecl(hasGlobalStorage(), hasType(isConstQualified()),
                              unless(isLocalVariable()),
-                             unless(matchesName("::k[A-Z]")))
+                             unless(matchesName("::(k[A-Z]|[A-Z]{2,})")))
                          .bind("global_const"),
                      this);
 }
 
 void GlobalVariableDeclarationCheck::check(
     const MatchFinder::MatchResult &Result) {
   if (const auto *Decl = Result.Nodes.getNodeAs<VarDecl>("global_var")) {
     diag(Decl->getLocation(),
          "non-const global variable '%0' must have a name which starts with "
          "'g[A-Z]'")

test/clang-tidy/google-objc-global-variable-declaration.m

 
 static NSString* const _notAlpha = @"NotBeginWithAlpha";
 // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: const global variable '_notAlpha' must have a name which starts with 'k[A-Z]' [google-objc-global-variable-declaration]
 // CHECK-FIXES: static NSString* const _notAlpha = @"NotBeginWithAlpha";
 
 static NSString* const k_Alpha = @"SecondNotAlpha";
 // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: const global variable 'k_Alpha' must have a name which starts with 'k[A-Z]' [google-objc-global-variable-declaration]
 // CHECK-FIXES: static NSString* const k_Alpha = @"SecondNotAlpha";
 
 static NSString* const kGood = @"hello";
+static NSString* const XYGood = @"hello";
 static NSString* gMyIntGood = 0;
 
+extern NSString* const GTLServiceErrorDomain;
+
+typedef NS_ENUM(NSInteger, GTLServiceError) {
+  GTLServiceErrorQueryResultMissing = -3000,
+  GTLServiceErrorWaitTimedOut       = -3001,
+};
+
 @implementation Foo
 - (void)f {
     int x = 0;
     static int bar;
     static const int baz = 42;
 }
 @end