[clang-tidy] Refactor how NamedDecl are renamed (#88735)

The handling of renaming failures and multiple usages related to those
failures is currently spread over several functions. Identifying the
failure NamedDecl for a given usage is also duplicated, once when
creating failures and again when identify usages. There are currently
two ways to a failed NamedDecl from a usage: use the canonical decl or
use the overridden method. With new methods about to be added, a cleanup
was in order.

The data flow is simplified as follows:
* The visitor always forwards NamedDecls to addUsage(NamedDecl).
* addUsage(NamedDecl) determines the failed NamedDecl and determines
potential new names based on that failure. Usages are registered using
addUsage(NamingCheckId).
* addUsage(NamingCheckId) is now protected and its single responsibility
is maintaining the integrity of the failure/usage map.

Author: revane

clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp

@@ -178,8 +178,11 @@ std::optional<RenamerClangTidyCheck::FailureInfo>
 ReservedIdentifierCheck::getDeclFailureInfo(const NamedDecl *Decl,
                                             const SourceManager &) const {
   assert(Decl && Decl->getIdentifier() && !Decl->getName().empty() &&
-         !Decl->isImplicit() &&
          "Decl must be an explicit identifier with a name.");
+  // Implicit identifiers cannot fail.
+  if (Decl->isImplicit())
+    return std::nullopt;
+
   return getFailureInfoImpl(
       Decl->getName(), isa<TranslationUnitDecl>(Decl->getDeclContext()),
       /*IsMacro = */ false, getLangOpts(), Invert, AllowedIdentifiers);

clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp

@@ -1374,6 +1374,10 @@ IdentifierNamingCheck::getFailureInfo(
 std::optional<RenamerClangTidyCheck::FailureInfo>
 IdentifierNamingCheck::getDeclFailureInfo(const NamedDecl *Decl,
                                           const SourceManager &SM) const {
+  // Implicit identifiers cannot be renamed.
+  if (Decl->isImplicit())
+    return std::nullopt;
+
   SourceLocation Loc = Decl->getLocation();
   const FileStyle &FileStyle = getStyleForFile(SM.getFilename(Loc));
   if (!FileStyle.isActive())

clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp

@@ -61,6 +61,7 @@ struct DenseMapInfo<clang::tidy::RenamerClangTidyCheck::NamingCheckId> {
 namespace clang::tidy {
 
 namespace {
+
 class NameLookup {
   llvm::PointerIntPair<const NamedDecl *, 1, bool> Data;
 
@@ -78,6 +79,7 @@ class NameLookup {
   operator bool() const { return !hasMultipleResolutions(); }
   const NamedDecl *operator*() const { return getDecl(); }
 };
+
 } // namespace
 
 static const NamedDecl *findDecl(const RecordDecl &RecDecl,
@@ -91,6 +93,44 @@ static const NamedDecl *findDecl(const RecordDecl &RecDecl,
   return nullptr;
 }
 
+/// Returns the function that \p Method is overridding. If There are none or
+/// multiple overrides it returns nullptr. If the overridden function itself is
+/// overridding then it will recurse up to find the first decl of the function.
+static const CXXMethodDecl *getOverrideMethod(const CXXMethodDecl *Method) {
+  if (Method->size_overridden_methods() != 1)
+    return nullptr;
+
+  while (true) {
+    Method = *Method->begin_overridden_methods();
+    assert(Method && "Overridden method shouldn't be null");
+    unsigned NumOverrides = Method->size_overridden_methods();
+    if (NumOverrides == 0)
+      return Method;
+    if (NumOverrides > 1)
+      return nullptr;
+  }
+}
+
+static bool hasNoName(const NamedDecl *Decl) {
+  return !Decl->getIdentifier() || Decl->getName().empty();
+}
+
+static const NamedDecl *getFailureForNamedDecl(const NamedDecl *ND) {
+  const auto *Canonical = cast<NamedDecl>(ND->getCanonicalDecl());
+  if (Canonical != ND)
+    return Canonical;
+
+  if (const auto *Method = dyn_cast<CXXMethodDecl>(ND)) {
+    if (const CXXMethodDecl *Overridden = getOverrideMethod(Method))
+      Canonical = cast<NamedDecl>(Overridden->getCanonicalDecl());
+
+    if (Canonical != ND)
+      return Canonical;
+  }
+
+  return ND;
+}
+
 /// Returns a decl matching the \p DeclName in \p Parent or one of its base
 /// classes. If \p AggressiveTemplateLookup is `true` then it will check
 /// template dependent base classes as well.
@@ -132,24 +172,6 @@ static NameLookup findDeclInBases(const CXXRecordDecl &Parent,
   return NameLookup(Found); // If nullptr, decl wasn't found.
 }
 
-/// Returns the function that \p Method is overridding. If There are none or
-/// multiple overrides it returns nullptr. If the overridden function itself is
-/// overridding then it will recurse up to find the first decl of the function.
-static const CXXMethodDecl *getOverrideMethod(const CXXMethodDecl *Method) {
-  if (Method->size_overridden_methods() != 1)
-    return nullptr;
-
-  while (true) {
-    Method = *Method->begin_overridden_methods();
-    assert(Method && "Overridden method shouldn't be null");
-    unsigned NumOverrides = Method->size_overridden_methods();
-    if (NumOverrides == 0)
-      return Method;
-    if (NumOverrides > 1)
-      return nullptr;
-  }
-}
-
 namespace {
 
 /// Callback supplies macros to RenamerClangTidyCheck::checkMacro
@@ -192,10 +214,6 @@ class RenamerClangTidyVisitor
       : Check(Check), SM(SM),
         AggressiveDependentMemberLookup(AggressiveDependentMemberLookup) {}
 
-  static bool hasNoName(const NamedDecl *Decl) {
-    return !Decl->getIdentifier() || Decl->getName().empty();
-  }
-
   bool shouldVisitTemplateInstantiations() const { return true; }
 
   bool shouldVisitImplicitCode() const { return false; }
@@ -246,29 +264,10 @@ class RenamerClangTidyVisitor
   }
 
   bool VisitNamedDecl(NamedDecl *Decl) {
-    if (hasNoName(Decl))
-      return true;
-
-    const auto *Canonical = cast<NamedDecl>(Decl->getCanonicalDecl());
-    if (Canonical != Decl) {
-      Check->addUsage(Canonical, Decl->getLocation(), SM);
-      return true;
-    }
-
-    // Fix overridden methods
-    if (const auto *Method = dyn_cast<CXXMethodDecl>(Decl)) {
-      if (const CXXMethodDecl *Overridden = getOverrideMethod(Method)) {
-        Check->addUsage(Overridden, Method->getLocation(), SM);
-        return true; // Don't try to add the actual decl as a Failure.
-      }
-    }
-
-    // Ignore ClassTemplateSpecializationDecl which are creating duplicate
-    // replacements with CXXRecordDecl.
-    if (isa<ClassTemplateSpecializationDecl>(Decl))
-      return true;
-
-    Check->checkNamedDecl(Decl, SM);
+    SourceRange UsageRange =
+        DeclarationNameInfo(Decl->getDeclName(), Decl->getLocation())
+            .getSourceRange();
+    Check->addUsage(Decl, UsageRange, SM);
     return true;
   }
 
@@ -413,82 +412,97 @@ void RenamerClangTidyCheck::registerPPCallbacks(
       std::make_unique<RenamerClangTidyCheckPPCallbacks>(SM, this));
 }
 
-void RenamerClangTidyCheck::addUsage(
-    const RenamerClangTidyCheck::NamingCheckId &Decl, SourceRange Range,
-    const SourceManager &SourceMgr) {
+std::pair<RenamerClangTidyCheck::NamingCheckFailureMap::iterator, bool>
+RenamerClangTidyCheck::addUsage(
+    const RenamerClangTidyCheck::NamingCheckId &FailureId,
+    SourceRange UsageRange, const SourceManager &SourceMgr) {
   // Do nothing if the provided range is invalid.
-  if (Range.isInvalid())
-    return;
+  if (UsageRange.isInvalid())
+    return {NamingCheckFailures.end(), false};
 
-  // 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();
+  // Get 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 = UsageRange.getBegin();
   FixLocation = SourceMgr.getSpellingLoc(FixLocation);
   if (FixLocation.isInvalid())
-    return;
+    return {NamingCheckFailures.end(), false};
+
+  auto EmplaceResult = NamingCheckFailures.try_emplace(FailureId);
+  NamingCheckFailure &Failure = EmplaceResult.first->second;
 
   // Try to insert the identifier location in the Usages map, and bail out if it
   // is already in there
-  RenamerClangTidyCheck::NamingCheckFailure &Failure =
-      NamingCheckFailures[Decl];
   if (!Failure.RawUsageLocs.insert(FixLocation).second)
-    return;
+    return EmplaceResult;
 
-  if (!Failure.shouldFix())
-    return;
+  if (Failure.FixStatus != RenamerClangTidyCheck::ShouldFixStatus::ShouldFix)
+    return EmplaceResult;
 
   if (SourceMgr.isWrittenInScratchSpace(FixLocation))
     Failure.FixStatus = RenamerClangTidyCheck::ShouldFixStatus::InsideMacro;
 
-  if (!utils::rangeCanBeFixed(Range, &SourceMgr))
+  if (!utils::rangeCanBeFixed(UsageRange, &SourceMgr))
     Failure.FixStatus = RenamerClangTidyCheck::ShouldFixStatus::InsideMacro;
+
+  return EmplaceResult;
 }
 
-void RenamerClangTidyCheck::addUsage(const NamedDecl *Decl, SourceRange Range,
+void RenamerClangTidyCheck::addUsage(const NamedDecl *Decl,
+                                     SourceRange UsageRange,
                                      const SourceManager &SourceMgr) {
-  // Don't keep track for non-identifier names.
-  auto *II = Decl->getIdentifier();
-  if (!II)
+  if (hasNoName(Decl))
+    return;
+
+  // Ignore ClassTemplateSpecializationDecl which are creating duplicate
+  // replacements with CXXRecordDecl.
+  if (isa<ClassTemplateSpecializationDecl>(Decl))
     return;
-  if (const auto *Method = dyn_cast<CXXMethodDecl>(Decl)) {
-    if (const CXXMethodDecl *Overridden = getOverrideMethod(Method))
-      Decl = Overridden;
-  }
-  Decl = cast<NamedDecl>(Decl->getCanonicalDecl());
-  return addUsage(
-      RenamerClangTidyCheck::NamingCheckId(Decl->getLocation(), II->getName()),
-      Range, SourceMgr);
-}
 
-void RenamerClangTidyCheck::checkNamedDecl(const NamedDecl *Decl,
-                                           const SourceManager &SourceMgr) {
-  std::optional<FailureInfo> MaybeFailure = getDeclFailureInfo(Decl, SourceMgr);
+  // We don't want to create a failure for every NamedDecl we find. Ideally
+  // there is just one NamedDecl in every group of "related" NamedDecls that
+  // becomes the failure. This NamedDecl and all of its related NamedDecls
+  // become usages. E.g. Since NamedDecls are Redeclarable, only the canonical
+  // NamedDecl becomes the failure and all redeclarations become usages.
+  const NamedDecl *FailureDecl = getFailureForNamedDecl(Decl);
+
+  std::optional<FailureInfo> MaybeFailure =
+      getDeclFailureInfo(FailureDecl, SourceMgr);
   if (!MaybeFailure)
     return;
 
-  FailureInfo &Info = *MaybeFailure;
-  NamingCheckFailure &Failure =
-      NamingCheckFailures[NamingCheckId(Decl->getLocation(), Decl->getName())];
-  SourceRange Range =
-      DeclarationNameInfo(Decl->getDeclName(), Decl->getLocation())
-          .getSourceRange();
-
-  const IdentifierTable &Idents = Decl->getASTContext().Idents;
-  auto CheckNewIdentifier = Idents.find(Info.Fixup);
+  NamingCheckId FailureId(FailureDecl->getLocation(), FailureDecl->getName());
+
+  auto [FailureIter, NewFailure] = addUsage(FailureId, UsageRange, SourceMgr);
+
+  if (FailureIter == NamingCheckFailures.end()) {
+    // Nothing to do if the usage wasn't accepted.
+    return;
+  }
+  if (!NewFailure) {
+    // FailureInfo has already been provided.
+    return;
+  }
+
+  // Update the stored failure with info regarding the FailureDecl.
+  NamingCheckFailure &Failure = FailureIter->second;
+  Failure.Info = std::move(*MaybeFailure);
+
+  // Don't overwritte the failure status if it was already set.
+  if (!Failure.shouldFix()) {
+    return;
+  }
+  const IdentifierTable &Idents = FailureDecl->getASTContext().Idents;
+  auto CheckNewIdentifier = Idents.find(Failure.Info.Fixup);
   if (CheckNewIdentifier != Idents.end()) {
     const IdentifierInfo *Ident = CheckNewIdentifier->second;
     if (Ident->isKeyword(getLangOpts()))
       Failure.FixStatus = ShouldFixStatus::ConflictsWithKeyword;
     else if (Ident->hasMacroDefinition())
       Failure.FixStatus = ShouldFixStatus::ConflictsWithMacroDefinition;
-  } else if (!isValidAsciiIdentifier(Info.Fixup)) {
+  } else if (!isValidAsciiIdentifier(Failure.Info.Fixup)) {
     Failure.FixStatus = ShouldFixStatus::FixInvalidIdentifier;
   }
-
-  Failure.Info = std::move(Info);
-  addUsage(Decl, Range, SourceMgr);
 }
 
 void RenamerClangTidyCheck::check(const MatchFinder::MatchResult &Result) {

clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h

@@ -115,15 +115,9 @@ class RenamerClangTidyCheck : public ClangTidyCheck {
   void expandMacro(const Token &MacroNameTok, const MacroInfo *MI,
                    const SourceManager &SourceMgr);
 
-  void addUsage(const RenamerClangTidyCheck::NamingCheckId &Decl,
-                SourceRange Range, const SourceManager &SourceMgr);
-
-  /// Convenience method when the usage to be added is a NamedDecl.
   void addUsage(const NamedDecl *Decl, SourceRange Range,
                 const SourceManager &SourceMgr);
 
-  void checkNamedDecl(const NamedDecl *Decl, const SourceManager &SourceMgr);
-
 protected:
   /// Overridden by derived classes, returns information about if and how a Decl
   /// failed the check. A 'std::nullopt' result means the Decl did not fail the
@@ -158,6 +152,14 @@ class RenamerClangTidyCheck : public ClangTidyCheck {
                                const NamingCheckFailure &Failure) const = 0;
 
 private:
+  // Manage additions to the Failure/usage map
+  //
+  // return the result of NamingCheckFailures::try_emplace() if the usage was
+  // accepted.
+  std::pair<NamingCheckFailureMap::iterator, bool>
+  addUsage(const RenamerClangTidyCheck::NamingCheckId &FailureId,
+           SourceRange UsageRange, const SourceManager &SourceMgr);
+
   NamingCheckFailureMap NamingCheckFailures;
   const bool AggressiveDependentMemberLookup;
 };