[clang-tools-extra] d7d1af3 - [clangd] Fix DocumentSymbol ranges

[Kirill Bobyrev via cfe-commits]

Author: Kirill Bobyrev
Date: 2020-07-13T15:02:53+02:00
New Revision: d7d1af39168ce8afd041f3ae8db1d1fd3d4f70ac

URL: https://github.com/llvm/llvm-project/commit/d7d1af39168ce8afd041f3ae8db1d1fd3d4f70ac
DIFF: https://github.com/llvm/llvm-project/commit/d7d1af39168ce8afd041f3ae8db1d1fd3d4f70ac.diff

LOG: [clangd] Fix DocumentSymbol ranges

Summary:
DocumentSymbol ranges were not previously tested and, as a result, had invalid
end location. This patch addresses the issue.

Reviewers: sammccall

Reviewed By: sammccall

Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, usaxena95, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D83668

Added: 
    

Modified: 
    clang-tools-extra/clangd/FindSymbols.cpp
    clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clangd/FindSymbols.cpp b/clang-tools-extra/clangd/FindSymbols.cpp
index 58e2ee1e21c7..f5d6a95aa713 100644
--- a/clang-tools-extra/clangd/FindSymbols.cpp
+++ b/clang-tools-extra/clangd/FindSymbols.cpp
@@ -136,17 +136,11 @@ llvm::Optional<DocumentSymbol> declToSym(ASTContext &Ctx, const NamedDecl &ND) {
   auto &SM = Ctx.getSourceManager();
 
   SourceLocation NameLoc = nameLocation(ND, SM);
-  // getFileLoc is a good choice for us, but we also need to make sure
-  // sourceLocToPosition won't switch files, so we call getSpellingLoc on top of
-  // that to make sure it does not switch files.
-  // FIXME: sourceLocToPosition should not switch files!
   SourceLocation BeginLoc = SM.getSpellingLoc(SM.getFileLoc(ND.getBeginLoc()));
   SourceLocation EndLoc = SM.getSpellingLoc(SM.getFileLoc(ND.getEndLoc()));
-  if (NameLoc.isInvalid() || BeginLoc.isInvalid() || EndLoc.isInvalid())
-    return llvm::None;
-
-  if (!SM.isWrittenInMainFile(NameLoc) || !SM.isWrittenInMainFile(BeginLoc) ||
-      !SM.isWrittenInMainFile(EndLoc))
+  const auto SymbolRange =
+      toHalfOpenFileRange(SM, Ctx.getLangOpts(), {BeginLoc, EndLoc});
+  if (!SymbolRange)
     return llvm::None;
 
   Position NameBegin = sourceLocToPosition(SM, NameLoc);
@@ -162,8 +156,8 @@ llvm::Optional<DocumentSymbol> declToSym(ASTContext &Ctx, const NamedDecl &ND) {
   SI.name = printName(Ctx, ND);
   SI.kind = SK;
   SI.deprecated = ND.isDeprecated();
-  SI.range =
-      Range{sourceLocToPosition(SM, BeginLoc), sourceLocToPosition(SM, EndLoc)};
+  SI.range = Range{sourceLocToPosition(SM, SymbolRange->getBegin()),
+                   sourceLocToPosition(SM, SymbolRange->getEnd())};
   SI.selectionRange = Range{NameBegin, NameEnd};
   if (!SI.range.contains(SI.selectionRange)) {
     // 'selectionRange' must be contained in 'range', so in cases where clang

diff  --git a/clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp b/clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp
index 31879e356ce0..07c42fcf2030 100644
--- a/clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp
+++ b/clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp
@@ -35,7 +35,7 @@ MATCHER_P(QName, Name, "") {
 }
 MATCHER_P(WithName, N, "") { return arg.name == N; }
 MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
-MATCHER_P(SymRange, Range, "") { return arg.location.range == Range; }
+MATCHER_P(SymRange, Range, "") { return arg.range == Range; }
 
 // GMock helpers for matching DocumentSymbol.
 MATCHER_P(SymNameRange, Range, "") { return arg.selectionRange == Range; }
@@ -712,6 +712,72 @@ TEST(DocumentSymbols, QualifiersWithTemplateArgs) {
                            WithName("Foo_type::method3")));
 }
 
+TEST(DocumentSymbolsTest, Ranges) {
+  TestTU TU;
+  Annotations Main(R"(
+      $foo[[int foo(bool Argument) {
+        return 42;
+      }]]
+
+      $variable[[char GLOBAL_VARIABLE]];
+
+      $ns[[namespace ns {
+      $bar[[class Bar {
+      public:
+        $ctor[[Bar() {}]]
+        $dtor[[~Bar()]];
+
+      private:
+        $field[[unsigned Baz]];
+
+        $getbaz[[unsigned getBaz() { return Baz; }]]
+      }]];
+      }]] // namespace ns
+
+      $forwardclass[[class ForwardClassDecl]];
+
+      $struct[[struct StructDefinition {
+        $structfield[[int *Pointer = nullptr]];
+      }]];
+      $forwardstruct[[struct StructDeclaration]];
+
+      $forwardfunc[[void forwardFunctionDecl(int Something)]];
+    )");
+  TU.Code = Main.code().str();
+  EXPECT_THAT(
+      getSymbols(TU.build()),
+      UnorderedElementsAre(
+          AllOf(WithName("foo"), WithKind(SymbolKind::Function),
+                SymRange(Main.range("foo"))),
+          AllOf(WithName("GLOBAL_VARIABLE"), WithKind(SymbolKind::Variable),
+                SymRange(Main.range("variable"))),
+          AllOf(
+              WithName("ns"), WithKind(SymbolKind::Namespace),
+              SymRange(Main.range("ns")),
+              Children(AllOf(
+                  WithName("Bar"), WithKind(SymbolKind::Class),
+                  SymRange(Main.range("bar")),
+                  Children(
+                      AllOf(WithName("Bar"), WithKind(SymbolKind::Constructor),
+                            SymRange(Main.range("ctor"))),
+                      AllOf(WithName("~Bar"), WithKind(SymbolKind::Constructor),
+                            SymRange(Main.range("dtor"))),
+                      AllOf(WithName("Baz"), WithKind(SymbolKind::Field),
+                            SymRange(Main.range("field"))),
+                      AllOf(WithName("getBaz"), WithKind(SymbolKind::Method),
+                            SymRange(Main.range("getbaz"))))))),
+          AllOf(WithName("ForwardClassDecl"), WithKind(SymbolKind::Class),
+                SymRange(Main.range("forwardclass"))),
+          AllOf(WithName("StructDefinition"), WithKind(SymbolKind::Struct),
+                SymRange(Main.range("struct")),
+                Children(AllOf(WithName("Pointer"), WithKind(SymbolKind::Field),
+                               SymRange(Main.range("structfield"))))),
+          AllOf(WithName("StructDeclaration"), WithKind(SymbolKind::Struct),
+                SymRange(Main.range("forwardstruct"))),
+          AllOf(WithName("forwardFunctionDecl"), WithKind(SymbolKind::Function),
+                SymRange(Main.range("forwardfunc")))));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang