[include-cleaner] Testing helpers for ast walking

[kadircet]

Summary

Address comments around verbosity of tests.

Depends on D135859

[hokein]

Just share my experience when reading the code, overall I have a feeling that we're regressing the code readability of the tests (left some comments on regressing samples):

1. with the sugared macros, the syntax of TargetCode and ReferenceCode are identical -- both using the ^ without any names, it is hard to distinguish them (we could use a different syntax [[]] in the ReferenceCode to identify the point);
2. the code of using EXPECT_REFERENCES_WITH_TYPES is not easy to read (especially with multiple RefType arguments), we're losing the relationship between RefType and ^ point, this is my biggest concern;
3. the EXPECT_{EXPLICIT, IMPLICIT, EXPLICIT}_REFERENCES is designed for a single reference point. When we have > 1 reference points, we have to use the fallback EXPECT_REFERENCES_WITH_TYPES even if all ref points are the same type (not sure it is generic enough, maybe it is fine as single-ref-point is the most common case);

That's being said, making the tests less verbose is good, but we seem to trade too much code readability for that. I'm fine with the original annotation version even though the annotations are somewhat noisy, another alternative -- if we use abbreviations like ($E, $I, $A) in tests, would it be better?

clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp

156

a bug in existing code? by looking at this example, we should use EXPECT_AMBIGUOUS_REFERENCES(Code, Code, RefType::Ambiguous, RefType::Ambiguous, RefType::Ambiguous)

234

I find this case is hard, and not easy for human to follow&verify -- we have 3 points in the target code, and their RefTypes are from the macro arguments.

279

When reading this code, the implicit vs explicit in these 4 consecutive statements is less obvious than before (each of them has the same length, same indentation and is a single-line)

289

It feels more natural to use EXPECT_AMBIGUOUS_REFERENCES(TargetCode, ReferenceCode, Ambiguous) for this case where all refs are the same type.

[kadircet]

In D137252#3907665, @hokein wrote:

Just share my experience when reading the code, overall I have a feeling that we're regressing the code readability of the tests (left some comments on regressing samples):

1. with the sugared macros, the syntax of TargetCode and ReferenceCode are identical -- both using the ^ without any names, it is hard to distinguish them (we could use a different syntax [[]] in the ReferenceCode to identify the point);

I see your point but this is not really a regression, right? that's the way the tests were written in the previous version as well. i am not sure how we can make this more explicit without introducing a bunch of boilerplate into each test case.

2. the code of using EXPECT_REFERENCES_WITH_TYPES is not easy to read (especially with multiple RefType arguments), we're losing the relationship between RefType and ^ point, this is my biggest concern;

That is one of the points I disliked the most as well, maybe we should keep the old test syntax for these cases (with abbreviations), i.e. rather than putting the types at the end, we'll put them into the annotations. WDYT?

3. the EXPECT_{EXPLICIT, IMPLICIT, EXPLICIT}_REFERENCES is designed for a single reference point. When we have > 1 reference points, we have to use the fallback EXPECT_REFERENCES_WITH_TYPES even if all ref points are the same type (not sure it is generic enough, maybe it is fine as single-ref-point is the most common case);

These macros are not for single references, but rather for single reference types. e.g. if all the references are of same kind.

clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp

156

no, that's actually a change in behaviour (in the base patch) but wasn't reflected here.

234

i agree. just to make my proposal in the main discussion more concrete, i am suggesting this:

EXPECT_REFERENCES_WITH_TYPES(
      R"cpp(
    namespace ns {
      void $A^x(); void $E^x(int); void $A^x(char);
    })cpp",
      "// c++-header\n using ns::^x; void foo() { x(3); }");

279

i am not sure if there's much difference between having them in the code vs in the macro names here actually.

the case on the left hand side of the diff is exactly the same, you've got implicit vs explicit right next to points, each with same length. IMO the author/reviewer needs to pay ~the same level of attention in either case.

289

right, that one should've been just EXPECT_AMBIGUOUS_REFERENCES, a residue left behind while i was tweaking the code

[hokein]

In D137252#3907743, @kadircet wrote:

In D137252#3907665, @hokein wrote:

Just share my experience when reading the code, overall I have a feeling that we're regressing the code readability of the tests (left some comments on regressing samples):

1. with the sugared macros, the syntax of TargetCode and ReferenceCode are identical -- both using the ^ without any names, it is hard to distinguish them (we could use a different syntax [[]] in the ReferenceCode to identify the point);

I see your point but this is not really a regression, right? that's the way the tests were written in the previous version as well. i am not sure how we can make this more explicit without introducing a bunch of boilerplate into each test case.

In the previous version, we can easily distinguish the TargetCode and ReferenceCode by looking at annotation $name in the code, while in the current version, we have to memorize the function argument order.

One alternative will be to merge TargetCode and ReferenceCode together:

int ^x; // using ^ as a reference point to the target symbol

int y = [[]]x; // using [[]]] as a ref point to the reference.

2. the code of using EXPECT_REFERENCES_WITH_TYPES is not easy to read (especially with multiple RefType arguments), we're losing the relationship between RefType and ^ point, this is my biggest concern;

That is one of the points I disliked the most as well, maybe we should keep the old test syntax for these cases (with abbreviations), i.e. rather than putting the types at the end, we'll put them into the annotations. WDYT?

It can work, probably it is ok, it looks like -- we keep the original annotation approach (with shorter names), and provide some syntax sugar for common cases. (though I personally prefer to use the shortened-annotation approach everywhere). We might want a third opinion @sammccall.

3. the EXPECT_{EXPLICIT, IMPLICIT, EXPLICIT}_REFERENCES is designed for a single reference point. When we have > 1 reference points, we have to use the fallback EXPECT_REFERENCES_WITH_TYPES even if all ref points are the same type (not sure it is generic enough, maybe it is fine as single-ref-point is the most common case);

These macros are not for single references, but rather for single reference types. e.g. if all the references are of same kind.

I see, I got confusing when reading the EXPECT_REFERENCES_WITH_TYPES("", "", Ambiguous, Ambiguous) code.

clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp

156

I don't get it. My understanding is

1. this patch is basically a NFC change on top of the https://reviews.llvm.org/D135859 and in https://reviews.llvm.org/D135859, we use three ambiguous ref types
2. and here, we are changing one of ambiguous ref types to an explicit type.

1 and 2 is conflicted, do you mean in the base version, it should be ambiguous, explicit, ambiguous?

279

I think there is a difference -- in the previous version, you don't need to read the function name, you can see the reference type from the ^ point in the reference code snippet, while in the current version, you first identify the ^ point, and *move* our eyeball to the macro name (extra step).

Maybe we can use shorter names for these macros (e.g. EXPECT_IMPLICIT, EXPECT_EXPLICIT?).

[sammccall]

Agree there are readability concerns here (long macro names, out-of-line kinds, complicated macros).

My suggestion would be to treat RefKind as an optional feature. Most tests would just assert the set of marked locations and test the cases where kind is interesting separately.

string markReferences(string TargetCode, string ReferencingCode, bool WithKind);
// markReferences("int x;", "int y = ^x;", false) => "int ^x;"
// markReferences("int x;", "int y = ^x;", true) => "int $explicit^x;"

#define EXPECT_REFS(T, R) EXPECT_EQ(T, markReferences(Annotations(T).code(), R, false)) << R;
#define EXPECT_TYPED_REFS(T, R) EXPECT_EQ(T, markReferences(Annotations(T).code(), R, true)) << R;

clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp

248

It's probably OK to use raw-strings inside macros these days...
Some versions of GCC had a problem with it, but the c++17 bump may have taken care of those.

clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp

 //===--- WalkASTTest.cpp ------------------------------------------- C++-*-===//
 //
 // 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 "AnalysisInternal.h"
+#include "clang-include-cleaner/Types.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/Basic/FileManager.h"
+#include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
 #include "clang/Frontend/TextDiagnostic.h"
 #include "clang/Testing/TestAST.h"
 #include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/Twine.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Testing/Support/Annotations.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include <cstddef>
+#include <map>
 #include <unordered_map>
 #include <utility>
 #include <vector>
 
 namespace clang::include_cleaner {
 namespace {
 using testing::Pair;
 using testing::UnorderedElementsAre;
 
 llvm::StringLiteral to_string(RefType RT) {
   switch (RT) {
   case RefType::Explicit:
     return "explicit";
   case RefType::Implicit:
     return "implicit";
   case RefType::Ambiguous:
     return "ambiguous";
   }
   llvm_unreachable("Unexpected RefType");
 }
 
-// Specifies a test of which symbols are referenced by a piece of code.
-// If `// c++-header` is present, treats referencing code as a header file.
-// Target should contain points annotated with the reference kind.
-// Example:
-//   Target:      int $explicit^foo();
-//   Referencing: int x = ^foo();
-// There must be exactly one referencing location marked.
-void testWalk(llvm::StringRef TargetCode, llvm::StringRef ReferencingCode) {
-  llvm::Annotations Target(TargetCode);
-  llvm::Annotations Referencing(ReferencingCode);
-
-  TestInputs Inputs(Referencing.code());
-  Inputs.ExtraFiles["target.h"] = Target.code().str();
+// Finds all the references to Target from Referencing.point().
+std::vector<std::pair<size_t, RefType>>
+referencedPoints(llvm::StringRef TargetCode, llvm::StringRef ReferencingCode,
+                 size_t ReferencingOffset) {
+  TestInputs Inputs(ReferencingCode);
+  Inputs.ExtraFiles["target.h"] = TargetCode.str();
   Inputs.ExtraArgs.push_back("-include");
   Inputs.ExtraArgs.push_back("target.h");
   Inputs.ExtraArgs.push_back("-std=c++17");
-  if (Referencing.code().contains("// c++-header\n"))
+  if (ReferencingCode.contains("// c++-header\n"))
     Inputs.ExtraArgs.push_back("-xc++-header");
   TestAST AST(Inputs);
   const auto &SM = AST.sourceManager();
 
   // We're only going to record references from the nominated point,
   // to the target file.
   FileID ReferencingFile = SM.getMainFileID();
   SourceLocation ReferencingLoc =
-      SM.getComposedLoc(ReferencingFile, Referencing.point());
+      SM.getComposedLoc(ReferencingFile, ReferencingOffset);
   FileID TargetFile = SM.translateFile(
       llvm::cantFail(AST.fileManager().getFileRef("target.h")));
 
   // Perform the walk, and capture the offsets of the referenced targets.
-  std::unordered_map<RefType, std::vector<size_t>> ReferencedOffsets;
+  std::map<size_t, RefType> ActualPoints;
   for (Decl *D : AST.context().getTranslationUnitDecl()->decls()) {
     if (ReferencingFile != SM.getDecomposedExpansionLoc(D->getLocation()).first)
       continue;
     walkAST(*D, [&](SourceLocation Loc, NamedDecl &ND, RefType RT) {
       if (SM.getFileLoc(Loc) != ReferencingLoc)
         return;
       auto NDLoc = SM.getDecomposedLoc(SM.getFileLoc(ND.getLocation()));
       if (NDLoc.first != TargetFile)
         return;
-      ReferencedOffsets[RT].push_back(NDLoc.second);
+      ActualPoints[NDLoc.second] = RT;
     });
   }
-  for (auto &Entry : ReferencedOffsets)
-    llvm::sort(Entry.second);
+  return {ActualPoints.begin(), ActualPoints.end()};
+}
 
+// Runs on the output of referencedPoints. Compares it to offsets n TargetCode
+// and reftype sequence in Types.
+MATCHER_P2(TargetsWithTypes, /*llvm::Annotations*/ Target,
+           /*std::vector<RefType>*/ Types, "") {
+  auto TargetPoints = Target.points();
+  if (TargetPoints.size() != Types.size()) {
+    *result_listener << "Mismatched annotations(" << TargetPoints.size()
+                     << ") vs types(" << Types.size() << ")";
+    return false;
+  }
+  auto ExpectedPoints = llvm::zip(TargetPoints, Types);
+  std::vector<std::pair<size_t, RefType>> ActualPoints = arg;
   // Compare results to the expected points.
   // For each difference, show the target point in context, like a diagnostic.
   std::string DiagBuf;
   llvm::raw_string_ostream DiagOS(DiagBuf);
   auto *DiagOpts = new DiagnosticOptions();
   DiagOpts->ShowLevel = 0;
   DiagOpts->ShowNoteIncludeStack = 0;
-  TextDiagnostic Diag(DiagOS, AST.context().getLangOpts(), DiagOpts);
-  auto DiagnosePoint = [&](llvm::StringRef Message, unsigned Offset) {
+  TextDiagnostic Diag(DiagOS, LangOptions(), DiagOpts);
+  SourceManagerForFile SM("target-code.h", Target.code());
+  auto DiagnosePoint = [&](llvm::Twine Message, unsigned Offset) {
     Diag.emitDiagnostic(
-        FullSourceLoc(SM.getComposedLoc(TargetFile, Offset), SM),
-        DiagnosticsEngine::Note, Message, {}, {});
+        FullSourceLoc(SM.get().getComposedLoc(SM.get().getMainFileID(), Offset),
+                      SM.get()),
+        DiagnosticsEngine::Note, Message.str(), {}, {});
   };
-  for (auto RT : {RefType::Explicit, RefType::Implicit, RefType::Ambiguous}) {
-    auto RTStr = to_string(RT);
-    for (auto Expected : Target.points(RTStr))
-      if (!llvm::is_contained(ReferencedOffsets[RT], Expected))
-        DiagnosePoint(("location not marked used with type " + RTStr).str(),
-                      Expected);
-    for (auto Actual : ReferencedOffsets[RT])
-      if (!llvm::is_contained(Target.points(RTStr), Actual))
-        DiagnosePoint(("location unexpectedly used with type " + RTStr).str(),
-                      Actual);
+  auto ExpectedIt = ExpectedPoints.begin();
+  auto ActualIt = ActualPoints.begin();
+  while (ExpectedIt != ExpectedPoints.end() && ActualIt != ActualPoints.end()) {
+    auto [ExpectedOff, ExpectedType] = *ExpectedIt;
+    auto [ActualOff, ActualType] = *ActualIt;
+    if (ExpectedOff == ActualOff) {
+      if (ExpectedType != ActualType) {
+        DiagnosePoint("location marked as used with type " +
+                          to_string(ActualType) + " but expected " +
+                          to_string(ExpectedType),
+                      ExpectedOff);
+      }
+      ++ExpectedIt;
+      ++ActualIt;
+    } else if (ExpectedOff < ActualOff) {
+      DiagnosePoint("location not marked used with type " +
+                        to_string(ExpectedType),
+                    ExpectedOff);
+      ++ExpectedIt;
+    } else {
+      DiagnosePoint("location unexpectedly used with type " +
+                        to_string(ActualType),
+                    ActualOff);
+      ++ActualIt;
+    }
+  }
+  while (ExpectedIt != ExpectedPoints.end()) {
+    DiagnosePoint("location not marked used with type " +
+                      to_string(std::get<1>(*ExpectedIt)),
+                  std::get<0>(*ExpectedIt));
+    ++ExpectedIt;
+  }
+  while (ActualIt != ActualPoints.end()) {
+    DiagnosePoint("location unexpectedly used with type " +
+                      to_string(ActualIt->second),
+                  ActualIt->first);
+    ++ActualIt;
   }
 
-  // If there were any differences, we print the entire referencing code once.
   if (!DiagBuf.empty())
-    ADD_FAILURE() << DiagBuf << "\nfrom code:\n" << ReferencingCode;
+    *result_listener << DiagBuf;
+  return DiagBuf.empty();
 }
 
+// Specifies a test of which symbols are referenced by a piece of code.
+// If `// c++-header` is present, treats referencing code as a header file.
+// Target should contain points that are used by the point in ReferencingCode.
+// Example:
+//   Target:      int ^foo();
+//   Referencing: int x = ^foo();
+// There must be exactly one referencing location marked.
+#define EXPECT_REFERENCES_WITH_TYPES(TargetCode, ReferencingCode, ...)         \
+  {                                                                            \
+    llvm::Annotations Target(TargetCode);                                      \
+    llvm::Annotations Referencing(ReferencingCode);                            \
+    EXPECT_THAT(referencedPoints(Target.code(), Referencing.code(),            \
+                                 Referencing.point()),                         \
+                TargetsWithTypes(Target, std::vector<RefType>({__VA_ARGS__}))) \
+        << "from code:\n"                                                      \
+        << ReferencingCode;                                                    \
+  }
+
+// Convenience macros for matching cases where all references are of the same
+// type.
+#define EXPECT_REFERENCES_WITH_TYPE(TargetCode, ReferencingCode, Type)         \
+  EXPECT_REFERENCES_WITH_TYPES(                                                \
+      TargetCode, ReferencingCode,                                             \
+      std::vector<RefType>(llvm::Annotations(TargetCode).points().size(),      \
+                           Type));
+#define EXPECT_EXPLICIT_REFERENCES(TargetCode, ReferencingCode)                \
+  EXPECT_REFERENCES_WITH_TYPE(TargetCode, ReferencingCode, RefType::Explicit)
+#define EXPECT_IMPLICIT_REFERENCES(TargetCode, ReferencingCode)                \
+  EXPECT_REFERENCES_WITH_TYPE(TargetCode, ReferencingCode, RefType::Implicit)
+#define EXPECT_AMBIGUOUS_REFERENCES(TargetCode, ReferencingCode)               \
+  EXPECT_REFERENCES_WITH_TYPE(TargetCode, ReferencingCode, RefType::Ambiguous)
+
 TEST(WalkAST, DeclRef) {
-  testWalk("int $explicit^x;", "int y = ^x;");
-  testWalk("int $explicit^foo();", "int y = ^foo();");
-  testWalk("namespace ns { int $explicit^x; }", "int y = ns::^x;");
-  testWalk("struct S { static int $explicit^x; };", "int y = S::^x;");
+  EXPECT_EXPLICIT_REFERENCES("int ^x;", "int y = ^x;");
+  EXPECT_EXPLICIT_REFERENCES("int ^x;", "int y = ^x;");
+  EXPECT_EXPLICIT_REFERENCES("int ^foo();", "int y = ^foo();");
+  EXPECT_EXPLICIT_REFERENCES("namespace ns { int ^x; }", "int y = ns::^x;");
+  EXPECT_EXPLICIT_REFERENCES("struct S { static int ^x; };", "int y = S::^x;");
   // Canonical declaration only.
-  testWalk("extern int $explicit^x; int x;", "int y = ^x;");
+  EXPECT_EXPLICIT_REFERENCES("extern int ^x; int x;", "int y = ^x;");
   // Return type of `foo` isn't used.
-  testWalk("struct S{}; S $explicit^foo();", "auto bar() { return ^foo(); }");
+  EXPECT_EXPLICIT_REFERENCES("struct S{}; S ^foo();",
+                             "auto bar() { return ^foo(); }");
 }
 
 TEST(WalkAST, TagType) {
-  testWalk("struct $explicit^S {};", "^S *y;");
-  testWalk("enum $explicit^E {};", "^E *y;");
-  testWalk("struct $explicit^S { static int x; };", "int y = ^S::x;");
+  EXPECT_EXPLICIT_REFERENCES("struct ^S {};", "^S *y;");
+  EXPECT_EXPLICIT_REFERENCES("enum ^E {};", "^E *y;");
+  EXPECT_EXPLICIT_REFERENCES("struct ^S { static int x; };", "int y = ^S::x;");
 }
 
 TEST(WalkAST, Alias) {
-  testWalk(R"cpp(
+  EXPECT_EXPLICIT_REFERENCES(R"cpp(
     namespace ns { int x; }
-    using ns::$explicit^x;
+    using ns::^x;
   )cpp",
                              "int y = ^x;");
-  testWalk("using $explicit^foo = int;", "^foo x;");
-  testWalk("struct S {}; using $explicit^foo = S;", "^foo x;");
+  EXPECT_EXPLICIT_REFERENCES("using ^foo = int;", "^foo x;");
+  EXPECT_EXPLICIT_REFERENCES("struct S {}; using ^foo = S;", "^foo x;");
 }
 
 TEST(WalkAST, Using) {
   // Make sure we ignore unused overloads.
-  testWalk(R"cpp(
+  EXPECT_EXPLICIT_REFERENCES(R"cpp(
     namespace ns {
-      void $explicit^x(); void x(int); void x(char);
+      void ^x(); void x(int); void x(char);
     })cpp",
                              "using ns::^x; void foo() { x(); }");
   // We should report unused overloads if main file is a header.
-  testWalk(R"cpp(
+  EXPECT_REFERENCES_WITH_TYPES(
+      R"cpp(
     namespace ns {
-      void $ambiguous^x(); void $ambiguous^x(int); void $ambiguous^x(char);
+      void ^x(); void ^x(int); void ^x(char);
     })cpp",
-           "// c++-header\n using ns::^x;");
-  testWalk("namespace ns { struct S; } using ns::$explicit^S;", "^S *s;");
+      "// c++-header\n using ns::^x; void foo() { x(3); }", RefType::Ambiguous,
+      RefType::Explicit, RefType::Ambiguous);
+  EXPECT_EXPLICIT_REFERENCES("namespace ns { struct S; } using ns::^S;",
+                             "^S *s;");
 }
 
 TEST(WalkAST, Namespaces) {
-  testWalk("namespace ns { void x(); }", "using namespace ^ns;");
+  EXPECT_EXPLICIT_REFERENCES("namespace ns { void x(); }",
+                             "using namespace ^ns;");
 }
 
 TEST(WalkAST, TemplateNames) {
-  testWalk("template<typename> struct $explicit^S {};", "^S<int> s;");
+  EXPECT_EXPLICIT_REFERENCES("template<typename> struct ^S {};", "^S<int> s;");
   // FIXME: Template decl has the wrong primary location for type-alias template
   // decls.
-  testWalk(R"cpp(
+  EXPECT_EXPLICIT_REFERENCES(R"cpp(
       template <typename> struct S {};
-      template <typename T> $explicit^using foo = S<T>;)cpp",
+      template <typename T> ^using foo = S<T>;)cpp",
                              "^foo<int> x;");
-  testWalk(R"cpp(
+  EXPECT_EXPLICIT_REFERENCES(R"cpp(
       namespace ns {template <typename> struct S {}; }
-      using ns::$explicit^S;)cpp",
+      using ns::^S;)cpp",
                              "^S<int> x;");
-  testWalk("template<typename> struct $explicit^S {};",
+  EXPECT_EXPLICIT_REFERENCES("template<typename> struct ^S {};",
                              R"cpp(
       template <template <typename> typename> struct X {};
       X<^S> x;)cpp");
-  testWalk("template<typename T> struct $explicit^S { S(T); };", "^S s(42);");
+  EXPECT_EXPLICIT_REFERENCES("template<typename T> struct ^S { S(T); };",
+                             "^S s(42);");
   // Should we mark the specialization instead?
-  testWalk(
-      "template<typename> struct $explicit^S {}; template <> struct S<int> {};",
+  EXPECT_EXPLICIT_REFERENCES(
+      "template<typename> struct ^S {}; template <> struct S<int> {};",
       "^S<int> s;");
 }
 
 TEST(WalkAST, MemberExprs) {
-  testWalk("struct S { void $explicit^foo(); };", "void foo() { S{}.^foo(); }");
-  testWalk(
-      "struct S { void foo(); }; struct X : S { using S::$explicit^foo; };",
+  EXPECT_EXPLICIT_REFERENCES("struct S { void ^foo(); };",
+                             "void foo() { S{}.^foo(); }");
+  EXPECT_EXPLICIT_REFERENCES(
+      "struct S { void foo(); }; struct X : S { using S::^foo; };",
       "void foo() { X{}.^foo(); }");
 }
 
 TEST(WalkAST, ConstructExprs) {
-  testWalk("struct $implicit^S {};", "S ^t;");
-  testWalk("struct S { $implicit^S(); };", "S ^t;");
-  testWalk("struct S { $explicit^S(int); };", "S ^t(42);");
-  testWalk("struct S { $implicit^S(int); };", "S t = ^42;");
+  EXPECT_IMPLICIT_REFERENCES("struct ^S {};", "S ^t;");
+  EXPECT_IMPLICIT_REFERENCES("struct S { ^S(); };", "S ^t;");
+  EXPECT_EXPLICIT_REFERENCES("struct S { ^S(int); };", "S ^t(42);");
+  EXPECT_IMPLICIT_REFERENCES("struct S { ^S(int); };", "S t = ^42;");
 }
 
 TEST(WalkAST, Functions) {
   // Definition uses declaration, not the other way around.
-  testWalk("void $explicit^foo();", "void ^foo() {}");
-  testWalk("void foo() {}", "void ^foo();");
+  EXPECT_EXPLICIT_REFERENCES("void ^foo();", "void ^foo() {}");
+  EXPECT_EXPLICIT_REFERENCES("void foo() {}", "void ^foo();");
 
   // Unresolved calls marks all the overloads.
-  testWalk("void $ambiguous^foo(int); void $ambiguous^foo(char);",
-           "template <typename T> void bar() { ^foo(T{}); }");
+  EXPECT_REFERENCES_WITH_TYPES(
+      "void ^foo(int); void ^foo(char);",
+      "template <typename T> void bar() { ^foo(T{}); }", RefType::Ambiguous,
+      RefType::Ambiguous);
 }
 
 TEST(WalkAST, Enums) {
-  testWalk("enum E { $explicit^A = 42, B = 43 };", "int e = ^A;");
-  testWalk("enum class $explicit^E : int;", "enum class ^E : int {};");
-  testWalk("enum class E : int {};", "enum class ^E : int ;");
+  EXPECT_EXPLICIT_REFERENCES("enum E { ^A = 42, B = 43 };", "int e = ^A;");
+  EXPECT_EXPLICIT_REFERENCES("enum class ^E : int;", "enum class ^E : int {};");
+  EXPECT_EXPLICIT_REFERENCES("enum class E : int {};", "enum class ^E : int ;");
 }
 
 } // namespace
 } // namespace clang::include_cleaner