[clang-tidy] Fix false positives in cppcoreguidelines-virtual-class-destructor

[carlosgalvezp]

Summary

Incorrectly triggers for template classes that inherit from a base class that has virtual destructor, as well as for forward declarations.
Any class inheriting from a base that has a virtual destructor will have their destructor also virtual, as per the Standard, so there's no need for them to explicitly declare it as virtual.
https://timsong-cpp.github.io/cppwp/n4140/class.dtor#9

If a class has a base class with a virtual destructor, its destructor (whether user- or implicitly-declared) is virtual.

Added unit test to prevent regression.
Fixes https://bugs.llvm.org/show_bug.cgi?id=51912

[carlosgalvezp]

Added additional test case for forward declarations, which is also broken on main.

[carlosgalvezp]

Rename Class to Struct

[carlosgalvezp]

-Remove "public" when checking for base virtual destructor, it's not relevant.
-More consistent naming.
-Fix formatting.

[aaron.ballman]

Removing myself as a reviewer (I've generally been stepping back from reviewing C++ Core Guideline checks given the continued lack of reasonable consideration for enforcement in the rules), but I did have some drive-by comments.

clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp

26

I'm confused as to why this is necessary -- this AST matcher calls through to CXXMethodDecl::isVirtual() which is different from isVirtualAsWritten() and should already account for inheriting the virtual specifier.

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp

206

Rather than use a comment, we typically put the test cases into a namespace. e.g.,

namespace PR51912 {
// tests cases for that bug live here
} // namespace PR51912

216–218

I think there are more interesting template test cases that should be checked.

// What to do when the derived type is definitely polymorphic
// but the base class may or may not be?
template <typename Ty>
struct Derived : Ty {
  virtual void func();
};

struct S {};
struct  T { virtual ~T(); };

void instantiate() {
  Derived<S> d1; // Diagnose
  Derived<T> d2; // Don't diagnose
}

220

[carlosgalvezp]

clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp

26

In the Bug report, @whisperity mentioned this problem could be somewhere else:

Nevertheless, if you check the AST for the test code at http://godbolt.org/z/6MqG4Kb85, we can see that the derived destructor is in fact not marked virtual. So it's Sema which does not give this flag to the AST-clients properly.

I don't even know what Sema is so I don't really know how to fix it at that level, and I wonder if it would break other things.

If anyone has some time to walk me through where the fix should go I'm happy to try it out!

[carlosgalvezp]

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp

216–218

Very interesting example.

The problem is that the diagnostic is shown where Derived is, not where the template is instantiated. How to go about that?

Seems like more testing and better diagnostics are needed to lower the amount of false positives, I wonder if we should disable the test meanwhile?

[whisperity]

clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp

26

Sema is the Clang module responsible for semantic analysis, hence the name. It is an egregiously complex and huge class (you know something's weird when a header of 80k lines is implemented over like 20 separate CPP files...) that is basically responsible for building the AST.

It was just a hunch from my side because I went into Godbolt to try seeing why the matching logic would fail!

26

Sadly I'm a bit short on time nowadays due to university and bureaucracy, but remember that either Clang-Query is your friend (available on Godbolt, with visual highlight of the matched things back in the source code!) and you can sandbox match expressions there (match ... for matching, let X ... for saving ... into X to reuse later), or you could do AnyASTNode->dump() in the code while development to observe what the node is on the output.

36

Nit: ProblematicRecord might be a better, and certainly shorter, name, while meaning the same thing.

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp

206

Nit: "PR" could be mistaken for Pull Request especially since the project now lives on GitHub (even though we don't use pull requests (yet?)), so maybe explicitly saying Bugzilla_51912 or something like that instead would be more obvious. And it's worthy to put the link in, still.

208

What does this prefix do? (I've never seen this before! 😮)

216–218

First, let's try to see if printing, to the user, the matched record, prints Derived<S> instead of just Derived, irrespective of the location. If the matcher matched the instantiation, the printing/formatting logic should really pick that up.

It would be very hard to get to the VarDecl with the specific type if your matcher logic starts the top-level matching from the type definitions (cxxRecordDecl(...)).

If we really want to put some sort of a diagnostic at the location at the places where the type is used, it could be done with another pass over the AST. However, that has an associated runtime cost, and also could create bazillions of note: used here-esque messages... But certainly doable. I believe this is typeLoc, but typeLoc is always a thing I never understand and every time I may end up using it it takes me a lot of reading to understand it for a short time to use it.

---

If the previous step failed, you could still go around in a much more convoluted way:

However, there is something called a ClassTemplateSpecializationDecl (see the AST for @aaron.ballman's example here: http://godbolt.org/z/9qd7d1rs6), which surely should have an associated matcher.

| |-ClassTemplateSpecializationDecl <line:1:1, line:4:1> line:2:8 struct Derived definition
| | |-DefinitionData polymorphic literal has_constexpr_non_copy_move_ctor can_const_default_init
| | | |-DefaultConstructor exists non_trivial constexpr defaulted_is_constexpr
| | | |-CopyConstructor simple non_trivial has_const_param implicit_has_const_param
| | | |-MoveConstructor exists simple non_trivial
| | | |-CopyAssignment simple non_trivial has_const_param implicit_has_const_param
| | | |-MoveAssignment exists simple non_trivial
| | | `-Destructor simple irrelevant trivial
| | |-public 'S':'S'
| | |-TemplateArgument type 'S'
| | | `-RecordType 'S'
| | |   `-CXXRecord 'S'
| | |-CXXRecordDecl prev 0x55ebcec4e600 <col:1, col:8> col:8 implicit struct Derived

The location for the "template specialisation" is still the location of the primary template (as it is not an explicit specialisation), but at least somehow in the AST the information of "Which type was this class template instantiated with?" (S) is stored, so it is very likely that you can either match on this directly, or if you can't match that way, you could match all of these ClassTemplateSpecializationDecls and check if their type parameter matches a ProblematicClassOrStruct.

Of course, this becomes extra nasty the moment we have multiple template parameters, or nested stuff, template templates (brrrr...).

This will still not give you the location of the variable definition, but at least you would have in your hand the template specialisation/instantiation instance properly.

[aaron.ballman]

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp

216–218

Oh, I may have caused some confusion with the comments in my example, sorry for that! I was imagining the diagnostics would be emitted on the line with the class declaration, but I commented on which instantiation I thought should diagnose. Being more explicit with what I was thinking:

// What to do when the derived type is definitely polymorphic
// but the base class may or may not be?
template <typename Ty>
struct Derived : Ty { // Diagnostic emitted here
  virtual void func();
};

struct S {};
struct  T { virtual ~T(); };

void instantiate() {
  Derived<S> d1; // Instantiation causes a diagnostic above
  Derived<T> d2; // Instantiation does not cause a diagnostic above
}

[carlosgalvezp]

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp

216–218

No worries, that was my understanding as well :) I was more thinking about how to tag the expected error/no-error with CHECK-MESSAGES. I would need to say that the same line both should get an error and not get an error.

Maybe I will solve it with some duplication creating another class template.

Thanks for the comments, will update asap!

[carlosgalvezp]

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp

208

I got the tip from @aaron.ballman :) Most likely it's not technically needed (the tests would catch unexpected warnings) but it expresses more clearly the expectations.

[carlosgalvezp]

Added additional required tests.

I might take some more time to fix the matcher since I need to get acquainted with AST, clang-query, etc, etc

[carlosgalvezp]

By the way, there is a compiler warning that is identical to this check. Wouldn't it make sense to just make both checks identical? Otherwise there's risk that they conflict one another:

https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaDeclCXX.cpp#L6726

[carlosgalvezp]

So for some reason, in Sema, the following returns True:

dtor->isVirtual()

However the clang-tidy matcher :

has(cxxDestructorDecl(isPublic(), isVirtual())),

Doesn't match this, and that's why we get the FPs. Why would that be, isn't isVirtual doing the same as dtor->isVirtual()?

[carlosgalvezp]

Ahh, it seems that for templates, the class has this AST:

|-ClassTemplateDecl <line:6:1, line:10:1> line:7:8 Derived
| |-TemplateTypeParmDecl <line:6:11, col:20> col:20 typename depth 0 index 0 T
| |-CXXRecordDecl <line:7:1, line:10:1> line:7:8 struct Derived definition
| `-ClassTemplateSpecializationDecl <line:6:1, line:10:1> line:7:8 struct Derived definition

So the derived destructor only shows up in ClassTemplateSpecializationDecl - the CXXRecordDecl doesn't have it (therefore implicit public non-virtual) and that's why the check triggers.

[carlosgalvezp]

Moved the condition of public-virtual / protected-non-virtual to the check stage instead of the match stage. This way is more similar to the equivalent Sema check and it passes all the tests. Let me know what you think!

[whisperity]

In D110614#3032818, @carlosgalvezp wrote:

Moved the condition of public-virtual / protected-non-virtual to the check stage instead of the match stage. This way is more similar to the equivalent Sema check and it passes all the tests. Let me know what you think!

I'll let @aaron.ballman take a look at how well the test coverage increased, this is a drive-by comment from my part due to being a bit too busy. I'll look at this at a later point, hopefully still this week!

Either way, you could create your own local matcher predicate function and use it as a submatcher, so you can do the test during matching instead of checking.

Adding

AST_MATCHER(Decl, myFancyMatcher) {
  return Node.something();
}

to the code will give you myFancyMatcher(), which you can embed. First parameter is the node type you want to match on. Inside the { } you are essentially within a function's scope, so you can write arbitrary code, returning true is you want to match.

If you check the AST Matcher library header, it uses this same macro extensively. Perhaps you could check how the isVirtual matcher is implemented there?

[carlosgalvezp]

Created custom matcher and moved the logic from check to match stage. Documented why we can't simply match a CXXDestructorDecl.

[carlosgalvezp]

Thanks for the feedback @whisperity , I'm learning so many new things :)

[carlosgalvezp]

Hi! Are there any more issues I should address? It would be nice to get it merged since it fixes quite a few FPs.

Tagging also @mgartmann as original author of this check in case he wants to comment.

Anyway this check will need to be extended in the future, since the C++ Core Guidelines has added a new bullet in their "Enforcement" section:
https://github.com/isocpp/CppCoreGuidelines/commit/e44a9fcbd40923e9d5d342e444cf8a811e4a3eae

[whisperity]

In D110614#3038519, @carlosgalvezp wrote:

Hi! Are there any more issues I should address? It would be nice to get it merged since it fixes quite a few FPs.

I'm not seeing anything immediate to add. I like the proper printing with the template parameters. 🙂
Just in case: add another set of tests where the template instantiation (both diagnosed and not diagnosed) is behind a typedef, just to see how well the diagnostic is printed and generated. Otherwise, I'm happy with this.

// Fixes https://bugs.llvm.org/show_bug.cgi?id=51912

I still think that maybe this whole fiasco is worth either a bug report against Sema on its own, or just a question-phrased mail to cfe-dev.

[mgartmann]

Please excuse my late reply! I have been on vacation for the last two weeks and didn't have the time to respond to this thread until now.

In D110614#3032760, @carlosgalvezp wrote:

So the derived destructor only shows up in ClassTemplateSpecializationDecl - the CXXRecordDecl doesn't have it (therefore implicit public non-virtual) and that's why the check triggers.

I came to the same conclusion with a quick troubleshoot. Honestly, I didn't expect SEMA to behave this way and to generate CXXRecordDecl nodes without any CXXDestructorDecl sub-node.
As a rather novice C++ programmer, I didn't have the described scenarios in mind when I initially created the check. Hence, the needed tests to find these false-positives beforehand were missing.
I would like to apologise to everyone for the trouble this has caused.

@carlosgalvezp Thanks a lot for putting your time and effort into fixing this bug!

In D110614#3038519, @carlosgalvezp wrote:

Anyway this check will need to be extended in the future, since the C++ Core Guidelines has added a new bullet in their "Enforcement" section:
https://github.com/isocpp/CppCoreGuidelines/commit/e44a9fcbd40923e9d5d342e444cf8a811e4a3eae

Thanks for pointing this out to me. Let me know if I can help you with this in any way. I would be available to implement the new enforcement from 2021-10-11 on.

Again, my dearest apologies for this bug.

[carlosgalvezp]

Thanks for your input @mgartmann ! No apologies needed, nothing is ever perfect on the first release :) Just wanted to check if you would agree with the fix or if there's some detail I've missed.

I'll add the tests asked by @whisperity

[carlosgalvezp]

Added test where an alias to the class template is created. @whisperity let me know if that's what you meant!

[carlosgalvezp]

Anything else I should address? Otherwise I'd need some help committing since I don't think I have access for that. Thanks!

[carlosgalvezp]

After more than 1 week without comments, I kindly ping reviewers @mgartmann @aaron.ballman @hokein @whisperity @njames93 to have a look and see if we can merge or is there something I should address. I have posted a question about Sema in cfe-dev, but didn't get any reply. I know the current solution is not ideal but at least it add tests and solves the problem. I'm happy to bring back a more "idiomatic" solution once the Sema issue has been investigated and solved (which could take a while).

Thanks a lot!

[aaron.ballman]

LGTM, unless @whisperity sees something else of concern.

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp

208

It's a feature of %check_clang_tidy from the test file -- it explicitly checks that no such message was produced in output (otherwise, if a diagnostic was emitted, we wouldn't fail the test unless it failed one of the CHECK lines).

[carlosgalvezp]

Thanks!! I'll wait for tomorrow morning (~8 PM here) to merge, to give some last minute opportunity for comments. Is this what I should monitor after merge to make sure nothing breaks?
https://lab.llvm.org/buildbot/#/console

[aaron.ballman]

In D110614#3067303, @carlosgalvezp wrote:

Thanks!! I'll wait for tomorrow morning (~8 PM here) to merge, to give some last minute opportunity for comments. Is this what I should monitor after merge to make sure nothing breaks?
https://lab.llvm.org/buildbot/#/console

SGTM! And yes, you'd monitor that (though I tend to use https://lab.llvm.org/buildbot/#/builders myself because the view works better for me personally).

clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp

 namespace clang {
 namespace tidy {
 namespace cppcoreguidelines {
 
 void VirtualClassDestructorCheck::registerMatchers(MatchFinder *Finder) {
   ast_matchers::internal::Matcher<CXXRecordDecl> InheritsVirtualMethod =
       hasAnyBase(hasType(cxxRecordDecl(has(cxxMethodDecl(isVirtual())))));
 
+  ast_matchers::internal::Matcher<CXXRecordDecl> InheritsVirtualDestructor =
+      hasAnyBase(hasType(cxxRecordDecl(has(cxxDestructorDecl(isVirtual())))));
+
   Finder->addMatcher(
       cxxRecordDecl(
           anyOf(has(cxxMethodDecl(isVirtual())), InheritsVirtualMethod),
           unless(anyOf(
+              InheritsVirtualDestructor,
               has(cxxDestructorDecl(isPublic(), isVirtual())),
               has(cxxDestructorDecl(isProtected(), unless(isVirtual()))))))
           .bind("ProblematicClassOrStruct"),
       this);
 }
 
 static CharSourceRange
 getVirtualKeywordRange(const CXXDestructorDecl &Destructor,
                        const SourceManager &SM, const LangOptions &LangOpts) {
   SourceLocation VirtualBeginLoc = Destructor.getBeginLoc();
   SourceLocation VirtualEndLoc = VirtualBeginLoc.getLocWithOffset(

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp

 // CHECK-FIXES: struct NonOverridingDerivedStruct : ProtectedNonVirtualBaseStruct {
 // CHECK-FIXES-NEXT: virtual ~NonOverridingDerivedStruct() = default;
 // CHECK-FIXES-NEXT: void m();
 // CHECK-FIXES-NEXT: };
 struct NonOverridingDerivedStruct : ProtectedNonVirtualBaseStruct {
   void m();
 };
 // inherits virtual method
+
+// The following two checks cover bug https://bugs.llvm.org/show_bug.cgi?id=51912
+// Forward declarations
+// CHECK-MESSAGES-NOT: :[[@LINE+1]]:8: warning: destructor of 'ForwardDeclaredStruct' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
+struct ForwardDeclaredStruct;
+
+struct ForwardDeclaredStruct : PublicVirtualBaseStruct {
+};
+
+// Templates
+// CHECK-MESSAGES-NOT: :[[@LINE+2]]:8: warning: destructor of 'TemplatedDerived' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
+template <typename T>
+struct TemplatedDerived : PublicVirtualBaseStruct {
+};
+
+// Templates need to be instantiated for diagnostics to show up
+void test_templates() {
+  TemplatedDerived<int> x;
+}