recordDecl() AST matcher

[Daniel Jasper via cfe-commits]

So, back in the day when we implemented the matchers, we decided on
actually wanting to remove all the CXX... AST nodes (there are more of
them). I don't know how this would work as recordDecl already exists. But
I'd be somewhat hesitant to introduce a cxxRecordDecl matcher if there is
still a chance that we want to move away from the CXX prefix.

Also note that AST matchers are used massively in the wild by now and I
would be very hesitant to make a change breaking backwards compatibility.
85 failures in the clang repositories themselves sounds scary to me. Not
saying we shouldn't do it at all. But we should be very clear on where
things should be in the long run.

On Mon, Sep 14, 2015 at 7: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 --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150914/0e529210/attachment-0001.html>