Revise the google-objc-global-variable-declaration check to match the style guide.

[yaqiji]

Summary

Revise the google-objc-global-variable-declaration check to match the style guide.

This commit updates the check as follows:
(1) Do not emit fixes for extern global constants.
(2) Allow the second character of prefixes for constants to be numeric (the new guideline is that global constants should generally be named with a prefix that begins with a capital letter followed by one or more capital letters or numbers).

https://google.github.io/styleguide/objcguide.html#prefixes

[yaqiji]

removed unnecessary lines

[yaqiji]

Removed unncessary comments.

[yaqiji]

Modified code to make it more readable.

[yaqiji]

Changes matcher, test and removed fix it hint for global constants

[benhamilton]

clang-tools-extra/clang-tidy/google/GlobalVariableDeclarationCheck.cpp

54–55

I don't see any guidance in the style guide about prefixing with g either. I assume we should remove that?

[stephanemoore]

clang-tools-extra/clang-tidy/google/GlobalVariableDeclarationCheck.cpp

54–55

Non-const static variable declaration names should have the prefix g:
"File scope or global variables have a prefix g."
https://google.github.io/styleguide/objcguide.html#variable-names

The prefixes section in the style guide (https://google.github.io/styleguide/objcguide.html#prefixes) is targeted towards prefixes for classes, protocols, global functions, and global constants (i.e., const qualified variable declaration names).

80

For legacy code, I think we still need to allow variable declarations of the following form:

static const int kFileCount = 12;
static const int kFOOFileCount = 12;

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

26

Thanks for removing the fixit on this case 😄 The fix was not compliant with Google Objective-C style.

30

Thanks for removing the fixit on this case 😄 The fix was not compliant with Google Objective-C style.

[yaqiji]

Updated matching criteria to allow variable declarations for legacy code

[stephanemoore]

Thanks for restoring the support for the legacy style! I noticed that there are some cases where we can preserve an existing fixit.

clang-tools-extra/clang-tidy/google/GlobalVariableDeclarationCheck.cpp

51

We can still emit a fix for static constants. Perhaps only early return if the variable declaration is both constant and static?

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

6

This is still a valid fix that we can emit.

Maybe we can just recommend a fix for static constants?

Maybe we can add a case that we don't believe should have an emitted fixit like so:

extern NSString *const GlobalConstant;

[yaqiji]

Added fixithint for const as well.

[stephanemoore]

clang-tools-extra/clang-tidy/google/GlobalVariableDeclarationCheck.cpp

54

Can you make sure this is formatted correctly? I would expect a space before the ? but I am not familiar with LLVM style on every detail.

54

Do the tests pass with the added GlobalConstant test case? I would expect that we need to add an early return to handle extern global constants but I could be mistaken.

85

What is the reason for changing the regex to match the entire name?

(I do not object to the change but I want to know if there is a policy that I am not aware of)

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

28

I would keep a k_Alpha test case so that we know that an underscore after the k is not accepted.

[yaqiji]

Modified CMakeList to include tests, otherwise clangTidyGoogleModule wouldn't compile.
Added k_Alpha test case back.
Added Extern case so that if it is a extern global constant, no suggestion will show up.
Removed .* and leave substrings only for matching.

[yaqiji]

Simplified method signature.

[stephanemoore]

Almost there. I think everything looks good after we resolve this last round of comments.

Can you also update the commit description. I believe that the current changes can be described roughly as follows:
"""
Do not emit fixes for extern global constants in google-objc-global-variable-declaration check.
"""

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

6

In general I recommend splitting stylistic changes independently of functional changes so that it's easy to identify changes that actually affected behavior.

I recommend one of the two following actions:
(1) Expand the commit description to describe the reformatting changes to google-objc-global-variable-declaration.m so that it's clear to readers that these were intentional changes included in this commit.
(2) Split out the reformatting changes unrelated to resolving the inconsistency between the check and the Objective-C style guide into an independent commit and submit that for independent review.

26

Oh wait. I believe this was here to verify that no fix was recommended. We should restore this.

(sorry for the error on my part 🤦)

30

Oh wait. I believe this was here to verify that no fix was recommended. We should restore this.

(sorry for the error on my part 🤦)

52

Can we add a test case as follows:

extern NSString *Y2Good;

[yaqiji]

Restored fix.
Added test case for extern Cap+Number prefixes.
Restored reformatting.

[yaqiji]

Removed extra space.

[stephanemoore]

Many thanks for being patient with me 🙏 I think there are just two more things ✌️

❧

Sorry, I think my earlier description was understated. Maybe this would be a better commit description:
"""
Revise the google-objc-global-variable-declaration check to match the style guide.

This commit updates the check as follows:
(1) Do not emit fixes for extern global constants.
(2) Allow the second character of prefixes for constants to be numeric (the new guideline is that global constants should generally be named with a prefix that begins with a capital letter followed by one or more capital letters or numbers).

https://google.github.io/styleguide/objcguide.html#prefixes
"""

(sorry I am a bit disorganized since I have been juggling various things today)

clang-tools-extra/clang-tidy/google/CMakeLists.txt

17 ↗

(On Diff #200614)

This change doesn't look like it belongs in this commit? Please revert.

[yaqiji]

Removed other mdodified file.

[stephanemoore]

(sorry I forgot to send this earlier)

Two small things and then I think everything is good.

clang-tools-extra/clang-tidy/google/GlobalVariableDeclarationCheck.cpp

29

This condition is probably technically fine since I don't believe it's possible for variables with auto or register storage classes to get here but if we were worried about that then I think we could change the condition to something like this:

if (IsConst && (Decl->getStorageClass() != SC_Static)) {

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

16–18

This is definitely outside the scope of this commit but this fix seems like it would only be appropriate in an implementation file 🤔

32

Isn't this a case where we would expect kNotCap as a suggested fixit?

40

Isn't this a case where we would expect kSecondNotCap as a suggested fixit?

[yaqiji]

Added fix message, and change global to nonstatic.

[stephanemoore]

Looks good! Thanks for being patient with me!

[yaqiji]

Thank you so much for reviewing the code and providing great feedbacks!

[stephanemoore]

It looks like all concerns have been addressed. Do you need me to land this commit for you?

[yaqiji]

Yes please, thank you :D

[stephanemoore]

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

48 ↗

(On Diff #201806)

Whoops, we forgot the warning that should be generated for this case 😅

Test failure(s):
http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/49124
http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/49124/steps/test/logs/stdio

I had to rollback the commit. Can you update the patch to resolve the test failure(s)?

[yaqiji]

Updated warning

[yaqiji]

Added check-fix

[yaqiji]

Removed check-fixes for those without fixithint.
Changed name of Y2Good to Y2Bad to match meaning.

[yaqiji]

Remove cmake content

[yaqiji]

Added to check no fixit is generated.

[stephanemoore]

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

46

This is interesting as it respects the Google Objective-C Style Guide as it is currently written. I think the truth might actually be that there are no guidelines for non-const extern variables as they are incredibly rare and presumably ubiquitously discouraged. In that respect, I am not sure that the fix here represents what would actually be recommended for Google Objective-C. With that said, the fix might be reasonable to emit based on the expectation that it would be exceedingly rare and it's unclear what better guidance we would provide. I think it's fine to continue allowing this existing behavior and continue monitoring.

[gribozavr]

Sorry for jumping in late, but renaming the declaration is not enough -- usages should also be updated; otherwise the developer is better off using a refactoring in their IDE or even a textual search and replace.

clang-tools-extra/clang-tidy/google/GlobalVariableDeclarationCheck.cpp

 
 namespace clang {
 namespace tidy {
 namespace google {
 namespace objc {
 
 namespace {
 
-AST_MATCHER(VarDecl, isLocalVariable) {
-  return Node.isLocalVarDecl();
-}
+AST_MATCHER(VarDecl, isLocalVariable) { return Node.isLocalVarDecl(); }
 
 FixItHint generateFixItHint(const VarDecl *Decl, bool IsConst) {
+  if (IsConst && (Decl->getStorageClass() != SC_Static)) {
+    // No fix available if it is not a static constant, since it is difficult
+    // to determine the proper fix in this case.
+    return FixItHint();
+  }
+
   char FC = Decl->getName()[0];
   if (!llvm::isAlpha(FC) || Decl->getName().size() == 1) {
     // No fix available if first character is not alphabetical character, or it
-    // is a single-character variable, since it is difficult to determine the 
+    // is a single-character variable, since it is difficult to determine the
     // proper fix in this case. Users should create a proper variable name by
     // their own.
     return FixItHint();
   }
   char SC = Decl->getName()[1];
   if ((FC == 'k' || FC == 'g') && !llvm::isAlpha(SC)) {
-    // No fix available if the prefix is correct but the second character is not
-    // alphabetical, since it is difficult to determine the proper fix in this
-    // case.
+    // No fix available if the prefix is correct but the second character is
+    // not alphabetical, since it is difficult to determine the proper fix in
+    // this case.
     return FixItHint();
   }
+
   auto NewName = (IsConst ? "k" : "g") +
                  llvm::StringRef(std::string(1, FC)).upper() +
                  Decl->getName().substr(1).str();
+
   return FixItHint::CreateReplacement(
       CharSourceRange::getTokenRange(SourceRange(Decl->getLocation())),
       llvm::StringRef(NewName));
 }
 }  // namespace
 
 void GlobalVariableDeclarationCheck::registerMatchers(MatchFinder *Finder) {
   // The relevant Style Guide rule only applies to Objective-C.
   if (!getLangOpts().ObjC)
     return;
 
   // need to add two matchers since we need to bind different ids to distinguish
   // constants and variables. Since bind() can only be called on node matchers,
   // we cannot make it in one matcher.
   //
   // 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]|[A-Z]{2,})")))
+                             unless(matchesName("::(k[A-Z])|([A-Z][A-Z0-9])")))
                          .bind("global_const"),
                      this);
 }
 
 void GlobalVariableDeclarationCheck::check(
     const MatchFinder::MatchResult &Result) {
   if (const auto *Decl = Result.Nodes.getNodeAs<VarDecl>("global_var")) {
     if (Decl->isStaticDataMember())
       return;
     diag(Decl->getLocation(),
          "non-const global variable '%0' must have a name which starts with "
          "'g[A-Z]'")
         << Decl->getName() << generateFixItHint(Decl, false);

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

 // RUN: %check_clang_tidy %s google-objc-global-variable-declaration %t
 
 @class NSString;
+
 static NSString* const myConstString = @"hello";
 // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: const global variable 'myConstString' must have a name which starts with an appropriate prefix [google-objc-global-variable-declaration]
 // CHECK-FIXES: static NSString* const kMyConstString = @"hello";
 
+extern NSString* const GlobalConstant = @"hey";
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: const global variable 'GlobalConstant' must have a name which starts with an appropriate prefix [google-objc-global-variable-declaration]
+
 static NSString* MyString = @"hi";
 // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: non-const global variable 'MyString' must have a name which starts with 'g[A-Z]' [google-objc-global-variable-declaration]
 // CHECK-FIXES: static NSString* gMyString = @"hi";
 
 NSString* globalString = @"test";
 // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: non-const global variable 'globalString' must have a name which starts with 'g[A-Z]' [google-objc-global-variable-declaration]
 // CHECK-FIXES: NSString* gGlobalString = @"test";
 
 static NSString* a = @"too simple";
 // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: non-const global variable 'a' must have a name which starts with 'g[A-Z]' [google-objc-global-variable-declaration]
 // CHECK-FIXES: static NSString* a = @"too simple";
 
 static NSString* noDef;
 // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: non-const global variable 'noDef' must have a name which starts with 'g[A-Z]' [google-objc-global-variable-declaration]
 // CHECK-FIXES: static NSString* gNoDef;
 
 static NSString* const _notAlpha = @"NotBeginWithAlpha";
 // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: const global variable '_notAlpha' must have a name which starts with an appropriate prefix [google-objc-global-variable-declaration]
 // CHECK-FIXES: static NSString* const _notAlpha = @"NotBeginWithAlpha";
 
+static NSString* const notCap = @"NotBeginWithCap";
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: const global variable 'notCap' must have a name which starts with an appropriate prefix [google-objc-global-variable-declaration]
+// CHECK-FIXES: static NSString* const kNotCap = @"NotBeginWithCap";
+
 static NSString* const k_Alpha = @"SecondNotAlpha";
 // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: const global variable 'k_Alpha' must have a name which starts with an appropriate prefix [google-objc-global-variable-declaration]
 // CHECK-FIXES: static NSString* const k_Alpha = @"SecondNotAlpha";
 
+static NSString* const SecondNotCap = @"SecondNotCapOrNumber";
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: const global variable 'SecondNotCap' must have a name which starts with an appropriate prefix [google-objc-global-variable-declaration]
+// CHECK-FIXES: static NSString* const kSecondNotCap = @"SecondNotCapOrNumber";
+
 static NSString* const kGood = @"hello";
 static NSString* const XYGood = @"hello";
+static NSString* const X1Good = @"hello";
 static NSString* gMyIntGood = 0;
+extern NSString* Y2Good;
+// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: non-const global variable 'Y2Good' must have a name which starts with 'g[A-Z]' [google-objc-global-variable-declaration]
+// CHECK-FIXES: extern NSString* gY2Good;
 
 extern NSString* const GTLServiceErrorDomain;
 
 enum GTLServiceError {
   GTLServiceErrorQueryResultMissing = -3000,
   GTLServiceErrorWaitTimedOut       = -3001,
 };
 
 @implementation Foo
 - (void)f {
-    int x = 0;
-    static int bar;
-    static const int baz = 42;
 }
 @end