[clang-itdy] Simplify virtual near-miss check

[steveire]

Summary

Diagnose the problem in templates in the context of the template
declaration instead of in the context of all of the (possibly very many)
template instantiations.

[njames93]

What happens when TBase is an explicit specialization. In that situation a warning for the explicit specialization may be useful.

template <> struct TBase<float> {
  virtual void tfunt(T t);
};

clang-tools-extra/test/clang-tidy/checkers/bugprone-virtual-near-miss.cpp

49

Can this be left in to show no fix was applied.

[steveire]

Update

[steveire]

What happens when TBase is an explicit specialization.

It's not changed before or after this patch. This check is documented as changing code based on the derived classes based on typos of methods overridden from the base.

Your suggestion makes sense to me, but it is orthogonal to this patch.

clang-tools-extra/test/clang-tidy/checkers/bugprone-virtual-near-miss.cpp

49

After this patch it actually does apply the fixit. I can see why the fixit would have been skipped when it was based on the template instantiation (because a specialization could do unexpected things), but I don't see why it should be excluded when matching based on the template declaration.

[njames93]

Fair enough, LGTM.

[njames93]

Can you take a look at https://llvm.org/PR49330, I'm guessing this patch introduced this regression.

[ymandel]

In D96224#2581501, @njames93 wrote:

Can you take a look at https://llvm.org/PR49330, I'm guessing this patch introduced this regression.

+1 We've had to disable the check in the meantime.

[steveire]

Thanks for the heads-up. I've reverted it now.

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

 public:
   VirtualNearMissCheck(StringRef Name, ClangTidyContext *Context)
       : ClangTidyCheck(Name, Context) {}
   bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
     return LangOpts.CPlusPlus;
   }
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  llvm::Optional<TraversalKind> getCheckTraversalKind() const override {
+    return TK_IgnoreUnlessSpelledInSource;
+  }
 
 private:
   /// Check if the given method is possible to be overridden by some other
   /// method. Operators and destructors are excluded.
   ///
   /// Results are memoized in PossibleMap.
   bool isPossibleToBeOverridden(const CXXMethodDecl *BaseMD);
 

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

for (const CXXMethodDecl *DerivedMD : DerivedRD->methods()) {
     }
   }
   OverriddenMap[Key] = IsOverridden;
   return IsOverridden;
 }
 
 void VirtualNearMissCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(
-      cxxMethodDecl(
-          unless(anyOf(isOverride(), isImplicit(), cxxConstructorDecl(),
-                       cxxDestructorDecl(), cxxConversionDecl(), isStatic(),
-                       isOverloadedOperator())))
+      cxxMethodDecl(unless(anyOf(isOverride(), cxxConstructorDecl(),
+                                 cxxDestructorDecl(), cxxConversionDecl(),
+                                 isStatic(), isOverloadedOperator())))
           .bind("method"),
       this);
 }
 
 void VirtualNearMissCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *DerivedMD = Result.Nodes.getNodeAs<CXXMethodDecl>("method");
   assert(DerivedMD);
 
   const ASTContext *Context = Result.Context;
 
   const auto *DerivedRD = DerivedMD->getParent()->getDefinition();
   assert(DerivedRD);
 
   for (const auto &BaseSpec : DerivedRD->bases()) {
-    if (const auto *BaseRD = BaseSpec.getType()->getAsCXXRecordDecl()) {
+    const auto *BaseRD = BaseSpec.getType()->getAsCXXRecordDecl();
+    if (const auto *TST =
+            dyn_cast<TemplateSpecializationType>(BaseSpec.getType())) {
+      auto TN = TST->getTemplateName();
+      BaseRD =
+          dyn_cast<CXXRecordDecl>(TN.getAsTemplateDecl()->getTemplatedDecl());
+    }
+
+    if (BaseRD) {
       for (const auto *BaseMD : BaseRD->methods()) {
         if (!isPossibleToBeOverridden(BaseMD))
           continue;
 
         if (isOverriddenByDerivedClass(BaseMD, DerivedRD))
           continue;
 
         unsigned EditDistance = BaseMD->getName().edit_distance(
             DerivedMD->getName(), EditDistanceThreshold);
         if (EditDistance > 0 && EditDistance <= EditDistanceThreshold) {
           if (checkOverrideWithoutName(Context, BaseMD, DerivedMD)) {
             // A "virtual near miss" is found.
             auto Range = CharSourceRange::getTokenRange(
                 SourceRange(DerivedMD->getLocation()));
 
-            bool ApplyFix = !BaseMD->isTemplateInstantiation() &&
-                            !DerivedMD->isTemplateInstantiation();
-            auto Diag =
-                diag(DerivedMD->getBeginLoc(),
-                     "method '%0' has a similar name and the same signature as "
-                     "virtual method '%1'; did you mean to override it?")
                 << DerivedMD->getQualifiedNameAsString()
-                << BaseMD->getQualifiedNameAsString();
-            if (ApplyFix)
-              Diag << FixItHint::CreateReplacement(Range, BaseMD->getName());
+                << BaseMD->getQualifiedNameAsString()
+                << FixItHint::CreateReplacement(Range, BaseMD->getName());
           }
         }
       }
     }
   }
 }
 
 } // namespace bugprone
 } // namespace tidy
 } // namespace clang

clang-tools-extra/test/clang-tidy/checkers/bugprone-virtual-near-miss.cpp

 struct TBase {
   virtual void tfunc(T t);
 };
 
 template <typename T>
 struct TDerived : TBase<T> {
   virtual void tfunk(T t);
   // Should not apply fix for template.
-  // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: method 'TDerived<double>::tfunk' has {{.*}} 'TBase<double>::tfunc'
-  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: method 'TDerived<int>::tfunk' has {{.*}} 'TBase<int>::tfunc'
-  // CHECK-FIXES: virtual void tfunk(T t);
+  // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: method 'TDerived::tfunk' has {{.*}} 'TBase::tfunc'
+  // CHECK-FIXES: virtual void tfunc(T t);
 };
 
 TDerived<int> T1;
 TDerived<double> T2;
 
 // Should not fix macro definition
 #define MACRO1 void funcM()
 // CHECK-FIXES: #define MACRO1 void funcM()