[clang-tidy] Allow renaming macro arguments

[revane]

Although the identifier-naming.cpp lit test expected macro arguments not to be renamed, the code seemed to already allow it. The code was simply not being exercised because a SourceManager argument wasn't being provided. With this change, renaming of macro arguments that expand to renamable decls is permitted.

[llvmbot]

@llvm/pr-subscribers-clang-tools-extra

@llvm/pr-subscribers-clang-tidy

Author: Edwin Vane (revane)

Changes

Although the identifier-naming.cpp lit test expected macro arguments not to be renamed, the code seemed to already allow it. The code was simply not being exercised because a SourceManager argument wasn't being provided. With this change, renaming of macro arguments that expand to renamable decls is permitted.

---

Full diff: https://github.com/llvm/llvm-project/pull/87792.diff

2 Files Affected:

• (modified) clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp (+3-4)
• (modified) clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming.cpp (+9-5)

diff --git a/clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp b/clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
index da1433aa2d05d4..45036822cf7a61 100644
--- a/clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
+++ b/clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
@@ -31,13 +31,12 @@ struct DenseMapInfo<clang::tidy::RenamerClangTidyCheck::NamingCheckId> {
   using NamingCheckId = clang::tidy::RenamerClangTidyCheck::NamingCheckId;
 
   static inline NamingCheckId getEmptyKey() {
-    return {DenseMapInfo<clang::SourceLocation>::getEmptyKey(),
-                         "EMPTY"};
+    return {DenseMapInfo<clang::SourceLocation>::getEmptyKey(), "EMPTY"};
   }
 
   static inline NamingCheckId getTombstoneKey() {
     return {DenseMapInfo<clang::SourceLocation>::getTombstoneKey(),
-                         "TOMBSTONE"};
+            "TOMBSTONE"};
   }
 
   static unsigned getHashValue(NamingCheckId Val) {
@@ -473,7 +472,7 @@ void RenamerClangTidyCheck::checkNamedDecl(const NamedDecl *Decl,
   }
 
   Failure.Info = std::move(Info);
-  addUsage(Decl, Range);
+  addUsage(Decl, Range, &SourceMgr);
 }
 
 void RenamerClangTidyCheck::check(const MatchFinder::MatchResult &Result) {
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming.cpp
index d2e89a7c9855c9..289e493e04009b 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming.cpp
@@ -108,10 +108,12 @@ USER_NS::object g_s2;
 // 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
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: invalid case style for global variable 'var1' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}SYSTEM_MACRO(g_var1);
 
 USER_MACRO(var2);
-// NO warnings or fixes expected as var2 is declared in a macro expansion
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: invalid case style for global variable 'var2' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}USER_MACRO(g_var2);
 
 #define BLA int FOO_bar
 BLA;
@@ -602,9 +604,11 @@ static void static_Function() {
 // CHECK-FIXES: {{^}}#define MY_TEST_MACRO(X) X()
 
 void MY_TEST_Macro(function) {}
-// CHECK-FIXES: {{^}}void MY_TEST_MACRO(function) {}
-}
-}
+// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: invalid case style for global function 'function' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}void MY_TEST_MACRO(Function) {}
+
+} // namespace InlineNamespace
+} // namespace FOO_NS
 
 template <typename t_t> struct a {
 // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: invalid case style for struct 'a'

[revane]

Macros are tricky business and I understand why renaming is not allowed within macros. For the cases in the lit test, the renaming seems safe. Is this generally the case (i.e. those permitted by clang::tidy::utils::rangeCanBeFixed())? What other test cases can we add? Or is this change just not safe? If so, how can we update the "InsideMacro" tests to actually fail even in the presence of a SourceManager.

[revane]

@AaronBallman What do you think?

[AaronBallman]

I am not certain we want to allow this without some kind of option. The trouble is: the user may not have the ability to change the definition of the macro to be able to appease the check. However, the clang-tidy folks may have different opinions/ideas!

[carlosgalvezp]

the user may not have the ability to change the definition of the macro to be able to appease the check

My understanding of this PR is that the user would only need to change what they pass into the macro, not the macro itself, or? E.g.

-MY_MACRO(foo)
+MY_MACRO(g_foo)

[revane]

Indeed. I can add some tests to make sure nested macros aren't affected but my thought was since this change allows renaming arguments (i.e. not the macro parameters) then it was safe since the code is not really "inside a macro". There are already other means to filter out system headers and other user-defined headers. I couldn't come up with other classes of errors where this replacement would be invalid.

[AaronBallman]

the user may not have the ability to change the definition of the macro to be able to appease the check

My understanding of this PR is that the user would only need to change what they pass into the macro, not the macro itself, or? E.g.

-MY_MACRO(foo)
+MY_MACRO(g_foo)

In this case, yes, but consider a macro like:

#define CAT_IMPL(l, r) l ## r
#define CAT(l, r) CAT_IMPL(l, r)
#define MY_MACRO(foo) int CAT(awesome_, CAT(foo, __COUNTER__)) = 0

or some other form where the user doesn't necessarily control the exact identifier of what gets declared. Won't we suggest trying to rename the macro argument here?

[revane]

Using the suggested code and this one usage:

MY_MACRO(myglob);

No suggestion is made. My understanding is because the full range of the NamedDecl would contain awesome_myglob which isn't entirely within a macro arg.

[AaronBallman]

Using the suggested code and this one usage:

MY_MACRO(myglob);

No suggestion is made. My understanding is because the full range of the NamedDecl would contain awesome_myglob which isn't entirely within a macro arg.

Oh, awesome, thank you for trying that out! Then no complaints from me. :-)

[carlosgalvezp]

Nice! Should we add this example as a test case?

[revane]

Already done. Unless you had something else in mind?

[carlosgalvezp]

Great! Nothing else from my side

[PiotrZSL]

LGTM