[PATCH] D98710: [clang-tidy] Add --skip-headers, part 1 (use setTraversalScope)

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchFinder.h:148
+    /// Check if a Decl should be skipped.
+    std::shared_ptr<DeclFilter> Filter;
   };
----------------
chh wrote:
> sammccall wrote:
> > hokein wrote:
> > > chh wrote:
> > > > sammccall wrote:
> > > > > I don't think the filter belongs here.
> > > > > The design of traversal scope is that it's a property of the AST that affects all traversals, so it shouldn't be a configuration property of particular traversals like ASTMatchFinder.
> > > > > 
> > > > > There's a question of what to do about `MatchFinder::newASTConsumer()`, which runs the finder immediately after an AST is created, and so covers the point at which we might set a scope. There are several good options, e.g.:
> > > > >  - Don't provide any facility for setting traversal scope when newASTConsumer() is used - it's not commonly needed, and the ASTConsumer is trivial to reimplement where that's actually needed
> > > > >  - Accept an optional `function<void(ASTContext&)>` which should run just before matching starts
> > > > > 
> > > > > This seems a bit subtle, but the difference between having this in MatchFinder proper vs just in newASTConsumer() is important I think, precisely because it's common to run the matchers directly on an existing AST.
> > > > > 
> > > > I have some similar concerns too.
> > > > 
> > > > clangd calls MatchFinder::matchAST explicitly after setTraversalScope,
> > > > but clang-tidy uses MultiplexConsumer and MatchFinder::newASTConsumer
> > > > is just one of the two consumers.
> > > > (1) I am not sure if it would be safe or even work to make big changes in
> > > > ClangTidy.cpp's CreateASTConsumer to call MatchFinder::matchAST explicitly.
> > > > (2) Similarly, I wonder if setTraversalScope will work for both MatchFinder
> > > > and other consumers in the same MultiplexConsumer.
> > > > 
> > > > Could you check if my current change to MatchFinder::HandleTranslationUnit
> > > > is right, especially in the saving/restoring of traversal scope?
> > > > 
> > > > I am assuming that each consumer in a MultiplexConsumer will have its own
> > > > chance to HandleTranslationUnit and HandleTopLevelDecl.
> > > > If that's correct, it seems to me that changing those two handlers in
> > > > MatchFinder is right for clang-tidy. Then they will need the optional
> > > > MatchFinder::MatchFinderOptions::DeclFilter.
> > > > 
> > > > 
> > > > but clang-tidy uses MultiplexConsumer and MatchFinder::newASTConsumer is just one of the two consumers.
> > > 
> > > Yeah, `MultiplexConsumer` is a separate interface in clang, and I don't think we have a strong reason to modify it. 
> > > 
> > > However, we could refine the bit in clang-tidy -- clang-tidy uses `MultiplexConsumer` to dispatch all events to two consumers: the MatcherFinder, the clang's static analyzer, we can get rid of the `MultiplexConsumer` by implementing a new ASTConsumer, so that we have enough flexibility to affect all traversals without touching all clang areas, so the API will look like
> > > 
> > > ```
> > > class ClangTidyASTConsumer : public ASTConsumer {
> > > 
> > > public:
> > >   void HandleTopLevelDecl(...) override {
> > >      // collect all main file decl
> > >   }
> > >   void HandleTranslationUnit(ASTContext &Context) override {
> > >     // set traversal scope.
> > >     MatcherFinderConsumer->HandleTranslationUnit(Context);
> > >     StaticAnalyzer->HandleTranslationUnit(Context);
> > >   }
> > >   
> > >   // implement other necessary Handle* overrides, and dispatch to StaticAnalyzer consumers;
> > > 
> > > private:
> > >   std::unique_ptr<ASTConsumer> MatcherFinderConsumer;
> > >   std::unique_ptr<...> StaticAnalyzer;
> > >   ... MainFileDecl;
> > > }; 
> > > ``` 
> > MultiplexConsumer is just used as a helper to glue together two concrete ASTConsumers, not arbitrary consumers, so we can reason about whether it's safe by reading the code.
> > 
> > The static analyzer's consumer appears to already track top-level decls rather than traversing from the root, exactly to avoid traversing stuff from the preamble. So I would expect setTraversalContext to work fine there.
> > 
> > The approach Haojian suggests of avoiding MultiplexConsumer, and making the tight coupling to the particular consumers explicit, also seems fine.
> > 
> > > If that's correct, it seems to me that changing those two handlers in MatchFinder is right for clang-tidy
> > 
> > This might be the most convenient thing for clang-tidy, but I don't think it's the right thing for MatchFinder, which is also important.
> I looked into Hokein's suggestion. Although in general correctly used inheritance is better, there are special cases that copying functionality from another class could be better. I wondered if it could work better by changing ClangTidyASTConsumer to behave like  MultiplexConsumer without inheritance.
> 
> After looking into MultiplexConsumer's many methods, what are used and could be used in the future by MatchFinder's MatchASTConsumer and static analyzer's AnalysisConsumer, the dependency on MultiplexConsumer to pass through all Consumer methods is obvious. So, ClangTidyASTConsumer really needs to behave like MultiplexConsumer, duplicating all existing and future methods, to keep MatchFinder and AnalysisConsumer working. This looks like a real inheritance use case and difficult to make it better without inheritance.
> 
> If MatchFinder users really don't want an optional filter for HandleTranslationUnit and HandleTopLevelDecl, I think an alternative is to define a child class maybe called TidyMatchFinder with the extra features needed now only by clang-tidy. But, there is also advantage in adding these into MatchFinder now for future non-clang-tidy users. That will be similar to the current optional MatchFinder's CheckProfiling and the set/getTraversalScope. Those features/interfaces started with one user/tool and now we see more users/tools. If they were defined in a child class specific to clangd or some profiler, we will now need to rename or move those features to a common base class to share.
> 
> Please feel free to suggest any alternative, or let me know if you are okay with current optional filter in MatchFinder, or if you insist on defining a new child class like TidyMatchFinder.
> Thanks.
> 
TL;DR is I think this is a clang-tidy feature that should not be visible in the matcher layer in any way.
If it's going to touch ASTMatcher I think we should have a strong design reason or a very strong implementation reason. That it's kind of awkward to code and might one day be useful isn't sufficient.

There are four layers here: AST nodes themselves, RecursiveASTVisitor (which defines a tree via parent/child relationships), the matcher framework, and finally clang-tidy.
The mechanics of filtering using a list are solved in RecursiveASTVisitor.
There seem to be two issues here against solving the rest in the clang-tidy layer.

---

First is conceptual: is having filter APIs in the matcher framework good? I don't think so, because:
 - it's duplicative: it mirrors the setTraversalScope functionality and it's unclear when to use which. Having two ways to accomplish a rare task seems excessive.
 - it conflicts with the design of this layer: matchers are all about non-destructive reading, but setting traversal scope has side effects. So it's only really usable with the ASTConsumer, which is a corner of the interface.
 - it's not necessary: it's always possible to get at the underlying ASTContext wherever a MatchFinder will be used.

You suggest this might be good for "future non-clang-tidy users" but I don't think we should add it speculatively for this purpose. It seems at least as likely that such users will never materialize, or that the solution that is right for clang-tidy is not right for them.

---

Second is practical. That ClangTidyASTConsumerFactory::CreateASTConsumer needs to dispatch both to MatchFinder and the static analyzer's (non-trivial) ASTConsumer. We have to add a call to setTraversalScope somewhere, and none of the approaches are perfectly natural.

However there are several that are OK:
 - subclass MultiplexingASTConsumer to inject the call in HandleTranslationUnit
 - subclass MultiplexingASTConsumer and inject the call and also the matcher-based-checks logic in HandleTranslationUnit (so MAC delegates only to the static analyzer)
 - write an ASTConsumer without MultiplexingASTConsumer that manually forwards only the actually needed methods to the static analyzer consumer
 - write an unrelated ASTConsumer that forwards every method to the static analyzer consumer
 - refactor static analyzer to expose its consumer class, and subclass that (yuck!)

These do require making *some* assumptions about how these consumers work. But that's OK - there are two of them, not thousands, and these interfaces change rarely. We don't need to architect defensively against any possible future change, and that's not culturally how LLVM operates either. I'd lean towards e.g. the first option, which requires fewer assumptions.


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

https://reviews.llvm.org/D98710