[clang-tidy] Handle decayed types and other improvements in VirtualNearMiss check.

[xazax.hun]

Summary

First of all thank you for this great check!

In this patch I propose one small cleanup and two changes in the behaviour. The first is to desugar decayed types in the arguments of functions. This way it is possible to detect override candidates when a method that has a pointer as an argument tries to override a method that has an array as an argument that is decayed to a pointer. The other change is not to take visibility into account since it is perfectly valid to override a public method with a private one and such code exists.

I have one more proposal that is not implemented in this patch yet: it is a common mistake to not to override a method because the developer forget to add the qualifiers. What about a configuration option to also report near misses when only a qualifier is missing?

What do you think?

[alexfh]

... What about a configuration option to also report near misses when only a qualifier is missing?

Might be a useful thing. We should first check if it makes sense to always ignore a qualifier.

clang-tidy/misc/VirtualNearMissCheck.cpp

55 ↗

(On Diff #44847)

Interesting, didn't know about Type::getPointeeType(). I'd better return, if the condition is not met. The next if would be not needed then and the variable definitions above could be moved after this if.

129 ↗

(On Diff #44847)

We should check whether this creates any false positives.

[xazax.hun]

• Address a review comment.
• Fix an assertion failure for methods that has non-identifier name e.g.: destructors.
• Ignore qualifiers.

[xazax.hun]

In D16179#326746, @alexfh wrote:

... What about a configuration option to also report near misses when only a qualifier is missing?

Might be a useful thing. We should first check if it makes sense to always ignore a qualifier.

In this iteration of the patch I made the checker ignore the qualifiers. I did not get any results from running it on clang and llvm.

clang-tidy/misc/VirtualNearMissCheck.cpp

126–127 ↗

(On Diff #45022)

This might not be representative, but I did not get any results on the LLVM codebase.

[congliu]

• Ignore qualifiers.

I don't think we should ignore qualifiers. Please see my inline comment for line 52 of the test file.

clang-tidy/misc/VirtualNearMissCheck.cpp

240 ↗

(On Diff #45022)

NamedDecl::getName() directly returns a StringRef. Why using "getNameAsString()"?

test/clang-tidy/misc-virtual-near-miss.cpp

52 ↗

(On Diff #45022)

If a function in derived class has a same name but different cv-qualifiers as a function in base class, they are not regarded as overriding. For example,

class Mother{ 
  virtual int method(int argc) const;
};
class Child : Mother{
  int method(int x);
};

In this case, Child::method does not overrides Mother::method, but hides it. So I think we should not warn for "methoe", because even if the programmer changes "methoe" to "method", it's not an overriding.

[xazax.hun]

clang-tidy/misc/VirtualNearMissCheck.cpp

240 ↗

(On Diff #45022)

Unfortunately getName will cause an assertion fail for methods that has a non-identifier name, such as destructors and overloaded operators.

test/clang-tidy/misc-virtual-near-miss.cpp

52 ↗

(On Diff #45022)

Sure, in order to override the method the developer also needs to make the method const in the child. But it is a common mistake to forget to add the qualifiers. The purpose of the change is to find those cases. I think it is more likely that the developer forgot to add the const qualifier than an intentional hiding of a virtual method.

[alexfh]

clang-tidy/misc/VirtualNearMissCheck.cpp

54–55 ↗

(On Diff #45022)

It takes a non-trivial effort to understand the equivalence of the comment and the condition. I think, pulling the negations one level up would make the condition read easier:

if (!(BaseReturnTy->isPointerType() && DerivedReturnTy->isPointerType()) &&
    !(BaseReturnTy->isReferenceType() && DerivedReturnTy->isReferenceType()))
  return;

Also, please move the definitions of the variables BTy, DTy, BRD, DRD after this if and merge them with their initialization.

240 ↗

(On Diff #45022)

Should we maybe exclude operators and destructors from this check? A typo in destructor name won't compile. Do you have an example of a case where the check could be useful in detecting a typo in the name of an overloaded operator?

It would be nice to avoid using the (more expensive) getNameAsString here.

[xazax.hun]

clang-tidy/misc/VirtualNearMissCheck.cpp

240 ↗

(On Diff #45022)

Destructors can not be mispelled. Overloaded operators however might be virtual, and the user might forget the qualifier and miss the override. Although that might be a very rare case. Do you think it is worth to exclude that case for performance? Operators might be problematic anyways, the edit distance tend to be low there.

[alexfh]

clang-tidy/misc/VirtualNearMissCheck.cpp

240 ↗

(On Diff #45022)

I'd leave operators alone until we find a realistic example where this check is able to detect a problem.

240 ↗

(On Diff #45022)

I'd leave operators alone until we find a realistic example where this check is able to detect a problem.

[LegalizeAdulthood]

clang-tidy/misc/VirtualNearMissCheck.cpp

54–55 ↗

(On Diff #45022)

IMO, it would be even better would be to extract a predicate function with an intention-revealing name.

[xazax.hun]

• Addressed review comments.

clang-tidy/misc/VirtualNearMissCheck.cpp

54–55 ↗

(On Diff #45022)

I could not come up with a good name, so I left it as it is. If you have something in mind, feel free to tell me.

[alexfh]

LG. Thanks!

clang-tools-extra/trunk/clang-tidy/misc/VirtualNearMissCheck.h

 public:
   VirtualNearMissCheck(StringRef Name, ClangTidyContext *Context)
       : ClangTidyCheck(Name, Context) {}
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 
 private:
   /// Check if the given method is possible to be overridden by some other
-  /// method.
+  /// method. Operators and destructors are excluded.
   ///
   /// Results are memoized in PossibleMap.
   bool isPossibleToBeOverridden(const CXXMethodDecl *BaseMD);
 
   /// Check if the given base method is overridden by some methods in the given
   /// derived class.
   ///
   /// Results are memoized in OverriddenMap.

clang-tools-extra/trunk/clang-tidy/misc/VirtualNearMissCheck.cpp

static bool checkOverridingFunctionReturnType(const ASTContext *Context,
   if (DerivedReturnTy->isDependentType() || BaseReturnTy->isDependentType())
     return false;
 
   // Check if return types are identical.
   if (Context->hasSameType(DerivedReturnTy, BaseReturnTy))
     return true;
 
   /// Check if the return types are covariant.
-  /// BTy is the class type in return type of BaseMD. For example,
-  ///    B* Base::md()
-  /// While BRD is the declaration of B.
-  QualType BTy, DTy;
-  const CXXRecordDecl *BRD, *DRD;
 
   // Both types must be pointers or references to classes.
-  if (const auto *DerivedPT = DerivedReturnTy->getAs<PointerType>()) {
-    if (const auto *BasePT = BaseReturnTy->getAs<PointerType>()) {
-      DTy = DerivedPT->getPointeeType();
-      BTy = BasePT->getPointeeType();
-    }
-  } else if (const auto *DerivedRT = DerivedReturnTy->getAs<ReferenceType>()) {
-    if (const auto *BaseRT = BaseReturnTy->getAs<ReferenceType>()) {
-      DTy = DerivedRT->getPointeeType();
-      BTy = BaseRT->getPointeeType();
-    }
-  }
-
-  // The return types aren't either both pointers or references to a class type.
-  if (DTy.isNull())
+  if (!(BaseReturnTy->isPointerType() && DerivedReturnTy->isPointerType()) &&
+      !(BaseReturnTy->isReferenceType() && DerivedReturnTy->isReferenceType()))
     return false;
 
-  DRD = DTy->getAsCXXRecordDecl();
-  BRD = BTy->getAsCXXRecordDecl();
+  /// BTy is the class type in return type of BaseMD. For example,
+  ///    B* Base::md()
+  /// While BRD is the declaration of B.
+  QualType DTy = DerivedReturnTy->getPointeeType();
+  QualType BTy = BaseReturnTy->getPointeeType();
+
+  const CXXRecordDecl *DRD = DTy->getAsCXXRecordDecl();
+  const CXXRecordDecl *BRD = BTy->getAsCXXRecordDecl();
   if (DRD == nullptr || BRD == nullptr)
     return false;
 
   if (DRD == BRD)
     return true;
 
   if (!Context->hasSameUnqualifiedType(DTy, BTy)) {
     // Begin checking whether the conversion from D to B is valid.
static bool checkOverridingFunctionReturnType(const ASTContext *Context,
   // The class type D should have the same cv-qualification as or less
   // cv-qualification than the class type B.
   if (DTy.isMoreQualifiedThan(BTy))
     return false;
 
   return true;
 }
 
+/// \returns decayed type for arrays and functions.
+static QualType getDecayedType(QualType Type) {
+  if (const auto *Decayed = Type->getAs<DecayedType>())
+    return Decayed->getDecayedType();
+  return Type;
+}
+
 /// \returns true if the param types are the same.
 static bool checkParamTypes(const CXXMethodDecl *BaseMD,
                             const CXXMethodDecl *DerivedMD) {
   unsigned NumParamA = BaseMD->getNumParams();
   unsigned NumParamB = DerivedMD->getNumParams();
   if (NumParamA != NumParamB)
     return false;
 
   for (unsigned I = 0; I < NumParamA; I++) {
-    if (BaseMD->getParamDecl(I)->getType() !=
-        DerivedMD->getParamDecl(I)->getType())
+    if (getDecayedType(BaseMD->getParamDecl(I)->getType()) !=
+        getDecayedType(DerivedMD->getParamDecl(I)->getType()))
       return false;
   }
   return true;
 }
 
 /// \returns true if derived method can override base method except for the
 /// name.
 static bool checkOverrideWithoutName(const ASTContext *Context,
                                      const CXXMethodDecl *BaseMD,
                                      const CXXMethodDecl *DerivedMD) {
-  if (BaseMD->getTypeQualifiers() != DerivedMD->getTypeQualifiers())
-    return false;
-
   if (BaseMD->isStatic() != DerivedMD->isStatic())
     return false;
 
-  if (BaseMD->getAccess() != DerivedMD->getAccess())
-    return false;
-
   if (BaseMD->getType() == DerivedMD->getType())
     return true;
 
   // Now the function types are not identical. Then check if the return types
   // are covariant and if the param types are the same.
   if (!checkOverridingFunctionReturnType(Context, BaseMD, DerivedMD))
     return false;
   return checkParamTypes(BaseMD, DerivedMD);

 bool VirtualNearMissCheck::isPossibleToBeOverridden(
     const CXXMethodDecl *BaseMD) {
   std::string Id = generateMethodId(BaseMD);
   auto Iter = PossibleMap.find(Id);
   if (Iter != PossibleMap.end())
     return Iter->second;
 
   bool IsPossible = !BaseMD->isImplicit() && !isa<CXXConstructorDecl>(BaseMD) &&
-                    BaseMD->isVirtual();
+                    !isa<CXXDestructorDecl>(BaseMD) && BaseMD->isVirtual() &&
+                    !BaseMD->isOverloadedOperator();
   PossibleMap[Id] = IsPossible;
   return IsPossible;
 }
 
 bool VirtualNearMissCheck::isOverriddenByDerivedClass(
     const CXXMethodDecl *BaseMD, const CXXRecordDecl *DerivedRD) {
   auto Key = std::make_pair(generateMethodId(BaseMD),
                             DerivedRD->getQualifiedNameAsString());
bool VirtualNearMissCheck::isOverriddenByDerivedClass(
   OverriddenMap[Key] = IsOverridden;
   return IsOverridden;
 }
 
 void VirtualNearMissCheck::registerMatchers(MatchFinder *Finder) {
   if (!getLangOpts().CPlusPlus)
     return;
 
-  Finder->addMatcher(cxxMethodDecl(unless(anyOf(isOverride(), isImplicit(),
-                                                cxxConstructorDecl())))
-                         .bind("method"),
-                     this);
+  Finder->addMatcher(
+      cxxMethodDecl(unless(anyOf(isOverride(), isImplicit(),
+                                 cxxConstructorDecl(), cxxDestructorDecl())))
 }
 
 void VirtualNearMissCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *DerivedMD = Result.Nodes.getNodeAs<CXXMethodDecl>("method");
-  assert(DerivedMD != nullptr);
+  assert(DerivedMD);
 
   if (DerivedMD->isStatic())
     return;
 
+  if (DerivedMD->isOverloadedOperator())
+    return;
+
   const ASTContext *Context = Result.Context;
 
   const auto *DerivedRD = DerivedMD->getParent();
 
   for (const auto &BaseSpec : DerivedRD->bases()) {
     if (const auto *BaseRD = BaseSpec.getType()->getAsCXXRecordDecl()) {
       for (const auto *BaseMD : BaseRD->methods()) {
         if (!isPossibleToBeOverridden(BaseMD))

clang-tools-extra/trunk/test/clang-tidy/misc-virtual-near-miss.cpp

 // RUN: %check_clang_tidy %s misc-virtual-near-miss %t
 
 struct Base {
   virtual void func();
   virtual void gunk();
+  virtual ~Base();
+  virtual Base &operator=(const Base &);
 };
 
 struct Derived : Base {
   // Should not warn "do you want to override 'gunk'?", because gunk is already
   // overriden by this class.
   virtual void funk();
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: method 'Derived::funk' has a similar name and the same signature as virtual method 'Base::func'; did you mean to override it? [misc-virtual-near-miss]
 
   void func2();
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: method 'Derived::func2' has {{.*}} 'Base::func'
 
   void func22(); // Should not warn.
 
   void gunk(); // Should not warn: gunk is override.
 
   void fun();
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: method 'Derived::fun' has {{.*}} 'Base::func'
+
+  Derived &operator==(const Base &); // Should not warn: operators are ignored.
 };
 
 class Father {
 public:
   Father();
   virtual void func();
   virtual Father *create(int i);
   virtual Base &&generate();
 };
 
 class Mother {
 public:
   Mother();
   static void method();
   virtual int method(int argc, const char **argv);
   virtual int method(int argc) const;
+  virtual int decay(const char *str);
 };
 
 class Child : private Father, private Mother {
 public:
   Child();
 
   virtual void func2();
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: method 'Child::func2' has {{.*}} 'Father::func'
 
   int methoe(int x, char **strs); // Should not warn: parameter types don't match.
 
-  int methoe(int x); // Should not warn: method is not const.
+  int methoe(int x);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: method 'Child::methoe' has {{.*}} 'Mother::method'
 
   void methof(int x, const char **strs); // Should not warn: return types don't match.
 
   int methoh(int x, const char **strs);
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: method 'Child::methoh' has {{.*}} 'Mother::method'
 
   virtual Child *creat(int i);
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: method 'Child::creat' has {{.*}} 'Father::create'
 
   virtual Derived &&generat();
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: method 'Child::generat' has {{.*}} 'Father::generate'
 
+  int decaz(const char str[]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: method 'Child::decaz' has {{.*}} 'Mother::decay'
+
 private:
-  void funk(); // Should not warn: access qualifers don't match.
+  void funk();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: method 'Child::funk' has {{.*}} 'Father::func'
 };