[clang-tidy] [readability-convert-member-functions-to-static] ignore functions that hide base class methods

[mgehre]

Summary

Fixes bug https://bugs.llvm.org/show_bug.cgi?id=43002

[aaron.ballman]

I'd like some help in understanding the motivation for this change. The bug report says But the intention behind this is clearly that there is a member function A::foo which derived classes overwrite. and I'm not certain that's so clear. It's been my experience that hidden member function names are unusual and it's hard to ascribe intent to them.

FWIW, I would find this new behavior to be confusing. My mental model for this check is "if it doesn't need to be a member function, make it a static function instead", and this behavior doesn't fit that model.

[mgehre]

I struggle myself to fully understand the motivation behind the bug report. This is certainly not code that I would write.
So I don't really care about that kind of code and I didn't want to be in the way of other people's way of writing C++.

Alternatively, we could ask the bug reporter to use // NOLINT to support his specific case. I will try to get some clarification
from the reporter.

[mgehre]

The discussions on the bug did not provide further insight (until now).

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

AST_MATCHER(CXXMethodDecl, usesThis) {
   } UsageOfThis;
 
   // TraverseStmt does not modify its argument.
   UsageOfThis.TraverseStmt(const_cast<Stmt *>(Node.getBody()));
 
   return UsageOfThis.Used;
 }
 
+/// Does any base class have a non-static method with the same name?
+AST_MATCHER(CXXMethodDecl, isHiding) {
+  const IdentifierInfo *ThisIdentifier = Node.getIdentifier();
+  if (!ThisIdentifier)
+    return false;
+
+  const CXXRecordDecl *Record = Node.getParent();
+
+  auto StaticOrDifferentName = [ThisIdentifier](const CXXMethodDecl *Method) {
+                          if (Method->isStatic())
+                            return true;
+
+                          const IdentifierInfo *Ident = Method->getIdentifier();
+                          if (!Ident)
+                            return true;
+
+                          return Ident->getName() != ThisIdentifier->getName();
+                        };
+
+  return !Record->forallBases([StaticOrDifferentName](const CXXRecordDecl *Base) {
+    return llvm::all_of(Base->methods(),
+                        StaticOrDifferentName);
+  });
+}
+
 void ConvertMemberFunctionsToStatic::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(
       cxxMethodDecl(
           isDefinition(), isUserProvided(),
           unless(anyOf(
               isExpansionInSystemHeader(), isVirtual(), isStatic(),
               hasTrivialBody(), isOverloadedOperator(), cxxConstructorDecl(),
               cxxDestructorDecl(), cxxConversionDecl(), isTemplate(),
               isDependentContext(),
               ofClass(anyOf(
                   isLambda(),
                   hasAnyDependentBases()) // Method might become virtual
                                           // depending on template base class.
                       ),
               isInsideMacroDefinition(),
-              hasCanonicalDecl(isInsideMacroDefinition()), usesThis())))
+              hasCanonicalDecl(isInsideMacroDefinition()), isHiding(),
+              usesThis())))
           .bind("x"),
       this);
 }
 
 /// \brief Obtain the original source code text from a SourceRange.
 static StringRef getStringFromRange(SourceManager &SourceMgr,
                                     const LangOptions &LangOpts,
                                     SourceRange Range) {

clang-tools-extra/test/clang-tidy/readability-convert-member-functions-to-static.cpp

#define T2(FunctionName) \
   T2(A2)
 
 #define VOLATILE volatile
   void volatileMacro() VOLATILE {
     // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'volatileMacro' can be made static
     return;
   }
 };
+
+class HiddingBase {
+  int f();
+
+  static int g();
+};
+
+class HiddingDerived : public HiddingBase {
+  int f() {
+    return 0;
+  }
+
+  int g() {
+    // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: method 'g' can be made static
+    // CHECK-FIXES: {{^}}  static int g() {
+    return 0;
+  }
+};