[PATCH] D56444: [AST] RecursiveASTVisitor visits lambda classes when implicit visitation is on.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D56444#1351174 <https://reviews.llvm.org/D56444#1351174>, @steveire wrote:

> In D56444#1351145 <https://reviews.llvm.org/D56444#1351145>, @sammccall wrote:
>
> > This manifests as assertion failures (or with assertions off, incorrect results) for some matchers, on some code - people have encountered several examples where this happens, it's hard to know where to target a more targeted fix.
> >  The invariant violated is "RAV traverses every AST node (when implicit traversal is enabled)" - is there a way to fix this issue sufficiently without addressing that?
>
>
> Yes - don't use RAV to traverse parents when AST-matching.


OK, this is certainly a much more invasive change, and isn't going to make the release.
I understand that you don't think this crash is worth fixing for 8.0, while others disagree.
I don't particularly have a dog in this fight, but having seen now three reports of this crash makes me nervous about ignoring it.

In D56444#1351182 <https://reviews.llvm.org/D56444#1351182>, @steveire wrote:

> In D56444#1351150 <https://reviews.llvm.org/D56444#1351150>, @sammccall wrote:
>
> > In D56444#1351130 <https://reviews.llvm.org/D56444#1351130>, @steveire wrote:
> >
> > > In D56444#1351125 <https://reviews.llvm.org/D56444#1351125>, @aaron.ballman wrote:
> > >
> > > > if the location isn't somewhere in user code, then don't consider the node or its children for traversal. However, that may be insufficient and equally as mysterious behavior.
> > >
> > >
> > > That is exactly what I've implemented. I skip invisible nodes in matching and dumping: http://ec2-18-191-7-3.us-east-2.compute.amazonaws.com:10240/z/EuYjAn
> >
> >
> > So what happens when someone asks about the parent of an invisible node?
> >
> > e.g. `match(hasParent(decl().bind("parent")), X, Ctx)` where X is the `operator()` function of a lambda class. (This is basically the case in the bug that this patch fixes)
>
>
> Assuming that whether to skip invisible nodes is part of the `Ctx`, the `X` would simply not be in context, just like if the `X` were not in the `TraversalScope` of the `Ctx`.


Ah, so any skipped nodes would not have any parents at all.
The implementation of the idea would just be removing the assert - instead of assuming non-visited nodes is a bug, we assume they're *meant* to be excluded.

However, such nodes are very easy to reach, inside or outside of a matcher.
e.g. `callExpr(callee(functionDecl()))` will match a lambda call, and "func" will be the `operator()`.
Having such nodes then not work with parent/ancestor matchers seems surprising and not **obviously** the best design choice (vs e.g. visiting all nodes including implicit, or breaking symmetry between downward and upward traversal).

I guess what I'm saying is your suggestion seems like a significant change to the design of (ancestor) matchers, and that the approach in this patch is the more conservative one - making e.g. the implicit `FunctionDecl`s visible to matchers is consistent with `clang -ast-dump` and the behavior of matchers on other parts of the AST, even if that's something you'd like to change on an LLVM-9 timeline.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56444/new/

https://reviews.llvm.org/D56444