recordDecl() AST matcher

[Aaron Ballman via cfe-commits]

On Fri, Sep 11, 2015 at 4:30 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> I don't think CXXRecordDecl is an anachronism, so much as an implementation
> detail; it makes sense to use a smaller class when in C mode, as we don't
> need most of the features and complexity that CXXRecordDecl brings with it.
> But... as a user of clang matchers, I don't think I'd want to care about the
> difference, and it'd be more convenient if I could nest (say) a hasMethod
> matcher within a recordDecl matcher, since it's completely obvious what that
> should mean. If I have a matcher that says:
>
>   recordDecl(or(hasMethod(...), hasField(...)))
>
> I would expect that to work in both C and C++ (and the only way it could
> match in C would be on a record with the specified field, since the
> hasMethod matcher would always fail).

Okay, so then it sounds like we want recordDecl to *mean* RecordDecl,
but we want the traversal and narrowing matchers that currently take a
CXXRecordDecl to instead take a RecordDecl and handle the CXX part
transparently? This means we would not need to add a cxxRecordDecl()
matcher, but could still access CXX-only functionality (like access
control, base classes, etc) through recordDecl()?

~Aaron

>
> On Fri, Sep 11, 2015 at 6:30 AM, Manuel Klimek <klimek at google.com> wrote:
>>
>> Richard! We need an informed opinion :D
>>
>> On Fri, Sep 11, 2015 at 3:07 PM Aaron Ballman <aaron at aaronballman.com>
>> wrote:
>>>
>>> Ping?
>>>
>>> On Tue, Sep 8, 2015 at 9:26 AM, Manuel Klimek <klimek at google.com> wrote:
>>> > On Tue, Sep 8, 2015 at 3:23 PM Aaron Ballman <aaron at aaronballman.com>
>>> > wrote:
>>> >>
>>> >> On Tue, Sep 8, 2015 at 9:18 AM, Manuel Klimek <klimek at google.com>
>>> >> wrote:
>>> >> > On Tue, Sep 8, 2015 at 2:23 PM Aaron Ballman
>>> >> > <aaron at aaronballman.com>
>>> >> > wrote:
>>> >> >>
>>> >> >> On Tue, Sep 8, 2015 at 5:40 AM, Manuel Klimek <klimek at google.com>
>>> >> >> wrote:
>>> >> >> > Yea, we had that discussion a few times, and I can never remember
>>> >> >> > why
>>> >> >> > we
>>> >> >> > ended up in the state we're in.
>>> >> >> > We definitely had a time where we switched to just using the
>>> >> >> > exact
>>> >> >> > same
>>> >> >> > name
>>> >> >> > as the node's class name for the matchers.
>>> >> >> > I *think* we didn't do it for cxxRecordDecl, because Richard said
>>> >> >> > that's
>>> >> >> > a
>>> >> >> > relic we should get rid of anyway, but I'm not sure.
>>> >> >>
>>> >> >> FWIW, I think the state we're in is the worst of all worlds. It's
>>> >> >> not
>>> >> >> intuitive that recordDecl() doesn't match a struct in C mode, and
>>> >> >> as
>>> >> >> it stands, there is no way to match a struct or union declaration
>>> >> >> in C
>>> >> >> at all.
>>> >> >
>>> >> >
>>> >> > Agreed. Best intentions. Worst possible outcome. That's software
>>> >> > development
>>> >> > :)
>>> >> >
>>> >> >> >
>>> >> >> > On Fri, Sep 4, 2015 at 8:32 PM Aaron Ballman
>>> >> >> > <aaron at aaronballman.com>
>>> >> >> > wrote:
>>> >> >> >>
>>> >> >> >> It turns out that the recordDecl() AST matcher doesn't match
>>> >> >> >> RecordDecl objects; instead, it matches CXXRecordDecl objects.
>>> >> >> >> This
>>> >> >> >> is... unfortunate... as it makes writing AST matchers more
>>> >> >> >> complicated
>>> >> >> >> because of having to translate between
>>> >> >> >> recordDecl()/CXXRecordDecl.
>>> >> >> >> It
>>> >> >> >> also makes it impossible to match a struct or union declaration
>>> >> >> >> in C
>>> >> >> >> or ObjC. However, given how prevalent recordDecl()'s use is in
>>> >> >> >> the
>>> >> >> >> wild (I'm guessing), changing it at this point would be a Bad
>>> >> >> >> Thing.
>>> >> >> >>
>>> >> >> >> For people trying to write AST matchers for languages like C or
>>> >> >> >> ObjC,
>>> >> >> >> I would like to propose adding:
>>> >> >> >>
>>> >> >> >> structDecl()
>>> >> >> >> unionDecl()
>>> >> >> >> tagDecl()
>>> >> >> >>
>>> >> >> >> These will match nicely with the existing enumDecl() AST
>>> >> >> >> matcher.
>>> >> >> >>
>>> >> >> >> Additionally, I would like to add cxxRecordDecl() to match
>>> >> >> >> CXXRecordDecl objects. While it duplicates the functionality
>>> >> >> >> exposed
>>> >> >> >> by recordDecl(), it more clearly matches the intention of which
>>> >> >> >> AST
>>> >> >> >> node it corresponds to.
>>> >> >> >>
>>> >> >> >> Finally, I would like to undocument recordDecl() and change our
>>> >> >> >> existing documentation and AST matcher uses to use
>>> >> >> >> cxxRecordDecl/structDecl() instead. Maybe someday we can
>>> >> >> >> deprecate
>>> >> >> >> recordDecl() more officially.
>>> >> >> >>
>>> >> >> >> I'm open to other ideas if there are better ways to move
>>> >> >> >> forward. If
>>> >> >> >> you think changing the meaning of recordDecl() is acceptable, I
>>> >> >> >> can
>>> >> >> >> also go that route (though I would still propose adding
>>> >> >> >> unionDecl()
>>> >> >> >> and cxxRecordDecl() in that case).
>>> >> >> >
>>> >> >> >
>>> >> >> > I think changing recordDecl is acceptable. I believe very few
>>> >> >> > tools
>>> >> >> > will
>>> >> >> > actually start doing wrong things because of it. I'd like more
>>> >> >> > opinions
>>> >> >> > first, though :)
>>> >> >>
>>> >> >> I was giving this more thought over the long weekend, and I think
>>> >> >> you
>>> >> >> may be right. I think changing recordDecl() to mean RecordDecl will
>>> >> >> fix more code than it breaks, so long as we take a holistic
>>> >> >> approach
>>> >> >> to the change and see which narrowing and traversal matchers we
>>> >> >> need
>>> >> >> to fix up at the same time. When I tried to think of AST matchers
>>> >> >> that
>>> >> >> mean CXXRecordDecl but *not* RecordDecl, they were horribly
>>> >> >> contrived
>>> >> >> because you usually are matching on additional selection criteria
>>> >> >> that
>>> >> >> is specific to C++ (such as hasMethod() or isDerivedFrom()) which
>>> >> >> would cause the match to continue to fail, as expected. Code that
>>> >> >> uses
>>> >> >> recordDecl() to mean RecordDecl will suddenly start to match in
>>> >> >> more
>>> >> >> cases, but that's likely to be a bug fix more than a breaking
>>> >> >> change.
>>> >> >> To the best of my understanding, the only breaking cases would be
>>> >> >> where you wrote recordDecl(), meant CXXRecordDecl, had no further
>>> >> >> narrowing or traversal matchers, and were compiling in C mode; with
>>> >> >> the result being additional unexpected matches.
>>> >> >
>>> >> >
>>> >> > Ah, there's one thing that can break: the compile can break:
>>> >> > recordDecl(hasMethod(...)) will *not* compile (it'll work in the
>>> >> > dynamic
>>> >> > matchers and fail as you suggest, but the in-C++ DSL does more
>>> >> > static
>>> >> > type
>>> >> > checking).
>>> >> > I don't think that's super bad though.
>>> >> >
>>> >> >>
>>> >> >> So perhaps it would make sense to:
>>> >> >>
>>> >> >> 1) Make recordDecl() mean RecordDecl
>>> >> >> 2) Do a comprehensive review of matchers that take a CXXRecordDecl
>>> >> >> and
>>> >> >> see if they should instead take a RecordDecl
>>> >> >> 3) Add unionDecl() as a node matcher (or should we add isUnion()
>>> >> >> and
>>> >> >> isStruct() as narrowing matchers?)
>>> >> >> 4) Add tagDecl() as a node matcher, but not add cxxRecordDecl()
>>> >> >
>>> >> >
>>> >> > Why not add cxxRecordDecl()? I think we need it if we want narrowing
>>> >> > matchers on CXXRecordDecl?
>>> >>
>>> >> If Richard thinks CXXRecordDecl is an anachronism, I figured we didn't
>>> >> want to expose it. Instead, we could make hasMethod (et al) accept a
>>> >> RecordDecl and do the type checking for the caller. Then
>>> >> recordDecl(hasMethod(...)) continues to compile and work, and when
>>> >> hasMethod is given a RecordDecl instead of a CXXRecordDecl, it simply
>>> >> matches nothing. But you bring up a good point about the C++ DSL being
>>> >> a problem still, I hadn't considered that.
>>> >
>>> >
>>> > First I want Richard to confirm that. I have a very bad memory, so I
>>> > might
>>> > as well misremember :)
>>> >
>>> >>
>>> >>
>>> >> ~Aaron
>>> >>
>>> >> >
>>> >> >>
>>> >> >>
>>> >> >> ~Aaron
>>> >> >>
>>> >> >> >
>>> >> >> >>
>>> >> >> >>
>>> >> >> >> Thanks!
>>> >> >> >>
>>> >> >> >> ~Aaron
>
>