[clang-tidy] New bugprone-derived-method-shadowing-base-method (#154746)

This PR introduces a new bugprone check to find methods in derived
classes that hide methods with the same signature in base classes.

The best description of what this new check does is the unit test code
in
`clang-tools-extra/test/clang-tidy/checkers/bugprone/derived-method-shadowing-base-method.cpp`.

---------

Co-authored-by: Tom James <tom.james@siemens.com>
Co-authored-by: EugeneZelenko <eugene.zelenko@gmail.com>
Co-authored-by: Baranov Victor <bar.victor.2002@gmail.com>

Author: 4 people

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

@@ -23,6 +23,7 @@
 #include "CopyConstructorInitCheck.h"
 #include "CrtpConstructorAccessibilityCheck.h"
 #include "DanglingHandleCheck.h"
+#include "DerivedMethodShadowingBaseMethodCheck.h"
 #include "DynamicStaticInitializersCheck.h"
 #include "EasilySwappableParametersCheck.h"
 #include "EmptyCatchCheck.h"
@@ -134,6 +135,8 @@ class BugproneModule : public ClangTidyModule {
         "bugprone-copy-constructor-init");
     CheckFactories.registerCheck<DanglingHandleCheck>(
         "bugprone-dangling-handle");
+    CheckFactories.registerCheck<DerivedMethodShadowingBaseMethodCheck>(
+        "bugprone-derived-method-shadowing-base-method");
     CheckFactories.registerCheck<DynamicStaticInitializersCheck>(
         "bugprone-dynamic-static-initializers");
     CheckFactories.registerCheck<EasilySwappableParametersCheck>(

clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt

@@ -19,6 +19,7 @@ add_clang_library(clangTidyBugproneModule STATIC
   CopyConstructorInitCheck.cpp
   CrtpConstructorAccessibilityCheck.cpp
   DanglingHandleCheck.cpp
+  DerivedMethodShadowingBaseMethodCheck.cpp
   DynamicStaticInitializersCheck.cpp
   EasilySwappableParametersCheck.cpp
   EmptyCatchCheck.cpp

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

@@ -0,0 +1,128 @@
+//===----------------------------------------------------------------------===//
+//
+// 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 "DerivedMethodShadowingBaseMethodCheck.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::bugprone {
+
+static bool sameBasicType(const ParmVarDecl *Lhs, const ParmVarDecl *Rhs) {
+  return Lhs && Rhs &&
+         Lhs->getType()
+                 .getCanonicalType()
+                 .getNonReferenceType()
+                 .getUnqualifiedType() == Rhs->getType()
+                                              .getCanonicalType()
+                                              .getNonReferenceType()
+                                              .getUnqualifiedType();
+}
+
+static bool namesCollide(const CXXMethodDecl &Lhs, const CXXMethodDecl &Rhs) {
+  if (Lhs.getNameAsString() != Rhs.getNameAsString())
+    return false;
+  if (Lhs.isConst() != Rhs.isConst())
+    return false;
+  if (Lhs.getNumParams() != Rhs.getNumParams())
+    return false;
+  for (unsigned int It = 0; It < Lhs.getNumParams(); ++It)
+    if (!sameBasicType(Lhs.getParamDecl(It), Rhs.getParamDecl(It)))
+      return false;
+  return true;
+}
+
+namespace {
+
+AST_MATCHER(CXXMethodDecl, nameCollidesWithMethodInBase) {
+  const CXXRecordDecl *DerivedClass = Node.getParent();
+  for (const auto &Base : DerivedClass->bases()) {
+    llvm::SmallVector<const CXXBaseSpecifier *, 8> Stack;
+    Stack.push_back(&Base);
+    while (!Stack.empty()) {
+      const CXXBaseSpecifier *CurrentBaseSpec = Stack.back();
+      Stack.pop_back();
+
+      if (CurrentBaseSpec->getAccessSpecifier() ==
+          clang::AccessSpecifier::AS_private)
+        continue;
+
+      const CXXRecordDecl *CurrentRecord =
+          CurrentBaseSpec->getType()->getAsCXXRecordDecl();
+      if (!CurrentRecord)
+        continue;
+
+      // For multiple inheritance, we ignore only the bases that come from the
+      // std:: namespace
+      if (CurrentRecord->isInStdNamespace())
+        continue;
+
+      for (const auto &BaseMethod : CurrentRecord->methods()) {
+        if (namesCollide(*BaseMethod, Node)) {
+          ast_matchers::internal::BoundNodesTreeBuilder Result(*Builder);
+          Builder->setBinding("base_method",
+                              clang::DynTypedNode::create(*BaseMethod));
+          return true;
+        }
+      }
+
+      for (const auto &SubBase : CurrentRecord->bases())
+        Stack.push_back(&SubBase);
+    }
+  }
+  return false;
+}
+
+// Same as clang-tools-extra/clang-tidy/modernize/UseEqualsDefaultCheck.cpp,
+// similar matchers are used elsewhere in LLVM
+AST_MATCHER(CXXMethodDecl, isOutOfLine) { return Node.isOutOfLine(); }
+
+} // namespace
+
+DerivedMethodShadowingBaseMethodCheck::DerivedMethodShadowingBaseMethodCheck(
+    StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context) {}
+
+void DerivedMethodShadowingBaseMethodCheck::registerMatchers(
+    MatchFinder *Finder) {
+  Finder->addMatcher(
+      cxxMethodDecl(
+          unless(anyOf(isOutOfLine(), isStaticStorageClass(), isImplicit(),
+                       cxxConstructorDecl(), isOverride(), isPrivate(),
+                       // isFinal(), //included with isOverride,
+                       // Templates are not handled yet
+                       ast_matchers::isTemplateInstantiation(),
+                       ast_matchers::isExplicitTemplateSpecialization())),
+          ofClass(cxxRecordDecl(isDerivedFrom(cxxRecordDecl()))
+                      .bind("derived_class")),
+          nameCollidesWithMethodInBase())
+          .bind("shadowing_method"),
+      this);
+}
+
+void DerivedMethodShadowingBaseMethodCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *ShadowingMethod =
+      Result.Nodes.getNodeAs<CXXMethodDecl>("shadowing_method");
+  const auto *DerivedClass =
+      Result.Nodes.getNodeAs<CXXRecordDecl>("derived_class");
+  const auto *BaseMethod = Result.Nodes.getNodeAs<CXXMethodDecl>("base_method");
+
+  if (!ShadowingMethod || !DerivedClass || !BaseMethod)
+    llvm_unreachable("Required binding not found");
+
+  diag(ShadowingMethod->getBeginLoc(),
+       "'%0' shadows method with the same name in class %1")
+      << ShadowingMethod->getQualifiedNameAsString() << BaseMethod->getParent();
+  diag(BaseMethod->getBeginLoc(), "previous definition of %0 is here",
+       DiagnosticIDs::Note)
+      << ShadowingMethod;
+}
+
+} // namespace clang::tidy::bugprone

clang-tools-extra/clang-tidy/bugprone/DerivedMethodShadowingBaseMethodCheck.h

@@ -0,0 +1,37 @@
+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_DERIVEDMETHODSHADOWINGBASEMETHODCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_DERIVEDMETHODSHADOWINGBASEMETHODCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::bugprone {
+
+/// Checks that a derived class does not define the same (non virtual) method as
+/// a base class
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/derived-method-shadowing-base-method.html
+class DerivedMethodShadowingBaseMethodCheck : public ClangTidyCheck {
+public:
+  DerivedMethodShadowingBaseMethodCheck(StringRef Name,
+                                        ClangTidyContext *Context);
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.CPlusPlus;
+  }
+  std::optional<TraversalKind> getCheckTraversalKind() const override {
+    return TK_IgnoreUnlessSpelledInSource;
+  }
+};
+
+} // namespace clang::tidy::bugprone
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_DERIVEDMETHODSHADOWINGBASEMETHODCHECK_H

clang-tools-extra/docs/ReleaseNotes.rst

@@ -163,6 +163,11 @@ New checks
   Detects default initialization (to 0) of variables with ``enum`` type where
   the enum has no enumerator with value of 0.
 
+- New :doc:`bugprone-derived-method-shadowing-base-method
+  <clang-tidy/checks/bugprone/derived-method-shadowing-base-method>` check.
+
+  Finds derived class methods that shadow a (non-virtual) base class method.
+
 - New :doc:`cppcoreguidelines-pro-bounds-avoid-unchecked-container-access
   <clang-tidy/checks/cppcoreguidelines/pro-bounds-avoid-unchecked-container-access>`
   check.

clang-tools-extra/docs/clang-tidy/checks/bugprone/derived-method-shadowing-base-method.rst

@@ -0,0 +1,30 @@
+.. title:: clang-tidy - bugprone-derived-method-shadowing-base-method
+
+bugprone-derived-method-shadowing-base-method
+=============================================
+
+Finds derived class methods that shadow a (non-virtual) base class method.
+
+In order to be considered "shadowing", methods must have the same signature
+(i.e. the same name, same number of parameters, same parameter types, etc).
+Only checks public, non-templated methods. 
+
+The below example is bugprone because consumers of the ``Derived`` class will
+expect the ``reset`` method to do the work of ``Base::reset()`` in addition to extra
+work required to reset the ``Derived`` class.  Common fixes include:
+
+- Making the ``reset`` method polymorphic
+- Re-naming ``Derived::reset`` if it's not meant to intersect with ``Base::reset``
+- Using ``using Base::reset`` to change the access specifier
+
+This is also a violation of the Liskov Substitution Principle.
+
+.. code-block:: c++
+
+  struct Base {
+    void reset() {/* reset the base class */};
+  };
+
+  struct Derived : public Base {
+    void reset() {/* reset the derived class, but not the base class */};
+  };

clang-tools-extra/docs/clang-tidy/checks/list.rst

@@ -91,6 +91,7 @@ Clang-Tidy Checks
    :doc:`bugprone-copy-constructor-init <bugprone/copy-constructor-init>`, "Yes"
    :doc:`bugprone-crtp-constructor-accessibility <bugprone/crtp-constructor-accessibility>`, "Yes"
    :doc:`bugprone-dangling-handle <bugprone/dangling-handle>`,
+   :doc:`bugprone-derived-method-shadowing-base-method <bugprone/derived-method-shadowing-base-method>`,
    :doc:`bugprone-dynamic-static-initializers <bugprone/dynamic-static-initializers>`,
    :doc:`bugprone-easily-swappable-parameters <bugprone/easily-swappable-parameters>`,
    :doc:`bugprone-empty-catch <bugprone/empty-catch>`,

clang-tools-extra/test/clang-tidy/checkers/bugprone/derived-method-shadowing-base-method.cpp

@@ -0,0 +1,139 @@
+// RUN: %check_clang_tidy %s bugprone-derived-method-shadowing-base-method %t
+
+class Base 
+{
+    void method();
+    void methodWithArg(int I);
+
+    virtual Base* getThis() = 0;
+};
+
+class A : public Base
+{
+public:
+    void method();
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'A::method' shadows method with the same name in class 'Base' [bugprone-derived-method-shadowing-base-method]
+// CHECK-MESSAGES: :5:5: note: previous definition of 'method' is here
+};
+
+// only declaration should be checked
+void A::method()
+{    
+}
+
+class B
+{
+public:
+    void method();
+};
+
+class D: public Base
+{
+
+};
+
+// test indirect inheritance
+class E : public D
+{
+public:
+    void method();
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'E::method' shadows method with the same name in class 'Base' [bugprone-derived-method-shadowing-base-method]
+};
+
+class H : public Base
+{
+public:
+    Base* getThis() override;
+    Base const* getThis() const;
+};
+
+class I : public Base
+{
+public:
+    // test with inline implementation
+    void method()
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'I::method' shadows method with the same name in class 'Base' [bugprone-derived-method-shadowing-base-method]
+    {
+
+    }
+};
+
+class J : public Base
+{
+public:
+    Base* getThis() final;
+};
+
+template<typename T>
+class TemplateBase
+{
+public:
+   virtual void size() const = 0;
+};
+    
+template<typename T>
+class K : public TemplateBase<T>
+{
+public:
+    void size() const final;
+};
+
+class L : public Base
+{
+public:
+// not same signature (take const ref) but still ambiguous
+    void methodWithArg(int const& I);
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'L::methodWithArg' shadows method with the same name in class 'Base' [bugprone-derived-method-shadowing-base-method]
+
+    void methodWithArg(int const I);
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'L::methodWithArg' shadows method with the same name in class 'Base' [bugprone-derived-method-shadowing-base-method]
+
+    void methodWithArg(int *I);
+    void methodWithArg(int const* I);
+};
+
+class M : public Base
+{
+public:
+    static void method();
+};
+
+class N : public Base
+{
+public:
+    template<typename T>
+    void methodWithArg(T I);
+    // TODO:  Templates are not handled yet
+    template<> void methodWithArg<int>(int I);
+};
+
+namespace std{
+    struct thread{
+        void join();
+    };
+}
+
+struct O: public std::thread{
+    void join();
+};
+
+struct P: public std::thread, Base{
+    void join();
+    void method();
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'P::method' shadows method with the same name in class 'Base' [bugprone-derived-method-shadowing-base-method]
+};
+
+class Q : public Base
+{
+public:
+    typedef int MyInt;
+// not same signature (take const ref) but still ambiguous
+    void methodWithArg(MyInt const& I);
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'Q::methodWithArg' shadows method with the same name in class 'Base' [bugprone-derived-method-shadowing-base-method]
+
+    void methodWithArg(MyInt const I);
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'Q::methodWithArg' shadows method with the same name in class 'Base' [bugprone-derived-method-shadowing-base-method]
+
+    void methodWithArg(MyInt *I);
+    void methodWithArg(MyInt const* I);
+};