recordDecl() AST matcher

[Aaron Ballman via cfe-commits]

This is the full patch that corrects all the compile errors MSVC was
generating. If we have any platform-specific checkers, they may have
been missed. But this should give an idea of the scope of the changes
we're asking folks to make.

~Aaron

On Mon, Sep 14, 2015 at 1:03 PM, Aaron Ballman <aaron at aaronballman.com> wrote:
> On Mon, Sep 14, 2015 at 11:49 AM, Manuel Klimek <klimek at google.com> wrote:
>>
>>
>> On Mon, Sep 14, 2015, 8:40 AM Aaron Ballman <aaron at aaronballman.com> wrote:
>>>
>>> On Sat, Sep 12, 2015 at 11:06 PM, Manuel Klimek <klimek at google.com> wrote:
>>> >
>>> >
>>> > On Sat, Sep 12, 2015, 9:25 PM Aaron Ballman <aaron at aaronballman.com>
>>> > wrote:
>>> >>
>>> >> 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.
>>> >
>>> >
>>> > That sums it up. My preferences are 2, 3, 1 in that order :)
>>>
>>> I've attached a patch that implements #2, but it comes with ~85 errors
>>> from C++ matchers that use recordDecl to mean cxxRecordDecl.
>>>
>>> http://pastebin.com/bxkRcqBV
>>>
>>> If this is an acceptable failure rate, I can also update the failing
>>> matchers to use cxxRecordDecl instead of recordDecl where applicable.
>>> Doing some spot-checking of the failing code, the failures are ones we
>>> anticipated, such as:
>>>
>>> constructorDecl(ofClass(recordDecl(
>>> hasDeclaration(recordDecl(hasMethod(hasName("begin")),
>>> hasMethod(hasName("end"))))
>>> etc
>>
>>
>> +Daniel for another opinion. I think this is fine, but I'd  prefer not to
>> end up in a meme :)
>
> FWIW, the vast majority of the errors are in ASTMatchersTests. There
> were only a few in actual real-world uses (71 in tests, 14 in real
> code).
>
> ~Aaron
>
>>
>>> ~Aaron
>>>
>>> >
>>> >>
>>> >> ~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
>>> >> >> >
>>> >> >> >
-------------- next part --------------
A non-text attachment was scrubbed...
Name: recordDecl.patch
Type: application/octet-stream
Size: 34893 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150914/b656e96d/attachment-0001.obj>