[clang-tidy] Fix identifier naming in macro args.

[jhen]

Summary

clang-tidy should fix identifier naming even when the identifier is
referenced inside a macro expansion, provided that the identifier enters
the macro expansion completely within a macro argument.

For example, this will allow fixes to the naming of the identifier
'global' when it is declared and used as follows:

int global;
#define USE_IN_MACRO(m) auto use_##m = m
USE_IN_MACRO(global);

[jhen]

alexfh, sorry if you are not the right person to review this change. I based my choice on this history of this file.

[jhen]

• Return to original checking for macro in range

[jhen]

I found a bug in my first patch that I have fixed now. I was trying to iterate over the source range by using SourceLocation::getLocWithOffset, but I realized that doesn't work, so I removed it and went back to the original method of checking SourceRange.getBegin().isMacroID() and SourceRange.getEnd().isMacroID().

[jhen]

• Prevent multiple fixes for macro expansion

[jhen]

I just found and fixed another bug in this patch. Before, I wasn't using the spelling location for the fixit hint. This meant that a macro argument that was expanded to two locations, for example, would have the same fixit hint applied to it twice. My new test case verifies that this does not happen anymore.

[jhen]

Adding arron.ballman as a reviewer as alexfh seems to be on leave for a few weeks.

[aaron.ballman]

clang-tidy/readability/IdentifierNamingCheck.cpp

657

We could do an early return if ShouldFix is already false and simplify this expression with ShouldFix = RangeCanBeFixed;, can't we?

[jhen]

• Early exit if not Failure.ShouldFix

[jhen]

clang-tidy/readability/IdentifierNamingCheck.cpp

657

Thanks for pointing that out. The early exit is much more efficient.

[aaron.ballman]

LGTM, thank you!

[jhen]

Thanks for the review!

clang-tidy/readability/IdentifierNamingCheck.cpp

if (isa<TemplateTemplateParmDecl>(D)) {
     return SK_Invalid;
   }
 
   return SK_Invalid;
 }
 
 static void addUsage(IdentifierNamingCheck::NamingCheckFailureMap &Failures,
                      const IdentifierNamingCheck::NamingCheckId &Decl,
-                     SourceRange Range) {
+                     SourceRange Range, SourceManager *SourceMgr = nullptr) {
   // Do nothing if the provided range is invalid.
   if (Range.getBegin().isInvalid() || Range.getEnd().isInvalid())
     return;
 
+  // If we have a source manager, use it to convert to the spelling location for
+  // performing the fix. This is necessary because macros can map the same
+  // spelling location to different source locations, and we only want to fix
+  // the token once, before it is expanded by the macro.
+  SourceLocation FixLocation = Range.getBegin();
+  if (SourceMgr)
+    FixLocation = SourceMgr->getSpellingLoc(FixLocation);
+  if (FixLocation.isInvalid())
+    return;
+
   // Try to insert the identifier location in the Usages map, and bail out if it
   // is already in there
   auto &Failure = Failures[Decl];
-  if (!Failure.RawUsageLocs.insert(Range.getBegin().getRawEncoding()).second)
+  if (!Failure.RawUsageLocs.insert(FixLocation.getRawEncoding()).second)
+    return;
+
+  if (!Failure.ShouldFix)
     return;
 
-  Failure.ShouldFix = Failure.ShouldFix && !Range.getBegin().isMacroID() &&
-                      !Range.getEnd().isMacroID();
+  // Check if the range is entirely contained within a macro argument.
+  SourceLocation MacroArgExpansionStartForRangeBegin;
+  SourceLocation MacroArgExpansionStartForRangeEnd;
+  bool RangeIsEntirelyWithinMacroArgument =
+      SourceMgr &&
+      SourceMgr->isMacroArgExpansion(Range.getBegin(),
+                                     &MacroArgExpansionStartForRangeBegin) &&
+      SourceMgr->isMacroArgExpansion(Range.getEnd(),
+                                     &MacroArgExpansionStartForRangeEnd) &&
+      MacroArgExpansionStartForRangeBegin == MacroArgExpansionStartForRangeEnd;
+
+  // Check if the range contains any locations from a macro expansion.
+  bool RangeContainsMacroExpansion = RangeIsEntirelyWithinMacroArgument ||
+                                     Range.getBegin().isMacroID() ||
+                                     Range.getEnd().isMacroID();
+
+  bool RangeCanBeFixed =
+      RangeIsEntirelyWithinMacroArgument || !RangeContainsMacroExpansion;
+  Failure.ShouldFix = RangeCanBeFixed;
 }
 
 /// Convenience method when the usage to be added is a NamedDecl
 static void addUsage(IdentifierNamingCheck::NamingCheckFailureMap &Failures,
-                     const NamedDecl *Decl, SourceRange Range) {
+                     const NamedDecl *Decl, SourceRange Range, SourceManager *SourceMgr = nullptr) {
   return addUsage(Failures, IdentifierNamingCheck::NamingCheckId(
                                 Decl->getLocation(), Decl->getNameAsString()),
-                  Range);
+                  Range, SourceMgr);
 }
 
 void IdentifierNamingCheck::check(const MatchFinder::MatchResult &Result) {
   if (const auto *Decl =
           Result.Nodes.getNodeAs<CXXConstructorDecl>("classRef")) {
     if (Decl->isImplicit())
       return;
 
for (const auto &Shadow : Decl->shadows()) {
       addUsage(NamingCheckFailures, Shadow->getTargetDecl(),
                Decl->getNameInfo().getSourceRange());
     }
     return;
   }
 
   if (const auto *DeclRef = Result.Nodes.getNodeAs<DeclRefExpr>("declRef")) {
     SourceRange Range = DeclRef->getNameInfo().getSourceRange();
-    addUsage(NamingCheckFailures, DeclRef->getDecl(), Range);
+    addUsage(NamingCheckFailures, DeclRef->getDecl(), Range, Result.SourceManager);
     return;
   }
 
   if (const auto *Decl = Result.Nodes.getNodeAs<NamedDecl>("decl")) {
     if (!Decl->getIdentifier() || Decl->getName().empty() || Decl->isImplicit())
       return;
 
     // Fix type aliases in value declarations

test/clang-tidy/readability-identifier-naming.cpp

 // NO warnings or fixes expected as USER_NS and object are declared in a header file
 
 SYSTEM_MACRO(var1);
 // NO warnings or fixes expected as var1 is from macro expansion
 
 USER_MACRO(var2);
 // NO warnings or fixes expected as var2 is declared in a macro expansion
 
+#define BLA int FOO_bar
+BLA;
+// NO warnings or fixes expected as FOO_bar is from macro expansion
+
+int global0;
+#define USE_NUMBERED_GLOBAL(number) auto use_global##number = global##number
+USE_NUMBERED_GLOBAL(0);
+// NO warnings or fixes expected as global0 is pieced together in a macro
+// expansion.
+
+int global1;
+#define USE_NUMBERED_BAL(prefix, number) \
+  auto use_##prefix##bal##number = prefix##bal##number
+USE_NUMBERED_BAL(glo, 1);
+// NO warnings or fixes expected as global1 is pieced together in a macro
+// expansion.
+
+int global2;
+#define USE_RECONSTRUCTED(glo, bal) auto use_##glo##bal = glo##bal
+USE_RECONSTRUCTED(glo, bal2);
+// NO warnings or fixes expected as global2 is pieced together in a macro
+// expansion.
+
 int global;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: invalid case style for global variable 'global'
+// CHECK-FIXES: {{^}}int g_global;{{$}}
 #define USE_IN_MACRO(m) auto use_##m = m
 USE_IN_MACRO(global);
-// NO warnings or fixes expected as global is used in a macro expansion
 
-#define BLA int FOO_bar
-BLA;
-// NO warnings or fixes expected as FOO_bar is from macro expansion
+int global3;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: invalid case style for global variable 'global3'
+// CHECK-FIXES: {{^}}int g_global3;{{$}}
+#define ADD_TO_SELF(m) (m) + (m)
+int g_twice_global3 = ADD_TO_SELF(global3);
+// CHECK-FIXES: {{^}}int g_twice_global3 = ADD_TO_SELF(g_global3);{{$}}
 
 enum my_enumeration {
 // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: invalid case style for enum 'my_enumeration'
 // CHECK-FIXES: {{^}}enum EMyEnumeration {{{$}}
     MyConstant = 1,
 // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: invalid case style for enum constant 'MyConstant'
 // CHECK-FIXES: {{^}}    MY_CONSTANT = 1,{{$}}
     your_CONST = 1,