[clang-tidy] Exclude template instantiations in modernize-use-override

[PiotrZSL]

Summary

Added IgnoreTemplateInstantiations option to modernize-use-override
check. Allows to ignore virtual function overrides that are part of
template instantiations. This can be useful in cases where the use
of the "override" keyword is not appropriate or causes issues with
template specialization.

Fixes #38276.

[njames93]

I'm not sure this is really a great improvement.
If the override candidate originates in a templated base class I think we should still warn

struct Foo {
  virtual void foo();
};

template <typeame T>
struct DirectBase : T {
  virtual void foo();
};

DirectBase<Foo> Y; // This instantiation should not warn about the above declaration.

template<typename T>
struct Bar {
  virtual void bar();
};

template <typename T>
struct TemplateBase : Bar<T> {
  virtual void bar();
};

TemplateBase<int> Y; // This instantiation should warn about the above declaration.
                     // Or potentially configurable to either warn or ignore this case.

[PiotrZSL]

Problem is that you may have specialization of Bar class wihtout bar base, and that specialization can be local to some compilation unit.
Like:

template<>
struct Bar<int> {
    void bar(); // function wihout virtual or no function at all.
};

And now your fix (override) won't compile.

Only configuration option possible to be done, is to check template instantiations or not, but this going to produce false-positives.
And probably we don't want false-positives in this check.

[njames93]

In D147924#4255209, @PiotrZSL wrote:

Problem is that you may have specialization of Bar class wihtout bar base, and that specialization can be local to some compilation unit.
Like:

template<>
struct Bar<int> {
    void bar(); // function wihout virtual or no function at all.
};

And now your fix (override) won't compile.

Only configuration option possible to be done, is to check template instantiations or not, but this going to produce false-positives.
And probably we don't want false-positives in this check.

My idea was to not check any template instantiations, instead look for template definitions with a template dependent base class that has potential override candidates.

I'd argue a (likely very rare) false positive that would prevent compilation is better than not diagnosing and then potentially(due to a later refactor) getting a near miss override candidate going undiagnosed and breaking the codebase.
It could also be nicer to just emit a warning with no fix, or a fix added to a note explaining the situation. (notes fixes aren't automatically applied when running clang tidy(and clang) for this reason.

Given the contention that is why i suggested this should just be user configurable.

[PiotrZSL]

Introduce option IgnoreTemplateInstantiations.

[carlosgalvezp]

Looks good, some small comments.

clang-tools-extra/test/clang-tidy/checkers/modernize/use-override-templates.cpp

15 ↗

(On Diff #512529)

I think it's a bit more readable to write a normal comment explaining that these functions should not trigger a warning. Since you have a least one positive CHECK-MESSAGES below then the check should no longer complain that there aren't any.

16 ↗

(On Diff #512529)

I believe I saw a review comment in another patch that these are not needed?

[PiotrZSL]

Remove not needed comments from tests, and added new comments

[carlosgalvezp]

clang-tools-extra/test/clang-tidy/checkers/modernize/use-override-templates.cpp

46 ↗

(On Diff #513866)

Same as above, start/end markers seem to not be needed.

[PiotrZSL]

Remove of fix start/end marker.

clang-tools-extra/test/clang-tidy/checkers/modernize/use-override-templates.cpp

46 ↗

(On Diff #513866)

I will remove it, but note that If I do that, test will still pass if I comment out removal of virtual keyword in code.

[carlosgalvezp]

clang-tools-extra/test/clang-tidy/checkers/modernize/use-override-templates.cpp

46 ↗

(On Diff #513866)

That's a good point, I didn't consider that. It's important that the test fails if that part is removed, so you are right and the markers are indeed needed. Sorry for the confusion! I can approve now, feel free to bring the markers back before landing.

clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp

 void UseOverrideCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "IgnoreDestructors", IgnoreDestructors);
   Options.store(Opts, "AllowOverrideAndFinal", AllowOverrideAndFinal);
   Options.store(Opts, "OverrideSpelling", OverrideSpelling);
   Options.store(Opts, "FinalSpelling", FinalSpelling);
 }
 
 void UseOverrideCheck::registerMatchers(MatchFinder *Finder) {
-  if (IgnoreDestructors)
-    Finder->addMatcher(
-        cxxMethodDecl(isOverride(), unless(cxxDestructorDecl())).bind("method"),
-        this);
-  else
-    Finder->addMatcher(cxxMethodDecl(isOverride()).bind("method"), this);
+
+  auto IgnoreDestructorMatcher =
+      IgnoreDestructors ? cxxMethodDecl(unless(cxxDestructorDecl()))
+                        : cxxMethodDecl();
+      cxxMethodDecl(isOverride(),
+                    unless(ast_matchers::isTemplateInstantiation()),
+                    IgnoreDestructorMatcher)
+          .bind("method"),
 }
 
 // Re-lex the tokens to get precise locations to insert 'override' and remove
 // 'virtual'.
 static SmallVector<Token, 16>
 parseTokens(CharSourceRange Range, const MatchFinder::MatchResult &Result) {
   const SourceManager &Sources = *Result.SourceManager;
   std::pair<FileID, unsigned> LocInfo =

clang-tools-extra/docs/ReleaseNotes.rst

- In :doc:`modernize-use-default-member-init
   constructors toward hand written constructors so that they are skipped if more
   than one exists.
 
 - Fixed false positive in :doc:`modernize-use-equals-default
   <clang-tidy/checks/modernize/use-equals-default>` check for special member
   functions containing macros or preprocessor directives, and out-of-line special
   member functions in unions.
 
+- Fixed false positive in :doc:`modernize-use-override
+  <clang-tidy/checks/modernize/use-override>` check that occurred when warnings
+  were generated for template classes that were valid for some instantiations
+  but potentially invalid for others.
+
 - Fixed reading `HungarianNotation.CString.*` options in
   :doc:`readability-identifier-naming
   <clang-tidy/checks/readability/identifier-naming>` check.
 
 - Renamed `HungarianNotation.CString` options `CharPrinter` and
   `WideCharPrinter` to `CharPointer` and `WideCharPointer` respectively in
   :doc:`readability-identifier-naming
   <clang-tidy/checks/readability/identifier-naming>` check.

clang-tools-extra/test/clang-tidy/checkers/modernize/use-override.cpp

 
 // Tests for templates.
 template <typename T> struct TemplateBase {
   virtual void f(T t);
 };
 
 template <typename T> struct DerivedFromTemplate : public TemplateBase<T> {
   virtual void f(T t);
-  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
-  // CHECK-FIXES: {{^}}  void f(T t) override;
+  // CHECK-MESSAGES-NOT: :[[@LINE-1]]:16: warning: prefer using
+  // CHECK-FIXES-NOT: {{^}}  void f(T t) override;
 };
 void f() { DerivedFromTemplate<int>().f(2); }
 
 template <class C>
 struct UnusedMemberInstantiation : public C {
   virtual ~UnusedMemberInstantiation() {}
-  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: prefer using
-  // CHECK-FIXES: {{^}}  ~UnusedMemberInstantiation() override {}
+  // CHECK-MESSAGES-NOT: :[[@LINE-1]]:11: warning: prefer using
+  // CHECK-FIXES-NOT: {{^}}  ~UnusedMemberInstantiation() override {}
 };
 struct IntantiateWithoutUse : public UnusedMemberInstantiation<Base> {};
 
 struct Base2 {
   virtual ~Base2() {}
   virtual void a();
 };
 
struct TryStmtAsBody : public Base {
   // CHECK-FIXES: {{^}}  void a() override try
   { b(); } catch(...) { c(); }
 
   virtual void d() try
   // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
   // CHECK-FIXES: {{^}}  void d() override try
   { e(); } catch(...) { f(); }
 };
+
+namespace PR38276 {
+  struct Base {
+    virtual void foo();
+  };
+
+  struct Base2 {
+    virtual void foo2();
+  };
+
+  template<typename T>
+  struct Derived : T {
+    virtual void foo();
+    // CHECK-MESSAGES-NOT: :[[@LINE-1]]:18: warning: prefer using 'override' or (rarely) 'final' instead of 'virtual' [cppcoreguidelines-explicit-virtual-functions,modernize-use-override]
+    // CHECK-FIXES-NOT: {{^}}    void foo() override;{{$}}
+    virtual void foo2();
+    // CHECK-MESSAGES-NOT: :[[@LINE-1]]:18: warning: prefer using 'override' or (rarely) 'final' instead of 'virtual' [cppcoreguidelines-explicit-virtual-functions,modernize-use-override]
+    // CHECK-FIXES-NOT: {{^}}    void foo2() override;{{$}}
+  };
+
+  void test() {
+    Derived<Base> b;
+    Derived<Base2> b2;
+  }
+
+  template<typename T>
+  struct BaseS {
+    virtual void boo();
+  };
+
+  template<>
+  struct BaseS<int> {
+    virtual void boo2();
+  };
+
+  struct BaseU {
+    virtual void boo3();
+  };
+
+  template<typename T>
+  struct Derived2 : BaseS<T>, BaseU {
+    virtual void boo();
+    // CHECK-MESSAGES-NOT: :[[@LINE-1]]:18: warning: prefer using 'override' or (rarely) 'final' instead of 'virtual' [cppcoreguidelines-explicit-virtual-functions,modernize-use-override]
+    // CHECK-FIXES-NOT: {{^}}    void boo() override;{{$}}
+    virtual void boo2();
+    // CHECK-MESSAGES-NOT: :[[@LINE-1]]:18: warning: prefer using 'override' or (rarely) 'final' instead of 'virtual' [cppcoreguidelines-explicit-virtual-functions,modernize-use-override]
+    // CHECK-FIXES-NOT: {{^}}    void boo2() override;{{$}}
+    virtual void boo3();
+    // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: prefer using 'override' or (rarely) 'final' instead of 'virtual' [cppcoreguidelines-explicit-virtual-functions,modernize-use-override]
+    // CHECK-FIXES: {{^}}    void boo3() override;{{$}}
+  };
+
+}