Odd (or perhaps inconsistent) behavior around `IgnoreUnlessSpelledInSource`

[tJener]

I'm noticing some odd behavior around the IgnoreUnlessSpelledInSource traversal mode, in particular when that mode changes.

---

Suppose we have the following source:

struct A {
    ~A();
};

struct B {
    explicit B(A);
};

void f() { B(A{}); }

For the sake of discussion, assume that the AST of the function f is as follows:

α FunctionDecl <line:9:1, col:20> col:6 f 'void ()'
β `-CompoundStmt <col:10, col:20>
γ   `-ExprWithCleanups <col:12, col:17> 'B'
δ     `-CXXFunctionalCastExpr <col:12, col:17> 'B' functional cast to B <ConstructorConversion>
ε       `-CXXConstructExpr <col:12, col:17> 'B' 'void (A)'
ζ         `-CXXFunctionalCastExpr <col:14, col:16> 'A' functional cast to A <NoOp>
η           `-CXXBindTemporaryExpr <col:15, col:16> 'A' (CXXTemporary 0xc230d88)
θ             `-InitListExpr <col:15, col:16> 'A'

The AST nodes ε, ζ, and θ are "spelled in source" by the definition of expr->IgnoreUnlessSpelledInSource() == expr.

We look at some matchers and what nodes they match.

---

cxxConstructExpr()

We match the CXXConstructExpr. Nothing surprising.

---

traverse(TK_IgnoreUnlessSpelledInSource, cxxConstructExpr())

We match the CXXConstructExpr. Again, nothing surprising.

---

expr(traverse(TK_IgnoreUnlessSpelledInSource, cxxConstructExpr()))

We match the CXXConstructExpr. From my perspective, this is matching expr in the AsIs traversal mode, switching to IgnoreUnlessSpelledInSource, and then refining the match to CXXConstructExprs that are spelled in source. Given that understanding, the result makes sense.

---

expr(anyOf(expr(traverse(clang::TK_IgnoreUnlessSpelledInSource,
                         cxxConstructExpr())),
           unless(anything())))

Notes regarding the matcher:

• anyOf doesn't allow me to only provide a single argument, so I'm using unless(anything()) to match nothing.
• The outermost expr is to resolve an ambiguous call to MatchFinder::addMatcher.

We match the CXXConstructExpr... three times. For each of γ, δ, and ε, the traverse matcher ignores the nodes not spelled in source and ends up passing the CXXConstructExpr to its inner matchers.

From my perspective, this matcher is no different than the previous matcher, where without any traversal matchers (referencing the categorization in the AST Matcher Reference, not the traverse matcher), the node matchers are refining the current node.

---

Perhaps this not a good idea to do. However, when writing generic utilities that allow arbitrary matchers to be composed, it's a question that needs to be answered. In particular, the path that led me to this rabbit trail was clang::tooling::Transformer and its RewriteRules.

llvm-project/clang/lib/Tooling/Transformer/RewriteRule.cpp

Lines 425 to 438 in 0c032fd

	// Each anyOf explicitly controls the traversal kind. The anyOf itself is set
	// to `TK_AsIs` to ensure no nodes are skipped, thereby deferring to the kind
	// of the branches. Then, each branch is either left as is, if the kind is
	// already set, or explicitly set to `TK_AsIs`. We choose this setting because
	// it is the default interpretation of matchers.
	std::vector<DynTypedMatcher> Matchers;
	for (const auto &Bucket : Buckets) {
	DynTypedMatcher M = DynTypedMatcher::constructVariadic(
	DynTypedMatcher::VO_AnyOf, Bucket.first,
	taggedMatchers("Tag", Bucket.second, TK_AsIs));
	M.setAllowBind(true);
	// `tryBind` is guaranteed to succeed, because `AllowBind` was set to true.
	Matchers.push_back(M.tryBind(RootID)->withTraversalKind(TK_AsIs));
	}

When a RewriteRule composes its matchers, it does so with a variadic AnyOf matcher. A user writing something like

makeRule(traverse(clang::TK_IgnoreUnlessSpelledInSource,
                  cxxConstructExpr().bind("ctor")),
         insertBefore(node("ctor"), cat("/*hi*/")))

inadvertently trips over this, matching multiple times where it seems like it should only match once.