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

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp:26
 
+  ast_matchers::internal::Matcher<CXXRecordDecl> InheritsVirtualDestructor =
+      hasAnyBase(hasType(cxxRecordDecl(has(cxxDestructorDecl(isVirtual())))));
----------------
carlosgalvezp wrote:
> aaron.ballman wrote:
> > 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.
> 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!
`Sema` is the Clang module responsible for **sema**ntic 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!


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp:26
 
+  ast_matchers::internal::Matcher<CXXRecordDecl> InheritsVirtualDestructor =
+      hasAnyBase(hasType(cxxRecordDecl(has(cxxDestructorDecl(isVirtual())))));
----------------
whisperity wrote:
> carlosgalvezp wrote:
> > aaron.ballman wrote:
> > > 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.
> > 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!
> `Sema` is the Clang module responsible for **sema**ntic 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!
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.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp:36
               has(cxxDestructorDecl(isProtected(), unless(isVirtual()))))))
           .bind("ProblematicClassOrStruct"),
       this);
----------------
Nit: `ProblematicRecord` might be a better, and certainly shorter, name, while meaning the same thing.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp:206
+
+// The following two checks cover bug https://bugs.llvm.org/show_bug.cgi?id=51912
+// Forward declarations
----------------
aaron.ballman wrote:
> 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
> ```
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.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp:208
+// Forward declarations
+// CHECK-MESSAGES-NOT: :[[@LINE+1]]:8: warning: destructor of 'ForwardDeclaredStruct' is public and non-virtual [cppcoreguidelines-virtual-class-destructor]
+struct ForwardDeclaredStruct;
----------------
What does this prefix do? (I've never seen this before! 😮)


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp:216-218
+template <typename T>
+struct TemplatedDerived : PublicVirtualBaseStruct {
+};
----------------
carlosgalvezp wrote:
> aaron.ballman wrote:
> > 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
> > }
> > ```
> > 
> 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?
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 `ClassTemplateSpecializationDecl`s 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 template`s (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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110614/new/

https://reviews.llvm.org/D110614