[clang][nfc] Define ConstRecursiveASTVisitor twin of RecursiveASTVisitor

Downstream whenever we reach out for a RecursiveASTVisitor we always
have to add a few const_casts to shoe it in. This NFC patch introduces a
const version of the same CRTP class.

To reduce code duplication, I factored out all the common logic (which is
all of it) into `RecursiveASTVisitorBase` and made `RecursiveASTVisitor`
and `ConstRecursiveASTVisitor` essentially the two instances of it, that
you should use depending on whether you want to modify AST in your
visitor.

This is very similar to the DynamicRecursiveASTVisitor structure. One
point of difference is that instead of type aliases I use inheritance to
reduce the diff of this change because templated alias is not accepted
in the implementation forwarding of the overridden member functions:
`return RecursiveASTVisitor::TraverseStmt(S);` works only if
`RecursiveASTVisitor` is defined as a derived class of
`RecursiveASTVisitorBase` and not as a parametric alias. This was not an
issue for DynamicRecursiveASTVisitor because it is not parametrised
bythe `Derived` type.

Unfortunately, I did not manager to maintain a full backwards
compatibility when it comes to the `friend` declarations, you have to
befriend the `RecursiveASTVisitorBase` and not `RecursiveASTVisitor`.
Moreover, the error message is not obvious, as it speaks of the member
function being private and does not point to the `friend` declaration.

Author: necto

clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.h

@@ -72,7 +72,7 @@ class StmtAncestorASTVisitor
   /// Accessor for DeclParents.
   const DeclParentMap &getDeclToParentStmtMap() { return DeclParents; }
 
-  friend class clang::RecursiveASTVisitor<StmtAncestorASTVisitor>;
+  friend class clang::RecursiveASTVisitorBase<StmtAncestorASTVisitor, /*Const=*/false>;
 
 private:
   StmtParentMap StmtAncestors;
@@ -98,7 +98,7 @@ class ComponentFinderASTVisitor
   /// Accessor for Components.
   const ComponentVector &getComponents() { return Components; }
 
-  friend class clang::RecursiveASTVisitor<ComponentFinderASTVisitor>;
+  friend class clang::RecursiveASTVisitorBase<ComponentFinderASTVisitor, /*Const=*/false>;
 
 private:
   ComponentVector Components;
@@ -155,7 +155,7 @@ class DependencyFinderASTVisitor
     return DependsOnInsideVariable;
   }
 
-  friend class clang::RecursiveASTVisitor<DependencyFinderASTVisitor>;
+  friend class clang::RecursiveASTVisitorBase<DependencyFinderASTVisitor, /*Const=*/false>;
 
 private:
   const StmtParentMap *StmtParents;
@@ -188,7 +188,7 @@ class DeclFinderASTVisitor
     return Found;
   }
 
-  friend class clang::RecursiveASTVisitor<DeclFinderASTVisitor>;
+  friend class clang::RecursiveASTVisitorBase<DeclFinderASTVisitor, /*Const=*/false>;
 
 private:
   std::string Name;
@@ -340,7 +340,7 @@ class ForLoopIndexUseVisitor
 private:
   /// Typedef used in CRTP functions.
   using VisitorBase = RecursiveASTVisitor<ForLoopIndexUseVisitor>;
-  friend class RecursiveASTVisitor<ForLoopIndexUseVisitor>;
+  friend class RecursiveASTVisitorBase<ForLoopIndexUseVisitor, /*Const=*/false>;
 
   /// Overriden methods for RecursiveASTVisitor's traversal.
   bool TraverseArraySubscriptExpr(ArraySubscriptExpr *E);

clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp

@@ -95,7 +95,7 @@ static bool paramReferredExactlyOnce(const CXXConstructorDecl *Ctor,
   /// \see ExactlyOneUsageVisitor::hasExactlyOneUsageIn()
   class ExactlyOneUsageVisitor
       : public RecursiveASTVisitor<ExactlyOneUsageVisitor> {
-    friend class RecursiveASTVisitor<ExactlyOneUsageVisitor>;
+    friend class RecursiveASTVisitorBase<ExactlyOneUsageVisitor, /*Const=*/false>;
 
   public:
     ExactlyOneUsageVisitor(const ParmVarDecl *ParamDecl)

clang/include/clang/AST/RecursiveASTVisitor.h

@@ -81,7 +81,6 @@ class OpenACCConstructStmt : public Stmt {
 class OpenACCAssociatedStmtConstruct : public OpenACCConstructStmt {
   friend class ASTStmtWriter;
   friend class ASTStmtReader;
-  template <typename Derived> friend class RecursiveASTVisitor;
   Stmt *AssociatedStmt = nullptr;
 
 protected:

clang/include/clang/AST/StmtOpenACC.h

@@ -363,7 +363,7 @@ class ParentMapContext::ParentMap::ASTVisitor
   ASTVisitor(ParentMap &Map) : Map(Map) {}
 
 private:
-  friend class RecursiveASTVisitor<ASTVisitor>;
+  friend class RecursiveASTVisitorBase<ASTVisitor, false>;
 
   using VisitorBase = RecursiveASTVisitor<ASTVisitor>;
 

clang/lib/AST/ParentMapContext.cpp

@@ -508,7 +508,7 @@ class BodyIndexer : public RecursiveASTVisitor<BodyIndexer> {
   bool TraverseTypeConstraint(const TypeConstraint *C) {
     IndexCtx.handleReference(C->getNamedConcept(), C->getConceptNameLoc(),
                              Parent, ParentDC);
-    return RecursiveASTVisitor::TraverseTypeConstraint(C);
+    return base::TraverseTypeConstraint(C);
   }
 };
 

clang/lib/Index/IndexBody.cpp

@@ -143,6 +143,82 @@ std::vector<VisitEvent> collectEvents(llvm::StringRef Code,
       Code, FileName);
   return std::move(Visitor).takeEvents();
 }
+class ConstCollectInterestingEvents
+    : public ConstRecursiveASTVisitor<ConstCollectInterestingEvents> {
+public:
+  bool TraverseFunctionDecl(const FunctionDecl *D) {
+    Events.push_back(VisitEvent::StartTraverseFunction);
+    bool Ret = ConstRecursiveASTVisitor::TraverseFunctionDecl(D);
+    Events.push_back(VisitEvent::EndTraverseFunction);
+
+    return Ret;
+  }
+
+  bool TraverseAttr(const Attr *A) {
+    Events.push_back(VisitEvent::StartTraverseAttr);
+    bool Ret = ConstRecursiveASTVisitor::TraverseAttr(A);
+    Events.push_back(VisitEvent::EndTraverseAttr);
+
+    return Ret;
+  }
+
+  bool TraverseEnumDecl(const EnumDecl *D) {
+    Events.push_back(VisitEvent::StartTraverseEnum);
+    bool Ret = ConstRecursiveASTVisitor::TraverseEnumDecl(D);
+    Events.push_back(VisitEvent::EndTraverseEnum);
+
+    return Ret;
+  }
+
+  bool TraverseTypedefTypeLoc(TypedefTypeLoc TL, bool TraverseQualifier) {
+    Events.push_back(VisitEvent::StartTraverseTypedefType);
+    bool Ret =
+        ConstRecursiveASTVisitor::TraverseTypedefTypeLoc(TL, TraverseQualifier);
+    Events.push_back(VisitEvent::EndTraverseTypedefType);
+
+    return Ret;
+  }
+
+  bool TraverseObjCInterfaceDecl(const ObjCInterfaceDecl *ID) {
+    Events.push_back(VisitEvent::StartTraverseObjCInterface);
+    bool Ret = ConstRecursiveASTVisitor::TraverseObjCInterfaceDecl(ID);
+    Events.push_back(VisitEvent::EndTraverseObjCInterface);
+
+    return Ret;
+  }
+
+  bool TraverseObjCProtocolDecl(const ObjCProtocolDecl *PD) {
+    Events.push_back(VisitEvent::StartTraverseObjCProtocol);
+    bool Ret = ConstRecursiveASTVisitor::TraverseObjCProtocolDecl(PD);
+    Events.push_back(VisitEvent::EndTraverseObjCProtocol);
+
+    return Ret;
+  }
+
+  bool TraverseObjCProtocolLoc(ObjCProtocolLoc ProtocolLoc) {
+    Events.push_back(VisitEvent::StartTraverseObjCProtocolLoc);
+    bool Ret = ConstRecursiveASTVisitor::TraverseObjCProtocolLoc(ProtocolLoc);
+    Events.push_back(VisitEvent::EndTraverseObjCProtocolLoc);
+
+    return Ret;
+  }
+
+  std::vector<VisitEvent> takeEvents() && { return std::move(Events); }
+
+private:
+  std::vector<VisitEvent> Events;
+};
+
+std::vector<VisitEvent> collectConstEvents(llvm::StringRef Code,
+                                           const Twine &FileName = "input.cc") {
+  ConstCollectInterestingEvents Visitor;
+  clang::tooling::runToolOnCode(
+      std::make_unique<ProcessASTAction>(
+          [&](const clang::ASTContext &Ctx) { Visitor.TraverseAST(Ctx); }),
+      Code, FileName);
+  return std::move(Visitor).takeEvents();
+}
+
 } // namespace
 
 TEST(RecursiveASTVisitorTest, AttributesInsideDecls) {
@@ -151,6 +227,7 @@ TEST(RecursiveASTVisitorTest, AttributesInsideDecls) {
 __attribute__((annotate("something"))) int foo() { return 10; }
   )cpp";
 
+  EXPECT_EQ(collectEvents(Code), collectConstEvents(Code));
   EXPECT_THAT(collectEvents(Code),
               ElementsAre(VisitEvent::StartTraverseFunction,
                           VisitEvent::StartTraverseAttr,
@@ -165,6 +242,7 @@ TEST(RecursiveASTVisitorTest, EnumDeclWithBase) {
   enum Bar : Foo;
   )cpp";
 
+  EXPECT_EQ(collectEvents(Code), collectConstEvents(Code));
   EXPECT_THAT(collectEvents(Code),
               ElementsAre(VisitEvent::StartTraverseEnum,
                           VisitEvent::StartTraverseTypedefType,
@@ -184,6 +262,7 @@ TEST(RecursiveASTVisitorTest, InterfaceDeclWithProtocols) {
   @end
   )cpp";
 
+  EXPECT_EQ(collectEvents(Code), collectConstEvents(Code));
   EXPECT_THAT(collectEvents(Code, "input.m"),
               ElementsAre(VisitEvent::StartTraverseObjCProtocol,
                           VisitEvent::EndTraverseObjCProtocol,
@@ -196,3 +275,53 @@ TEST(RecursiveASTVisitorTest, InterfaceDeclWithProtocols) {
                           VisitEvent::EndTraverseObjCProtocolLoc,
                           VisitEvent::EndTraverseObjCInterface));
 }
+
+TEST(ConstRecursiveASTVisitorTest, ConstCorrectness) {
+  // This test verifies that ConstRecursiveASTVisitor properly enforces
+  // const-correctness.
+  // The derived class defines const versions of the Visit* methods,
+  // and they should correctly override the default implementations,
+  // which is demonstrated by non-0 counters.
+
+  class ConstCorrectnessValidator
+      : public ConstRecursiveASTVisitor<ConstCorrectnessValidator> {
+  public:
+    bool VisitFunctionDecl(const FunctionDecl *D) {
+      FunctionDeclCount++;
+      return true;
+    }
+
+    bool VisitStmt(const Stmt *S) {
+      StmtCount++;
+      return true;
+    }
+
+    int getFunctionDeclCount() const { return FunctionDeclCount; }
+    int getStmtCount() const { return StmtCount; }
+
+  private:
+    int FunctionDeclCount = 0;
+    int StmtCount = 0;
+  };
+
+  llvm::StringRef Code = R"cpp(
+    int foo() {
+      return 42;
+    }
+    void bar() {
+      int x = 0;
+      x += 2;
+    }
+  )cpp";
+
+  ConstCorrectnessValidator Visitor;
+  clang::tooling::runToolOnCode(
+      std::make_unique<ProcessASTAction>(
+          [&](clang::ASTContext &Ctx) { Visitor.TraverseAST(Ctx); }),
+      Code);
+
+  // Verify that the visitor found the expected number of nodes
+  EXPECT_EQ(Visitor.getFunctionDeclCount(), 2); // foo and bar
+  // There are at least 3 statements: return 42; int x = 0; x += 2;
+  EXPECT_GE(Visitor.getStmtCount(), 3);
+}

clang/unittests/AST/RecursiveASTVisitorTest.cpp

@@ -3983,10 +3983,10 @@ void EmitClangAttrASTVisitor(const RecordKeeper &Records, raw_ostream &OS) {
     const Record &R = *Attr;
     if (!R.getValueAsBit("ASTNode"))
       continue;
-    OS << "  bool Traverse"
-       << R.getName() << "Attr(" << R.getName() << "Attr *A);\n";
-    OS << "  bool Visit"
-       << R.getName() << "Attr(" << R.getName() << "Attr *A) {\n"
+    OS << "  bool Traverse" << R.getName()
+       << "Attr(MaybeConst<" << R.getName() << "Attr> *A);\n";
+    OS << "  bool Visit" << R.getName()
+       << "Attr(MaybeConst<" << R.getName() << "Attr> *A) {\n"
        << "    return true; \n"
        << "  }\n";
   }
@@ -3998,9 +3998,9 @@ void EmitClangAttrASTVisitor(const RecordKeeper &Records, raw_ostream &OS) {
     if (!R.getValueAsBit("ASTNode"))
       continue;
 
-    OS << "template <typename Derived>\n"
-       << "bool VISITORCLASS<Derived>::Traverse"
-       << R.getName() << "Attr(" << R.getName() << "Attr *A) {\n"
+    OS << "template <typename Derived, bool IsConst>\n"
+       << "bool VISITORCLASS<Derived, IsConst>::Traverse" << R.getName()
+       << "Attr(MaybeConst<" << R.getName() << "Attr> *A) {\n"
        << "  if (!getDerived().VisitAttr(A))\n"
        << "    return false;\n"
        << "  if (!getDerived().Visit" << R.getName() << "Attr(A))\n"
@@ -4018,8 +4018,9 @@ void EmitClangAttrASTVisitor(const RecordKeeper &Records, raw_ostream &OS) {
   }
 
   // Write generic Traverse routine
-  OS << "template <typename Derived>\n"
-     << "bool VISITORCLASS<Derived>::TraverseAttr(Attr *A) {\n"
+  OS << "template <typename Derived, bool IsConst>\n"
+     << "bool VISITORCLASS<Derived, IsConst>::TraverseAttr("
+     << "MaybeConst<Attr> *A) {\n"
      << "  if (!A)\n"
      << "    return true;\n"
      << "\n"
@@ -4032,7 +4033,8 @@ void EmitClangAttrASTVisitor(const RecordKeeper &Records, raw_ostream &OS) {
 
     OS << "    case attr::" << R.getName() << ":\n"
        << "      return getDerived().Traverse" << R.getName() << "Attr("
-       << "cast<" << R.getName() << "Attr>(A));\n";
+       << "const_cast<MaybeConst<" << R.getName()
+       << "Attr> *>(static_cast<const " << R.getName() << "Attr *>(A)));\n";
   }
   OS << "  }\n";  // end switch
   OS << "  llvm_unreachable(\"bad attribute kind\");\n";