recordDecl() AST matcher

[Aaron Ballman via cfe-commits]

On Sat, Sep 12, 2015 at 8:22 AM, Manuel Klimek <klimek at google.com> wrote:
>
>
> On Fri, Sep 11, 2015 at 10:39 PM Aaron Ballman <aaron at aaronballman.com>
> wrote:
>>
>> 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()?
>
>
> I'm against that proposal. I think we have tried to make the matchers more
> "user friendly" in the past, and all those attempts have failed miserably;
> in the end, users will do ast-dump to see what they want to match, and then
> be confused when the matchers do follow the AST 99% of the time, but try to
> be smart 1% of the time.
> I think given that we want to keep CXXRecordDecl, the right solution is to
> have a cxxRecordDecl() matcher.

Personally, I think this makes the most sense, at least to me. The
recommendation I've always heard (and given) is to use -ast-dump and
write matchers from there. (Consequently, the more I work with type
traversal matchers, the more I wish we had -ast-dump-types to give
even *more* information for writing matchers.)

But the question still remains, what do we do with recordDecl? Right
now, it means CXXRecordDecl instead of RecordDecl. If we change it to
mean RecordDecl instead, there's a chance we'll break existing,
reasonable code. Are we okay with that risk? If we're at least
conceptually okay with it, I could make the change locally and see
just how much of our own code breaks, and report back. But if that
turns out to be problematic, do we want to deprecate recordDecl and
replace it with structDecl as our fallback position? Or is there a
better solution?

Basically, I see a few ways to solve this (and there may be other ways
I'm not thinking about yet):

1) Undocument/deprecate recordDecl, add structDecl, unionDecl, and
cxxRecordDecl. This does not match the AST because we have no
StructDecl or UnionDecl types. The more I think about this option, the
less I like it. It's easy to implement, but seems hard to relate to
the AST.
2) Make recordDecl match RecordDecl, don't touch other matchers. Add
way to distinguish unions from structs (e.g., isUnion(), isStruct()),
add cxxRecordDecl. This matches the AST most closely, but may break
code. I think that I prefer this approach, but it depends heavily on
what "may break code" looks like in practice.
3) Make recordDecl match RecordDecl, fix other matchers that currently
take CXXRecordDecl to instead take RecordDecl and handle sensibly when
possible. Add a way to distinguish unions from structs, add
cxxRecordDecl. This doesn't match the AST because we will have
matchers taking a RecordDecl when the AST would require a
CXXRecordDecl, but is likely to break less code.

~Aaron


> Richard: if CXXRecordDecl was really an implementation detail, it would be
> hidden behind a RecordDecl class, as an implementation detail. The reasons
> why we don't want it to be an implementation detail in the code
> (performance, data structure size) don't matter - in the end, it's in the
> AST API.
>
>>
>>
>> ~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
>> >
>> >