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) {
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) {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
