[clang-tools-extra] b764edc - [clangd] Try harder to get accurate ranges for documentSymbols in macros

[Nathan Ridge via cfe-commits]

Author: Nathan Ridge
Date: 2020-10-12T19:26:36-04:00
New Revision: b764edc59ff7768e052bc2b9e76e3bb69dd5147b

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

LOG: [clangd] Try harder to get accurate ranges for documentSymbols in macros

Fixes https://github.com/clangd/clangd/issues/500

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

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 3169ffd98038..abd03ecb0464 100644
--- a/clang-tools-extra/clangd/FindSymbols.cpp
+++ b/clang-tools-extra/clangd/FindSymbols.cpp
@@ -171,7 +171,6 @@ namespace {
 llvm::Optional<DocumentSymbol> declToSym(ASTContext &Ctx, const NamedDecl &ND) {
   auto &SM = Ctx.getSourceManager();
 
-  SourceLocation NameLoc = nameLocation(ND, SM);
   SourceLocation BeginLoc = SM.getSpellingLoc(SM.getFileLoc(ND.getBeginLoc()));
   SourceLocation EndLoc = SM.getSpellingLoc(SM.getFileLoc(ND.getEndLoc()));
   const auto SymbolRange =
@@ -179,10 +178,6 @@ llvm::Optional<DocumentSymbol> declToSym(ASTContext &Ctx, const NamedDecl &ND) {
   if (!SymbolRange)
     return llvm::None;
 
-  Position NameBegin = sourceLocToPosition(SM, NameLoc);
-  Position NameEnd = sourceLocToPosition(
-      SM, Lexer::getLocForEndOfToken(NameLoc, 0, SM, Ctx.getLangOpts()));
-
   index::SymbolInfo SymInfo = index::getSymbolInfo(&ND);
   // FIXME: this is not classifying constructors, destructors and operators
   //        correctly (they're all "methods").
@@ -194,10 +189,35 @@ llvm::Optional<DocumentSymbol> declToSym(ASTContext &Ctx, const NamedDecl &ND) {
   SI.deprecated = ND.isDeprecated();
   SI.range = Range{sourceLocToPosition(SM, SymbolRange->getBegin()),
                    sourceLocToPosition(SM, SymbolRange->getEnd())};
-  SI.selectionRange = Range{NameBegin, NameEnd};
+
+  SourceLocation NameLoc = ND.getLocation();
+  SourceLocation FallbackNameLoc;
+  if (NameLoc.isMacroID()) {
+    if (isSpelledInSource(NameLoc, SM)) {
+      // Prefer the spelling loc, but save the expansion loc as a fallback.
+      FallbackNameLoc = SM.getExpansionLoc(NameLoc);
+      NameLoc = SM.getSpellingLoc(NameLoc);
+    } else {
+      NameLoc = SM.getExpansionLoc(NameLoc);
+    }
+  }
+  auto ComputeSelectionRange = [&](SourceLocation L) -> Range {
+    Position NameBegin = sourceLocToPosition(SM, L);
+    Position NameEnd = sourceLocToPosition(
+        SM, Lexer::getLocForEndOfToken(L, 0, SM, Ctx.getLangOpts()));
+    return Range{NameBegin, NameEnd};
+  };
+
+  SI.selectionRange = ComputeSelectionRange(NameLoc);
+  if (!SI.range.contains(SI.selectionRange) && FallbackNameLoc.isValid()) {
+    // 'selectionRange' must be contained in 'range'. In cases where clang
+    // reports unrelated ranges, we first try falling back to the expansion
+    // loc for the selection range.
+    SI.selectionRange = ComputeSelectionRange(FallbackNameLoc);
+  }
   if (!SI.range.contains(SI.selectionRange)) {
-    // 'selectionRange' must be contained in 'range', so in cases where clang
-    // reports unrelated ranges we need to reconcile somehow.
+    // If the containment relationship still doesn't hold, throw away
+    // 'range' and use 'selectionRange' for both.
     SI.range = SI.selectionRange;
   }
   return SI;

diff  --git a/clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp b/clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp
index a7a0188a85fd..43658284937e 100644
--- a/clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp
+++ b/clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp
@@ -639,19 +639,27 @@ TEST(DocumentSymbols, FromMacro) {
     #define FF(name) \
       class name##_Test {};
 
-    $expansion[[FF]](abc);
+    $expansion1[[FF]](abc);
 
     #define FF2() \
-      class $spelling[[Test]] {};
+      class Test {};
 
-    FF2();
+    $expansion2[[FF2]]();
+
+    #define FF3() \
+      void waldo()
+
+    $fullDef[[FF3() {
+      int var = 42;
+    }]]
   )");
   TU.Code = Main.code().str();
   EXPECT_THAT(
       getSymbols(TU.build()),
       ElementsAre(
-          AllOf(WithName("abc_Test"), SymNameRange(Main.range("expansion"))),
-          AllOf(WithName("Test"), SymNameRange(Main.range("spelling")))));
+          AllOf(WithName("abc_Test"), SymNameRange(Main.range("expansion1"))),
+          AllOf(WithName("Test"), SymNameRange(Main.range("expansion2"))),
+          AllOf(WithName("waldo"), SymRange(Main.range("fullDef")))));
 }
 
 TEST(DocumentSymbols, FuncTemplates) {