Page MenuHomePhabricator

Update matchers to be traverse-aware
ClosedPublic

Authored by steveire on Fri, Nov 6, 3:45 PM.

Details

Summary

Don't match Stmt or Decl nodes not spelled in the source when using
TK_IgnoreUnlessSpelledInSource. This prevents accidental modification
of source code at incorrect locations.

Diff Detail

Event Timeline

steveire created this revision.Fri, Nov 6, 3:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Nov 6, 3:45 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
steveire requested review of this revision.Fri, Nov 6, 3:45 PM

I think this change should come with a warning in the release notes because it silently changes the behavior of existing matchers in a way that may be surprising to users. I think the behavior is correct in that these constructs are all implicit nodes that aren't really spelled in source, but users may have come to rely on the existing behavior. I will note that the default argument case is somewhat interesting to me because there is a defensible argument to be made that the expression is spelled in source (it's spelled at the location of the function declaration rather than at the call site) and that same logic applies to explicitly defaulted function declarations. However, given the goal of trying to model this on the syntactic form of what the user wrote, I think the proposed behavior is correct.

clang/include/clang/ASTMatchers/ASTMatchers.h
3115

Given how often this gets repeated (and what a mouthful it is), how about adding a static helper function ClassifyTraversal() (or some such) that returns the traversal kind given a Finder? (Alternatively, isTraversalAsIs() or some specific traversal behavior.)

4054–4062

Please spell out the type.

4059

Likewise here.

4080

And here.

4303

Spell the type out?

5456

In general (not specific to this matcher), I'd like to see some tests for explicitly defaulted functions vs implicitly defaulted ones. e.g., I think we should handle these differently, shouldn't we?

struct S {}; // Has implicitly defaulted constructor
struct T {
  T() = default; // Has explicitly defaulted constructor
};

Specific to this matcher, I am a bit concerned about the behavior of answering "isDefinition" based on whether a function is defaulted or not. The user has already gotten some FunctionDecl object, so why would the implicit vs explicit nature of the declaration matter for deciding whether the node is a definition? The behavior with hasBody makes sense to me because the body may be implicitly provided, but whether something is or is not a definition is a different kind of question.

7060

Same question here about why defaulted matters as above.

steveire updated this revision to Diff 304104.Tue, Nov 10, 1:48 AM
steveire marked an inline comment as done.

Update

I added the release note to D90982, which this change follows.

clang/include/clang/ASTMatchers/ASTMatchers.h
3115

Done in D91144.

5456

I agree for both isDefinition and isInline. hasBody is the one to use for these cases.

aaron.ballman added inline comments.Tue, Nov 10, 5:46 AM
clang/include/clang/ASTMatchers/ASTMatchers.h
3115

Thanks! Just an FYI, but it looks like the changes in D91144 are reflected in this review. I don't mind but wanted to mention it in case it causes you problems when trying to apply the commits to master.

5456

Awesome, thanks for verifying!

clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
2368

Was this originally testing behavior with explicitly initializing an implicitly generated ctor (since that's also an implicit node)? Perhaps this change should be reverted and we make a second test?

2429

You can drop the .bind() I believe.

steveire added inline comments.Tue, Nov 10, 12:22 PM
clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
2368

Good point, done.

aaron.ballman added inline comments.Thu, Nov 12, 1:12 PM
clang/include/clang/ASTMatchers/ASTMatchers.h
3115

If the traversal is not AsIs, that doesn't mean it's IgnoreUnlessSpelledInSource -- it could be IgnoreImplicitCastsAndParentheses, right? So I think the logic for some of these AST matchers is incorrect when in IgnoreImplicitCastsAndParentheses traversal mode and should be double-checked. We should probably add some extra test coverage for that mode.

4083

Huh, I never noticed that we implicitly are ignoring parens and implicit casts for this (without checking the traversal mode or documenting the behavior!). That seems less-than-ideal in some ways. (No change required, I only noticed it because it made me think through whether we need it on the isa<> check above, which we don't.)

steveire added inline comments.Mon, Nov 16, 7:41 AM
clang/include/clang/ASTMatchers/ASTMatchers.h
3115

As far as I know, the problem of implicit nodes has been known for a long time. Adding calls to IgnoreParenImpCasts() in certain places like hasArgument was one attempt at making the implicit nodes less convenient for users. As far as I know, IgnoreImplicitCastsAndParentheses was just another attempt at the same thing. I must have discovered that by reading code history at some point. Both previous attempts didn't go far enough to actually solve the problem, but IgnoreUnlessSpelledInSource does go all the way, and traverse puts control in the hands of the user. D20801 at least seems to have been an attempt to put control back in the hands of the user. And it was a follow-up to D18243.

So, once this and D90982 are merged, I think it makes sense to start to remove IgnoreImplicitCastsAndParentheses entirely. It is legacy, incompletely useful and just causes some mess in the code.

Two modes aught to be enough for anybody.

4083

Yes, I think calls to ignore* like this within matcher implementations should be removed, giving the user control instead.

aaron.ballman added inline comments.Mon, Nov 16, 9:01 AM
clang/include/clang/ASTMatchers/ASTMatchers.h
3115

As far as I know, IgnoreImplicitCastsAndParentheses was just another attempt at the same thing. I must have discovered that by reading code history at some point.

I don't recall the history of this traversal mode, but I think you're correct.

So, once this and D90982 are merged, I think it makes sense to start to remove IgnoreImplicitCastsAndParentheses entirely. It is legacy, incompletely useful and just causes some mess in the code.

I agree. I see at least two ways to proceed:

  1. Change this patch to handle three modes so that we can land it without incorrect behavior, then do a follow-up patch to rip out the IgnoreImplicitCastsAndParentheses mode.
  2. Rip out IgnoreImplicitCastsAndParentheses first and then land this patch as-is.

My preference is for #1 so that we don't land this patch in an awkward state (in case the removal of the other mode gets delayed for some reason). Given what I mention below, that shouldn't result in any undue churn, I'd hope.

Two modes aught to be enough for anybody.

Heh, I would like to think that, but given that this enumeration is used for generic traversal of ASTs, I'm less convinced that two modes will be all we ever want to support. I think it's a bit more future-proof to add a helper method along the lines of isTraversalIgnoringImplicitNodes() rather than using !isTraversalAsIs().

4054–4062

auto -> unsigned please

4058

auto -> const Expr * or just get rid of the local variable entirely by sinking the initialization into the isa<>.

4080

const auto * -> const Expr *

4083

I agree. I'm a little bit worried about what will break when we make the change, but I think that model is the more defensible one. (It looks like there are a handful of similar cases in ASTMatchers.h, so we should try to hit them all).

aaron.ballman added inline comments.Tue, Nov 17, 6:50 AM
clang/include/clang/ASTMatchers/ASTMatchers.h
4083

This probably shouldn't compile given that there's no declaration of Arg?

steveire added inline comments.Tue, Nov 17, 6:55 AM
clang/include/clang/ASTMatchers/ASTMatchers.h
4083

Correct. Fixed now.

aaron.ballman accepted this revision.Tue, Nov 17, 6:57 AM

LGTM, thank you!

This revision is now accepted and ready to land.Tue, Nov 17, 6:57 AM
This revision was landed with ongoing or failed builds.Tue, Nov 17, 8:50 AM
This revision was automatically updated to reflect the committed changes.