clang-tidy's misc-use-override checker should not touch template classes which inherit from their template argument

[Quuxplusone]

Bugzilla Link	PR24496
Status	NEW
Importance	P normal
Reported by	Ehsan Akhgari [:ehsan] (ehsan.akhgari@gmail.com)
Reported on	2015-08-19 07:45:54 -0700
Last modified on	2015-08-19 11:53:21 -0700
Version	unspecified
Hardware	PC All
CC	alexfh@google.com, djasper@google.com, klimek@google.com
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

See the following program, for example:

struct B {
  virtual void f() = 0;
};
struct OB {
  virtual void f() = 0;
  virtual ~OB();
};

template<class B>
struct D:B {
  void f();
  ~D();
};

D<B> d;
D<OB> od;

Right now clang-tidy rewrites both functions in D as override, which makes the
program invalid, since ~B is not virtual.

I think in this case, it's best to leave the D template alone, since rewriting
any of its methods can cause similar bugs.  I'm trying to write a patch for it,
but I'm stuck on figuring out how to detect that a base class is a template
argument.  Here is what I have so far:

diff --git a/clang-tidy/misc/UseOverrideCheck.cpp b/clang-
tidy/misc/UseOverrideCheck.cpp
index ec71d1b..c1927a2 100644
--- a/clang-tidy/misc/UseOverrideCheck.cpp
+++ b/clang-tidy/misc/UseOverrideCheck.cpp
@@ -61,12 +61,27 @@ void UseOverrideCheck::check(const MatchFinder::MatchResult
&Result) {
   if (!Result.Context->getLangOpts().CPlusPlus11)
     return;

-  const FunctionDecl *Method = Result.Nodes.getStmtAs<FunctionDecl>("method");
+  const CXXMethodDecl *Method =
Result.Nodes.getStmtAs<CXXMethodDecl>("method");
   const SourceManager &Sources = *Result.SourceManager;
-
   assert(Method != nullptr);
+
+  // If this is a template inheriting from a template argument, be conservative
+  // and do not change the method.
+  const CXXRecordDecl *Record = Method->getParent();
+  const ClassTemplateSpecializationDecl *Spec =
dyn_cast<ClassTemplateSpecializationDecl>(Record);
+  if (Spec) {
+    const CXXRecordDecl *Template = Spec->getSpecializedTemplate()-
>getTemplatedDecl();
+    for (auto Base : Template->bases()) {
+      // This incorrectly filters things such as:
+      //   template<class T> struct D : B<T> {};
+      if (Base.getType()->isInstantiationDependentType()) {
+        return;
+      }
+    }
+  }
+
   if (Method->getInstantiatedFromMemberFunction() != nullptr)
-    Method = Method->getInstantiatedFromMemberFunction();
+    Method = dyn_cast<CXXMethodDecl>(Method-
>getInstantiatedFromMemberFunction());

   if (Method->isImplicit() || Method->getLocation().isMacroID() ||
       Method->isOutOfLine())

Any pointers appreciated!

[Quuxplusone]

Do you have a real-life use case for this?

[Quuxplusone]

(In reply to comment #1)
> Do you have a real-life use case for this?

Yes.  This bug breaks this code in Mozilla: <http://mxr.mozilla.org/mozilla-
central/source/dom/media/systemservices/MediaParent.h?rev=9e4579598ba8#57>.
Here, Parent is instantiated with two types, one which has a virtual
destructor, and one without one.

[Quuxplusone]

But is there a good reason for NonE10s not to have a virtual destructor? C++ compilers will usually warn you about that.

[Quuxplusone]

(In reply to comment #3)
> But is there a good reason for NonE10s not to have a virtual destructor? C++
> compilers will usually warn you about that.

We never delete one of these objects through a NonE10s pointer, but I guess we
could add a virtual destructor to NonE10s.  However, I still think that the
rewrite of the code in comment 0 is wrong even for f(), since the base class
type may not have a virtual f().

[Quuxplusone]

I understand. But I am not convinced that never using override just because there is some base class depending on a template parameter is the right solution. I think the more common use case will still be that you'd want this override.

Not using the override can be just as wrong. People still might want to learn that they have mis-typed some method name or maybe even that they are instantiating this class with an incompatible type name.

In truth, I to some extent consider this construct a flaw in the language specification. There is simply no right thing for clang-tidy to do. Thus, we should make it do the right thing in the majority of cases. So far, I am still convinced that adding override is better in the majority of cases. I have yet to see a use case where people use this pattern deliberately in real-life code.

[Quuxplusone]

OK, fair enough!