[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

16

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.

17

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

47

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

47

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

47

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.h

bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
     return LangOpts.CPlusPlus11;
   }
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
   void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
 
 private:
   const bool IgnoreDestructors;
+  const bool IgnoreTemplateInstantiations;
   const bool AllowOverrideAndFinal;
   const StringRef OverrideSpelling;
   const StringRef FinalSpelling;
 };
 
 } // namespace clang::tidy::modernize
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USEOVERRIDECHECK_H

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

 
 using namespace clang::ast_matchers;
 
 namespace clang::tidy::modernize {
 
 UseOverrideCheck::UseOverrideCheck(StringRef Name, ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       IgnoreDestructors(Options.get("IgnoreDestructors", false)),
+      IgnoreTemplateInstantiations(
+          Options.get("IgnoreTemplateInstantiations", false)),
       AllowOverrideAndFinal(Options.get("AllowOverrideAndFinal", false)),
       OverrideSpelling(Options.get("OverrideSpelling", "override")),
       FinalSpelling(Options.get("FinalSpelling", "final")) {}
 
 void UseOverrideCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "IgnoreDestructors", IgnoreDestructors);
+  Options.store(Opts, "IgnoreTemplateInstantiations",
+                IgnoreTemplateInstantiations);
   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"),
+
+  auto IgnoreDestructorMatcher =
+      IgnoreDestructors ? cxxMethodDecl(unless(cxxDestructorDecl()))
+                        : cxxMethodDecl();
+  auto IgnoreTemplateInstantiationsMatcher =
+      IgnoreTemplateInstantiations
+          ? cxxMethodDecl(unless(ast_matchers::isTemplateInstantiation()))
+          : cxxMethodDecl();
+  Finder->addMatcher(cxxMethodDecl(isOverride(),
+                                   IgnoreTemplateInstantiationsMatcher,
+                                   IgnoreDestructorMatcher)
+                         .bind("method"),
                      this);
-  else
-    Finder->addMatcher(cxxMethodDecl(isOverride()).bind("method"), this);
 }
 
 // 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.
 
+- Improved :doc:`modernize-use-override
+  <clang-tidy/checks/modernize/use-override>` check with new
+  `IgnoreTemplateInstantiations` option to optionally ignore virtual function
+  overrides that are part of template instantiations.
+
 - 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/docs/clang-tidy/checks/modernize/use-override.rst

 
 Options
 -------
 
 .. option:: IgnoreDestructors
 
    If set to `true`, this check will not diagnose destructors. Default is `false`.
 
+.. option:: IgnoreTemplateInstantiations
+
+   If set to `true`, instructs this check to ignore virtual function overrides
+   that are part of template instantiations. Default is `false`.
+
 .. option:: AllowOverrideAndFinal
 
    If set to `true`, this check will not diagnose ``override`` as redundant
    with ``final``. This is useful when code will be compiled by a compiler with
    warning/error checking flags requiring ``override`` explicitly on overridden
    members, such as ``gcc -Wsuggest-override``/``gcc -Werror=suggest-override``.
    Default is `false`.
 

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

+// RUN: %check_clang_tidy %s modernize-use-override %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-override.IgnoreTemplateInstantiations, value: true}]}"
+
+struct Base {
+  virtual void foo();
+};
+
+struct Base2 {
+  virtual void foo2();
+};
+
+template<typename T>
+struct Derived : T {
+  // should not warn, comes from template instance
+  virtual void foo();
+  virtual void foo2();
+};
+
+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 {
+  // should not warn, comes from template instance
+  virtual void boo();
+  virtual void boo2();
+  // should warn, comes from non-template BaseU
+  virtual void boo3();
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using 'override' or (rarely) 'final' instead of 'virtual' [modernize-use-override]
+  // CHECK-FIXES: {{^  }}void boo3() override;
+};