[clang-tidy] Ignore macros defined within declarations in modernize-macro-to-enum

[LegalizeAdulthood]

Summary

Modernize-macro-to-enum shouldn't try to convert macros to enums
when they are defined inside a declaration or definition, only
when the macros are defined at the top level. Since preprocessing
is disconnected from AST traversal, match nodes in the AST and then
invalidate source ranges spanning AST nodes before issuing diagnostics.

ClangTidyCheck::onEndOfTranslationUnit is called before
PPCallbacks::EndOfMainFile, so defer final diagnostics to the
PPCallbacks implementation.

Fixes #54883

[LegalizeAdulthood]

Add comments describing test cases

[LegalizeAdulthood]

Delete debugging aids

[aaron.ballman]

This seems like a case where we might want a configuration option (maybe). Some of the test cases where you silence the diagnostic look like places I would expect us to be able to use a (local) enumeration. e.g.,

void g(int x)
{
  if (x != 0) {
#define INSIDE1 1
#define INSIDE2 2
// No reason the above can't be replaced with enum { INSIDE1 = 1, INSIDE2 = 2 };
    if (INSIDE1 > 1) {
      f();
    }
  } else {
    if (INSIDE2 == 1) {
      f();
    }
  }
}

struct S {
#define INSIDE5 5
#define INSIDE6 6
// The above could reasonably be: enum { INSIDE5 = 5, INSIDE6 = 6 }; especially in C++
  char storage[INSIDE5];
};

WDYT?

[LegalizeAdulthood]

In D124066#3463008, @aaron.ballman wrote:

This seems like a case where we might want a configuration option (maybe). [...]
WDYT?

In my bug report on this problem, I sketch out a plan of attack:

1. DON'T BREAK MY CODE -- that is this review :)
2. Do some analysis of macro expansion locations to determine the nearest enclosing scope at which the enum should be declared.

https://github.com/llvm/llvm-project/issues/54883

[LegalizeAdulthood]

Should probably split that bug report into 2 bugs for the two phases of attack.

[LegalizeAdulthood]

The main problem with scenario 2) is the weird interaction between when PPCallbacks methods are invoked and AST traversal is invoked.

[aaron.ballman]

In D124066#3463109, @LegalizeAdulthood wrote:

In D124066#3463008, @aaron.ballman wrote:

This seems like a case where we might want a configuration option (maybe). [...]
WDYT?

In my bug report on this problem, I sketch out a plan of attack:

1. DON'T BREAK MY CODE -- that is this review :)
2. Do some analysis of macro expansion locations to determine the nearest enclosing scope at which the enum should be declared.

https://github.com/llvm/llvm-project/issues/54883

Ah, thank you for the explanation. I like not breaking code. :-) LGTM aside from a possible simplification for the matchers.

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

561–567

Can we mix these together with anyOf() so we're not adding so many matchers? e.g.,

Finder->addMatcher(anyOf(varDecl(TopLevelDecl), functionDecl(topLevelDecl()), ...).bind("top"), this);

[LegalizeAdulthood]

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

561–567

Oh, good idea, yep can do that.

[LegalizeAdulthood]

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

561–567

Can't actually do anyOf as it is a narrowing matcher, but I think I can collapse all of these to just matching on decl, so I'm trying that.

[LegalizeAdulthood]

Update from review comments

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

 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_MACROTOENUMCHECK_H
 
 #include "../ClangTidyCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace modernize {
 
+class MacroToEnumCallbacks;
+
 /// Replaces groups of related macros with an unscoped anonymous enum.
 ///
 /// For the user-facing documentation see:
 /// http://clang.llvm.org/extra/clang-tidy/checks/modernize-macro-to-enum.html
 class MacroToEnumCheck : public ClangTidyCheck {
 public:
   MacroToEnumCheck(StringRef Name, ClangTidyContext *Context)
       : ClangTidyCheck(Name, Context) {}
   void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
                            Preprocessor *ModuleExpanderPP) override;
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  MacroToEnumCallbacks *PPCallback{};
 };
 
 } // namespace modernize
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_MACROTOENUMCHECK_H

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

FileState()
       : ConditionScopes(0), LastLine(0), GuardScanner(IncludeGuard::None) {}
 
   int ConditionScopes;
   unsigned int LastLine;
   IncludeGuard GuardScanner;
   SourceLocation LastMacroLocation;
 };
 
+} // namespace
+
 class MacroToEnumCallbacks : public PPCallbacks {
 public:
   MacroToEnumCallbacks(MacroToEnumCheck *Check, const LangOptions &LangOptions,
                        const SourceManager &SM)
       : Check(Check), LangOpts(LangOptions), SM(SM) {}
 
   void FileChanged(SourceLocation Loc, FileChangeReason Reason,
                    SrcMgr::CharacteristicKind FileType,
public:
   }
   void Endif(SourceLocation Loc, SourceLocation IfLoc) override;
   void PragmaDirective(SourceLocation Loc,
                        PragmaIntroducerKind Introducer) override;
 
   // After we've seen everything, issue warnings and fix-its.
   void EndOfMainFile() override;
 
+  void invalidateRange(SourceRange Range);
+
 private:
   void newEnum() {
     if (Enums.empty() || !Enums.back().empty())
       Enums.emplace_back();
   }
   bool insideConditional() const {
     return (CurrentFile->GuardScanner == IncludeGuard::DefineGuard &&
             CurrentFile->ConditionScopes > 1) ||
void clearLastMacroLocation() {
     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 invalidateExpressionNames();
+  void issueDiagnostics();
   void warnMacroEnum(const EnumMacro &Macro) const;
   void fixEnumMacro(const MacroList &MacroList) const;
 
   MacroToEnumCheck *Check;
   const LangOptions &LangOpts;
   const SourceManager &SM;
   SmallVector<MacroList> Enums;
   SmallVector<FileState> Files;
llvm::erase_if(Enums, [Id](const MacroList &MacroList) {
       return llvm::any_of(MacroList, [&Id](const EnumMacro &Macro) {
         return getTokenName(Macro.Name) == Id;
       });
     });
   }
 }
 
 void MacroToEnumCallbacks::EndOfMainFile() {
     invalidateExpressionNames();
+    issueDiagnostics();
+}
 
+void MacroToEnumCallbacks::invalidateRange(SourceRange Range) {
+  llvm::erase_if(Enums, [Range](const MacroList &MacroList) {
+    return llvm::any_of(MacroList, [Range](const EnumMacro &Macro) {
+      return Macro.Directive->getLocation() >= Range.getBegin() &&
+             Macro.Directive->getLocation() <= Range.getEnd();
+    });
+  });
+}
+
+void MacroToEnumCallbacks::issueDiagnostics() {
   for (const MacroList &MacroList : Enums) {
     if (MacroList.empty())
       continue;
 
     for (const EnumMacro &Macro : MacroList)
       warnMacroEnum(Macro);
 
     fixEnumMacro(MacroList);
void MacroToEnumCallbacks::fixEnumMacro(const MacroList &MacroList) const {
   SourceLocation End = Lexer::getLocForEndOfToken(
       MacroList.back().Directive->getMacroInfo()->getDefinitionEndLoc(), 0, SM,
       LangOpts);
   End = SM.translateLineCol(SM.getFileID(End),
                             SM.getSpellingLineNumber(End) + 1, 1);
   Diagnostic << FixItHint::CreateInsertion(End, "};\n");
 }
 
-} // namespace
-
 void MacroToEnumCheck::registerPPCallbacks(const SourceManager &SM,
                                            Preprocessor *PP,
                                            Preprocessor *ModuleExpanderPP) {
-  PP->addPPCallbacks(
-      std::make_unique<MacroToEnumCallbacks>(this, getLangOpts(), SM));
+  auto Callback = std::make_unique<MacroToEnumCallbacks>(this, getLangOpts(), SM);
+  PPCallback = Callback.get();
+  PP->addPPCallbacks(std::move(Callback));
+}
+
+void MacroToEnumCheck::registerMatchers(ast_matchers::MatchFinder *Finder) {
+  using namespace ast_matchers;
+  auto TopLevelDecl = hasParent(translationUnitDecl());
+  Finder->addMatcher(decl(TopLevelDecl).bind("top"), this);
+}
+
+static bool isValid(SourceRange Range) {
+  return Range.getBegin().isValid() && Range.getEnd().isValid();
+}
+
+static bool empty(SourceRange Range) {
+  return Range.getBegin() == Range.getEnd();
+}
+
+void MacroToEnumCheck::check(
+    const ast_matchers::MatchFinder::MatchResult &Result) {
+  auto *TLDecl = Result.Nodes.getNodeAs<Decl>("top");
+  if (TLDecl == nullptr)
+      return;
+
+  SourceRange Range = TLDecl->getSourceRange();
+  if (auto *TemplateFn = Result.Nodes.getNodeAs<FunctionTemplateDecl>("top")) {
+    if (TemplateFn->isThisDeclarationADefinition() && TemplateFn->hasBody())
+      Range = SourceRange{TemplateFn->getBeginLoc(),
+                          TemplateFn->getUnderlyingDecl()->getBodyRBrace()};
+  }
+
+  if (isValid(Range) && !empty(Range))
+    PPCallback->invalidateRange(Range);
 }
 
 } // namespace modernize
 } // namespace tidy
 } // namespace clang

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

-// RUN: %check_clang_tidy -std=c++14-or-later %s modernize-macro-to-enum %t -- -- -I%S/Inputs/modernize-macro-to-enum
+// RUN: %check_clang_tidy -std=c++14-or-later %s modernize-macro-to-enum %t -- -- -I%S/Inputs/modernize-macro-to-enum -fno-delayed-template-parsing
 // C++14 or later required for binary literals.
 
 #if 1
 #include "modernize-macro-to-enum.h"
 
 // These macros are skipped due to being inside a conditional compilation block.
 #define GOO_RED 1
 #define GOO_GREEN 2
void f()
   draw(BLUE);
   DO_RED;
   DO_GREEN;
   DO_BLUE;
   FN_RED(0);
   FN_GREEN(0);
   FN_BLUE(0);
 }
+
+// Ignore macros defined inside a top-level function definition.
+void g(int x)
+{
+  if (x != 0) {
+#define INSIDE1 1
+#define INSIDE2 2
+    if (INSIDE1 > 1) {
+      f();
+    }
+  } else {
+    if (INSIDE2 == 1) {
+      f();
+    }
+  }
+}
+
+// Ignore macros defined inside a top-level function declaration.
+extern void g2(
+#define INSIDE3 3
+#define INSIDE4 4
+);
+
+// Ignore macros defined inside a record (structure) declaration.
+struct S {
+#define INSIDE5 5
+#define INSIDE6 6
+  char storage[INSIDE5];
+};
+class C {
+#define INSIDE7 7
+#define INSIDE8 8
+};
+
+// Ignore macros defined inside a template function definition.
+template <int N>
+#define INSIDE9 9
+bool fn()
+{
+  #define INSIDE10 10
+  return INSIDE9 > 1 || INSIDE10 < N;
+}
+
+// Ignore macros defined inside a variable declaration.
+extern int
+#define INSIDE11 11
+v;
+
+// Ignore macros defined inside a template class definition.
+template <int N>
+class C2 {
+public:
+#define INSIDE12 12
+    char storage[N];
+  bool f() {
+    return N > INSIDE12;
+  }
+  bool g();
+};
+
+// Ignore macros defined inside a template member function definition.
+template <int N>
+#define INSIDE13 13
+bool C2<N>::g() {
+#define INSIDE14 14
+  return N < INSIDE12 || N > INSIDE13 || INSIDE14 > N;
+};
+
+// Ignore macros defined inside a template type alias.
+template <typename T>
+class C3 {
+  T data;
+};
+template <typename T>
+#define INSIDE15 15
+using Data = C3<T[INSIDE15]>;
+
+// Ignore macros defined inside a type alias.
+using Data2 =
+#define INSIDE16 16
+    char[INSIDE16];
+
+// Ignore macros defined inside a (constexpr) variable definition.
+constexpr int
+#define INSIDE17 17
+value = INSIDE17;