[Clang][AST] BindingDecl ASTDump for tuple like structure

[isuckatcs]

Summary

@xazax.hun noticed that the AST textual dumps in case of BindingDecl are incomplete. As @NoQ pointed out, there is a special VarDecl which is unique to tuple like structures, and it turned out that this declaration is not visited by the ASTNodeTraverser.

This patch is supposed to fix this issue by telling the ASTNodeTraverser to visit the required VarDecl if it is available.

[NoQ]

It looks like this class is used for more than dumping. History digging points to @steveire who introduced this class in order to re-use it for ASTMatcher traversal. Stephen, does this look like the right thing to do? Holding vars aren't visible in the source, should they be skipped in matcher traversal when they are set up to skip invisible nodes?

This class appears to be covered with unittests in unittests/AST/ASTTraverserTest.cpp, so there's no excuse for us not to add a test for the new functionality (sorry for double negation, I mean we should add a test).

[isuckatcs]

@NoQ What I found is that TextNodeDumper.h:73 DumpWithIndent() through DoAddChild calls ASTNodeTraverser.h:88 Visit()
which is the function that calls visitors responsible for printing (to be more precise it is a lambda that dispatches the visitors).

First in ASTNodeTraverser.h:93 TextNodeDumper.cpp:242 Visit() is called, which in the end calls TextNodeDumper.cpp:1801 VisitBindingDecl()
that results in printing the BindingDecl itself.

After this is done ASTNodeTraverser.h:467 VisitBindingDecl() is called in ASTNodeTraverser.h:97. This is the function I modified to
dispatch a visitor on D->getHoldingVar() if available. Before the modification it only dispatched a visitor to D->getBinding() and
after this function returned, BindingDecl and it's child nodes have already been printed. The visitor dispatched on D->getBinding()
resulted in printing the binding and it's child nodes.

Based on these results it seems that dumping D->getHoldingVar() must happen after BindingDecl (the parent node) is dumped
but before VisitBindingDecl returns (the child nodes are dumped). So dispatching the visitor in VisitBindingDecl() seemed to be
the only logical option.

I ran clang tests which didn't result in any error but that is not an accure result, because it seems in ASTTraverserTest.cpp structured
bindings are only tested in case of arrays. So besides writing a testcase for tuple like structures, one should be added for binding to data
members.

Is it a good idea to add a testcase for data members now, or should I do it in a separate patch later?

[isuckatcs]

Added unit tests for the change, as requested.

[isuckatcs]

Squashed previous diffs

[NoQ]

clang/unittests/AST/ASTTraverserTest.cpp

1080

Unfortunately we can't do that :( Such include would cause the system header to be pulled, which could technically make the test depend on the system on which it runs.

In order to write a test we typically "mock" system headers by bringing in just enough declarations for the rest of the code to work. Eg., you could write something like (I didn't test this):

typedef __typeof(sizeof(int)) size_t;

struct Pair {
  int x, y;
};

template <size_t I> int &get(const Pair &p);

int &get<0>(const Pair &p) { return p.x; }
int &get<1>(const Pair &p) { return p.x; }

[isuckatcs]

Created a mock tuple like structure instead of including std::tuple

[xazax.hun]

Hi Daniel,

Just a small technical nit, we usually want patches on Phabricator to include context to make it as easy as possible to review the changes. See https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface for instructions. Although I do see context in your other patches, so you probably are already aware :) Just wanted to make 100% sure.

Overall, the approach looks good to me, I added some nits inline. I am not sure about the correctness of the original code with respect to TK_IgnoreUnlessSpelledInSource and structured bindings, but that does not need to be addressed in this PR. Also, I think we do not need to wait for @steveire's response indefinitely. If he does not show up in a couple of day feel free to commit. We can always address his comments in a follow-up PR later.

clang/include/clang/AST/ASTNodeTraverser.h

474

Hmm, interesting. The binding itself is spelled in the source, right? I wonder if we are actually skipping them inadvertently. Moreover, it looks like DecompositionDecl is visited even when TK_IgnoreUnlessSpelledInSource is set. Even though the object defined by that node is not actually spelled in the source code. I wonder if the original author got this backwards. On the other hand, I think this PR is fine as is, and probably this should be fixed in a separate PR, and that PR should have targeted tests to demonstrate that particular problem.

clang/unittests/AST/ASTTraverserTest.cpp

1193

I am actually not sure about the use of RValue references here. What is the intent?

1199

I think you could get away with not even defining these getters, a declaration can be sufficient for the code to compile and that can make the tests a bit smaller. Although some people prefer to keep small code snippets self contained, so feel free to leave the definitions in.

[isuckatcs]

clang/unittests/AST/ASTTraverserTest.cpp

1193

I got compile time errors, and after some searching I found a stackoverflow post where someone suggested the use of RValue references.

I did some testing and it seems that only the parameter has to be an RValue reference to get a compiling code. But if the parameter is passed by RValue I think it makes sense to make the return type and RValue too.

The other problem with returning by LValue is that it will result in a different AST than the actual, non mock binding, and for testing purposes I wanted to keep the AST the same.

[isuckatcs]

• Removed unnecessary get definitions
• Inluded missing context from previous diff

[xazax.hun]

I think we waited long enough for other reviewers to chime in. So we can land this. @NoQ, do you agree?

[NoQ]

Yeah I think this is good to go!

clang/include/clang/AST/ASTNodeTraverser.h

void VisitDecompositionDecl(const DecompositionDecl *D) {
     VisitVarDecl(D);
     for (const auto *B : D->bindings())
       Visit(B);
   }
 
   void VisitBindingDecl(const BindingDecl *D) {
     if (Traversal == TK_IgnoreUnlessSpelledInSource)
       return;
+
+    if (const auto *V = D->getHoldingVar())
+      Visit(V);
+
     if (const auto *E = D->getBinding())
       Visit(E);
   }
 
   void VisitFileScopeAsmDecl(const FileScopeAsmDecl *D) {
     Visit(D->getAsmString());
   }
 
   void VisitCapturedDecl(const CapturedDecl *D) { Visit(D->getBody()); }

clang/unittests/AST/ASTTraverserTest.cpp

EXPECT_EQ(dumpASTString(TK_IgnoreUnlessSpelledInSource, TUDecl),
               R"cpp(
 TranslationUnitDecl
 `-VarDecl 'i'
   `-IntegerLiteral
 )cpp");
   }
 
   auto AST2 = buildASTFromCodeWithArgs(R"cpp(
 struct Simple {
 };
 struct Other {
 };
 
 struct Record : Simple, Other {
   Record() : Simple(), m_i(42) {}
 private:
   int m_i;

 void decomposition()
 {
     int arr[3];
     auto &[f, s, t] = arr;
 
     f = 42;
 }
 
+typedef __typeof(sizeof(int)) size_t;
+
+struct Pair
+{
+    int x, y;
+};
+
+// Note: these utilities are required to force binding to tuple like structure
+namespace std
+{
+    template <typename E>
+    struct tuple_size
+    {
+    };
+
+    template <>
+    struct tuple_size<Pair>
+    {
+        static constexpr size_t value = 2;
+    };
+
+    template <size_t I, class T>
+    struct tuple_element
+    {
+        using type = int;
+    };
+
+};
+
+template <size_t I>
+int &&get(Pair &&p);
+
+void decompTuple()
+{
+    Pair p{1, 2};
+    auto [a, b] = p;
+
+    a = 3;
+}
+
 )cpp",
                                        {"-std=c++20"});
 
   {
     auto BN = ast_matchers::match(
         cxxRecordDecl(hasName("Record"), unless(isImplicit())).bind("rec"),
         AST2->getASTContext());
     EXPECT_EQ(BN.size(), 1u);
EXPECT_EQ(dumpASTString(TK_IgnoreUnlessSpelledInSource,
               R"cpp(
 DecompositionDecl ''
 |-DeclRefExpr 'arr'
 |-BindingDecl 'f'
 |-BindingDecl 's'
 `-BindingDecl 't'
 )cpp");
   }
+
+  {
+    auto FN = ast_matchers::match(
+        functionDecl(hasName("decompTuple"),
+                     hasDescendant(decompositionDecl().bind("decomp"))),
+        AST2->getASTContext());
+    EXPECT_EQ(FN.size(), 1u);
+
+    EXPECT_EQ(
+        dumpASTString(TK_AsIs, FN[0].getNodeAs<DecompositionDecl>("decomp")),
+        R"cpp(
+DecompositionDecl ''
+|-CXXConstructExpr
+| `-ImplicitCastExpr
+|   `-DeclRefExpr 'p'
+|-BindingDecl 'a'
+| |-VarDecl 'a'
+| | `-CallExpr
+| |   |-ImplicitCastExpr
+| |   | `-DeclRefExpr 'get'
+| |   `-ImplicitCastExpr
+| |     `-DeclRefExpr ''
+| `-DeclRefExpr 'a'
+`-BindingDecl 'b'
+  |-VarDecl 'b'
+  | `-CallExpr
+  |   |-ImplicitCastExpr
+  |   | `-DeclRefExpr 'get'
+  |   `-ImplicitCastExpr
+  |     `-DeclRefExpr ''
+  `-DeclRefExpr 'b'
+)cpp");
+
+    EXPECT_EQ(dumpASTString(TK_IgnoreUnlessSpelledInSource,
+                            FN[0].getNodeAs<DecompositionDecl>("decomp")),
+              R"cpp(
+DecompositionDecl ''
+|-DeclRefExpr 'p'
+|-BindingDecl 'a'
+`-BindingDecl 'b'
+)cpp");
+  }
 }
 
 TEST(Traverse, IgnoreUnlessSpelledInSourceTemplateInstantiations) {
 
   auto AST = buildASTFromCode(R"cpp(
 template<typename T>
 struct TemplStruct {
   TemplStruct() {}