[clang-tidy] Modernize-macro-to-enum should skip macros used in other macros

[LegalizeAdulthood]

Summary

If a macro is used in the expansion of another macro, that can cause
a compile error if the macro is replaced with an enum. Token-pasting is
an example where converting a macro defined as an integral constant can
cause code to no longer compile.

This change causes such macros to be skipped from the conversion
process in order to prevent fixits from creating code that no longer
compiles.

A subsequent enhancement will examine macro usage in more detail to
allow more cases to be handled without breaking code.

Fixes #54948

[LegalizeAdulthood]

Gentle ping.

I have another change based on this that fixes #55055
Enhance modernize-macro-to-enum to recognize constant literal expressions

[aaron.ballman]

LGTM with a few small nits.

clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp

230

ArrayRef is intended to be cheap in the same way as StringRef, so we don't pass const references for it.

316–318

I know the coding guide says we can elide braces there, but because the for loop substatement is multiple lines, I think braces help readability.

[LegalizeAdulthood]

Update from review comments

clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp

private:
   void clearLastMacroLocation() {
     CurrentFile->LastLine = 0;
     CurrentFile->LastMacroLocation = SourceLocation{};
   }
   void clearCurrentEnum(SourceLocation Loc);
   void conditionStart(const SourceLocation &Loc);
   void checkCondition(SourceRange ConditionRange);
   void checkName(const Token &MacroNameTok);
-  void rememberExpressionName(const Token &MacroNameTok);
+  void rememberExpressionName(const Token &Tok);
+  void rememberExpressionTokens(ArrayRef<Token> MacroTokens);
   void invalidateExpressionNames();
   void issueDiagnostics();
   void warnMacroEnum(const EnumMacro &Macro) const;
   void fixEnumMacro(const MacroList &MacroList) const;
 
   MacroToEnumCheck *Check;
   const LangOptions &LangOpts;
   const SourceManager &SM;
void MacroToEnumCallbacks::checkName(const Token &MacroNameTok) {
   StringRef Id = getTokenName(MacroNameTok);
   llvm::erase_if(Enums, [&Id](const MacroList &MacroList) {
     return llvm::any_of(MacroList, [&Id](const EnumMacro &Macro) {
       return getTokenName(Macro.Name) == Id;
     });
   });
 }
 
-void MacroToEnumCallbacks::rememberExpressionName(const Token &MacroNameTok) {
-  std::string Id = getTokenName(MacroNameTok).str();
+void MacroToEnumCallbacks::rememberExpressionName(const Token &Tok) {
+  std::string Id = getTokenName(Tok).str();
   auto Pos = llvm::lower_bound(ExpressionNames, Id);
   if (Pos == ExpressionNames.end() || *Pos != Id) {
     ExpressionNames.insert(Pos, Id);
   }
 }
 
+void MacroToEnumCallbacks::rememberExpressionTokens(
+    ArrayRef<Token> MacroTokens) {
+  for (Token Tok : MacroTokens) {
+    if (Tok.isAnyIdentifier())
+      rememberExpressionName(Tok);
+  }
+}
+
 void MacroToEnumCallbacks::FileChanged(SourceLocation Loc,
                                        FileChangeReason Reason,
                                        SrcMgr::CharacteristicKind FileType,
                                        FileID PrevFID) {
   newEnum();
   if (Reason == EnterFile) {
     Files.emplace_back();
     if (!SM.isInMainFile(Loc))
       Files.back().GuardScanner = IncludeGuard::FileChanged;
   } else if (Reason == ExitFile) {
     assert(CurrentFile->ConditionScopes == 0);
     Files.pop_back();
   }
   CurrentFile = &Files.back();
 }
 
+// Any defined but rejected macro is scanned for identifiers that
+// are to be excluded as enums.
 void MacroToEnumCallbacks::MacroDefined(const Token &MacroNameTok,
                                         const MacroDirective *MD) {
   // Include guards are never candidates for becoming an enum.
   if (CurrentFile->GuardScanner == IncludeGuard::IfGuard) {
     CurrentFile->GuardScanner = IncludeGuard::DefineGuard;
     return;
   }
 
   if (insideConditional())
     return;
 
   if (SM.getFilename(MD->getLocation()).empty())
     return;
 
   const MacroInfo *Info = MD->getMacroInfo();
   ArrayRef<Token> MacroTokens = Info->tokens();
-  if (Info->isFunctionLike() || Info->isBuiltinMacro() || MacroTokens.empty())
+  if (Info->isBuiltinMacro() || MacroTokens.empty())
+    return;
+  if (Info->isFunctionLike()) {
+    rememberExpressionTokens(MacroTokens);
     return;
+  }
 
   // Return Lit when +Lit, -Lit or ~Lit; otherwise return Unknown.
   Token Unknown;
   Unknown.setKind(tok::TokenKind::unknown);
   auto GetUnopArg = [Unknown](Token First, Token Second) {
     return First.isOneOf(tok::TokenKind::minus, tok::TokenKind::plus,
                          tok::TokenKind::tilde)
                ? Second
if (MacroTokens.size() > 2) {
     size_t Size = End >= Begin ? (End - Begin + 1U) : 0U;
 
     // It was a single token inside matching parens.
     if (Size == 1)
       Tok = MacroTokens[Begin];
     else if (Size == 2)
       // It can be +Lit, -Lit or ~Lit.
       Tok = GetUnopArg(MacroTokens[Begin], MacroTokens[End]);
-    else
+    else {
       // Zero or too many tokens after we stripped matching parens.
+      rememberExpressionTokens(MacroTokens);
       return;
+    }
   } else if (MacroTokens.size() == 2) {
     // It can be +Lit, -Lit, or ~Lit.
     Tok = GetUnopArg(MacroTokens.front(), MacroTokens.back());
   }
 
   if (!Tok.isLiteral() || isStringLiteral(Tok.getKind()) ||
-      !isIntegralConstant(Tok))
+      !isIntegralConstant(Tok)) {
+    if (Tok.isAnyIdentifier())
+      rememberExpressionName(Tok);
     return;
+  }
 
   if (!isConsecutiveMacro(MD))
     newEnum();
   Enums.back().emplace_back(MacroNameTok, MD);
   rememberLastMacroLocation(MD);
 }
 
 // Any macro that is undefined removes all adjacent macros from consideration as

clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp

 
 // These macros do not expand to integral constants.
 #define HELLO "Hello, "
 #define WORLD "World"
 #define EPS1 1.0F
 #define EPS2 1e5
 #define EPS3 1.
 
-#define DO_RED draw(RED)
-#define DO_GREEN draw(GREEN)
-#define DO_BLUE draw(BLUE)
-
-#define FN_RED(x) draw(RED | x)
-#define FN_GREEN(x) draw(GREEN | x)
-#define FN_BLUE(x) draw(BLUE | x)
-
 extern void draw(unsigned int Color);
 
 void f()
 {
+  // Usage of macros converted to enums should still compile.
   draw(RED);
-  draw(GREEN);
-  draw(BLUE);
-  DO_RED;
-  DO_GREEN;
-  DO_BLUE;
-  FN_RED(0);
-  FN_GREEN(0);
-  FN_BLUE(0);
+  draw(GREEN | RED);
+  draw(BLUE + RED);
 }
 
 // Ignore macros defined inside a top-level function definition.
 void g(int x)
 {
   if (x != 0) {
 #define INSIDE1 1
 #define INSIDE2 2

 using Data2 =
 #define INSIDE16 16
     char[INSIDE16];
 
 // Ignore macros defined inside a (constexpr) variable definition.
 constexpr int
 #define INSIDE17 17
 value = INSIDE17;
+
+// Ignore macros used in the expansion of other macros
+#define INSIDE18 18
+#define INSIDE19 19
+
+#define CONCAT(n_, s_) n_##s_
+#define FN_NAME(n_, s_) CONCAT(n_, s_)
+
+extern void FN_NAME(g, INSIDE18)();
+
+void gg()
+{
+    g18();
+}