[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

906

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

1

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 ↗

(On Diff #339948)

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

160 ↗

(On Diff #339948)

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 ↗

(On Diff #339995)

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 ↗

(On Diff #339995)

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/ClangdLSPServer.cpp

void ClangdLSPServer::onMemoryUsage(const NoParams &,
   llvm::BumpPtrAllocator DetailAlloc;
   MemoryTree MT(&DetailAlloc);
   profile(MT);
   Reply(std::move(MT));
 }
 
 void ClangdLSPServer::onAST(const ASTParams &Params,
                             Callback<llvm::Optional<ASTNode>> CB) {
-  Server->getAST(Params.textDocument.uri.file(), Params.range, std::move(CB));
+  Server->getAST(Params.textDocument.uri.file(), Params.range ? *Params.range : Range{},
+                 std::move(CB));
 }
 
 ClangdLSPServer::ClangdLSPServer(Transport &Transp, const ThreadsafeFS &TFS,
                                  const ClangdLSPServer::Options &Opts)
     : ShouldProfile(/*Period=*/std::chrono::minutes(5),
                     /*Delay=*/std::chrono::minutes(1)),
       ShouldCleanupMemory(/*Period=*/std::chrono::minutes(1),
                           /*Delay=*/std::chrono::minutes(1)),

clang-tools-extra/clangd/ClangdServer.cpp

 void ClangdServer::getAST(PathRef File, 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());
         unsigned Start, End;
         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))
           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()) {
+              const SelectionTree::Node *N = R.start == R.end ? &T.root() : T.commonAncestor();
+              if (N) {
                 CB(dumpAST(N->ASTNode, Inputs->AST.getTokens(),
                            Inputs->AST.getASTContext()));
                 return true;
               }
               return false;
             });
         if (!Success)
           CB(llvm::None);

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 top-level 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"}