[clangd] Allow AST request without range

[ckandeler]

Summary

If no range is given, return the translation unit AST.
This is useful for tooling operations that require e.g. the full path to
a node.

[sammccall]

Thanks! I'm not sure why range was required to start with, probably just lacking a use case.

Only real issue is I think we should update the signature on ClangdServer.

This is useful for tooling operations that require e.g. the full path to

a node.

This could be a fairly bulky/inefficient way of getting that though, for large files!

clang-tools-extra/clangd/ClangdServer.cpp

913

I don't think it makes sense to give an empty range this special meaning.
Can we change ClangdServer::getAST() to take an Optional<Range> instead?

clang-tools-extra/clangd/test/ast-no-range.test

2

We try to cover the features we can in gtests (here unittests/DumpASTTests.cpp) - could you add one?

(Having a lit test too is reasonable as it tests the serialization of requests etc, but not essential)

[ckandeler]

In D101057#2708415, @sammccall wrote:

Only real issue is I think we should update the signature on ClangdServer.

Is that an action point for me?

This is useful for tooling operations that require e.g. the full path to

a node.

This could be a fairly bulky/inefficient way of getting that though, for large files!

Right, though there are probably other good reasons to have access to the full AST.
For this particular example, it might make more sense to (optionally) provide the node's parent
or indeed the AST path, but I think both of these would be more intrusive code-wise.

[ckandeler]

Addressed reviewer comments.

[ckandeler]

Addressed clang-tidy and clang-format comments

[sammccall]

clang-tools-extra/clangd/ClangdServer.cpp

895

Ooh, there's a problem here...

TraverseDecl(TranslationUnitDecl) is going to deserialize the whole preamble and try to return the AST for every decl in every included header.
Apart from a huge amount of data, it's going to be extremely slow, we try to avoid ever doing this.

I think this should be observable in a unit test: if you set TestTU.Code and TestTU.HeaderCode, we want decls from the latter not to be visible in the dumped tree.

---

Instead, dumpAST should call DumpVisitor.TraverseAST(), which runs over the TraversalScope forest (i.e. all the top-level decls of the current file). But we still need the TranslationUnitDecl in order to have a single root.

Therefore I think we need this logic for "full tree" dumping:

DumpVisitor.traverseNode("declaration", TUDecl, [&] { DumpVisitor.TraverseAST() });

I think it's OK to keep the dumpAST signature as-is (i.e. pass a DynTypedNode with TUDecl in it as you're doing now) and just override TraverseTranslationUnitDecl with this special case. However both dumpAST() and the callsite should have a comment saying that TranslationUnitDecl is safe/has these special semantics, as that's not typical in clangd.

---

(The previous version of this patch had this problem too, it was just less obvious. Sorry for not catching it in the first round!)

clang-tools-extra/clangd/ClangdServer.h

347–348

Nit: we tend to pass Range etc by value rather than const ref.
I actually have no idea why! But probably best to be consistent and if we clean this up we can do it everywhere at once.

clang-tools-extra/clangd/Protocol.h

1728–1729

nit: top-level->root
(We usually use "top-level" to mean children of TUDecl)

clang-tools-extra/clangd/unittests/DumpASTTests.cpp

164

as mentioned, this test should have decls in the header file too (HeaderCode, which is implicitly included) to make sure that we're not returning those

[ckandeler]

Fixed TU traversal

[ckandeler]

"top-level" -> "root"

[sammccall]

Fantastic, thank you! Just a couple of nits.

Want me to land this for you?

clang-tools-extra/clangd/DumpAST.cpp

339

The naming causes a bit of confusion over whether this is a (CRTP) override of a RecursiveASTVisitor method. (It's not, as that method is called TraverseTranslationUnitDecl).

I'd suggest either:

• (trivial) rename to traverseTUDecl, and move this away from the comment above that says this is overriding traversal
• (maybe more elegant) instead of this function change the lambda inside TraverseDecl to be if (is TUDecl) TraverseAST() else Base::TraverseDecl(D). Then I don't think the special case in dumpAST() is needed at all.

405

this comment should rather go in the .h file

[ckandeler]

Fixed remaining issues

[ckandeler]

In D101057#2711997, @sammccall wrote:

Want me to land this for you?

Yes, please. I don't have push privileges. Thanks for the prompt and helpful review.

clang-tools-extra/clangd/ClangdServer.h

public:
 
   /// Get all document links in a file.
   void documentLinks(PathRef File, Callback<std::vector<DocumentLink>> CB);
 
   void semanticHighlights(PathRef File,
                           Callback<std::vector<HighlightingToken>>);
 
   /// Describe the AST subtree for a piece of code.
-  void getAST(PathRef File, Range R, Callback<llvm::Optional<ASTNode>> CB);
+  void getAST(PathRef File, llvm::Optional<Range> R,
+              Callback<llvm::Optional<ASTNode>> CB);
 
   /// Runs an arbitrary action that has access to the AST of the specified file.
   /// The action will execute on one of ClangdServer's internal threads.
   /// The AST is only valid for the duration of the callback.
   /// As with other actions, the file must have been opened.
   void customAction(PathRef File, llvm::StringRef Name,
                     Callback<InputsAndAST> Action);
 

clang-tools-extra/clangd/ClangdServer.cpp

auto Action =
         if (!InpAST)
           return CB(InpAST.takeError());
         CB(clangd::getSemanticHighlightings(InpAST->AST));
       };
   WorkScheduler->runWithAST("SemanticHighlights", File, std::move(Action),
                             Transient);
 }
 
-void ClangdServer::getAST(PathRef File, Range R,
+void ClangdServer::getAST(PathRef File, llvm::Optional<Range> R,
                           Callback<llvm::Optional<ASTNode>> CB) {
   auto Action =
       [R, CB(std::move(CB))](llvm::Expected<InputsAndAST> Inputs) mutable {
         if (!Inputs)
           return CB(Inputs.takeError());
+        if (!R) {
+          // It's safe to pass in the TU, as dumpAST() does not
+          // deserialize the preamble.
+          auto Node = DynTypedNode::create(
+                *Inputs->AST.getASTContext().getTranslationUnitDecl());
+          return CB(dumpAST(Node, Inputs->AST.getTokens(),
+                            Inputs->AST.getASTContext()));
+        }
         unsigned Start, End;
-        if (auto Offset = positionToOffset(Inputs->Inputs.Contents, R.start))
+        if (auto Offset = positionToOffset(Inputs->Inputs.Contents, R->start))
           Start = *Offset;
         else
           return CB(Offset.takeError());
-        if (auto Offset = positionToOffset(Inputs->Inputs.Contents, R.end))
+        if (auto Offset = positionToOffset(Inputs->Inputs.Contents, R->end))
           End = *Offset;
         else
           return CB(Offset.takeError());
-
         bool Success = SelectionTree::createEach(
             Inputs->AST.getASTContext(), Inputs->AST.getTokens(), Start, End,
             [&](SelectionTree T) {
               if (const SelectionTree::Node *N = T.commonAncestor()) {
                 CB(dumpAST(N->ASTNode, Inputs->AST.getTokens(),
                            Inputs->AST.getASTContext()));
                 return true;
               }
               return false;
             });
         if (!Success)
           CB(llvm::None);

clang-tools-extra/clangd/DumpAST.h

 #include "clang/AST/ASTContext.h"
 
 namespace clang {
 namespace syntax {
 class TokenBuffer;
 } // namespace syntax
 namespace clangd {
 
+// Note: It's safe for the node to be a TranslationUnitDecl, as this function
+//       does not deserialize the preamble.
 ASTNode dumpAST(const DynTypedNode &, const syntax::TokenBuffer &Tokens,
                 const ASTContext &);
 
 } // namespace clangd
 } // namespace clang
 
 #endif

clang-tools-extra/clangd/DumpAST.cpp

 
 public:
   ASTNode Root;
   DumpVisitor(const syntax::TokenBuffer &Tokens, const ASTContext &Ctx)
       : Tokens(Tokens), Ctx(Ctx) {}
 
   // Override traversal to record the nodes we care about.
   // Generally, these are nodes with position information (TypeLoc, not Type).
+
   bool TraverseDecl(Decl *D) {
-    return !D || isInjectedClassName(D) ||
-           traverseNode("declaration", D, [&] { Base::TraverseDecl(D); });
+    return !D || isInjectedClassName(D) || traverseNode("declaration", D, [&] {
+      if (isa<TranslationUnitDecl>(D))
+        Base::TraverseAST(const_cast<ASTContext &>(Ctx));
+      else
+        Base::TraverseDecl(D);
+    });
   }
   bool TraverseTypeLoc(TypeLoc TL) {
     return !TL || traverseNode("type", TL, [&] { Base::TraverseTypeLoc(TL); });
   }
   bool TraverseTemplateName(const TemplateName &TN) {
     return traverseNode("template name", TN,
                         [&] { Base::TraverseTemplateName(TN); });
   }
public:
   // We only want to traverse the *syntactic form* to understand the selection.
   bool TraversePseudoObjectExpr(PseudoObjectExpr *E) {
     return TraverseStmt(E->getSyntacticForm());
   }
 };
 
 } // namespace
 
 ASTNode dumpAST(const DynTypedNode &N, const syntax::TokenBuffer &Tokens,
                 const ASTContext &Ctx) {
   DumpVisitor V(Tokens, Ctx);
   // DynTypedNode only works with const, RecursiveASTVisitor only non-const :-(
   if (const auto *D = N.get<Decl>())
     V.TraverseDecl(const_cast<Decl *>(D));
   else if (const auto *S = N.get<Stmt>())
     V.TraverseStmt(const_cast<Stmt *>(S));
   else if (const auto *NNSL = N.get<NestedNameSpecifierLoc>())

clang-tools-extra/clangd/Protocol.h

 /// Payload for textDocument/ast request.
 /// This request is a clangd extension.
 struct ASTParams {
   /// The text document.
   TextDocumentIdentifier textDocument;
 
   /// The position of the node to be dumped.
   /// The highest-level node that entirely contains the range will be returned.
-  Range range;
+  /// If no range is given, the root translation unit node will be returned.
+  llvm::Optional<Range> range;
 };
 bool fromJSON(const llvm::json::Value &, ASTParams &, llvm::json::Path);
 
 /// Simplified description of a clang AST node.
 /// This is clangd's internal representation of C++ code.
 struct ASTNode {
   /// The general kind of node, such as "expression"
   /// Corresponds to the base AST node type such as Expr.

clang-tools-extra/clangd/test/ast-no-range.test

+# RUN: clangd -lit-test < %s | FileCheck %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///simple.cpp","languageId":"cpp","version":1,"text":"int x;"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/ast","params":{"textDocument":{"uri":"test:///simple.cpp"}}}
+#      CHECK:  "id": 1,
+# CHECK-NEXT:  "jsonrpc": "2.0",
+# CHECK-NEXT:  "result": {
+# CHECK-NEXT:    "arcana": "{{TranslationUnitDecl.*}}"
+# CHECK-NEXT:    "children": [
+# CHECK-NEXT:      {
+#      CHECK:        "arcana": "VarDecl {{.*}} x 'int'",
+# CHECK-NEXT:        "children": [
+# CHECK-NEXT:          {
+# CHECK-NEXT:            "arcana": "QualType {{.*}} 'int' ",
+# CHECK-NEXT:            "detail": "int",
+# CHECK-NEXT:            "kind": "Builtin",
+# CHECK-NEXT:            "range": {
+# CHECK-NEXT:              "end": {
+# CHECK-NEXT:                "character": 3,
+# CHECK-NEXT:                "line": 0
+# CHECK-NEXT:              },
+# CHECK-NEXT:              "start": {
+# CHECK-NEXT:                "character": 0,
+# CHECK-NEXT:                "line": 0
+# CHECK-NEXT:              }
+# CHECK-NEXT:            },
+# CHECK-NEXT:            "role": "type"
+# CHECK-NEXT:          }
+# CHECK-NEXT:        ],
+# CHECK-NEXT:        "detail": "x",
+# CHECK-NEXT:        "kind": "Var",
+# CHECK-NEXT:        "range": {
+# CHECK-NEXT:          "end": {
+# CHECK-NEXT:            "character": 5,
+# CHECK-NEXT:            "line": 0
+# CHECK-NEXT:          },
+# CHECK-NEXT:          "start": {
+# CHECK-NEXT:            "character": 0,
+# CHECK-NEXT:            "line": 0
+# CHECK-NEXT:          }
+# CHECK-NEXT:        },
+# CHECK-NEXT:        "role": "declaration"
+# CHECK-NEXT:      }
+# CHECK-NEXT:    ],
+# CHECK-NEXT:    "kind": "TranslationUnit",
+# CHECK-NEXT:    "role": "declaration"
+# CHECK-NEXT:  }
+---
+{"jsonrpc":"2.0","id":2,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}

clang-tools-extra/clangd/unittests/DumpASTTests.cpp

 #include "TestTU.h"
 #include "llvm/Support/ScopedPrinter.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace clangd {
 namespace {
+using testing::Contains;
+using testing::Not;
 using testing::SizeIs;
 
+MATCHER_P(WithDetail, str, "") { return arg.detail == str; }
+
 TEST(DumpASTTests, BasicInfo) {
   std::pair</*Code=*/std::string, /*Expected=*/std::string> Cases[] = {
       {R"cpp(
 float root(int *x) {
   return *x + 1;
 }
       )cpp",
        R"(
TEST(DumpASTTests, Range) {
   ParsedAST AST = TestTU::withCode(Case.code()).build();
   auto Node = dumpAST(DynTypedNode::create(findDecl(AST, "x")), AST.getTokens(),
                       AST.getASTContext());
   EXPECT_EQ(Node.range, Case.range("var"));
   ASSERT_THAT(Node.children, SizeIs(1)) << "Expected one child typeloc";
   EXPECT_EQ(Node.children.front().range, Case.range("type"));
 }
 
+TEST(DumpASTTests, NoRange) {
+  auto TU = TestTU::withHeaderCode("void funcFromHeader();");
+  TU.Code = "int varFromSource;";
+  ParsedAST AST = TU.build();
+  auto Node = dumpAST(
+      DynTypedNode::create(*AST.getASTContext().getTranslationUnitDecl()),
+      AST.getTokens(), AST.getASTContext());
+  ASSERT_THAT(Node.children, Contains(WithDetail("varFromSource")));
+  ASSERT_THAT(Node.children, Not(Contains(WithDetail("funcFromHeader"))));
+  EXPECT_THAT(Node.arcana, testing::StartsWith("TranslationUnitDecl "));
+  ASSERT_FALSE(Node.range.hasValue())
+      << "Expected no range for translation unit";
+}
+
 TEST(DumpASTTests, Arcana) {
   ParsedAST AST = TestTU::withCode("int x;").build();
   auto Node = dumpAST(DynTypedNode::create(findDecl(AST, "x")), AST.getTokens(),
                       AST.getASTContext());
   EXPECT_THAT(Node.arcana, testing::StartsWith("VarDecl "));
   EXPECT_THAT(Node.arcana, testing::EndsWith(" 'int'"));
   ASSERT_THAT(Node.children, SizeIs(1)) << "Expected one child typeloc";
   EXPECT_THAT(Node.children.front().arcana, testing::StartsWith("QualType "));
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang