[cfe-dev] [clang-tidy][Sema][AST] Different behavior Sema and AST matcher

[Sam McCall via cfe-dev]

Sorry, should have done more digging first.

The matcher does match the main template class body as suspicious, but then
the check() callback immediately to look up a destructor and fails, so
class template bodies will never be warned on directly. Rather the false
positive happens when the template is instantiated.

If you look carefully at the AST dump, there are *two* declarations of the
specialization TemplatedDerived: a ClassTemplateSpecializationDecl, and a
CXXRecordDecl redeclaration nested within it: the injected-class-name. The
latter is what is being matched.
The injected-class-name is a simple redeclaration. It has no children of
its own, so it never matches has() which just refers to AST structure.
However if you do semantic lookups on it, it will find things in the "main"
declaration. It matches InheritsVirtualMethod, because hasAnyBase is a
semantic lookup. It doesn't match has(cxxDestructorDecl()) at all, but
getDestructor() does work.
This is why your patch fixes that case, but it's pretty indirect. It'd be
better to exclude the injected-class-name directly I suppose. Maybe by
excluding isImplicit()?

(Incidentally, I'm not sure why the derived class needs to be a template
specialization for hasAnyBase() to match the injected-class-name, so still
want to poke at this some more)

On Thu, Oct 28, 2021 at 9:57 PM Sam McCall <sammccall at google.com> wrote:

> My reading is:
>  - isVirtual() matcher indeed calls CXXMethodDecl::isVirtual
>  - CXXMethodDecl::isVirtual does what you expect for non-template code:
> has "virtual" specifier or overrides anything
>  - class template bodies don't contain implicit CXXDestructorDecls. These
> are only created when the class is instantiated, probably because in the
> general case that's when the analysis can happen. This is why the "unless"
> of your matcher does not apply.
>  - I'm confused about how your patch fixes this, because my reading of the
> code is that getDestructor() on the class template body should return null.
> Will dig into this a little.
>
> On Thu, Oct 28, 2021 at 9:05 PM David Blaikie <dblaikie at gmail.com> wrote:
>
>> On Thu, Oct 28, 2021 at 1:24 AM Carlos Galvez <carlosgalvezp at gmail.com>
>> wrote:
>>
>>> > What does the original (buggy) implementation use instead of
>>> cxxDestructorDecl::isVirtual?
>>>
>>> It used this matcher:
>>>
>>>       cxxRecordDecl(
>>>           anyOf(has(cxxMethodDecl(isVirtual())), InheritsVirtualMethod),
>>>           unless(anyOf(
>>>               has(cxxDestructorDecl(isPublic(), isVirtual())),
>>>               has(cxxDestructorDecl(isProtected(),
>>> unless(isVirtual()))))))
>>>           .bind("ProblematicClassOrStruct"),
>>>
>>> So the line:
>>>
>>>               has(cxxDestructorDecl(isPublic(), isVirtual())),
>>>
>>> Should have done the job, but it didn't.
>>>
>>
>> That's the bit I'm trying to ask about/understand - do you know why
>> "cxxDestructorDecl(isVirtual())" was inadequate, if
>> "cxxDestructorDecl::isVirtual() (from Sema)" is adequate? I would've
>> expected "cxxDestructorDecl(isVirtual())" to call
>> CXXDestructorDecl::isVirtual - but I guess it doesn't?
>>
>>
>>>
>>>
>>> On Mon, Oct 25, 2021 at 7:06 PM David Blaikie <dblaikie at gmail.com>
>>> wrote:
>>>
>>>> On Sun, Oct 10, 2021 at 6:46 AM Carlos Galvez via cfe-dev <
>>>> cfe-dev at lists.llvm.org> wrote:
>>>>
>>>>> Hi!
>>>>>
>>>>> As proposed by @whisperity I'm posting here to ask about a strange
>>>>> situation we found in the following bug fix:
>>>>>
>>>>> https://reviews.llvm.org/D110614
>>>>>
>>>>> We had a matcher to detect if a class has a public virtual destructor:
>>>>>
>>>>> has(cxxDestructorDecl(isPublic(), isVirtual())),
>>>>>
>>>>> However this didn't work for class templates, see details:
>>>>>
>>>>> https://reviews.llvm.org/D110614#3032760
>>>>>
>>>>> The workaround was to create a custom matcher that seems to do the
>>>>> same thing as the original matcher, but it actually works for templates
>>>>> because it calls cxxDestructorDecl::isVirtual() (from Sema), which
>>>>> correctly propagates the virtual specifier from the base class.
>>>>>
>>>>
>>>> What does the original (buggy) implementation use instead of
>>>> cxxDestructorDecl::isVirtual? If the original's just using the wrong query
>>>> for testing virtuality that seems like a reasonable thing to fix as a bug?
>>>>
>>>>
>>>>>
>>>>> Is this a concern, and should someone look into Sema/AST? (I'm
>>>>> unfortunately a n00b here). Or should we just merge the clang-tidy fix and
>>>>> move on?
>>>>>
>>>>> Thanks!
>>>>> _______________________________________________
>>>>> cfe-dev mailing list
>>>>> cfe-dev at lists.llvm.org
>>>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>>>>
>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20211028/b6421133/attachment.html>