WIP: (squash) Improved error handling

Cleanup
Danger, this commit will be sqashed, do not rely on it

Author: Febbe

clang-tools-extra/clangd/ClangdLSPServer.cpp

@@ -76,7 +76,7 @@ std::optional<int64_t> decodeVersion(llvm::StringRef Encoded) {
 const llvm::StringLiteral ApplyFixCommand = "clangd.applyFix";
 const llvm::StringLiteral ApplyTweakCommand = "clangd.applyTweak";
 const llvm::StringLiteral ApplyRenameCommand = "clangd.applyRename";
-constexpr llvm::StringLiteral SearchASTCommand = "clangd.searchAST";
+constexpr llvm::StringLiteral SearchASTMethod = "textDocument/searchAST";
 
 CodeAction toCodeAction(const ClangdServer::CodeActionResult::Rename &R,
                         const URIForFile &File) {
@@ -853,14 +853,13 @@ void ClangdLSPServer::onCommandApplyRename(const RenameParams &R,
   });
 }
 
-void ClangdLSPServer::onCommandSearchAST(const SearchASTArgs &Args,
-                                         Callback<llvm::json::Value> Reply) {
+void ClangdLSPServer::onMethodSearchAST(const SearchASTArgs &Args,
+                                        Callback<llvm::json::Value> Reply) {
   Server->findAST(Args, [Reply = std::move(Reply)](
                             llvm::Expected<std::vector<std::vector<ASTNode>>>
                                 BoundNodes) mutable {
     if (!BoundNodes)
       return Reply(BoundNodes.takeError());
-    auto v = llvm::json::Value(*BoundNodes);
     return Reply(*BoundNodes);
   });
 }
@@ -1741,7 +1740,7 @@ void ClangdLSPServer::bindMethods(LSPBinder &Bind,
   Bind.command(ApplyFixCommand, this, &ClangdLSPServer::onCommandApplyEdit);
   Bind.command(ApplyTweakCommand, this, &ClangdLSPServer::onCommandApplyTweak);
   Bind.command(ApplyRenameCommand, this, &ClangdLSPServer::onCommandApplyRename);
-  Bind.command(SearchASTCommand, this, &ClangdLSPServer::onCommandSearchAST);
+  Bind.method(SearchASTMethod, this, &ClangdLSPServer::onMethodSearchAST);
 
   ApplyWorkspaceEdit = Bind.outgoingMethod("workspace/applyEdit");
   PublishDiagnostics = Bind.outgoingNotification("textDocument/publishDiagnostics");

clang-tools-extra/clangd/ClangdLSPServer.h

@@ -186,7 +186,7 @@ class ClangdLSPServer : private ClangdServer::Callbacks,
   void onCommandApplyEdit(const WorkspaceEdit &, Callback<llvm::json::Value>);
   void onCommandApplyTweak(const TweakArgs &, Callback<llvm::json::Value>);
   void onCommandApplyRename(const RenameParams &, Callback<llvm::json::Value>);
-  void onCommandSearchAST(const SearchASTArgs &, Callback<llvm::json::Value>);
+  void onMethodSearchAST(const SearchASTArgs &, Callback<llvm::json::Value>);
 
   /// Outgoing LSP calls.
   LSPBinder::OutgoingMethod<ApplyWorkspaceEditParams,

clang-tools-extra/clangd/ClangdServer.cpp

@@ -817,14 +817,16 @@ void ClangdServer::findAST(SearchASTArgs const &Args,
         if (!InpAST)
           return CB(InpAST.takeError());
         auto BoundNodes = clangd::locateASTQuery(InpAST->AST, Args);
-        if (BoundNodes.empty())
+        if (!BoundNodes)
+          return CB(BoundNodes.takeError());
+        if (BoundNodes->empty())
           return CB(error("No matching AST nodes found"));
 
         auto &&AST = InpAST->AST;
         // Convert BoundNodes to a vector of vectors to ASTNode's.
         std::vector<std::vector<ASTNode>> Result;
-        Result.reserve(BoundNodes.size());
-        for (auto &&BN : BoundNodes) {
+        Result.reserve(BoundNodes->size());
+        for (auto &&BN : *BoundNodes) {
           auto &&Map = BN.getMap();
           std::vector<ASTNode> Nodes;
           Nodes.reserve(Map.size());

clang-tools-extra/clangd/XRefs.cpp

@@ -73,6 +73,7 @@
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
+#include <algorithm>
 #include <cmath>
 #include <optional>
 #include <string>
@@ -782,57 +783,54 @@ const syntax::Token *findNearbyIdentifier(const SpelledWord &Word,
 }
 
 auto locateASTQuery(ParsedAST &AST, SearchASTArgs const &Query)
-    -> std::vector<ast_matchers::BoundNodes> {
+    -> llvm::Expected<std::vector<ast_matchers::BoundNodes>> {
   using namespace ast_matchers;
   using namespace ast_matchers::dynamic;
   using ast_matchers::dynamic::Parser;
 
   Diagnostics Diag;
   auto MatcherSource = llvm::StringRef(Query.searchQuery).ltrim();
-  auto OrigMatcherSource = MatcherSource;
 
   std::optional<DynTypedMatcher> Matcher = Parser::parseMatcherExpression(
       MatcherSource,
       nullptr /* is this sema instance usefull, to reduce overhead?*/,
       nullptr /*we currently don't support let*/, &Diag);
   if (!Matcher) {
-    elog("Not a valid top-level matcher.\n");
-    return {/* TODO */};
+    return error("Not a valid top-level matcher: {}.", Diag.toString());
   }
-  auto ActualSource = OrigMatcherSource.slice(0, OrigMatcherSource.size() -
-                                                     MatcherSource.size());
-  auto *Q = new query::MatchQuery(ActualSource, *Matcher);
-  Q->RemainingContent = MatcherSource;
 
-  // Q->run(AST);:
-  //==
 
   struct CollectBoundNodes : MatchFinder::MatchCallback {
-    std::vector<BoundNodes> &Bindings;
-    CollectBoundNodes(std::vector<BoundNodes> &Bindings) : Bindings(Bindings) {}
+    std::vector<BoundNodes> *Bindings;
+    CollectBoundNodes(std::vector<BoundNodes> &Bindings)
+        : Bindings(&Bindings) {}
     void run(const MatchFinder::MatchResult &Result) override {
-      Bindings.push_back(Result.Nodes);
+      Bindings->push_back(Result.Nodes);
     }
   };
 
-  MatchFinder Finder;
-  std::vector<BoundNodes> Matches;
   DynTypedMatcher MaybeBoundMatcher = *Matcher;
   if (Query.BindRoot) {
     std::optional<DynTypedMatcher> M = Matcher->tryBind("root");
     if (M)
       MaybeBoundMatcher = *M;
   }
+  std::vector<BoundNodes> Matches;
   CollectBoundNodes Collect(Matches);
+
+  MatchFinder::MatchFinderOptions Opt;
+  Opt.IgnoreSystemHeaders = true;
+  MatchFinder Finder{Opt};
   if (!Finder.addDynamicMatcher(MaybeBoundMatcher, &Collect)) {
-    log("Not a valid top-level matcher.\n");
-    return {/* TODO */};
+    return error("Can't add matcher.");
   }
 
   ASTContext &Ctx = AST.getASTContext();
+
+  auto OldTK = Ctx.getParentMapContext().getTraversalKind();
   Ctx.getParentMapContext().setTraversalKind(Query.Tk);
   Finder.matchAST(Ctx);
-
+  Ctx.getParentMapContext().setTraversalKind(OldTK);
   return Matches;
 }
 

clang-tools-extra/clangd/XRefs.h

@@ -40,7 +40,7 @@ struct LocatedAST {
 llvm::raw_ostream &operator<<(llvm::raw_ostream &, const LocatedAST &);
 
 auto locateASTQuery(ParsedAST &AST, SearchASTArgs const &)
-    -> std::vector<ast_matchers::BoundNodes>;
+    -> llvm::Expected<std::vector<ast_matchers::BoundNodes>>;
 
 // Describes where a symbol is declared and defined (as far as clangd knows).
 // There are three cases: