[clang-tidy] RenamerClangTidy now renames dependent member expr when …

…the member can be resolved

Summary:
Sometimes in templated code Member references are reported as `DependentScopeMemberExpr` because that's what the standard dictates, however in many trivial cases it is easy to resolve the reference to its actual Member.
Take this code:
```
template<typename T>
class A{
  int value;
  A& operator=(const A& Other){
    value = Other.value;
    this->value = Other.value;
    return *this;
  }
};
```
When ran with `clang-tidy file.cpp -checks=readability-identifier-naming --config="{CheckOptions: [{key: readability-identifier-naming.MemberPrefix, value: m_}]}" -fix`
Current behaviour:
```
template<typename T>
class A{
  int m_value;
  A& operator=(const A& Other){
    m_value = Other.value;
    this->value = Other.value;
    return *this;
  }
};
```
As `this->value` and `Other.value` are Dependent they are ignored when creating the fix-its, however this can easily be resolved.
Proposed behaviour:
```
template<typename T>
class A{
  int m_value;
  A& operator=(const A& Other){
    m_value = Other.m_value;
    this->m_value = Other.m_value;
    return *this;
  }
};
```

Reviewers: aaron.ballman, JonasToth, alexfh, hokein, gribozavr2

Reviewed By: aaron.ballman

Subscribers: merge_guards_bot, xazax.hun, cfe-commits

Tags: #clang, #clang-tools-extra

Differential Revision: https://reviews.llvm.org/D73052

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

@@ -47,6 +47,7 @@ ReservedIdentifierCheck::ReservedIdentifierCheck(StringRef Name,
           Options.get("AllowedIdentifiers", ""))) {}
 
 void ReservedIdentifierCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  RenamerClangTidyCheck::storeOptions(Opts);
   Options.store(Opts, "Invert", Invert);
   Options.store(Opts, "AllowedIdentifiers",
                 utils::options::serializeStringList(AllowedIdentifiers));

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

@@ -170,6 +170,7 @@ IdentifierNamingCheck::IdentifierNamingCheck(StringRef Name,
 IdentifierNamingCheck::~IdentifierNamingCheck() = default;
 
 void IdentifierNamingCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  RenamerClangTidyCheck::storeOptions(Opts);
   for (size_t i = 0; i < SK_Count; ++i) {
     if (NamingStyles[i]) {
       if (NamingStyles[i]->Case) {

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

@@ -14,8 +14,7 @@
 #include "clang/Lex/PPCallbacks.h"
 #include "clang/Lex/Preprocessor.h"
 #include "llvm/ADT/DenseMapInfo.h"
-#include "llvm/Support/Debug.h"
-#include "llvm/Support/Format.h"
+#include "llvm/ADT/PointerIntPair.h"
 
 #define DEBUG_TYPE "clang-tidy"
 
@@ -93,9 +92,16 @@ class RenamerClangTidyCheckPPCallbacks : public PPCallbacks {
 
 RenamerClangTidyCheck::RenamerClangTidyCheck(StringRef CheckName,
                                              ClangTidyContext *Context)
-    : ClangTidyCheck(CheckName, Context) {}
+    : ClangTidyCheck(CheckName, Context),
+      AggressiveDependentMemberLookup(
+          Options.getLocalOrGlobal("AggressiveDependentMemberLookup", false)) {}
 RenamerClangTidyCheck::~RenamerClangTidyCheck() = default;
 
+void RenamerClangTidyCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "AggressiveDependentMemberLookup",
+                AggressiveDependentMemberLookup);
+}
+
 void RenamerClangTidyCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(namedDecl().bind("decl"), this);
   Finder->addMatcher(usingDecl().bind("using"), this);
@@ -106,20 +112,12 @@ void RenamerClangTidyCheck::registerMatchers(MatchFinder *Finder) {
                      this);
   Finder->addMatcher(typeLoc().bind("typeLoc"), this);
   Finder->addMatcher(nestedNameSpecifierLoc().bind("nestedNameLoc"), this);
+  auto MemberRestrictions =
+      unless(forFunction(anyOf(isDefaulted(), isImplicit())));
+  Finder->addMatcher(memberExpr(MemberRestrictions).bind("memberExpr"), this);
   Finder->addMatcher(
-      functionDecl(unless(cxxMethodDecl(isImplicit())),
-                   hasBody(forEachDescendant(memberExpr().bind("memberExpr")))),
-      this);
-  Finder->addMatcher(
-      cxxConstructorDecl(
-          unless(isImplicit()),
-          forEachConstructorInitializer(
-              allOf(isWritten(), withInitializer(forEachDescendant(
-                                     memberExpr().bind("memberExpr")))))),
+      cxxDependentScopeMemberExpr(MemberRestrictions).bind("depMemberExpr"),
       this);
-  Finder->addMatcher(fieldDecl(hasInClassInitializer(
-                         forEachDescendant(memberExpr().bind("memberExpr")))),
-                     this);
 }
 
 void RenamerClangTidyCheck::registerPPCallbacks(
@@ -169,6 +167,78 @@ static void addUsage(RenamerClangTidyCheck::NamingCheckFailureMap &Failures,
                   Range, SourceMgr);
 }
 
+const NamedDecl *findDecl(const RecordDecl &RecDecl, StringRef DeclName) {
+  for (const Decl *D : RecDecl.decls()) {
+    if (const auto *ND = dyn_cast<NamedDecl>(D)) {
+      if (ND->getDeclName().isIdentifier() && ND->getName().equals(DeclName))
+        return ND;
+    }
+  }
+  return nullptr;
+}
+
+namespace {
+class NameLookup {
+  llvm::PointerIntPair<const NamedDecl *, 1, bool> Data;
+
+public:
+  explicit NameLookup(const NamedDecl *ND) : Data(ND, false) {}
+  explicit NameLookup(llvm::NoneType) : Data(nullptr, true) {}
+  explicit NameLookup(std::nullptr_t) : Data(nullptr, false) {}
+  NameLookup() : NameLookup(nullptr) {}
+
+  bool hasMultipleResolutions() const { return Data.getInt(); }
+  bool hasDecl() const {
+    assert(!hasMultipleResolutions() && "Found multiple decls");
+    return Data.getPointer() != nullptr;
+  }
+  const NamedDecl *getDecl() const {
+    assert(!hasMultipleResolutions() && "Found multiple decls");
+    return Data.getPointer();
+  }
+  operator bool() const { return !hasMultipleResolutions(); }
+  const NamedDecl *operator*() const { return getDecl(); }
+};
+} // namespace
+
+/// 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.
+/// If a matching decl is found in multiple base classes then it will return a
+/// flag indicating the multiple resolutions.
+NameLookup findDeclInBases(const CXXRecordDecl &Parent, StringRef DeclName,
+                           bool AggressiveTemplateLookup) {
+  if (const NamedDecl *InClassRef = findDecl(Parent, DeclName))
+    return NameLookup(InClassRef);
+  const NamedDecl *Found = nullptr;
+
+  for (CXXBaseSpecifier Base : Parent.bases()) {
+    const auto *Record = Base.getType()->getAsCXXRecordDecl();
+    if (!Record && AggressiveTemplateLookup) {
+      if (const auto *TST =
+              Base.getType()->getAs<TemplateSpecializationType>()) {
+        if (const auto *TD = llvm::dyn_cast_or_null<ClassTemplateDecl>(
+                TST->getTemplateName().getAsTemplateDecl()))
+          Record = TD->getTemplatedDecl();
+      }
+    }
+    if (!Record)
+      continue;
+    if (auto Search =
+            findDeclInBases(*Record, DeclName, AggressiveTemplateLookup)) {
+      if (*Search) {
+        if (Found)
+          return NameLookup(
+              llvm::None); // Multiple decls found in different base classes.
+        Found = *Search;
+        continue;
+      }
+    } else
+      return NameLookup(llvm::None); // Propagate multiple resolution back up.
+  }
+  return NameLookup(Found); // If nullptr, decl wasnt found.
+}
+
 void RenamerClangTidyCheck::check(const MatchFinder::MatchResult &Result) {
   if (const auto *Decl =
           Result.Nodes.getNodeAs<CXXConstructorDecl>("classRef")) {
@@ -272,6 +342,32 @@ void RenamerClangTidyCheck::check(const MatchFinder::MatchResult &Result) {
     return;
   }
 
+  if (const auto *DepMemberRef =
+          Result.Nodes.getNodeAs<CXXDependentScopeMemberExpr>(
+              "depMemberExpr")) {
+    QualType BaseType = DepMemberRef->isArrow()
+                            ? DepMemberRef->getBaseType()->getPointeeType()
+                            : DepMemberRef->getBaseType();
+    if (BaseType.isNull())
+      return;
+    const CXXRecordDecl *Base = BaseType.getTypePtr()->getAsCXXRecordDecl();
+    if (!Base)
+      return;
+    DeclarationName DeclName = DepMemberRef->getMemberNameInfo().getName();
+    if (!DeclName.isIdentifier())
+      return;
+    StringRef DependentName = DeclName.getAsIdentifierInfo()->getName();
+
+    if (NameLookup Resolved = findDeclInBases(
+            *Base, DependentName, AggressiveDependentMemberLookup)) {
+      if (*Resolved)
+        addUsage(NamingCheckFailures, *Resolved,
+                 DepMemberRef->getMemberNameInfo().getSourceRange(),
+                 Result.SourceManager);
+    }
+    return;
+  }
+
   if (const auto *Decl = Result.Nodes.getNodeAs<NamedDecl>("decl")) {
     // Fix using namespace declarations.
     if (const auto *UsingNS = dyn_cast<UsingDirectiveDecl>(Decl))

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

@@ -40,6 +40,10 @@ class RenamerClangTidyCheck : public ClangTidyCheck {
                            Preprocessor *ModuleExpanderPP) override final;
   void onEndOfTranslationUnit() override final;
 
+  /// Derived classes that override this function should call this method from
+  /// the overridden method.
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+
   /// This enum will be used in %select of the diagnostic message.
   /// Each value below IgnoreFailureThreshold should have an error message.
   enum class ShouldFixStatus {
@@ -142,6 +146,7 @@ class RenamerClangTidyCheck : public ClangTidyCheck {
 
 private:
   NamingCheckFailureMap NamingCheckFailures;
+  const bool AggressiveDependentMemberLookup;
 };
 
 } // namespace tidy

clang-tools-extra/docs/ReleaseNotes.rst

@@ -176,6 +176,12 @@ New check aliases
 Changes in existing checks
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
+- Improved :doc:'readability-identifier-naming
+  <clang-tidy/checks/readability-identifier-naming>` check.
+
+  Now able to rename member references in class template definitions with 
+  explicit access.
+
 - Improved :doc:`readability-qualified-auto
   <clang-tidy/checks/readability-qualified-auto>` check now supports a
   `AddConstToQualified` to enable adding ``const`` qualifiers to variables

clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst

@@ -36,6 +36,7 @@ Options
 The following options are describe below:
 
  - :option:`AbstractClassCase`, :option:`AbstractClassPrefix`, :option:`AbstractClassSuffix`
+ - :option:`AggressiveDependentMemberLookup`
  - :option:`ClassCase`, :option:`ClassPrefix`, :option:`ClassSuffix`
  - :option:`ClassConstantCase`, :option:`ClassConstantPrefix`, :option:`ClassConstantSuffix`
  - :option:`ClassMemberCase`, :option:`ClassMemberPrefix`, :option:`ClassMemberSuffix`
@@ -127,6 +128,64 @@ After:
       pre_abstract_class_post();
     };
 
+.. option:: AggressiveDependentMemberLookup
+
+    When set to `1` the check will look in dependent base classes for dependent
+    member references that need changing. This can lead to errors with template
+    specializations so the default value is `0`.
+
+For example using values of:
+
+   - ClassMemberCase of ``lower_case``
+
+Before:
+
+.. code-block:: c++
+
+    template <typename T>
+    struct Base {
+      T BadNamedMember;
+    };
+
+    template <typename T>
+    struct Derived : Base<T> {
+      void reset() {
+        this->BadNamedMember = 0;
+      }
+    };
+
+After if AggressiveDependentMemberLookup is ``0``:
+
+.. code-block:: c++
+
+    template <typename T>
+    struct Base {
+      T bad_named_member;
+    };
+
+    template <typename T>
+    struct Derived : Base<T> {
+      void reset() {
+        this->BadNamedMember = 0;
+      }
+    };
+
+After if AggressiveDependentMemberLookup is ``1``:
+
+.. code-block:: c++
+
+    template <typename T>
+    struct Base {
+      T bad_named_member;
+    };
+
+    template <typename T>
+    struct Derived : Base<T> {
+      void reset() {
+        this->bad_named_member = 0;
+      }
+    };
+
 .. option:: ClassCase
 
     When defined, the check will ensure class names conform to the