WIP: Add an API to simplify setting TraversalKind in clang-tidy matchers

[sammccall]

Thanks for putting this together!
As mentioned on the cfe-dev thread, for our out-of-tree checks we're planning to wrap with traverse() instead, to capture the full matcher logic more locally at the cost of some readability.
So if this won't be used for in-tree checks, we'd probably want to hold off on this unless someone in particular plans to use it.

clang/include/clang/ASTMatchers/ASTMatchFinder.h

118

I don't really get why this would be optional.
A check's implementation isn't really going to be robust to "whatever the default is", it's going to be tested against one or the other.
So None isn't really a sensible return value - can't the base class simply return the actual default?

clang/lib/ASTMatchers/ASTMatchFinder.cpp

1063

Writing to stderr from a library isn't ideal. (e.g. in multithreaded programs - clangd uses a wrapped stderr as its log stream, this bypasses the mutex)

[steveire]

Update

[steveire]

Reviving this so it can be used to port clang-tidy checks to IgnoreUnlessSpelledInSource.

clang/include/clang/ASTMatchers/ASTMatchFinder.h

118

ASTMatchFinder doesn't know the ASTContext, so it can't access the default. That's why I made it an optional.

[steveire]

Update

[aaron.ballman]

clang/include/clang/ASTMatchers/ASTMatchFinder.h

118

The fact that it's optional is a bit weird. Given that this is a temporary API, I see two approaches: hold our nose because the ugliness shouldn't be long-lived, or have the default be specified in two places with comments saying "don't forget to update over here if you change this value" so we can get rid of the Optional<> here. WDYT?

clang/lib/ASTMatchers/ASTMatchFinder.cpp

1062–1068

Elide the braces.

1080–1086

Elide the braces.

[steveire]

clang/include/clang/ASTMatchers/ASTMatchFinder.h

118

My preference is to keep the optional. Having multiple places to update makes it likely they'll get out of sync.

[aaron.ballman]

clang/include/clang/ASTMatchers/ASTMatchFinder.h

118

Then I think it's fine to leave it as Optional<> given that it's a temporary API for easing the transition. @sammccall, are you okay with that?

In terms of eventually removing this API, what's your plan? I noticed we can't do something nice like slapping a [[deprecated]] attribute on the declaration and have overriders get a notification that the method they're overriding is deprecated. Will we just pull the API entirely and force users to react to the code breakage?

[steveire]

Update

[steveire]

clang/include/clang/ASTMatchers/ASTMatchFinder.h

118

Will we just pull the API entirely and force users to react to the code breakage?

Yes, I think something like that.

[aaron.ballman]

LGTM! I'm assuming that @sammccall may be busy given the holiday season, so I think it's fine to land this as-is if you don't hear from him in the next day. If he has additional concerns, I believe we can address them pretty easily post-commit.

[thakis]

This breaks check-clang everywhere: http://45.33.8.238/linux/38812/step_7.txt

Please run tests before landing.

Please take a look, and revert for now if it takes a while to fix.

clang/include/clang/ASTMatchers/ASTMatchFinder.h

public:
     /// Optionally override to do per translation unit tasks.
     virtual void onEndOfTranslationUnit() {}
 
     /// An id used to group the matchers.
     ///
     /// This id is used, for example, for the profiling output.
     /// It defaults to "<unknown>".
     virtual StringRef getID() const;
+
+    /// TraversalKind to use while matching and processing
+    /// the result nodes. This API is temporary to facilitate
+    /// third parties porting existing code to the default
+    /// behavior of clang-tidy.
+    virtual llvm::Optional<ast_type_traits::TraversalKind>
+    getCheckTraversalKind() const;
   };
 
   /// Called when parsing is finished. Intended for testing only.
   class ParsingDoneTestCallback {
   public:
     virtual ~ParsingDoneTestCallback();
     virtual void run() = 0;
   };

clang/lib/ASTMatchers/ASTMatchFinder.cpp

#endif
   class MatchVisitor : public BoundNodesTreeBuilder::Visitor {
   public:
     MatchVisitor(ASTContext* Context,
                  MatchFinder::MatchCallback* Callback)
       : Context(Context),
         Callback(Callback) {}
 
     void visitMatch(const BoundNodes& BoundNodesView) override {
+      TraversalKindScope RAII(*Context, Callback->getCheckTraversalKind());
       Callback->run(MatchFinder::MatchResult(BoundNodesView, Context));
     }
 
   private:
     ASTContext* Context;
     MatchFinder::MatchCallback* Callback;
   };
 

 
 MatchFinder::MatchFinder(MatchFinderOptions Options)
     : Options(std::move(Options)), ParsingDone(nullptr) {}
 
 MatchFinder::~MatchFinder() {}
 
 void MatchFinder::addMatcher(const DeclarationMatcher &NodeMatch,
                              MatchCallback *Action) {
+  if (auto TK = Action->getCheckTraversalKind()) {
+    llvm::errs() << "Overriding getCheckTraversalKind is deprecated. Port to "
+                    "traverse() matcher instead.";
+    Matchers.DeclOrStmt.emplace_back(traverse(*TK, NodeMatch), Action);
+  } else {
     Matchers.DeclOrStmt.emplace_back(NodeMatch, Action);
+  }
   Matchers.AllCallbacks.insert(Action);
 }
 
 void MatchFinder::addMatcher(const TypeMatcher &NodeMatch,
                              MatchCallback *Action) {
   Matchers.Type.emplace_back(NodeMatch, Action);
   Matchers.AllCallbacks.insert(Action);
 }
 
 void MatchFinder::addMatcher(const StatementMatcher &NodeMatch,
                              MatchCallback *Action) {
+  if (auto TK = Action->getCheckTraversalKind()) {
+    llvm::errs() << "Overriding getCheckTraversalKind is deprecated. Port to "
+                    "traverse() matcher instead.";
+    Matchers.DeclOrStmt.emplace_back(traverse(*TK, NodeMatch), Action);
+  } else {
     Matchers.DeclOrStmt.emplace_back(NodeMatch, Action);
+  }
   Matchers.AllCallbacks.insert(Action);
 }
 
 void MatchFinder::addMatcher(const NestedNameSpecifierMatcher &NodeMatch,
                              MatchCallback *Action) {
   Matchers.NestedNameSpecifier.emplace_back(NodeMatch, Action);
   Matchers.AllCallbacks.insert(Action);
 }

 
 void MatchFinder::registerTestCallbackAfterParsing(
     MatchFinder::ParsingDoneTestCallback *NewParsingDone) {
   ParsingDone = NewParsingDone;
 }
 
 StringRef MatchFinder::MatchCallback::getID() const { return "<unknown>"; }
 
+llvm::Optional<ast_type_traits::TraversalKind>
+MatchFinder::MatchCallback::getCheckTraversalKind() const {
+  return llvm::None;
+}
+
 } // end namespace ast_matchers
 } // end namespace clang