[clangd] Use identifier range as the definition range.

[hokein]

Summary

This also matches the range in symbol index.

[malaperle]

I was going to change the symbol index to do the opposite :) The range of definitions including the bodies of functions, etc is used in a "peek definition" feature by several LSP clients. So for example in VSCode, you can hold Ctrl and hover on a function call and see its definition in a popup. There was some discussion about this in https://reviews.llvm.org/D35894

[sammccall]

clangd/SourceCode.cpp

55 ↗

(On Diff #137549)

As discussed offline, this heuristic is... limited.

#define X Y
class X{}

and now we say the definition of the class Y is on the first line.

I think we should really be basing the heuristics on the whole definition range, even if we're then going to return just the position of the name.

This patch just moves logic around so we don't need to do it now, but it could use a FIXME

clangd/SourceCode.h

34 ↗

(On Diff #137549)

This is a useful function, but it doesn't belong in SourceCode with its current scope. See the file header :)

We could add an "AST.h" or similar file, or expand the scope of SourceCode, but we haven't needed to yet because clang/AST normally has what we need.

Can this be a method on NamedDecl or a function alongside?
But since (later comment) the logic seems wrong, I'm not sure it makes sense to move it at the moment. Maybe create a separate header?

36 ↗

(On Diff #137549)

Not sure it's neccesary to spell out the details of what's happening inside macros, but it would be useful to describe the normal case :-)
e.g. This is usually the spelling location, where the name of the decl occurs in source code.

39 ↗

(On Diff #137549)

This name seems opaque to me.

findName?

39 ↗

(On Diff #137549)

would it be more useful to return the range, and just pick out the start if you're not interested in both?

clangd/XRefs.cpp

136 ↗

(On Diff #137549)

The name of this function is *really* confusing, so it took me a long time to understand the callsite.
Consider rename to makeLocaction

194 ↗

(On Diff #137549)

I think this comment relates to the previous line, not the next one.
But consider just removing it - if getDeclSourceLoc had a clearer name, it'd be self-documenting.

[sammccall]

In D44247#1031345, @malaperle wrote:

I was going to change the symbol index to do the opposite :) The range of definitions including the bodies of functions, etc is used in a "peek definition" feature by several LSP clients. So for example in VSCode, you can hold Ctrl and hover on a function call and see its definition in a popup. There was some discussion about this in https://reviews.llvm.org/D35894

Interesting - that seemed like a more natural interpretation of LSP to me.
Others talked me down, motivated (I think) by nicer behavior of jump-to-definition... will bring it up again :)

[sammccall]

In D44247#1031366, @sammccall wrote:

In D44247#1031345, @malaperle wrote:

I was going to change the symbol index to do the opposite :) The range of definitions including the bodies of functions, etc is used in a "peek definition" feature by several LSP clients. So for example in VSCode, you can hold Ctrl and hover on a function call and see its definition in a popup. There was some discussion about this in https://reviews.llvm.org/D35894

Interesting - that seemed like a more natural interpretation of LSP to me.
Others talked me down, motivated (I think) by nicer behavior of jump-to-definition... will bring it up again :)

Man, this seemed compelling to me, but there's a little bit of wiggle room in the spec (what's the "definition location"), so we looked at the MS language servers...
... and both their TS and C++ implementations return the range of the name only, despite that (IMO) being a weird interpretation of the spec.
As for VSCode:

• the "ctrl-to-hover" behavior starts at beginning of the line containing the range, and has a heuristic for when to stop (even if you return the whole definition range, I think). So whole-range is a bit better here (particularly when type/template is on a separate line) but actually still not great.
• "peek definition" shows the selected range in the middle of a block, with the range highlighted. Having the whole code highlighted actually looks kinda bad :/
• "go to definition" puts your cursor at the start of the range, and the identifier seems much better here.

So I *want* to agree, but we'll be fighting the other language servers and editors (and @ioeric, @ilya-biryukov who think we'd be breaking more important workflows)... I think we're actually better off just returning the name.

[malaperle]

In D44247#1031429, @sammccall wrote:

In D44247#1031366, @sammccall wrote:

In D44247#1031345, @malaperle wrote:

I was going to change the symbol index to do the opposite :) The range of definitions including the bodies of functions, etc is used in a "peek definition" feature by several LSP clients. So for example in VSCode, you can hold Ctrl and hover on a function call and see its definition in a popup. There was some discussion about this in https://reviews.llvm.org/D35894

Interesting - that seemed like a more natural interpretation of LSP to me.
Others talked me down, motivated (I think) by nicer behavior of jump-to-definition... will bring it up again :)

Man, this seemed compelling to me, but there's a little bit of wiggle room in the spec (what's the "definition location"), so we looked at the MS language servers...
... and both their TS and C++ implementations return the range of the name only, despite that (IMO) being a weird interpretation of the spec.
As for VSCode:

• the "ctrl-to-hover" behavior starts at beginning of the line containing the range, and has a heuristic for when to stop (even if you return the whole definition range, I think). So whole-range is a bit better here (particularly when type/template is on a separate line) but actually still not great.
• "peek definition" shows the selected range in the middle of a block, with the range highlighted. Having the whole code highlighted actually looks kinda bad :/
• "go to definition" puts your cursor at the start of the range, and the identifier seems much better here.

So I *want* to agree, but we'll be fighting the other language servers and editors (and @ioeric, @ilya-biryukov who think we'd be breaking more important workflows)... I think we're actually better off just returning the name.

Good points, thanks for investigating this. I agree that it should return the range of the name only so the direction of this patch looks good to me.

[hokein]

Address review comments.

[hokein]

Thanks for the comments!

clangd/SourceCode.cpp

55 ↗

(On Diff #137549)

Thanks for pointing it out! Added a FIXME.

clangd/SourceCode.h

34 ↗

(On Diff #137549)

Created a separate file.

39 ↗

(On Diff #137549)

Since this function only returns name location, a source location is enough IMO.

clang-tools-extra/trunk/clangd/AST.h

+//===--- AST.h - Utility AST functions  -------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// Various code that examines C++ source code using AST.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_AST_H_
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_AST_H_
+
+#include "clang/Basic/SourceLocation.h"
+
+namespace clang {
+class SourceManager;
+class Decl;
+
+namespace clangd {
+
+/// Find the identifier source location of the given D.
+///
+/// The returned location is usually the spelling location where the name of the
+/// decl occurs in the code.
+SourceLocation findNameLoc(const clang::Decl* D);
+
+} // namespace clangd
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_AST_H_

clang-tools-extra/trunk/clangd/AST.cpp

+//===--- AST.cpp - Utility AST functions  -----------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "AST.h"
+
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Decl.h"
+#include "clang/Basic/SourceManager.h"
+
+namespace clang {
+namespace clangd {
+using namespace llvm;
+
+SourceLocation findNameLoc(const clang::Decl* D) {
+  const auto& SM = D->getASTContext().getSourceManager();
+  // FIXME: Revisit the strategy, the heuristic is limitted when handling
+  // macros, we should use the location where the whole definition occurs.
+  SourceLocation SpellingLoc = SM.getSpellingLoc(D->getLocation());
+  if (D->getLocation().isMacroID()) {
+    std::string PrintLoc = SpellingLoc.printToString(SM);
+    if (llvm::StringRef(PrintLoc).startswith("<scratch") ||
+        llvm::StringRef(PrintLoc).startswith("<command line>")) {
+      // We use the expansion location for the following symbols, as spelling
+      // locations of these symbols are not interesting to us:
+      //   * symbols formed via macro concatenation, the spelling location will
+      //     be "<scratch space>"
+      //   * symbols controlled and defined by a compile command-line option
+      //     `-DName=foo`, the spelling location will be "<command line>".
+      SpellingLoc = SM.getExpansionRange(D->getLocation()).first;
+    }
+  }
+  return SpellingLoc;
+}
+
+} // namespace clangd
+} // namespace clang

clang-tools-extra/trunk/clangd/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Support
   )
 
 add_clang_library(clangDaemon
+  AST.cpp
   ClangdLSPServer.cpp
   ClangdServer.cpp
   ClangdUnit.cpp
   CodeComplete.cpp
   CodeCompletionStrings.cpp
   CompileArgsCache.cpp
   Compiler.cpp
   Context.cpp

clang-tools-extra/trunk/clangd/XRefs.cpp

 //===--- XRefs.cpp ----------------------------------------------*- C++-*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===---------------------------------------------------------------------===//
 #include "XRefs.h"
+#include "AST.h"
 #include "Logger.h"
 #include "SourceCode.h"
 #include "URI.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/Index/IndexDataConsumer.h"
 #include "clang/Index/IndexingAction.h"
 #include "llvm/Support/Path.h"
 namespace clang {
if (!Lexer::getRawToken(SearchedLocation, Result, Mgr, AST.getLangOpts(),
           MacroInfos.push_back(MacroDecl{IdentifierInfo->getName(), MacroInf});
         }
       }
     }
   }
 };
 
 llvm::Optional<Location>
-getDeclarationLocation(ParsedAST &AST, const SourceRange &ValSourceRange) {
+makeLocation(ParsedAST &AST, const SourceRange &ValSourceRange) {
   const SourceManager &SourceMgr = AST.getASTContext().getSourceManager();
   const LangOptions &LangOpts = AST.getASTContext().getLangOpts();
   SourceLocation LocStart = ValSourceRange.getBegin();
 
   const FileEntry *F =
       SourceMgr.getFileEntryForID(SourceMgr.getFileID(LocStart));
   if (!F)
     return llvm::None;
std::vector<Location> findDefinitions(ParsedAST &AST, Position Pos) {
   IndexOpts.IndexFunctionLocals = true;
 
   indexTopLevelDecls(AST.getASTContext(), AST.getTopLevelDecls(),
                      DeclMacrosFinder, IndexOpts);
 
   std::vector<const Decl *> Decls = DeclMacrosFinder->takeDecls();
   std::vector<MacroDecl> MacroInfos = DeclMacrosFinder->takeMacroInfos();
 
-  for (auto Item : Decls) {
-    auto L = getDeclarationLocation(AST, Item->getSourceRange());
+  for (auto D : Decls) {
+    auto Loc = findNameLoc(D);
+    auto L = makeLocation(AST, SourceRange(Loc, Loc));
     if (L)
       Result.push_back(*L);
   }
 
   for (auto Item : MacroInfos) {
     SourceRange SR(Item.Info->getDefinitionLoc(),
                    Item.Info->getDefinitionEndLoc());
-    auto L = getDeclarationLocation(AST, SR);
+    auto L = makeLocation(AST, SR);
     if (L)
       Result.push_back(*L);
   }
 
   return Result;
 }
 
 namespace {

clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp

 //===--- SymbolCollector.cpp -------------------------------------*- C++-*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "SymbolCollector.h"
+#include "../AST.h"
 #include "../CodeCompletionStrings.h"
 #include "../Logger.h"
 #include "../URI.h"
 #include "CanonicalIncludes.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Index/IndexSymbol.h"

 // Return the symbol location of the given declaration `D`.
 //
 // For symbols defined inside macros:
 //   * use expansion location, if the symbol is formed via macro concatenation.
 //   * use spelling location, otherwise.
 llvm::Optional<SymbolLocation> getSymbolLocation(
     const NamedDecl &D, SourceManager &SM, const SymbolCollector::Options &Opts,
     const clang::LangOptions &LangOpts, std::string &FileURIStorage) {
-  SourceLocation SpellingLoc = SM.getSpellingLoc(D.getLocation());
-  if (D.getLocation().isMacroID()) {
-    std::string PrintLoc = SpellingLoc.printToString(SM);
-    if (llvm::StringRef(PrintLoc).startswith("<scratch") ||
-        llvm::StringRef(PrintLoc).startswith("<command line>")) {
-      // We use the expansion location for the following symbols, as spelling
-      // locations of these symbols are not interesting to us:
-      //   * symbols formed via macro concatenation, the spelling location will
-      //     be "<scratch space>"
-      //   * symbols controlled and defined by a compile command-line option
-      //     `-DName=foo`, the spelling location will be "<command line>".
-      SpellingLoc = SM.getExpansionRange(D.getLocation()).first;
-    }
-  }
-
-  auto U = toURI(SM, SM.getFilename(SpellingLoc), Opts);
+  SourceLocation NameLoc = findNameLoc(&D);
+  auto U = toURI(SM, SM.getFilename(NameLoc), Opts);
   if (!U)
     return llvm::None;
   FileURIStorage = std::move(*U);
   SymbolLocation Result;
   Result.FileURI = FileURIStorage;
-  Result.StartOffset = SM.getFileOffset(SpellingLoc);
+  Result.StartOffset = SM.getFileOffset(NameLoc);
   Result.EndOffset = Result.StartOffset + clang::Lexer::MeasureTokenLength(
-                                              SpellingLoc, SM, LangOpts);
+                                              NameLoc, SM, LangOpts);
   return std::move(Result);
 }
 
 // Checks whether \p ND is a definition of a TagDecl (class/struct/enum/union)
 // in a header file, in which case clangd would prefer to use ND as a canonical
 // declaration.
 // FIXME: handle symbol types that are not TagDecl (e.g. functions), if using
 // the the first seen declaration as canonical declaration is not a good enough

clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp

)cpp";
   runAddDocument(Server, FooCpp, SourceContents);
   EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags());
 }
 
 // Test ClangdServer.reparseOpenedFiles.
 TEST_F(ClangdVFSTest, ReparseOpenedFiles) {
   Annotations FooSource(R"cpp(
 #ifdef MACRO
-$one[[static void bob() {}]]
+static void $one[[bob]]() {}
 #else
-$two[[static void bob() {}]]
+static void $two[[bob]]() {}
 #endif
 
 int main () { bo^b (); return 0; }
 )cpp");
 
   Annotations BarSource(R"cpp(
 #ifdef MACRO
 this is an error

clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp

 }
 
 MATCHER_P(RangeIs, R, "") { return arg.range == R; }
 
 TEST(GoToDefinition, All) {
   const char *Tests[] = {
       R"cpp(// Local variable
         int main() {
-          [[int bonjour]];
+          int [[bonjour]];
           ^bonjour = 2;
           int test1 = bonjour;
         }
       )cpp",
 
       R"cpp(// Struct
         namespace ns1 {
-        [[struct MyClass {}]];
+        struct [[MyClass]] {};
         } // namespace ns1
         int main() {
           ns1::My^Class* Params;
         }
       )cpp",
 
       R"cpp(// Function definition via pointer
-        [[int foo(int) {}]]
+        int [[foo]](int) {}
         int main() {
           auto *X = &^foo;
         }
       )cpp",
 
       R"cpp(// Function declaration via call
-        [[int foo(int)]];
+        int [[foo]](int);
         int main() {
           return ^foo(42);
         }
       )cpp",
 
       R"cpp(// Field
-        struct Foo { [[int x]]; };
+        struct Foo { int [[x]]; };
         int main() {
           Foo bar;
           bar.^x;
         }
       )cpp",
 
       R"cpp(// Field, member initializer
         struct Foo {
-          [[int x]];
+          int [[x]];
           Foo() : ^x(0) {}
         };
       )cpp",
 
       R"cpp(// Field, GNU old-style field designator
-        struct Foo { [[int x]]; };
+        struct Foo { int [[x]]; };
         int main() {
           Foo bar = { ^x : 1 };
         }
       )cpp",
 
       R"cpp(// Field, field designator
-        struct Foo { [[int x]]; };
+        struct Foo { int [[x]]; };
         int main() {
           Foo bar = { .^x = 2 };
         }
       )cpp",
 
       R"cpp(// Method call
-        struct Foo { [[int x()]]; };
+        struct Foo { int [[x]](); };
         int main() {
           Foo bar;
           bar.^x();
         }
       )cpp",
 
       R"cpp(// Typedef
-        [[typedef int Foo]];
+        typedef int [[Foo]];
         int main() {
           ^Foo bar;
         }
       )cpp",
 
       /* FIXME: clangIndex doesn't handle template type parameters
       R"cpp(// Template type parameter
         template <[[typename T]]>
         void foo() { ^T t; }
       )cpp", */
 
       R"cpp(// Namespace
-        [[namespace ns {
+        namespace [[ns]] {
         struct Foo { static void bar(); }
-        }]] // namespace ns
+        } // namespace ns
         int main() { ^ns::Foo::bar(); }
       )cpp",
 
       R"cpp(// Macro
         #define MACRO 0
         #define [[MACRO 1]]
         int main() { return ^MACRO; }
         #define MACRO 2
         #undef macro
       )cpp",
 
       R"cpp(// Forward class declaration
         class Foo;
-        [[class Foo {}]];
+        class [[Foo]] {};
         F^oo* foo();
       )cpp",
 
       R"cpp(// Function declaration
         void foo();
         void g() { f^oo(); }
-        [[void foo() {}]]
+        void [[foo]]() {}
+      )cpp",
+
+      R"cpp(
+        #define FF(name) class name##_Test {};
+        [[FF]](my);
+        void f() { my^_Test a; }
+      )cpp",
+
+      R"cpp(
+         #define FF() class [[Test]] {};
+         FF();
+         void f() { T^est a; }
       )cpp",
   };
   for (const char *Test : Tests) {
     Annotations T(Test);
     auto AST = build(T.code());
     EXPECT_THAT(findDefinitions(AST, T.point()),
                 ElementsAre(RangeIs(T.range())))
         << Test;
   }
 }
 
 TEST(GoToDefinition, RelPathsInCompileCommand) {
   Annotations SourceAnnotations(R"cpp(
-[[int foo]];
+int [[foo]];
 int baz = f^oo;
 )cpp");
 
   IgnoreDiagnostics DiagConsumer;
   MockCompilationDatabase CDB(/*UseRelPaths=*/true);
   MockFSProvider FS;
   ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());