[clang-tidy] RenamerClangTidy now renames dependent member expr when the member can be resolved

[njames93]

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;
  }
};

[njames93]

• replace auto when type isnt explicit

[JonasToth]

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

280

can Base be a const pointer?

In which case will this pointer be a nullptr? Your test-cases seem to exercise only cases where Base is not-null.

clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-member-decl-usage.cpp

188

Could you please provide a test-case with non-type template parameters as well?

Another question but maybe/most likely off topic: are how are member-pointers handled? Both for data and member-functions.
-> Maybe a test-case for such a situation would be good, as well? :)

Crème de la Crème would be if templated member-pointers are treated properly, too.

[merge_guards_bot]

Unit tests: pass. 61955 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[merge_guards_bot]

Unit tests: pass. 61955 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[njames93]

• Added not type template test case

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

280

Base can be a const pointer. However I don't think it can be a nullptr, but I put the check in there just in case it ever is, maybe an assert would be the correct solution.

clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-member-decl-usage.cpp

188

The test for non-type isn't strictly needed. in these test cases the template type isn't used but they still show up as dependent scope exprs.

MemberPointers is something that this doesn't handle and probably something I wont add as you can have issues with specialisation meaning the member may not exist in a specialization

[merge_guards_bot]

Unit tests: pass. 62027 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[njames93]

hmmm annoyingly this change causes a crash in a side project I'm creating, gotta go bug hunting now

[njames93]

• Added some sanity tests that fix weird crashes
• Added support for CXXMethodDecls that are referenced with depended scope member expr

[merge_guards_bot]

Unit tests: fail. 62026 tests passed, 1 failed and 783 were skipped.

failed: Clang Tools.clang-tidy/checkers/readability-identifier-naming-member-decl-usage.cpp

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[merge_guards_bot]

Unit tests: fail. 62026 tests passed, 1 failed and 783 were skipped.

failed: Clang Tools.clang-tidy/checkers/readability-identifier-naming-member-decl-usage.cpp

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[njames93]

• rebase master figure out the llvm pre-merge lint test fail

[merge_guards_bot]

Unit tests: fail. 62040 tests passed, 1 failed and 783 were skipped.

failed: Clang Tools.clang-tidy/checkers/readability-identifier-naming-member-decl-usage.cpp

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[njames93]

• Fix assertion causing failing tests in debug

[merge_guards_bot]

Unit tests: pass. 62041 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[njames93]

• Aggressive dependent lookup option added

[merge_guards_bot]

Unit tests: pass. 62329 tests passed, 0 failed and 838 were skipped.

clang-tidy: fail. clang-tidy found 0 errors and 1 warnings. 0 of them are added as review comments below (why?).

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[njames93]

• Fix clang tidy warning

[merge_guards_bot]

Unit tests: pass. 62329 tests passed, 0 failed and 838 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[njames93]

• Streamline memberExpr matchers

[merge_guards_bot]

Unit tests: fail. 62347 tests passed, 1 failed and 839 were skipped.

failed: libc++.std/containers/sequences/array/array_creation/to_array.fail.cpp

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[njames93]

I toyed with the idea of checking the access specifier for the dependent base classes, but I'm pretty sure it doesn't matter. If the compile finds 2 look ups in one base class it will pick the later dependancy regardless of whether its private or not. And if the compiler finds 2 look ups in multiple inheritance bases it will always be an error, regardless of access specifier

[njames93]

Ping

[njames93]

• Simplified member expr restrictions

[njames93]

• Made AggressiveDependentMemberLookup option Global

[njames93]

• Small refactor of code

[njames93]

Ping

[njames93]

• rebase and small nit

[njames93]

Clean up code and remove unrelated changes

[aaron.ballman]

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

187–189

Make these explicit?

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

44 ↗

(On Diff #257151)

I'd appreciate some comments here explaining when this should be overridden. I'd also like to understand why we need storeOptions and storeCheckOptions because the two names are so similar to one another.

[njames93]

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

44 ↗

(On Diff #257151)

I'm still unsure what the best approach here is. RenamerClangTidyCheck::storeOptions needs to be called to store the AggressiveDependentMemberLookup Option (as well as any other renaming specific option that may be added down the line). However derived classes need to also store their respective options at the same time. Due to how c++ works there isn't a nice way to ensure base methods are called at the same time as derived methods.
Anyway that's the reason the names are similar they do a very similar job only one is meant for the RenamerClangTidyCheck class and the other for derived classes.
I agree a comment is needed to explain why so I'll add that in for now

[njames93]

• Address nits

[aaron.ballman]

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

44 ↗

(On Diff #257151)

I think it's reasonable to expect developers to call Base::storeOptions() from within Derived::storeOptions() and we don't need a second method to override. You're correct that there's not a good way to ensure the base method is called, but test cases should exist for the options in a check and so I would imagine that a developer who forgets to call the base class function will have failing test cases to help alert them to the issue. WDYT?

[njames93]

• Tweaked options to remove second virtual method

[aaron.ballman]

LGTM, thank you for this!

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

 //===--- RenamerClangTidyCheck.cpp - clang-tidy ---------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "RenamerClangTidyCheck.h"
 #include "ASTUtils.h"
 #include "clang/AST/CXXInheritance.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Frontend/CompilerInstance.h"
 #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"
 
 #define DEBUG_TYPE "clang-tidy"
 
 using namespace clang::ast_matchers;
 
 namespace llvm {
 
 /// Specialisation of DenseMapInfo to allow NamingCheckId objects in DenseMaps
void RenamerClangTidyCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(usingDecl().bind("using"), this);
   Finder->addMatcher(declRefExpr().bind("declRef"), this);
   Finder->addMatcher(cxxConstructorDecl(unless(isImplicit())).bind("classRef"),
                      this);
   Finder->addMatcher(cxxDestructorDecl(unless(isImplicit())).bind("classRef"),
                      this);
   Finder->addMatcher(typeLoc().bind("typeLoc"), this);
   Finder->addMatcher(nestedNameSpecifierLoc().bind("nestedNameLoc"), this);
+  auto MemberOrDependent =
+      expr(eachOf(memberExpr().bind("memberExpr"),
+                  cxxDependentScopeMemberExpr().bind("depMemberExpr")));
   Finder->addMatcher(
       functionDecl(unless(cxxMethodDecl(isImplicit())),
-                   hasBody(forEachDescendant(memberExpr().bind("memberExpr")))),
+                   hasBody(forEachDescendant(MemberOrDependent))),
       this);
   Finder->addMatcher(
-      cxxConstructorDecl(
-          unless(isImplicit()),
-          forEachConstructorInitializer(
-              allOf(isWritten(), withInitializer(forEachDescendant(
-                                     memberExpr().bind("memberExpr")))))),
+      cxxConstructorDecl(unless(isImplicit()),
+                         forEachConstructorInitializer(allOf(
+                             isWritten(), withInitializer(forEachDescendant(
+                                              MemberOrDependent))))),
       this);
-  Finder->addMatcher(fieldDecl(hasInClassInitializer(
-                         forEachDescendant(memberExpr().bind("memberExpr")))),
-                     this);
+  Finder->addMatcher(
+      fieldDecl(hasInClassInitializer(forEachDescendant(MemberOrDependent))),
 }
 
 void RenamerClangTidyCheck::registerPPCallbacks(
     const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {
   ModuleExpanderPP->addPPCallbacks(
       std::make_unique<RenamerClangTidyCheckPPCallbacks>(ModuleExpanderPP,
                                                          this));
 }
if (const auto *Decl =
     for (const auto *Init : Decl->inits()) {
       if (!Init->isWritten() || Init->isInClassMemberInitializer())
         continue;
       if (const FieldDecl *FD = Init->getAnyMember())
         addUsage(NamingCheckFailures, FD,
                  SourceRange(Init->getMemberLocation()));
       // Note: delegating constructors and base class initializers are handled
       // via the "typeLoc" matcher.
     }
     return;
   }
 
   if (const auto *Decl =
           Result.Nodes.getNodeAs<CXXDestructorDecl>("classRef")) {
 
     SourceRange Range = Decl->getNameInfo().getSourceRange();
     if (Range.getBegin().isInvalid())
       return;
     // The first token that will be found is the ~ (or the equivalent trigraph),
void RenamerClangTidyCheck::check(const MatchFinder::MatchResult &Result) {
   if (const auto *MemberRef =
           Result.Nodes.getNodeAs<MemberExpr>("memberExpr")) {
     SourceRange Range = MemberRef->getMemberNameInfo().getSourceRange();
     addUsage(NamingCheckFailures, MemberRef->getMemberDecl(), Range,
              Result.SourceManager);
     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;
+    auto *IdentInfo =
+        DepMemberRef->getMemberNameInfo().getName().getAsIdentifierInfo();
+    if (!IdentInfo)
+      return;
+    StringRef DependentName = IdentInfo->getName();
+    for (const FieldDecl *Field : Base->fields()) {
+      if (Field->getParent() == Base &&
+          Field->getName().equals(DependentName)) {
+        SourceRange Range = DepMemberRef->getMemberNameInfo().getSourceRange();
+        addUsage(NamingCheckFailures, Field, Range, Result.SourceManager);
+        return;
+      }
+    }
+    for (const CXXMethodDecl *Method : Base->methods()) {
+      if (Method->getParent() == Base &&
+          Method->getName().equals(DependentName)) {
+        SourceRange Range = DepMemberRef->getMemberNameInfo().getSourceRange();
+        addUsage(NamingCheckFailures, Method, Range, Result.SourceManager);
+        return;
+      }
+    }
+  }
+
   if (const auto *Decl = Result.Nodes.getNodeAs<NamedDecl>("decl")) {
     if (!Decl->getIdentifier() || Decl->getName().empty() || Decl->isImplicit())
       return;
 
     // Fix type aliases in value declarations.
     if (const auto *Value = Result.Nodes.getNodeAs<ValueDecl>("decl")) {
       if (const Type *TypePtr = Value->getType().getTypePtrOrNull()) {
         if (const auto *Typedef = TypePtr->getAs<TypedefType>())

clang-tools-extra/docs/ReleaseNotes.rst

 - New alias :doc:`cert-dcl51-cpp
   <clang-tidy/checks/cert-dcl51-cpp>` to
   :doc:`bugprone-reserved-identifier
   <clang-tidy/checks/bugprone-reserved-identifier>` was added.
 
 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.
 
 Renamed checks
 ^^^^^^^^^^^^^^
 
 
 Improvements to include-fixer
 -----------------------------
 

clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-member-decl-usage.cpp

 // RUN: %check_clang_tidy %s readability-identifier-naming %t -- \
 // RUN:   -config='{CheckOptions: [ \
 // RUN:     {key: readability-identifier-naming.MemberCase, value: CamelCase}, \
-// RUN:     {key: readability-identifier-naming.ParameterCase, value: CamelCase} \
+// RUN:     {key: readability-identifier-naming.ParameterCase, value: CamelCase}, \
+// RUN:     {key: readability-identifier-naming.MethodCase, value: camelBack} \
 // RUN:  ]}' -- -fno-delayed-template-parsing
 
 int set_up(int);
 int clear(int);
 
 class Foo {
 public:
   const int bar_baz; // comment-0
return x.getBar2(); // gotta have a reference to the getBar2 so that the
                       // compiler will generate the function and resolve
                       // the DependantScopeMemberExpr
 }
 
 namespace Bug41122 {
 namespace std {
 
 // for this example we aren't bothered about how std::vector is treated
 template <typename T> //NOLINT
-class vector { //NOLINT
+class vector {        //NOLINT
 public:
   void push_back(bool); //NOLINT
-  void pop_back(); //NOLINT
-}; //NOLINT
-}; // namespace std
+  void pop_back();      //NOLINT
+};                      //NOLINT
+};                      // namespace std
 
-class Foo { 
+class Foo {
   std::vector<bool> &stack;
   // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: invalid case style for member 'stack' [readability-identifier-naming]
 public:
   Foo(std::vector<bool> &stack)
       // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: invalid case style for parameter 'stack' [readability-identifier-naming]
       // CHECK-FIXES: {{^}}  Foo(std::vector<bool> &Stack)
       : stack(stack) {
     // CHECK-FIXES: {{^}}      : Stack(Stack) {
     stack.push_back(true);
     // CHECK-FIXES: {{^}}    Stack.push_back(true);
   }
   ~Foo() {
     stack.pop_back();

 public:
   Container() : pointer(&storage[0]) {}
   // CHECK-FIXES: {{^}}  Container() : Pointer(&Storage[0]) {}
 };
 
 void foo() {
   Container<int, 5> container;
 }
-}; // namespace CtorInits
+} // namespace CtorInits
+
+namespace resolved_dependance {
+template <typename T>
+struct A0 {
+  int value;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for member 'value'
+  A0 &operator=(const A0 &Other) {
+    value = Other.value;       // A0
+    this->value = Other.value; // A0
+    // CHECK-FIXES:      {{^}}    Value = Other.Value;       // A0
+    // CHECK-FIXES-NEXT: {{^}}    this->Value = Other.Value; // A0
+    return *this;
+  }
+  void outOfLineReset();
+};
+
+template <typename T>
+void A0<T>::outOfLineReset() {
+  this->value -= value; // A0
+  // CHECK-FIXES: {{^}}  this->Value -= Value; // A0
+}
+
+template <typename T>
+struct A1 {
+  int value; // A1
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for member 'value'
+  // CHECK-FIXES: {{^}}  int Value; // A1
+  int GetValue() const { return value; } // A1
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for method 'GetValue'
+  // CHECK-FIXES {{^}}  int getValue() const { return Value; } // A1
+  void SetValue(int Value) { this->value = Value; } // A1
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: invalid case style for method 'SetValue'
+  // CHECK-FIXES {{^}}  void setValue(int Value) { this->Value = Value; } // A1
+  A1 &operator=(const A1 &Other) {
+    this->SetValue(Other.GetValue()); // A1
+    this->value = Other.value;        // A1
+    // CHECK-FIXES:      {{^}}    this->setValue(Other.getValue()); // A1
+    // CHECK-FIXES-NEXT: {{^}}    this->Value = Other.Value;        // A1
+    return *this;
+  }
+  void outOfLineReset();
+};
+
+template <typename T>
+void A1<T>::outOfLineReset() {
+  this->value -= value; // A1
+  // CHECK-FIXES: {{^}}  this->Value -= Value; // A1
+}
+
+template <unsigned T>
+struct A2 {
+  int value; // A2
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for member 'value'
+  // CHECK-FIXES: {{^}}  int Value; // A2
+  A2 &operator=(const A2 &Other) {
+    value = Other.value;       // A2
+    this->value = Other.value; // A2
+    // CHECK-FIXES:      {{^}}    Value = Other.Value;       // A2
+    // CHECK-FIXES-NEXT: {{^}}    this->Value = Other.Value; // A2
+    return *this;
+  }
+};
+
+//create some instances to check it works
+A1<int> AInt{};
+A1<int> BInt = (AInt.outOfLineReset(), AInt);
+A1<unsigned> AUnsigned{};
+A1<unsigned> BUnsigned = AUnsigned;
+} //namespace resolved_dependance
+
+namespace unresolved_dependance {
+template <typename T>
+struct DependentBase {
+  int depValue;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for member 'depValue'
+};
+
+template <typename T>
+struct Derived : DependentBase<T> {
+  Derived &operator=(const Derived &Other) {
+    this->depValue = Other.depValue;
+    // CHECK-FIXES-NOT: {{^}}    this->DepValue = Other.DepValue;
+    return *this;
+  }
+};
+
+Derived<int> AInt{};
+Derived<int> BInt = AInt;
+
+} //namespace unresolved_dependance