[ASTMatchers] Fix matching failure in IgnoreUnlessSpelledInSource mode

[njames93]

Summary

For the hasAnyCtorInitializer matcher, the matcher finds the first innermatch that is successful.
Then it checks if it should be ignored, and if so, the whole match fails.
This opens up an issue when an implicit initializer matchers the inner matcher but is disregarded for being implicit, preventing an explicit initializer matching.

This also happens with the hasMethod matcher, although I can't get a test to fail using that.
Presumably because implicit methods are generated after all explicit methods.

[aaron.ballman]

clang/include/clang/ASTMatchers/ASTMatchers.h

3296–3297

It amuses me that this lack of const auto * was not similarly flagged as the one below, but feel free to correct this one as well.

4358–4360

Since you're touching the line anyway, might as well fix the lint warning and use const auto *.

clang/include/clang/ASTMatchers/ASTMatchersInternal.h

817–819

Comment looks like it can be re-flowed to 80 cols.

"if Finder is ignore them" -> "if Finder is ignoring them"

This comment is a bit odd because I would expect the same behavior from matchesFirstInPointerRange -- e.g., if the finder says "ignore unless spelled in source", I would not expect to have to call a function that also says to honor that, so this feels a bit fragile. I was sort of thinking that an (optional?) parameter to matchesFirstInPointerPair would be slightly better (and reduce code duplication), but that feels similarly fragile.

Would the logic be wrong to always honor what the Finder says about ignoring implicit nodes?

[njames93]

clang/include/clang/ASTMatchers/ASTMatchers.h

3296–3297

The reason it isn't flagged is because MatchIt is of type specific_decl_iterator, which isn't a pointer.

clang/include/clang/ASTMatchers/ASTMatchersInternal.h

817–819

The issue is not all nodes have a nice way to say they are implicit and should be ignored.

[njames93]

Address comments.

[aaron.ballman]

clang/include/clang/ASTMatchers/ASTMatchers.h

3296–3297

Oh, interesting. How I hate auto as a code reviewer...

clang/include/clang/ASTMatchers/ASTMatchersInternal.h

817–819

This compounds the problem by making the AST matchers harder to write because now we're enshrining that there's two different APIs that do the same thing but you have to remember which one needs to be called in what mode and for which kinds of nodes.

"Go fix the AST nodes" is not a solution for this patch, but I think our long-term goal should be to try to add some uniformity at the AST node level so that we can eventually remove APIs like this and hopefully reduce the amount of places we're doing isa<> checks for specific node types to try to decide whether it was spelled in the source or not. Obviously, I'm not asking you to do that work -- just trying to see if we have some agreement on an aspirational goal.

Concretely, what do you think about an API like:

// FIXME: This is necessary only because there's no good general way to know 
// whether an AST node was spelled in source or not, so callers of matchesFirstInPointerRange
// need to decide whether they want to ignore implicit nodes or not. Ideally, we could use the
// Finder object only to make this decision, but we're not there quite yet.
enum class MatchFirstPointerInRangeMode {
  AsIs,
  IgnoreUnlessSpelledInSource
};

template <typename MatcherT, typename IteratorT>
IteratorT matchesFirstInPointerRange(const MatcherT &Matcher, IteratorT Start,
                                     IteratorT End, ASTMatchFinder *Finder,
                                     BoundNodesTreeBuilder *Builder,
                                     MatchFirstPointerInRangeMode Mode = MatchFirstPointerInRangeMode::AsIs);

[steveire]

clang/include/clang/ASTMatchers/ASTMatchers.h

4358–4360

This is iteration from a begin() to an end(). It's appropriate to use auto for an iterator type instead of const auto *.

You don't have to change it back now, but the request to change it in the first place isn't consistent. I also don't think it's ever appropriate to use const auto * or auto * instead of just auto, so make of that what you wish :).

Anyway, to completely dodge the issue you can inline it and get rid of the MatchIt altogether without changing the meaning of the code.

clang/include/clang/ASTMatchers/ASTMatchersInternal.h

817–819

enum class MatchFirstPointerInRangeMode {
  AsIs,
  IgnoreUnlessSpelledInSource
};

Why introduce a new enum instead of just using clang::TraversalKind?

[steveire]

Thanks for this change! I don't feel strongly about whether a new method or a new parameter is appropriate, so it's a +1 from me already. Giving other AST nodes consistent API about whether they're spelled in source sounds good to me too at some point so the new method or param may be changed/removed again in the future.

clang/include/clang/ASTMatchers/ASTMatchers.h

 ///   class B { void member(); };
 /// \endcode
 ///
 /// \c cxxRecordDecl(hasMethod(hasName("func"))) matches the declaration of
 /// \c A but not \c B.
 AST_MATCHER_P(CXXRecordDecl, hasMethod, internal::Matcher<CXXMethodDecl>,
               InnerMatcher) {
   BoundNodesTreeBuilder Result(*Builder);
-  auto MatchIt = matchesFirstInPointerRange(InnerMatcher, Node.method_begin(),
-                                            Node.method_end(), Finder, &Result);
+  auto MatchIt = matchesFirstInPointerRangeIgnoreUnspelled(
+      InnerMatcher, Node.method_begin(), Node.method_end(), Finder, &Result);
   if (MatchIt == Node.method_end())
     return false;
-
-  if (Finder->isTraversalIgnoringImplicitNodes() && (*MatchIt)->isImplicit())
-    return false;
   *Builder = std::move(Result);
   return true;
 }
 
 /// Matches the generated class of lambda expressions.
 ///
 /// Given:
 /// \code

 ///   };
 /// \endcode
 /// cxxRecordDecl(has(cxxConstructorDecl(
 ///   hasAnyConstructorInitializer(anything())
 /// )))
 ///   record matches Foo, hasAnyConstructorInitializer matches foo_(1)
 AST_MATCHER_P(CXXConstructorDecl, hasAnyConstructorInitializer,
               internal::Matcher<CXXCtorInitializer>, InnerMatcher) {
-  auto MatchIt = matchesFirstInPointerRange(InnerMatcher, Node.init_begin(),
-                                            Node.init_end(), Finder, Builder);
-  if (MatchIt == Node.init_end())
-    return false;
-  return (*MatchIt)->isWritten() || !Finder->isTraversalIgnoringImplicitNodes();
+  const auto *MatchIt = matchesFirstInPointerRangeIgnoreUnspelled(
+      InnerMatcher, Node.init_begin(), Node.init_end(), Finder, Builder);
+  return MatchIt != Node.init_end();
 }
 
 /// Matches the field declaration of a constructor initializer.
 ///
 /// Given
 /// \code
 ///   struct Foo {
 ///     Foo() : foo_(1) { }

clang/include/clang/ASTMatchers/ASTMatchersInternal.h

for (IteratorT I = Start; I != End; ++I) {
     if (Matcher.matches(*I, Finder, &Result)) {
       *Builder = std::move(Result);
       return I;
     }
   }
   return End;
 }
 
+inline bool isSpelledInSource(const Decl *Node) { return !Node->isImplicit(); }
+
+inline bool isSpelledInSource(const CXXCtorInitializer *Node) {
+  return Node->isWritten();
+}
+
 /// Finds the first node in a pointer range that matches the given
 /// matcher.
 template <typename MatcherT, typename IteratorT>
 IteratorT matchesFirstInPointerRange(const MatcherT &Matcher, IteratorT Start,
                                      IteratorT End, ASTMatchFinder *Finder,
                                      BoundNodesTreeBuilder *Builder) {
   for (IteratorT I = Start; I != End; ++I) {
     BoundNodesTreeBuilder Result(*Builder);
     if (Matcher.matches(**I, Finder, &Result)) {
       *Builder = std::move(Result);
       return I;
     }
   }
   return End;
 }
 
+/// Finds the first node in a pointer range that matches the given
+/// matcher. Ignores any nodes that aren't spelled in source if Finder is
+/// ignoring implicit nodes.
+template <typename MatcherT, typename IteratorT>
+IteratorT matchesFirstInPointerRangeIgnoreUnspelled(
+    const MatcherT &Matcher, IteratorT Start, IteratorT End,
+    ASTMatchFinder *Finder, BoundNodesTreeBuilder *Builder) {
+  bool IgnoreIsSpelledInSource = Finder->isTraversalIgnoringImplicitNodes();
+  for (IteratorT I = Start; I != End; ++I) {
+    if (IgnoreIsSpelledInSource && !isSpelledInSource(*I))
+      continue;
+    BoundNodesTreeBuilder Result(*Builder);
+    if (Matcher.matches(**I, Finder, &Result)) {
+      *Builder = std::move(Result);
+      return I;
+    }
+  }
+  return End;
+}
+
 template <typename T, std::enable_if_t<!std::is_base_of<FunctionDecl, T>::value>
                           * = nullptr>
 inline bool isDefaultedHelper(const T *) {
   return false;
 }
 inline bool isDefaultedHelper(const FunctionDecl *FD) {
   return FD->isDefaulted();
 }

clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

)cpp";
     EXPECT_TRUE(matchesConditionally(
         Code,
         traverse(TK_IgnoreUnlessSpelledInSource,
                  cxxConstructorDecl(
                      isDefaulted(),
                      hasExplicitSpecifier(callExpr(has(declRefExpr()))))),
         true, {"-std=c++20"}));
   }
+  Code = R"cpp(
+    struct Foo{
+      int A = 4, B;
+      Foo() : B(5) {}
+    };
+  )cpp";
+  {
+    auto M =
+        cxxConstructorDecl(hasAnyConstructorInitializer(cxxCtorInitializer()));
+    EXPECT_TRUE(matches(Code, traverse(TK_AsIs, M)));
+    EXPECT_TRUE(matches(Code, traverse(TK_IgnoreUnlessSpelledInSource, M)));
+  }
 }
 
 template <typename MatcherT>
 bool matcherTemplateWithBinding(StringRef Code, const MatcherT &M) {
   return matchAndVerifyResultTrue(
       Code, M.bind("matchedStmt"),
       std::make_unique<VerifyIdIsBoundTo<ReturnStmt>>("matchedStmt", 1));
 }