Add tests for sequences of callbacks that RecursiveASTVisitor produces

[gribozavr]

Summary

These tests show a bug: post-order traversal introduces an extra call to
WalkUp*, that is not present in pre-order traversal. I'm fixing this bug
in a follow-up commit.

[ymandel]

clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp

17

Add class comment?

18

Consider using an enum rather than a bool.

[eduucaldas]

clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp

2

I find this name too general.
We are testing here the behavior of TraverseStmt specifically, perhaps TraverseStmt.cpp would be a more appropriate name

18

Agreed.
This would avoid all the /*VisitPostOrder=*/false

82

E for Expr? Ok, Expr is base class of IntegerLiteral.
I propose to use either:
S for Stmt, it is a more homogeneus name and also a base class of IntegerLiteral
Or
IL for IntegerLiteral, and then we stick with this convention

125

Good! Here the post order is still calling WalkUpFrom even though our Traverse doesn't call it in IntegerLiteral!

149–162

E for Expr? Ok, Expr is base class of IntegerLiteral.
I propose to use either:
S for Stmt, it is a more homogeneus name and also a base class of IntegerLiteral
Or
IL for IntegerLiteral, and then we stick with this convention

Here it gets even more confusing.

154

I think overriding WalkUpFromDerived when you already have WalkUpFromStmt doesn't bring much value.
In the case of fully derived AST nodes we just get the repeated information about the type of this node, e.g.
WalkUpFromIntegerLiteral IntegerLiteral(x)
instead of
WalkUpFromStmt IntegerLiteral(x)

In the case of intermediate AST nodes, as WalkUpFromExpr, we get some information but do we need that?
Here for instance, the information we get is:
WalkUpFromExpr Node => Node is an Expr
WalkUpFromStmt Node => Node is a Stmt
I don't think this information is very valuable

301–310

I think we spotted something funny here. RAV didn't call our overriden TraverseBinaryOperator.

[gribozavr2]

clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp

17

Added.

18

Changed to enum.

82

Changed to a more specific abbreviation everywhere I could find.

149–162

Changed the name to IL.

154

I added these overrides not to collect more information, but to get more coverage. It is true that if we look carefully at the implementation we can probably infer that WalkUpFrom chaining works fine. I'm adding tests to ensure that it continues to work correctly in future. It would be nice if we could factor out a separate test that shows just that chaining logic, but I don't see how to easily do it. WalkUpFrom methods are called from a bunch of places depending on what Traverse methods are overridden, whether we are in the data recursion optimization, and whether we are in the post-order traversal mode. Those focused tests would have to define the same combinations of callbacks in RecursiveASTVisitor as these tests here.

301–310

I added a FIXME that explains that this is a potential bug.

[gribozavr]

Addressed code review comments.

[gribozavr]

Added calls to default implementations.

[eduucaldas]

A more general feedback.
From our conversation one of the issues was that the tests wre only surfacing overriden methods. For instance, whenever we recorded a WalkUpFromExpr, and thus the callback showed up in the test, we actually did not walk up.
Now, in all the test cases we are calling the default implementation. We are not surfacing that WalkUpFrom can not walk up.

clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp

77

We don't use CallbackName.

108–111

I think you meant to unify recordCallback and recodDefaultImplentation into one, and that's why you had added this __func__ to recordDefaultImplementation.

118

Do we need this this?

[gribozavr]

Removed an unused argument from recordDefaultImplementation.

[gribozavr]

Unified recordCallback and recordDefaultImplementation into one call.

[gribozavr2]

Now, in all the test cases we are calling the default implementation. We are not surfacing that WalkUpFrom can not walk up.

I think we are. The log of callbacks called from the default implementation is indented to the right, so we clearly see what the default implementation does, and what the behavior would have been if we didn't call the default implementation.

clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp

77

Removed (initially I had a different implementation).

108–111

I was passing __func__ to recordDefaultImplementation for a different reason (I had a different output format initially), but unifying the two functions like you're suggesting makes a lot of sense. Changed the code to do that.

118

Yes, we're calling a method on this from the base class. RecordingVisitorBase::WalkUpFromStmt(S); is implicitly calling a member function, this-> RecordingVisitorBase::WalkUpFromStmt(S);.

[eduucaldas]

clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp

118

True! The & default captures this already, but a better suggestion would be to use the capture: [S, this] or perhaps to just use S as an argument of the lambda.
That is a nitpick though, feel free to push the patch :)

[gribozavr]

Simplified capture lists in lambdas

[gribozavr2]

clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp

118

The & default captures this already

Oh indeed, thanks! I changed the capture list to [&] because I think it is not interesting what this closure captures exactly. It is not stored anywhere beyond the recordCallback call and invoked only once, so there are no lifetime concerns for variables captured by reference.

[nemanjai]

Is there a way this test case can somehow be broken up into multiple files? The test case takes a very long time to compile which causes intermittent but frequent failures on one of our bots that runs on a fairly small VM. Most of the failures listed here: http://lab.llvm.org:8011/builders/clang-ppc64le-linux?numbuilds=100 are timeout failures when compiling this test. It seems unfortunate to have to increase the timeout limit on the bot just for this - especially if this can be broken up into multiple files.

[nemanjai]

Hi @gribozavr do you think we can do something about this test?

clang/unittests/Tooling/CMakeLists.txt

add_clang_unittest(ToolingTests
   ExecutionTest.cpp
   FixItTest.cpp
   HeaderIncludesTest.cpp
   LexicallyOrderedRecursiveASTVisitorTest.cpp
   LookupTest.cpp
   QualTypeNamesTest.cpp
   RangeSelectorTest.cpp
   RecursiveASTVisitorTests/Attr.cpp
+  RecursiveASTVisitorTests/Callbacks.cpp
   RecursiveASTVisitorTests/Class.cpp
   RecursiveASTVisitorTests/ConstructExpr.cpp
   RecursiveASTVisitorTests/CXXBoolLiteralExpr.cpp
   RecursiveASTVisitorTests/CXXMemberCall.cpp
   RecursiveASTVisitorTests/CXXMethodDecl.cpp
   RecursiveASTVisitorTests/CXXOperatorCallExprTraverser.cpp
   RecursiveASTVisitorTests/DeclRefExpr.cpp
   RecursiveASTVisitorTests/ImplicitCtor.cpp

clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.cpp

+//===--- clang/unittests/Tooling/RecursiveASTVisitorTests/Callbacks.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 {
+
+enum class ShouldTraversePostOrder : bool {
+  No = false,
+  Yes = true,
+};
+
+/// Base class for tests for RecursiveASTVisitor tests that validate the
+/// sequence of calls to user-defined callbacks like Traverse*(), WalkUp*(),
+/// Visit*().
+template <typename Derived>
+class RecordingVisitorBase : public TestVisitor<Derived> {
+  ShouldTraversePostOrder ShouldTraversePostOrderValue;
+
+public:
+  RecordingVisitorBase(ShouldTraversePostOrder ShouldTraversePostOrderValue)
+      : ShouldTraversePostOrderValue(ShouldTraversePostOrderValue) {}
+
+  bool shouldTraversePostOrder() const {
+    return static_cast<bool>(ShouldTraversePostOrderValue);
+  }
+
+  // Callbacks received during traversal.
+  std::string CallbackLog;
+  unsigned CallbackLogIndent = 0;
+
+  std::string stmtToString(Stmt *S) {
+    StringRef ClassName = S->getStmtClassName();
+    if (IntegerLiteral *IL = dyn_cast<IntegerLiteral>(S)) {
+      return (ClassName + "(" + IL->getValue().toString(10, false) + ")").str();
+    }
+    if (BinaryOperator *BO = dyn_cast<BinaryOperator>(S)) {
+      return (ClassName + "(" + BinaryOperator::getOpcodeStr(BO->getOpcode()) +
+              ")")
+          .str();
+    }
+    if (CallExpr *CE = dyn_cast<CallExpr>(S)) {
+      if (FunctionDecl *Callee = CE->getDirectCallee()) {
+        if (Callee->getIdentifier()) {
+          return (ClassName + "(" + Callee->getName() + ")").str();
+        }
+      }
+    }
+    if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(S)) {
+      if (NamedDecl *ND = DRE->getFoundDecl()) {
+        if (ND->getIdentifier()) {
+          return (ClassName + "(" + ND->getName() + ")").str();
+        }
+      }
+    }
+    return ClassName.str();
+  }
+
+  /// Record the fact that the user-defined callback member function
+  /// \p CallbackName was called with the argument \p S. Then, record the
+  /// effects of calling the default implementation \p CallDefaultFn.
+  template <typename CallDefault>
+  void recordCallback(StringRef CallbackName, Stmt *S,
+                      CallDefault CallDefaultFn) {
+    for (unsigned i = 0; i != CallbackLogIndent; ++i) {
+      CallbackLog += "  ";
+    }
+    CallbackLog += (CallbackName + " " + stmtToString(S) + "\n").str();
+    ++CallbackLogIndent;
+    CallDefaultFn();
+    --CallbackLogIndent;
+  }
+};
+
+template <typename VisitorTy>
+::testing::AssertionResult visitorCallbackLogEqual(VisitorTy Visitor,
+                                                   StringRef Code,
+                                                   StringRef ExpectedLog) {
+  Visitor.runOver(Code);
+  // EXPECT_EQ shows the diff between the two strings if they are different.
+  EXPECT_EQ(ExpectedLog.trim().str(),
+            StringRef(Visitor.CallbackLog).trim().str());
+  if (ExpectedLog.trim() != StringRef(Visitor.CallbackLog).trim()) {
+    return ::testing::AssertionFailure();
+  }
+  return ::testing::AssertionSuccess();
+}
+
+} // namespace
+
+TEST(RecursiveASTVisitor, StmtCallbacks_TraverseLeaf) {
+  class RecordingVisitor : public RecordingVisitorBase<RecordingVisitor> {
+  public:
+    RecordingVisitor(ShouldTraversePostOrder ShouldTraversePostOrderValue)
+        : RecordingVisitorBase(ShouldTraversePostOrderValue) {}
+
+    bool TraverseIntegerLiteral(IntegerLiteral *IL) {
+      recordCallback(__func__, IL, [&]() {
+        RecordingVisitorBase::TraverseIntegerLiteral(IL);
+      });
+      return true;
+    }
+
+    bool WalkUpFromStmt(Stmt *S) {
+      recordCallback(__func__, S,
+                     [&]() { RecordingVisitorBase::WalkUpFromStmt(S); });
+      return true;
+    }
+  };
+
+  StringRef Code = R"cpp(
+void add(int, int);
+void test() {
+  1;
+  2 + 3;
+  add(4, 5);
+}
+)cpp";
+
+  EXPECT_TRUE(visitorCallbackLogEqual(
+      RecordingVisitor(ShouldTraversePostOrder::No), Code,
+      R"txt(
+WalkUpFromStmt CompoundStmt
+TraverseIntegerLiteral IntegerLiteral(1)
+  WalkUpFromStmt IntegerLiteral(1)
+WalkUpFromStmt BinaryOperator(+)
+TraverseIntegerLiteral IntegerLiteral(2)
+  WalkUpFromStmt IntegerLiteral(2)
+TraverseIntegerLiteral IntegerLiteral(3)
+  WalkUpFromStmt IntegerLiteral(3)
+WalkUpFromStmt CallExpr(add)
+WalkUpFromStmt ImplicitCastExpr
+WalkUpFromStmt DeclRefExpr(add)
+TraverseIntegerLiteral IntegerLiteral(4)
+  WalkUpFromStmt IntegerLiteral(4)
+TraverseIntegerLiteral IntegerLiteral(5)
+  WalkUpFromStmt IntegerLiteral(5)
+)txt"));
+
+  EXPECT_TRUE(visitorCallbackLogEqual(
+      RecordingVisitor(ShouldTraversePostOrder::Yes), Code,
+      R"txt(
+TraverseIntegerLiteral IntegerLiteral(1)
+  WalkUpFromStmt IntegerLiteral(1)
+WalkUpFromStmt IntegerLiteral(1)
+TraverseIntegerLiteral IntegerLiteral(2)
+  WalkUpFromStmt IntegerLiteral(2)
+WalkUpFromStmt IntegerLiteral(2)
+TraverseIntegerLiteral IntegerLiteral(3)
+  WalkUpFromStmt IntegerLiteral(3)
+WalkUpFromStmt IntegerLiteral(3)
+WalkUpFromStmt BinaryOperator(+)
+WalkUpFromStmt DeclRefExpr(add)
+WalkUpFromStmt ImplicitCastExpr
+TraverseIntegerLiteral IntegerLiteral(4)
+  WalkUpFromStmt IntegerLiteral(4)
+WalkUpFromStmt IntegerLiteral(4)
+TraverseIntegerLiteral IntegerLiteral(5)
+  WalkUpFromStmt IntegerLiteral(5)
+WalkUpFromStmt IntegerLiteral(5)
+WalkUpFromStmt CallExpr(add)
+WalkUpFromStmt CompoundStmt
+)txt"));
+}
+
+TEST(RecursiveASTVisitor, StmtCallbacks_TraverseLeaf_WalkUpFromLeaf) {
+  class RecordingVisitor : public RecordingVisitorBase<RecordingVisitor> {
+  public:
+    RecordingVisitor(ShouldTraversePostOrder ShouldTraversePostOrderValue)
+        : RecordingVisitorBase(ShouldTraversePostOrderValue) {}
+
+    bool TraverseIntegerLiteral(IntegerLiteral *IL) {
+      recordCallback(__func__, IL, [&]() {
+        RecordingVisitorBase::TraverseIntegerLiteral(IL);
+      });
+      return true;
+    }
+
+    bool WalkUpFromStmt(Stmt *S) {
+      recordCallback(__func__, S,
+                     [&]() { RecordingVisitorBase::WalkUpFromStmt(S); });
+      return true;
+    }
+
+    bool WalkUpFromExpr(Expr *E) {
+      recordCallback(__func__, E,
+                     [&]() { RecordingVisitorBase::WalkUpFromExpr(E); });
+      return true;
+    }
+
+    bool WalkUpFromIntegerLiteral(IntegerLiteral *IL) {
+      recordCallback(__func__, IL, [&]() {
+        RecordingVisitorBase::WalkUpFromIntegerLiteral(IL);
+      });
+      return true;
+    }
+  };
+
+  StringRef Code = R"cpp(
+void add(int, int);
+void test() {
+  1;
+  2 + 3;
+  add(4, 5);
+}
+)cpp";
+
+  EXPECT_TRUE(visitorCallbackLogEqual(
+      RecordingVisitor(ShouldTraversePostOrder::No), Code,
+      R"txt(
+WalkUpFromStmt CompoundStmt
+TraverseIntegerLiteral IntegerLiteral(1)
+  WalkUpFromIntegerLiteral IntegerLiteral(1)
+    WalkUpFromExpr IntegerLiteral(1)
+      WalkUpFromStmt IntegerLiteral(1)
+WalkUpFromExpr BinaryOperator(+)
+  WalkUpFromStmt BinaryOperator(+)
+TraverseIntegerLiteral IntegerLiteral(2)
+  WalkUpFromIntegerLiteral IntegerLiteral(2)
+    WalkUpFromExpr IntegerLiteral(2)
+      WalkUpFromStmt IntegerLiteral(2)
+TraverseIntegerLiteral IntegerLiteral(3)
+  WalkUpFromIntegerLiteral IntegerLiteral(3)
+    WalkUpFromExpr IntegerLiteral(3)
+      WalkUpFromStmt IntegerLiteral(3)
+WalkUpFromExpr CallExpr(add)
+  WalkUpFromStmt CallExpr(add)
+WalkUpFromExpr ImplicitCastExpr
+  WalkUpFromStmt ImplicitCastExpr
+WalkUpFromExpr DeclRefExpr(add)
+  WalkUpFromStmt DeclRefExpr(add)
+TraverseIntegerLiteral IntegerLiteral(4)
+  WalkUpFromIntegerLiteral IntegerLiteral(4)
+    WalkUpFromExpr IntegerLiteral(4)
+      WalkUpFromStmt IntegerLiteral(4)
+TraverseIntegerLiteral IntegerLiteral(5)
+  WalkUpFromIntegerLiteral IntegerLiteral(5)
+    WalkUpFromExpr IntegerLiteral(5)
+      WalkUpFromStmt IntegerLiteral(5)
+)txt"));
+
+  EXPECT_TRUE(visitorCallbackLogEqual(
+      RecordingVisitor(ShouldTraversePostOrder::Yes), Code,
+      R"txt(
+TraverseIntegerLiteral IntegerLiteral(1)
+  WalkUpFromIntegerLiteral IntegerLiteral(1)
+    WalkUpFromExpr IntegerLiteral(1)
+      WalkUpFromStmt IntegerLiteral(1)
+WalkUpFromIntegerLiteral IntegerLiteral(1)
+  WalkUpFromExpr IntegerLiteral(1)
+    WalkUpFromStmt IntegerLiteral(1)
+TraverseIntegerLiteral IntegerLiteral(2)
+  WalkUpFromIntegerLiteral IntegerLiteral(2)
+    WalkUpFromExpr IntegerLiteral(2)
+      WalkUpFromStmt IntegerLiteral(2)
+WalkUpFromIntegerLiteral IntegerLiteral(2)
+  WalkUpFromExpr IntegerLiteral(2)
+    WalkUpFromStmt IntegerLiteral(2)
+TraverseIntegerLiteral IntegerLiteral(3)
+  WalkUpFromIntegerLiteral IntegerLiteral(3)
+    WalkUpFromExpr IntegerLiteral(3)
+      WalkUpFromStmt IntegerLiteral(3)
+WalkUpFromIntegerLiteral IntegerLiteral(3)
+  WalkUpFromExpr IntegerLiteral(3)
+    WalkUpFromStmt IntegerLiteral(3)
+WalkUpFromExpr BinaryOperator(+)
+  WalkUpFromStmt BinaryOperator(+)
+WalkUpFromExpr DeclRefExpr(add)
+  WalkUpFromStmt DeclRefExpr(add)
+WalkUpFromExpr ImplicitCastExpr
+  WalkUpFromStmt ImplicitCastExpr
+TraverseIntegerLiteral IntegerLiteral(4)
+  WalkUpFromIntegerLiteral IntegerLiteral(4)
+    WalkUpFromExpr IntegerLiteral(4)
+      WalkUpFromStmt IntegerLiteral(4)
+WalkUpFromIntegerLiteral IntegerLiteral(4)
+  WalkUpFromExpr IntegerLiteral(4)
+    WalkUpFromStmt IntegerLiteral(4)
+TraverseIntegerLiteral IntegerLiteral(5)
+  WalkUpFromIntegerLiteral IntegerLiteral(5)
+    WalkUpFromExpr IntegerLiteral(5)
+      WalkUpFromStmt IntegerLiteral(5)
+WalkUpFromIntegerLiteral IntegerLiteral(5)
+  WalkUpFromExpr IntegerLiteral(5)
+    WalkUpFromStmt IntegerLiteral(5)
+WalkUpFromExpr CallExpr(add)
+  WalkUpFromStmt CallExpr(add)
+WalkUpFromStmt CompoundStmt
+)txt"));
+}
+
+TEST(RecursiveASTVisitor, StmtCallbacks_WalkUpFromLeaf) {
+  class RecordingVisitor : public RecordingVisitorBase<RecordingVisitor> {
+  public:
+    RecordingVisitor(ShouldTraversePostOrder ShouldTraversePostOrderValue)
+        : RecordingVisitorBase(ShouldTraversePostOrderValue) {}
+
+    bool WalkUpFromStmt(Stmt *S) {
+      recordCallback(__func__, S,
+                     [&]() { RecordingVisitorBase::WalkUpFromStmt(S); });
+      return true;
+    }
+
+    bool WalkUpFromExpr(Expr *E) {
+      recordCallback(__func__, E,
+                     [&]() { RecordingVisitorBase::WalkUpFromExpr(E); });
+      return true;
+    }
+
+    bool WalkUpFromIntegerLiteral(IntegerLiteral *IL) {
+      recordCallback(__func__, IL, [&]() {
+        RecordingVisitorBase::WalkUpFromIntegerLiteral(IL);
+      });
+      return true;
+    }
+  };
+
+  StringRef Code = R"cpp(
+void add(int, int);
+void test() {
+  1;
+  2 + 3;
+  add(4, 5);
+}
+)cpp";
+
+  EXPECT_TRUE(visitorCallbackLogEqual(
+      RecordingVisitor(ShouldTraversePostOrder::No), Code,
+      R"txt(
+WalkUpFromStmt CompoundStmt
+WalkUpFromIntegerLiteral IntegerLiteral(1)
+  WalkUpFromExpr IntegerLiteral(1)
+    WalkUpFromStmt IntegerLiteral(1)
+WalkUpFromExpr BinaryOperator(+)
+  WalkUpFromStmt BinaryOperator(+)
+WalkUpFromIntegerLiteral IntegerLiteral(2)
+  WalkUpFromExpr IntegerLiteral(2)
+    WalkUpFromStmt IntegerLiteral(2)
+WalkUpFromIntegerLiteral IntegerLiteral(3)
+  WalkUpFromExpr IntegerLiteral(3)
+    WalkUpFromStmt IntegerLiteral(3)
+WalkUpFromExpr CallExpr(add)
+  WalkUpFromStmt CallExpr(add)
+WalkUpFromExpr ImplicitCastExpr
+  WalkUpFromStmt ImplicitCastExpr
+WalkUpFromExpr DeclRefExpr(add)
+  WalkUpFromStmt DeclRefExpr(add)
+WalkUpFromIntegerLiteral IntegerLiteral(4)
+  WalkUpFromExpr IntegerLiteral(4)
+    WalkUpFromStmt IntegerLiteral(4)
+WalkUpFromIntegerLiteral IntegerLiteral(5)
+  WalkUpFromExpr IntegerLiteral(5)
+    WalkUpFromStmt IntegerLiteral(5)
+)txt"));
+
+  EXPECT_TRUE(visitorCallbackLogEqual(
+      RecordingVisitor(ShouldTraversePostOrder::Yes), Code,
+      R"txt(
+WalkUpFromIntegerLiteral IntegerLiteral(1)
+  WalkUpFromExpr IntegerLiteral(1)
+    WalkUpFromStmt IntegerLiteral(1)
+WalkUpFromIntegerLiteral IntegerLiteral(2)
+  WalkUpFromExpr IntegerLiteral(2)
+    WalkUpFromStmt IntegerLiteral(2)
+WalkUpFromIntegerLiteral IntegerLiteral(3)
+  WalkUpFromExpr IntegerLiteral(3)
+    WalkUpFromStmt IntegerLiteral(3)
+WalkUpFromExpr BinaryOperator(+)
+  WalkUpFromStmt BinaryOperator(+)
+WalkUpFromExpr DeclRefExpr(add)
+  WalkUpFromStmt DeclRefExpr(add)
+WalkUpFromExpr ImplicitCastExpr
+  WalkUpFromStmt ImplicitCastExpr
+WalkUpFromIntegerLiteral IntegerLiteral(4)
+  WalkUpFromExpr IntegerLiteral(4)
+    WalkUpFromStmt IntegerLiteral(4)
+WalkUpFromIntegerLiteral IntegerLiteral(5)
+  WalkUpFromExpr IntegerLiteral(5)
+    WalkUpFromStmt IntegerLiteral(5)
+WalkUpFromExpr CallExpr(add)
+  WalkUpFromStmt CallExpr(add)
+WalkUpFromStmt CompoundStmt
+)txt"));
+}
+
+TEST(RecursiveASTVisitor, StmtCallbacks_TraverseBinaryOperator) {
+  class RecordingVisitor : public RecordingVisitorBase<RecordingVisitor> {
+  public:
+    RecordingVisitor(ShouldTraversePostOrder ShouldTraversePostOrderValue)
+        : RecordingVisitorBase(ShouldTraversePostOrderValue) {}
+
+    bool TraverseBinaryOperator(BinaryOperator *BO) {
+      recordCallback(__func__, BO, [&]() {
+        RecordingVisitorBase::TraverseBinaryOperator(BO);
+      });
+      return true;
+    }
+
+    bool WalkUpFromStmt(Stmt *S) {
+      recordCallback(__func__, S,
+                     [&]() { RecordingVisitorBase::WalkUpFromStmt(S); });
+      return true;
+    }
+  };
+
+  StringRef Code = R"cpp(
+void add(int, int);
+void test() {
+  1;
+  2 + 3;
+  add(4, 5);
+}
+)cpp";
+
+  EXPECT_TRUE(visitorCallbackLogEqual(
+      RecordingVisitor(ShouldTraversePostOrder::No), Code,
+      R"txt(
+WalkUpFromStmt CompoundStmt
+WalkUpFromStmt IntegerLiteral(1)
+WalkUpFromStmt BinaryOperator(+)
+WalkUpFromStmt IntegerLiteral(2)
+WalkUpFromStmt IntegerLiteral(3)
+WalkUpFromStmt CallExpr(add)
+WalkUpFromStmt ImplicitCastExpr
+WalkUpFromStmt DeclRefExpr(add)
+WalkUpFromStmt IntegerLiteral(4)
+WalkUpFromStmt IntegerLiteral(5)
+)txt"));
+
+  EXPECT_TRUE(visitorCallbackLogEqual(
+      RecordingVisitor(ShouldTraversePostOrder::Yes), Code,
+      R"txt(
+WalkUpFromStmt IntegerLiteral(1)
+WalkUpFromStmt IntegerLiteral(2)
+WalkUpFromStmt IntegerLiteral(3)
+WalkUpFromStmt BinaryOperator(+)
+WalkUpFromStmt DeclRefExpr(add)
+WalkUpFromStmt ImplicitCastExpr
+WalkUpFromStmt IntegerLiteral(4)
+WalkUpFromStmt IntegerLiteral(5)
+WalkUpFromStmt CallExpr(add)
+WalkUpFromStmt CompoundStmt
+)txt"));
+}
+
+TEST(RecursiveASTVisitor,
+     StmtCallbacks_TraverseBinaryOperator_WalkUpFromBinaryOperator) {
+  class RecordingVisitor : public RecordingVisitorBase<RecordingVisitor> {
+  public:
+    RecordingVisitor(ShouldTraversePostOrder ShouldTraversePostOrderValue)
+        : RecordingVisitorBase(ShouldTraversePostOrderValue) {}
+
+    bool TraverseBinaryOperator(BinaryOperator *BO) {
+      recordCallback(__func__, BO, [&]() {
+        RecordingVisitorBase::TraverseBinaryOperator(BO);
+      });
+      return true;
+    }
+
+    bool WalkUpFromStmt(Stmt *S) {
+      recordCallback(__func__, S,
+                     [&]() { RecordingVisitorBase::WalkUpFromStmt(S); });
+      return true;
+    }
+
+    bool WalkUpFromExpr(Expr *E) {
+      recordCallback(__func__, E,
+                     [&]() { RecordingVisitorBase::WalkUpFromExpr(E); });
+      return true;
+    }
+
+    bool WalkUpFromBinaryOperator(BinaryOperator *BO) {
+      recordCallback(__func__, BO, [&]() {
+        RecordingVisitorBase::WalkUpFromBinaryOperator(BO);
+      });
+      return true;
+    }
+  };
+
+  StringRef Code = R"cpp(
+void add(int, int);
+void test() {
+  1;
+  2 + 3;
+  add(4, 5);
+}
+)cpp";
+
+  // FIXME: It is arguably a bug in RecursiveASTVisitor that
+  // WalkUpFromBinaryOperator is called, but TraverseBinaryOperator is not
+  // called.
+  EXPECT_TRUE(visitorCallbackLogEqual(
+      RecordingVisitor(ShouldTraversePostOrder::No), Code,
+      R"txt(
+WalkUpFromStmt CompoundStmt
+WalkUpFromExpr IntegerLiteral(1)
+  WalkUpFromStmt IntegerLiteral(1)
+WalkUpFromBinaryOperator BinaryOperator(+)
+  WalkUpFromExpr BinaryOperator(+)
+    WalkUpFromStmt BinaryOperator(+)
+WalkUpFromExpr IntegerLiteral(2)
+  WalkUpFromStmt IntegerLiteral(2)
+WalkUpFromExpr IntegerLiteral(3)
+  WalkUpFromStmt IntegerLiteral(3)
+WalkUpFromExpr CallExpr(add)
+  WalkUpFromStmt CallExpr(add)
+WalkUpFromExpr ImplicitCastExpr
+  WalkUpFromStmt ImplicitCastExpr
+WalkUpFromExpr DeclRefExpr(add)
+  WalkUpFromStmt DeclRefExpr(add)
+WalkUpFromExpr IntegerLiteral(4)
+  WalkUpFromStmt IntegerLiteral(4)
+WalkUpFromExpr IntegerLiteral(5)
+  WalkUpFromStmt IntegerLiteral(5)
+)txt"));
+
+  EXPECT_TRUE(visitorCallbackLogEqual(
+      RecordingVisitor(ShouldTraversePostOrder::Yes), Code,
+      R"txt(
+WalkUpFromExpr IntegerLiteral(1)
+  WalkUpFromStmt IntegerLiteral(1)
+WalkUpFromExpr IntegerLiteral(2)
+  WalkUpFromStmt IntegerLiteral(2)
+WalkUpFromExpr IntegerLiteral(3)
+  WalkUpFromStmt IntegerLiteral(3)
+WalkUpFromBinaryOperator BinaryOperator(+)
+  WalkUpFromExpr BinaryOperator(+)
+    WalkUpFromStmt BinaryOperator(+)
+WalkUpFromExpr DeclRefExpr(add)
+  WalkUpFromStmt DeclRefExpr(add)
+WalkUpFromExpr ImplicitCastExpr
+  WalkUpFromStmt ImplicitCastExpr
+WalkUpFromExpr IntegerLiteral(4)
+  WalkUpFromStmt IntegerLiteral(4)
+WalkUpFromExpr IntegerLiteral(5)
+  WalkUpFromStmt IntegerLiteral(5)
+WalkUpFromExpr CallExpr(add)
+  WalkUpFromStmt CallExpr(add)
+WalkUpFromStmt CompoundStmt
+)txt"));
+}
+
+TEST(RecursiveASTVisitor, StmtCallbacks_WalkUpFromBinaryOperator) {
+  class RecordingVisitor : public RecordingVisitorBase<RecordingVisitor> {
+  public:
+    RecordingVisitor(ShouldTraversePostOrder ShouldTraversePostOrderValue)
+        : RecordingVisitorBase(ShouldTraversePostOrderValue) {}
+
+    bool WalkUpFromStmt(Stmt *S) {
+      recordCallback(__func__, S,
+                     [&]() { RecordingVisitorBase::WalkUpFromStmt(S); });
+      return true;
+    }
+
+    bool WalkUpFromExpr(Expr *E) {
+      recordCallback(__func__, E,
+                     [&]() { RecordingVisitorBase::WalkUpFromExpr(E); });
+      return true;
+    }
+
+    bool WalkUpFromBinaryOperator(BinaryOperator *BO) {
+      recordCallback(__func__, BO, [&]() {
+        RecordingVisitorBase::WalkUpFromBinaryOperator(BO);
+      });
+      return true;
+    }
+  };
+
+  StringRef Code = R"cpp(
+void add(int, int);
+void test() {
+  1;
+  2 + 3;
+  add(4, 5);
+}
+)cpp";
+
+  EXPECT_TRUE(visitorCallbackLogEqual(
+      RecordingVisitor(ShouldTraversePostOrder::No), Code,
+      R"txt(
+WalkUpFromStmt CompoundStmt
+WalkUpFromExpr IntegerLiteral(1)
+  WalkUpFromStmt IntegerLiteral(1)
+WalkUpFromBinaryOperator BinaryOperator(+)
+  WalkUpFromExpr BinaryOperator(+)
+    WalkUpFromStmt BinaryOperator(+)
+WalkUpFromExpr IntegerLiteral(2)
+  WalkUpFromStmt IntegerLiteral(2)
+WalkUpFromExpr IntegerLiteral(3)
+  WalkUpFromStmt IntegerLiteral(3)
+WalkUpFromExpr CallExpr(add)
+  WalkUpFromStmt CallExpr(add)
+WalkUpFromExpr ImplicitCastExpr
+  WalkUpFromStmt ImplicitCastExpr
+WalkUpFromExpr DeclRefExpr(add)
+  WalkUpFromStmt DeclRefExpr(add)
+WalkUpFromExpr IntegerLiteral(4)
+  WalkUpFromStmt IntegerLiteral(4)
+WalkUpFromExpr IntegerLiteral(5)
+  WalkUpFromStmt IntegerLiteral(5)
+)txt"));
+
+  EXPECT_TRUE(visitorCallbackLogEqual(
+      RecordingVisitor(ShouldTraversePostOrder::Yes), Code,
+      R"txt(
+WalkUpFromExpr IntegerLiteral(1)
+  WalkUpFromStmt IntegerLiteral(1)
+WalkUpFromExpr IntegerLiteral(2)
+  WalkUpFromStmt IntegerLiteral(2)
+WalkUpFromExpr IntegerLiteral(3)
+  WalkUpFromStmt IntegerLiteral(3)
+WalkUpFromBinaryOperator BinaryOperator(+)
+  WalkUpFromExpr BinaryOperator(+)
+    WalkUpFromStmt BinaryOperator(+)
+WalkUpFromExpr DeclRefExpr(add)
+  WalkUpFromStmt DeclRefExpr(add)
+WalkUpFromExpr ImplicitCastExpr
+  WalkUpFromStmt ImplicitCastExpr
+WalkUpFromExpr IntegerLiteral(4)
+  WalkUpFromStmt IntegerLiteral(4)
+WalkUpFromExpr IntegerLiteral(5)
+  WalkUpFromStmt IntegerLiteral(5)
+WalkUpFromExpr CallExpr(add)
+  WalkUpFromStmt CallExpr(add)
+WalkUpFromStmt CompoundStmt
+)txt"));
+}
+
+TEST(RecursiveASTVisitor, StmtCallbacks_TraverseCallExpr) {
+  class RecordingVisitor : public RecordingVisitorBase<RecordingVisitor> {
+  public:
+    RecordingVisitor(ShouldTraversePostOrder ShouldTraversePostOrderValue)
+        : RecordingVisitorBase(ShouldTraversePostOrderValue) {}
+
+    bool TraverseCallExpr(CallExpr *CE) {
+      recordCallback(__func__, CE,
+                     [&]() { RecordingVisitorBase::TraverseCallExpr(CE); });
+      return true;
+    }
+
+    bool WalkUpFromStmt(Stmt *S) {
+      recordCallback(__func__, S,
+                     [&]() { RecordingVisitorBase::WalkUpFromStmt(S); });
+      return true;
+    }
+  };
+
+  StringRef Code = R"cpp(
+void add(int, int);
+void test() {
+  1;
+  2 + 3;
+  add(4, 5);
+}
+)cpp";
+
+  EXPECT_TRUE(visitorCallbackLogEqual(
+      RecordingVisitor(ShouldTraversePostOrder::No), Code,
+      R"txt(
+WalkUpFromStmt CompoundStmt
+WalkUpFromStmt IntegerLiteral(1)
+WalkUpFromStmt BinaryOperator(+)
+WalkUpFromStmt IntegerLiteral(2)
+WalkUpFromStmt IntegerLiteral(3)
+TraverseCallExpr CallExpr(add)
+  WalkUpFromStmt CallExpr(add)
+  WalkUpFromStmt ImplicitCastExpr
+  WalkUpFromStmt DeclRefExpr(add)
+  WalkUpFromStmt IntegerLiteral(4)
+  WalkUpFromStmt IntegerLiteral(5)
+)txt"));
+
+  EXPECT_TRUE(visitorCallbackLogEqual(
+      RecordingVisitor(ShouldTraversePostOrder::Yes), Code,
+      R"txt(
+WalkUpFromStmt IntegerLiteral(1)
+WalkUpFromStmt IntegerLiteral(2)
+WalkUpFromStmt IntegerLiteral(3)
+WalkUpFromStmt BinaryOperator(+)
+TraverseCallExpr CallExpr(add)
+  WalkUpFromStmt DeclRefExpr(add)
+  WalkUpFromStmt ImplicitCastExpr
+  WalkUpFromStmt IntegerLiteral(4)
+  WalkUpFromStmt IntegerLiteral(5)
+  WalkUpFromStmt CallExpr(add)
+WalkUpFromStmt CallExpr(add)
+WalkUpFromStmt CompoundStmt
+)txt"));
+}
+
+TEST(RecursiveASTVisitor, StmtCallbacks_TraverseCallExpr_WalkUpFromCallExpr) {
+  class RecordingVisitor : public RecordingVisitorBase<RecordingVisitor> {
+  public:
+    RecordingVisitor(ShouldTraversePostOrder ShouldTraversePostOrderValue)
+        : RecordingVisitorBase(ShouldTraversePostOrderValue) {}
+
+    bool TraverseCallExpr(CallExpr *CE) {
+      recordCallback(__func__, CE,
+                     [&]() { RecordingVisitorBase::TraverseCallExpr(CE); });
+      return true;
+    }
+
+    bool WalkUpFromStmt(Stmt *S) {
+      recordCallback(__func__, S,
+                     [&]() { RecordingVisitorBase::WalkUpFromStmt(S); });
+      return true;
+    }
+
+    bool WalkUpFromExpr(Expr *E) {
+      recordCallback(__func__, E,
+                     [&]() { RecordingVisitorBase::WalkUpFromExpr(E); });
+      return true;
+    }
+
+    bool WalkUpFromCallExpr(CallExpr *CE) {
+      recordCallback(__func__, CE,
+                     [&]() { RecordingVisitorBase::WalkUpFromCallExpr(CE); });
+      return true;
+    }
+  };
+
+  StringRef Code = R"cpp(
+void add(int, int);
+void test() {
+  1;
+  2 + 3;
+  add(4, 5);
+}
+)cpp";
+
+  EXPECT_TRUE(visitorCallbackLogEqual(
+      RecordingVisitor(ShouldTraversePostOrder::No), Code,
+      R"txt(
+WalkUpFromStmt CompoundStmt
+WalkUpFromExpr IntegerLiteral(1)
+  WalkUpFromStmt IntegerLiteral(1)
+WalkUpFromExpr BinaryOperator(+)
+  WalkUpFromStmt BinaryOperator(+)
+WalkUpFromExpr IntegerLiteral(2)
+  WalkUpFromStmt IntegerLiteral(2)
+WalkUpFromExpr IntegerLiteral(3)
+  WalkUpFromStmt IntegerLiteral(3)
+TraverseCallExpr CallExpr(add)
+  WalkUpFromCallExpr CallExpr(add)
+    WalkUpFromExpr CallExpr(add)
+      WalkUpFromStmt CallExpr(add)
+  WalkUpFromExpr ImplicitCastExpr
+    WalkUpFromStmt ImplicitCastExpr
+  WalkUpFromExpr DeclRefExpr(add)
+    WalkUpFromStmt DeclRefExpr(add)
+  WalkUpFromExpr IntegerLiteral(4)
+    WalkUpFromStmt IntegerLiteral(4)
+  WalkUpFromExpr IntegerLiteral(5)
+    WalkUpFromStmt IntegerLiteral(5)
+)txt"));
+
+  EXPECT_TRUE(visitorCallbackLogEqual(
+      RecordingVisitor(ShouldTraversePostOrder::Yes), Code,
+      R"txt(
+WalkUpFromExpr IntegerLiteral(1)
+  WalkUpFromStmt IntegerLiteral(1)
+WalkUpFromExpr IntegerLiteral(2)
+  WalkUpFromStmt IntegerLiteral(2)
+WalkUpFromExpr IntegerLiteral(3)
+  WalkUpFromStmt IntegerLiteral(3)
+WalkUpFromExpr BinaryOperator(+)
+  WalkUpFromStmt BinaryOperator(+)
+TraverseCallExpr CallExpr(add)
+  WalkUpFromExpr DeclRefExpr(add)
+    WalkUpFromStmt DeclRefExpr(add)
+  WalkUpFromExpr ImplicitCastExpr
+    WalkUpFromStmt ImplicitCastExpr
+  WalkUpFromExpr IntegerLiteral(4)
+    WalkUpFromStmt IntegerLiteral(4)
+  WalkUpFromExpr IntegerLiteral(5)
+    WalkUpFromStmt IntegerLiteral(5)
+  WalkUpFromCallExpr CallExpr(add)
+    WalkUpFromExpr CallExpr(add)
+      WalkUpFromStmt CallExpr(add)
+WalkUpFromCallExpr CallExpr(add)
+  WalkUpFromExpr CallExpr(add)
+    WalkUpFromStmt CallExpr(add)
+WalkUpFromStmt CompoundStmt
+)txt"));
+}
+
+TEST(RecursiveASTVisitor, StmtCallbacks_WalkUpFromCallExpr) {
+  class RecordingVisitor : public RecordingVisitorBase<RecordingVisitor> {
+  public:
+    RecordingVisitor(ShouldTraversePostOrder ShouldTraversePostOrderValue)
+        : RecordingVisitorBase(ShouldTraversePostOrderValue) {}
+
+    bool WalkUpFromStmt(Stmt *S) {
+      recordCallback(__func__, S,
+                     [&]() { RecordingVisitorBase::WalkUpFromStmt(S); });
+      return true;
+    }
+
+    bool WalkUpFromExpr(Expr *E) {
+      recordCallback(__func__, E,
+                     [&]() { RecordingVisitorBase::WalkUpFromExpr(E); });
+      return true;
+    }
+
+    bool WalkUpFromCallExpr(CallExpr *CE) {
+      recordCallback(__func__, CE,
+                     [&]() { RecordingVisitorBase::WalkUpFromCallExpr(CE); });
+      return true;
+    }
+  };
+
+  StringRef Code = R"cpp(
+void add(int, int);
+void test() {
+  1;
+  2 + 3;
+  add(4, 5);
+}
+)cpp";
+
+  EXPECT_TRUE(visitorCallbackLogEqual(
+      RecordingVisitor(ShouldTraversePostOrder::No), Code,
+      R"txt(
+WalkUpFromStmt CompoundStmt
+WalkUpFromExpr IntegerLiteral(1)
+  WalkUpFromStmt IntegerLiteral(1)
+WalkUpFromExpr BinaryOperator(+)
+  WalkUpFromStmt BinaryOperator(+)
+WalkUpFromExpr IntegerLiteral(2)
+  WalkUpFromStmt IntegerLiteral(2)
+WalkUpFromExpr IntegerLiteral(3)
+  WalkUpFromStmt IntegerLiteral(3)
+WalkUpFromCallExpr CallExpr(add)
+  WalkUpFromExpr CallExpr(add)
+    WalkUpFromStmt CallExpr(add)
+WalkUpFromExpr ImplicitCastExpr
+  WalkUpFromStmt ImplicitCastExpr
+WalkUpFromExpr DeclRefExpr(add)
+  WalkUpFromStmt DeclRefExpr(add)
+WalkUpFromExpr IntegerLiteral(4)
+  WalkUpFromStmt IntegerLiteral(4)
+WalkUpFromExpr IntegerLiteral(5)
+  WalkUpFromStmt IntegerLiteral(5)
+)txt"));
+
+  EXPECT_TRUE(visitorCallbackLogEqual(
+      RecordingVisitor(ShouldTraversePostOrder::Yes), Code,
+      R"txt(
+WalkUpFromExpr IntegerLiteral(1)
+  WalkUpFromStmt IntegerLiteral(1)
+WalkUpFromExpr IntegerLiteral(2)
+  WalkUpFromStmt IntegerLiteral(2)
+WalkUpFromExpr IntegerLiteral(3)
+  WalkUpFromStmt IntegerLiteral(3)
+WalkUpFromExpr BinaryOperator(+)
+  WalkUpFromStmt BinaryOperator(+)
+WalkUpFromExpr DeclRefExpr(add)
+  WalkUpFromStmt DeclRefExpr(add)
+WalkUpFromExpr ImplicitCastExpr
+  WalkUpFromStmt ImplicitCastExpr
+WalkUpFromExpr IntegerLiteral(4)
+  WalkUpFromStmt IntegerLiteral(4)
+WalkUpFromExpr IntegerLiteral(5)
+  WalkUpFromStmt IntegerLiteral(5)
+WalkUpFromCallExpr CallExpr(add)
+  WalkUpFromExpr CallExpr(add)
+    WalkUpFromStmt CallExpr(add)
+WalkUpFromStmt CompoundStmt
+)txt"));
+}