[AST] Fix RecursiveASTVisitor visiting implicit constructor initializers.

[jvikstrom]

Summary

RecursiveASTVisitor was visiting implcit constructor initializers. When the TypeLoc for the implicit construcor intializer was visited the SourceLocation would be invalid. This caused semantic highlighting in clangd to emit error logs.

Fixes this by checking if the constructor is written or if the visitor should visit implicit decls.

[hokein]

I think this is a reasonable fix, just a few comments on the test.

clang/unittests/Tooling/RecursiveASTVisitorTests/InitListExprPreOrder.cpp

26 ↗

(On Diff #213285)

there may be other reasons to cause an invalid source location. I think we can use the Init->isWritten() to verify the implicit initializer is being visited.

37 ↗

(On Diff #213285)

maybe name it VisitedImplicitInitializer?

62 ↗

(On Diff #213285)

nit: inline the Config.

[ilya-biryukov]

The fix looks good.

clang/unittests/Tooling/RecursiveASTVisitorTests/InitListExprPreOrder.cpp

17 ↗

(On Diff #213285)

Could you create a new test?
This test is clearly meant to test only InitListExpr, we should keep it focused on one thing.

[jvikstrom]

Moved test to its own test file (also addressed comments about the test).

[jvikstrom]

Call the base Traverse.. function in the visitor.

[hokein]

clang/unittests/Tooling/RecursiveASTVisitorTests/ImplicitCtorInitializer.cpp

2 ↗

(On Diff #213294)

we should make them one line even it exceeds 80 columns.

3 ↗

(On Diff #213294)

I think you forgot to add the new file to the cmake file.

[ilya-biryukov]

LGTM from my side, a few optional NITs.
Feel free to land as soon as @hokein stamps.

clang/unittests/Tooling/RecursiveASTVisitorTests/ImplicitCtorInitializer.cpp

16 ↗

(On Diff #213294)

NIT: this comment is about the TEST rather than the helper class. Maybe move it there?

29 ↗

(On Diff #213294)

NIT: alternatively use different match identifiers for written and unwritten initializers:

if (Init->isWritten())
  Match("written-inititiazlier", ...);
else
  Match("implicit-initializer", ...);

...
TEST() {
  Visitor.expectMatch("written-initializer");
  Visitor.disallowMatch("implicit-initializer"); // would that work with invalid source locs, though?
}

This allows to reuse the mechanism used by other tests without extra code. But up to you, definitely not a big deal.

[jvikstrom]

Address comments.

[jvikstrom]

clang/unittests/Tooling/RecursiveASTVisitorTests/ImplicitCtorInitializer.cpp

29 ↗

(On Diff #213294)

disallowMatch requires a valid SourceLocation otherwise that would definitely be the way to do it.

Maybe that is something that should be added (the ability to disallow matches no matter what location) to ExpectedLocationVisitor?

[hokein]

clang/unittests/Tooling/RecursiveASTVisitorTests/ImplicitCtorInitializer.cpp

26 ↗

(On Diff #213323)

is it intentional to run for case !Init->IsWritten() where the source location is invalid?

cfe/trunk/include/clang/AST/RecursiveASTVisitor.h

if (TypeSourceInfo *TSI = D->getTypeSourceInfo()) {
     for (ParmVarDecl *Parameter : D->parameters()) {
       TRY_TO(TraverseDecl(Parameter));
     }
   }
 
   if (CXXConstructorDecl *Ctor = dyn_cast<CXXConstructorDecl>(D)) {
     // Constructor initializers.
     for (auto *I : Ctor->inits()) {
+      if (I->isWritten() || getDerived().shouldVisitImplicitCode())
         TRY_TO(TraverseConstructorInitializer(I));
     }
   }
 
   if (D->isThisDeclarationADefinition()) {
     TRY_TO(TraverseStmt(D->getBody())); // Function body.
   }
   return true;
 }

cfe/trunk/unittests/Tooling/CMakeLists.txt

add_clang_unittest(ToolingTests
   RecursiveASTVisitorTests/Attr.cpp
   RecursiveASTVisitorTests/Class.cpp
   RecursiveASTVisitorTests/ConstructExpr.cpp
   RecursiveASTVisitorTests/CXXBoolLiteralExpr.cpp
   RecursiveASTVisitorTests/CXXMemberCall.cpp
   RecursiveASTVisitorTests/CXXOperatorCallExprTraverser.cpp
   RecursiveASTVisitorTests/DeclRefExpr.cpp
   RecursiveASTVisitorTests/ImplicitCtor.cpp
+  RecursiveASTVisitorTests/ImplicitCtorInitializer.cpp
   RecursiveASTVisitorTests/InitListExprPostOrder.cpp
   RecursiveASTVisitorTests/InitListExprPostOrderNoQueue.cpp
   RecursiveASTVisitorTests/InitListExprPreOrder.cpp
   RecursiveASTVisitorTests/InitListExprPreOrderNoQueue.cpp
   RecursiveASTVisitorTests/IntegerLiteral.cpp
   RecursiveASTVisitorTests/LambdaDefaultCapture.cpp
   RecursiveASTVisitorTests/LambdaExpr.cpp
   RecursiveASTVisitorTests/LambdaTemplateParams.cpp

cfe/trunk/unittests/Tooling/RecursiveASTVisitorTests/ImplicitCtorInitializer.cpp

+//=- unittest/Tooling/RecursiveASTVisitorTests/ImplicitCtorInitializer.cpp -=//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "TestVisitor.h"
+
+using namespace clang;
+
+namespace {
+
+class CXXCtorInitializerVisitor
+    : public ExpectedLocationVisitor<CXXCtorInitializerVisitor> {
+public:
+  CXXCtorInitializerVisitor(bool VisitImplicitCode)
+      : VisitImplicitCode(VisitImplicitCode) {}
+
+  bool shouldVisitImplicitCode() const { return VisitImplicitCode; }
+
+  bool TraverseConstructorInitializer(CXXCtorInitializer *Init) {
+    if (!Init->isWritten())
+      VisitedImplicitInitializer = true;
+    Match("initializer", Init->getSourceLocation());
+    return ExpectedLocationVisitor<
+        CXXCtorInitializerVisitor>::TraverseConstructorInitializer(Init);
+  }
+
+  bool VisitedImplicitInitializer = false;
+
+private:
+  bool VisitImplicitCode;
+};
+
+// Check to ensure that CXXCtorInitializer is not visited when implicit code
+// should not be visited and that it is visited when implicit code should be
+// visited.
+TEST(RecursiveASTVisitor, CXXCtorInitializerVisitNoImplicit) {
+  for (bool VisitImplCode : {true, false}) {
+    CXXCtorInitializerVisitor Visitor(VisitImplCode);
+    Visitor.ExpectMatch("initializer", 7, 17);
+    EXPECT_TRUE(Visitor.runOver(R"cpp(
+        class A {};
+        class B : public A {
+          B() {};
+        };
+        class C : public A {
+          C() : A() {}
+        };
+      )cpp",
+                                CXXCtorInitializerVisitor::Lang_CXX));
+    EXPECT_EQ(Visitor.VisitedImplicitInitializer, VisitImplCode);
+  }
+}
+} // end anonymous namespace