Page MenuHomePhabricator

[ASTMatchers] Add matcher `hasParentIgnoringImplicit`.
AbandonedPublic

Authored by ymandel on Sep 24 2020, 8:04 PM.

Details

Summary

This matcher combines the features of the hasParent matcher with
skip-implicit-expression-nodes behavior of ignoringImplicit. It finds the
first ancestor which is reachable through only implict expression nodes and
matches the given argument matcher.

Diff Detail

Event Timeline

ymandel created this revision.Sep 24 2020, 8:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 24 2020, 8:04 PM
ymandel requested review of this revision.Sep 24 2020, 8:04 PM
ymandel updated this revision to Diff 294312.Sep 25 2020, 7:51 AM

update dynamic registry and the ast matcher doc.

ymandel updated this revision to Diff 294348.Sep 25 2020, 9:33 AM

Fixed to use more standard type adaptors. Registration now works.

This seems to be strongly related to the TK_IgnoreUnlessSpelledInSource traversal behavior; is there a reason you can't use that traversal mode with hasParent() (does it behave differently)?

This seems to be strongly related to the TK_IgnoreUnlessSpelledInSource traversal behavior; is there a reason you can't use that traversal mode with hasParent() (does it behave differently)?

Aaron, that's a good point. I hadn't realized that the traversal mode supported parent traversal. That said, even with it available, I have strong reservations about modal matching, especially given that it had severe issues when TK_IgnoreUnlessSpelledInSource went live a few months ago as the default setting. I don't know what the resolution of those discussions were, but I thought we simply agreed to restore the default, and left fixing the underlying issues as future work.

But, even it if worked as desired, to achieve the same effect, you would have to restore the current mode as well once you're reached your desired destination. e.g. inspired by the tests, given some matcher m,

integerLiteral(hasParentIgnoringImplicit(varDecl(m)))

becomes

integerLiteral(traversal(TK_IgnoreUnlessSpelledInSource, hasParent(varDecl(traversal(TK_AsIs, m)))))

which seems a lot worse to me. We could however implement this new one in terms of traverse, but that would go back to the question of whether it is working correctly and also gets somewhat tricky (specifically, restoring the ambient traversal mode). Do you know the current status?

thanks

ymandel updated this revision to Diff 294363.Sep 25 2020, 10:47 AM

restored changes to unrelated parts of html docs.

aaron.ballman added a subscriber: steveire.

This seems to be strongly related to the TK_IgnoreUnlessSpelledInSource traversal behavior; is there a reason you can't use that traversal mode with hasParent() (does it behave differently)?

Aaron, that's a good point. I hadn't realized that the traversal mode supported parent traversal. That said, even with it available, I have strong reservations about modal matching, especially given that it had severe issues when TK_IgnoreUnlessSpelledInSource went live a few months ago as the default setting. I don't know what the resolution of those discussions were, but I thought we simply agreed to restore the default, and left fixing the underlying issues as future work.

That is the resolution we came to.

But, even it if worked as desired, to achieve the same effect, you would have to restore the current mode as well once you're reached your desired destination. e.g. inspired by the tests, given some matcher m,

integerLiteral(hasParentIgnoringImplicit(varDecl(m)))

becomes

integerLiteral(traversal(TK_IgnoreUnlessSpelledInSource, hasParent(varDecl(traversal(TK_AsIs, m)))))

which seems a lot worse to me.

It's certainly more wordy, but do you envision this matcher functionality to be needed so often that we need to introduce a second way to achieve the same thing?

We could however implement this new one in terms of traverse, but that would go back to the question of whether it is working correctly and also gets somewhat tricky (specifically, restoring the ambient traversal mode). Do you know the current status?

I'm adding @steveire in case he has any updates or opinions, but my understanding of the current status is that the defaults have been restored but no further work has been done for implicit node matching.

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.

ymandel added a comment.EditedSep 30 2020, 6:42 AM

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.

We should decide our general policy on these implict matchers vs the traverse matchers. 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).

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())))));
}

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?

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.

I appreciate your point, but I'm personally inclined to allow progress while we figure these larger issues out. That said, I'm in no rush and would like a solution that we're both happy with. How do you propose we proceed, especially given that traverse does not currently support this case? It seems that progress is in part blocked on hearing back from steveire, but it's been over a week since you added him to the review thread. Is there some other way to ping him?

Thanks!

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.

I appreciate your point, but I'm personally inclined to allow progress while we figure these larger issues out. That said, I'm in no rush and would like a solution that we're both happy with. How do you propose we proceed, especially given that traverse does not currently support this case? It seems that progress is in part blocked on hearing back from steveire, but it's been over a week since you added him to the review thread. Is there some other way to ping him?

Thank you for your patience while we sort this out. I've pinged @steveire off-list, so hopefully he'll respond with his thoughts. If we don't hear from Stephen in the next week or so, I think we should proceed with your proposed patch to get you unblocked (adding one more "ignore implicit" variant isn't the end of the world, I just want us to be thoughtful about whether we want to add new matchers in this space or to work on the traversal mode instead).

Thank you for your patience while we sort this out. I've pinged @steveire off-list, so hopefully he'll respond with his thoughts. If we don't hear from Stephen in the next week or so, I think we should proceed with your proposed patch to get you unblocked (adding one more "ignore implicit" variant isn't the end of the world, I just want us to be thoughtful about whether we want to add new matchers in this space or to work on the traversal mode instead).

Sure! Thanks for reaching out to Stephen. This sounds good to me.

I don't get email notifications when I'm pinged on Phab for some reason. I didn't know about this until the email from Aaron.

// Fails
EXPECT_TRUE(
    matches(Input, declRefExpr(traverse(TK_IgnoreImplicitCastsAndParentheses,
                                        hasParent(returnStmt())))));

Why do you use TK_IgnoreImplicitCastsAndParentheses instead of TK_IgnoreUnlessSpelledInSource? All you seem to be doing is showing that the former is broken. I've always thought we should remove the former. AsIs and IgnoreUnlessSpelledInSource should be enough for anyone.

@steveire, would you consider this [when using TK_IgnoreImplicitCastsAndParentheses] to be a bug in the traversal behavior?

It's probably a bug in how TK_IgnoreImplicitCastsAndParentheses is processed?

I have strong reservations about modal matching, especially given that it had severe issues

I think "severe" is an overstatement.

I'm only worried we're making AST matching more confusing by having two different ways of inconsistently accomplishing the same-ish thing.

The traverse matcher and IgnoreUnlessSpelledInSource were introduced so that matchers like hasParentIgnoringImplicit (and all the other hasParentIgnoring*) would never be needed (and so that the already-existing ignore* matchers would be needed only rarely). I agree with Aaron that it's not a good design to continue.

I think the existence of this new hasParentIgnoringImplicit matcher is further motivation that IgnoreUnlessSpelledInSource should be used more, especially when writing new matcher code. It is simpler. I get the impression people haven't tried it and prefer to write the complicated stuff instead.

I still don't see a problem with traverse being modal, but that fact seems to make you not use it, @ymandel ?

TL;DR Stephen's fix works; I'll drop this patch.

Why do you use TK_IgnoreImplicitCastsAndParentheses instead of TK_IgnoreUnlessSpelledInSource? All you seem to be doing is showing that the former is broken. I've always thought we should remove the former. AsIs and IgnoreUnlessSpelledInSource should be enough for anyone.

@steveire, would you consider this [when using TK_IgnoreImplicitCastsAndParentheses] to be a bug in the traversal behavior?

It's probably a bug in how TK_IgnoreImplicitCastsAndParentheses is processed?

Changing to TK_IgnoreUnlessSpelledInSource fixes the test, as you suggested. Thanks!

As for why I chose the former -- it was the closest match to what I was doing and the comments on the struct didn't guide me one way or another.

I have strong reservations about modal matching, especially given that it had severe issues

I think "severe" is an overstatement.

I think it was an unhelpful word choice -- sorry about that. I could go into more detail as to why I chose that, but without detail it's not a helpful description. Suffice it to say that we had multiple instances of suprising behaviour, which lost quite a few hours.

I'm only worried we're making AST matching more confusing by having two different ways of inconsistently accomplishing the same-ish thing.

The traverse matcher and IgnoreUnlessSpelledInSource were introduced so that matchers like hasParentIgnoringImplicit (and all the other hasParentIgnoring*) would never be needed (and so that the already-existing ignore* matchers would be needed only rarely). I agree with Aaron that it's not a good design to continue.

I think the existence of this new hasParentIgnoringImplicit matcher is further motivation that IgnoreUnlessSpelledInSource should be used more, especially when writing new matcher code. It is simpler. I get the impression people haven't tried it and prefer to write the complicated stuff instead.

I still don't see a problem with traverse being modal, but that fact seems to make you not use it, @ymandel ?

This is a longer discussion and not necessarily worth having on this review thread. Are you attending the dev meeting today? If so, want to chat during the coffee break? Otherwise, it seems like it could be helpful to schedule a meeting to discuss for some other time.

That said, I'm more inclined towards its use as a specialty operator, for situations like this, than as a general purpose tool for matchers. I can't say I much like the current regime of explicit ignore operators either.

ymandel abandoned this revision.Oct 8 2020, 6:37 AM

TL;DR Stephen's fix works; I'll drop this patch.

This is a longer discussion and not necessarily worth having on this review thread. Are you attending the dev meeting today? If so, want to chat during the coffee break? Otherwise, it seems like it could be helpful to schedule a meeting to discuss for some other time.

I didn't go to the coffee break, but am in the afters hangout.