[PATCH] D88275: [ASTMatchers] Add matcher `hasParentIgnoringImplicit`.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D88275#2303283 <https://reviews.llvm.org/D88275#2303283>, @ymandel wrote:

>> I'm not concerned about the basic idea behind the proposed matcher, I'm only worried we're making AST matching more confusing by having two different ways of inconsistently accomplishing the same-ish thing.
>
> Aaron, I appreciate this concern, but I would argue that this matcher isn't making things any worse. We already have the various `ignoringImplicit` matchers, and this new one simply parallels those, but for parents. So, it is in some sense "completing" an existing API, which together is an alternative to  `traverse`.

I'm not certain I agree with that reasoning because you can extend it to literally any match that may interact with implicit nodes, which is the whole point to the spelled in source traversal mode. I'm not certain it's a good design for us to continue to add one-off ways to ignore implicit nodes.

> We should decide our general policy on these implict matchers vs the traverse matchers.

I agree.

> Personally, I view `traverse` as an experimental feature that we're still figuring out and so would prefer that it not block progress on new matchers. But, I'm open to discussion. I can implement this one in my own codebase in the meantime if this requires longer discussion (that is, it's not blocking me, fwiw).

FWIW, I think of `traverse` as experimental as well. I can see the argument for not letting an experimental feature block progress on new matchers, too. I'm mostly worried about the case where we add the new matches and keep the `traverse` API, but they have subtly different behaviors and no clear policy on when to use which form.

> Also, I don't believe that traverse work in this case. When I change the test to use `traverse`, it fails:
>
>   TEST(HasParentIgnoringImplicit, TraverseMatchesExplicitParents) {
>     std::string Input = R"cc(
>       float f() {
>           int x = 3;
>           int y = 3.0;
>           return y;
>       }
>     )cc";
>      // ---> Passes because there are no implicit parents.
>     EXPECT_TRUE(matches(
>         Input, integerLiteral(traverse(TK_IgnoreImplicitCastsAndParentheses,
>                                        hasParent(varDecl())))));
>     // Fails
>     EXPECT_TRUE(
>         matches(Input, declRefExpr(traverse(TK_IgnoreImplicitCastsAndParentheses,
>                                             hasParent(returnStmt())))));
>     // Fails
>     EXPECT_TRUE(
>         matches(Input, floatLiteral(traverse(TK_IgnoreImplicitCastsAndParentheses,
>                                              hasParent(varDecl())))));
>   }

Interesting catch and not the behavior I would expect from `traverse`. @steveire, would you consider this to be a bug in the traversal behavior?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88275