[PATCH] D159497: [RFC][clangd] Check if SelectionTree is complete

[Kugan Vivekanandarajah via Phabricator via cfe-commits]

kuganv created this revision.
Herald added subscribers: kadircet, arphaman.
Herald added a project: All.
kuganv retitled this revision from "[clangd] Check if SelectionTree is complete" to "[RFC][clangd] Check if SelectionTree is complete".
kuganv edited the summary of this revision.
kuganv added reviewers: sammccall, kadircet, nridge, DmitryPolukhin, ivanmurashko.
kuganv updated this revision to Diff 556418.
kuganv edited the summary of this revision.
kuganv added a comment.
kuganv edited the summary of this revision.
kuganv published this revision for review.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

Rebased


clangd in the presence of compilation error can navigate to completely irrelevant destinations. This is often a problem for headers that cannot be compiled standalone. For example, go-to-def often jumps to containing namespace because the selection tree just consists of namespace (when  relevant AST nodes were missing due to compilation error). This is known issue that is hard to solve without using some heuristic.  One such heuristic is to check the nodes in the selection tree for the exact line match. In the absence of such match, we do not provide navigation. IMO, this is better than jumping to wrong position.

TODO:

- This currently does not have test cases but I will add test cases based on the feedback for the heuristic.
- Enable only when there is compilation error.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D159497

Files:
  clang-tools-extra/clangd/Selection.cpp
  clang-tools-extra/clangd/Selection.h
  clang-tools-extra/clangd/XRefs.cpp


Index: clang-tools-extra/clangd/XRefs.cpp
===================================================================
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -179,6 +179,12 @@
   unsigned Offset = AST.getSourceManager().getDecomposedSpellingLoc(Pos).second;
   std::vector<std::pair<const NamedDecl *, DeclRelationSet>> Result;
   auto ResultFromTree = [&](SelectionTree ST) {
+    // If the SelectionTree does not have nodes from Pos, we will navigate to
+    // wrong locations. In these cases, it is better to return empty results
+    // rather than jumping to irrelevant location.
+    if (!ST.TochesSourceLoc(Pos, AST.getSourceManager())) {
+      return false;
+    }
     if (const SelectionTree::Node *N = ST.commonAncestor()) {
       if (NodeKind)
         *NodeKind = N->ASTNode.getNodeKind();
Index: clang-tools-extra/clangd/Selection.h
===================================================================
--- clang-tools-extra/clangd/Selection.h
+++ clang-tools-extra/clangd/Selection.h
@@ -146,6 +146,7 @@
   const Node *commonAncestor() const;
   // The selection node corresponding to TranslationUnitDecl.
   const Node &root() const { return *Root; }
+  bool TochesSourceLoc(SourceLocation Pos, const SourceManager &SM);
 
 private:
   // Creates a selection tree for the given range in the main file.
Index: clang-tools-extra/clangd/Selection.cpp
===================================================================
--- clang-tools-extra/clangd/Selection.cpp
+++ clang-tools-extra/clangd/Selection.cpp
@@ -1104,6 +1104,29 @@
   return Ancestor != Root ? Ancestor : nullptr;
 }
 
+// Check if source line at Pos touches any node in SelectionTree. In the
+// presence of compilation error, we will have only root nodes without the
+// nodes that coresponds to tokens from Pos. In these cases, clangd cannot
+// provide meaningful navigation and often jumps to the conataining scope
+// identifier such as napespace.
+bool SelectionTree::TochesSourceLoc(SourceLocation Pos,
+                                    const SourceManager &SM) {
+  const auto DecomposedPos = SM.getDecomposedSpellingLoc(Pos);
+  int Line = SM.getLineNumber(DecomposedPos.first, DecomposedPos.second);
+  for (const auto &Node : Nodes) {
+    const auto Begin =
+        SM.getDecomposedSpellingLoc(getSourceRange(Node.ASTNode).getBegin());
+    const auto End =
+        SM.getDecomposedSpellingLoc(getSourceRange(Node.ASTNode).getEnd());
+    int startLine = SM.getLineNumber(Begin.first, Begin.second);
+    int endLine = SM.getLineNumber(End.first, End.second);
+    if (startLine == Line || endLine == Line) {
+      return true;
+    }
+  }
+  return false;
+}
+
 const DeclContext &SelectionTree::Node::getDeclContext() const {
   for (const Node *CurrentNode = this; CurrentNode != nullptr;
        CurrentNode = CurrentNode->Parent) {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D159497.556418.patch
Type: text/x-patch
Size: 2866 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20230911/a6f3e186/attachment.bin>