[clangd] Document highlights for clangd

[Nebiroth]

Summary

Implementation of Document Highlights Request as described in LSP.

[ioeric]

Drive-by comment: in general, have you considered reusing the existing declarations and occurrences finding functionalities in clang-rename? AFAIK, it deals with templates and macros pretty well.

o https://github.com/llvm-mirror/clang/blob/master/include/clang/Tooling/Refactoring/Rename/USRFinder.h
o https://github.com/llvm-mirror/clang/blob/master/include/clang/Tooling/Refactoring/Rename/USRLocFinder.h

[Nebiroth]

This updated patch still does not handle highlighting macro references correctly. I will make another patch at a later time for this issue.

In D38425#922408, @ioeric wrote:

Drive-by comment: in general, have you considered reusing the existing declarations and occurrences finding functionalities in clang-rename? AFAIK, it deals with templates and macros pretty well.

o https://github.com/llvm-mirror/clang/blob/master/include/clang/Tooling/Refactoring/Rename/USRFinder.h
o https://github.com/llvm-mirror/clang/blob/master/include/clang/Tooling/Refactoring/Rename/USRLocFinder.h

I'll take a look at it later. Thanks!

[malaperle]

Hi William, can you quickly go over the diff before I do a review? There are a few more-obvious things that can be addressed, i.e. commented lines, extra new/deleted lines, temporary code, lower case variable names, etc.

[Nebiroth]

Removed some commented lines and temporary code
Streamlined and removed some code that overlaps/conflicts with code hover patch so it's easier to merge (patch D35894)

[malaperle]

clangd/ClangdLSPServer.cpp

67 ↗

(On Diff #123848)

extra line

242 ↗

(On Diff #123848)

Items -> Highlights?

clangd/ClangdServer.h

291 ↗

(On Diff #123848)

can this thing fail? Should it be Expected<Tagged... ??

clangd/ClangdUnit.cpp

942 ↗

(On Diff #123848)

I think this class should be exactly the same as the code hover patch, so changes from there and the comments on that patch should apply here too.

944 ↗

(On Diff #123848)

just Decls?

945 ↗

(On Diff #123848)

just MacroInfos?

946 ↗

(On Diff #123848)

remove, see comment below.

961 ↗

(On Diff #123848)

Last

969 ↗

(On Diff #123848)

This comment needs to be updated (like the code hover patch)

971 ↗

(On Diff #123848)

Last

992 ↗

(On Diff #123848)

extra line

996 ↗

(On Diff #123848)

I think you need to do this in DocumentHighlightsFinder not here. Each occurrence can have its kind, not just the user selected location.

1013 ↗

(On Diff #123848)

I don't think you need any of those getters. Only use the "takes"

1079 ↗

(On Diff #123848)

It's not "a given FileID and file", the finder is given a list of Decls

1122 ↗

(On Diff #123848)

This is duplicated code with getDeclarationLocation. It also doesn't use tryGetRealPathName
I also don't think this is called??

1165 ↗

(On Diff #123848)

I don't think this is called?

1191 ↗

(On Diff #123848)

This doesn't add or have a location in its signature so I think it should be named differently.
DocumentHighligh getDocumentHighlight(Decl*) ? Or addDocumentHighlight(Decl*) if you move it in DocumentHighlightsFinder.

1232 ↗

(On Diff #123848)

call takeDecls. TempDecls -> SelectedDecls?

1233 ↗

(On Diff #123848)

remove? see comment below

1243 ↗

(On Diff #123848)

DocumentHighlights. They are not Locations (so they are not confused with the struct in Protocol)

1245 ↗

(On Diff #123848)

replace all this code (1242-1265) by moving it in DocumentHighlightsFinder? then call takeHighlights.

1251 ↗

(On Diff #123848)

I think we should remove macro handling for now, it doesn't highlight occurrences anyway.

1265 ↗

(On Diff #123848)

remove std::move, returning such local object will use copy elision

clangd/ClangdUnit.h

318 ↗

(On Diff #123848)

remove?

clangd/Protocol.h

637 ↗

(On Diff #123848)

remove

639 ↗

(On Diff #123848)

needed?

clangd/ProtocolHandlers.cpp

49 ↗

(On Diff #123848)

revert change

clangd/ProtocolHandlers.h

34 ↗

(On Diff #123848)

revert change

[malaperle]

clangd/ClangdUnit.cpp

1245 ↗

(On Diff #123848)

Also, use a range-based for loop

1247 ↗

(On Diff #123848)

The vector returned by getKinds() doesn't match the size of getSourceRanges(). This is because the kinds vector is computed in DeclarationLocationsFinder which only computes the kind of the occurrence the user has selected, not the kinds of all occurrences for a given Decl. Once you move the code to create the highlights in DocumentHighlightsFinder, you won't need a vector of kinds and this won't be a problem.

[malaperle]

clangd/ClangdUnit.cpp

997 ↗

(On Diff #123848)

With this code, I always get "text" kind. It's because index::SymbolRoleSet is a bitfield so you have to check the write, read bits. Something like:

DocumentHighlightKind Kind = DocumentHighlightKind::Text;
if (static_cast<index::SymbolRoleSet>(index::SymbolRole::Write) & Roles) {
  Kind = DocumentHighlightKind::Write;
} else if (static_cast<index::SymbolRoleSet>(index::SymbolRole::Read) & Roles) {
  Kind = DocumentHighlightKind::Read;
}

[malaperle]

In D38425#922408, @ioeric wrote:

Drive-by comment: in general, have you considered reusing the existing declarations and occurrences finding functionalities in clang-rename? AFAIK, it deals with templates and macros pretty well.

o https://github.com/llvm-mirror/clang/blob/master/include/clang/Tooling/Refactoring/Rename/USRFinder.h
o https://github.com/llvm-mirror/clang/blob/master/include/clang/Tooling/Refactoring/Rename/USRLocFinder.h

This looks interesting. It does solve the problem that currently we traverse the whole tree. However, I don't think it has support for macros (they are not covered by USRs I believe) and it looks like it only can return one NamedDecl* at a location, and it's possible to have multiple, non-named Decls. Perhaps we should look at improving this!

[Nebiroth]

clangd/ClangdServer.h

291 ↗

(On Diff #123848)

Oops, yeah this should be a llvm::Expected

[Nebiroth]

Getting DocumentHighlightKind is now done in DocumentHighlightsFinder
Removed duplicated and unused code
Refactored method and variable names

[malaperle]

clangd/ClangdLSPServer.cpp

242 ↗

(On Diff #123848)

Items -> Highlights?

clangd/ClangdServer.h

291 ↗

(On Diff #123848)

can this thing fail? Should it be Expected<Tagged... ??

clangd/ClangdUnit.cpp

956 ↗

(On Diff #124224)

std::move

965 ↗

(On Diff #124224)

std::move

1017 ↗

(On Diff #124224)

remove

1018 ↗

(On Diff #124224)

remove

1028 ↗

(On Diff #124224)

remove

1037 ↗

(On Diff #124224)

remove

1039 ↗

(On Diff #124224)

remove

1079 ↗

(On Diff #124224)

either this should be named getDocumentHighlight or it should do the push back and return void

[Nebiroth]

Fixed a few outstanding issues that were reported as completed

[malaperle]

test/clangd/documenthighlight.test

1 ↗

(On Diff #124230)

we should test that we can get kind == 1, 2, 3

16 ↗

(On Diff #124230)

kind 0 is invalid

23 ↗

(On Diff #124230)

kind 216? 220?

29 ↗

(On Diff #124230)

kind 0 is invalid

[Nebiroth]

Fixed test checking for values from an incorrect bit shift

[malaperle]

There were some things I missed, sorry about that!

clangd/main.cpp

1 ↗

(On Diff #124238)

This files needs to be removed

test/clangd/documenthighlight.test

10 ↗

(On Diff #124238)

I think we should cover "kind":2 as well. It's probably not a big change.

15 ↗

(On Diff #124238)

This comment should be updated, it was copy/pasted probably?

22 ↗

(On Diff #124238)

This comment should be updated, it was copy/pasted probably?

29 ↗

(On Diff #124238)

This comment should be updated, it was copy/pasted probably?

[Nebiroth]

Removed temporary test file
Updated test to account for read-access symbol verification

[malaperle]

I tested the patch and it works quite well! I think those are the last comments from me. Sorry, I should have bundled them together a bit more :(

clangd/ClangdUnit.cpp

1019 ↗

(On Diff #124391)

Not used.

[malaperle]

clangd/ClangdUnit.cpp

1029 ↗

(On Diff #124391)

Last

1069 ↗

(On Diff #124391)

Don't call getLocForEndOfToken here again. This is the reason why sometimes it includes an extra character. Just to explain a bit, supposed we have
int Foo;

the first call to getLocForEnd in handleDeclOccurence will compute the position at
int Foo|;
Then if you call is again, it will go at the end of the ';' token so it will now be
int Foo;|

[Nebiroth]

Minor code cleanup
Fixed highlights sometimes obtaining one too many characters inside range
Updated test

[malaperle]

clangd/ClangdLSPServer.cpp

246 ↗

(On Diff #124391)

I get a test failure here because there is an assertion that the Expected<> needs to be checked. I can't really think of any failure case right now where we wouldn't just return an empty array of highlights. But I think it's better for consistency and future-proofing to keep the Expected<>.
I think you can just do like in onRename for now

if (!Highlights) {
  C.replyError(ErrorCode::InternalError,
               llvm::toString(Highlights.takeError()));
  return;
}

[malaperle]

clangd/ClangdLSPServer.cpp

246 ↗

(On Diff #124391)

Just to be clear, the error I see in the output is:
Expected<T> must be checked before access or destruction.
Expected<T> value was in success state. (Note: Expected<T> values in success mode must still be checked prior to being destroyed).

It asserts when in ClangdLSPServer::onDocumentHighlight, referencing Highlights->Value

[Nebiroth]

Added verification for llvm::Expected in onDocumentHighlight
Removed unused variable in ClangdUnit

[Nebiroth]

Fixed wrong content header making the test fail

[ilya-biryukov]

clangd/ClangdLSPServer.cpp

67 ↗

(On Diff #123848)

Still there

clangd/ClangdServer.cpp

513 ↗

(On Diff #124834)

Other functions don't crash in this case anymore, we should follow the same pattern here (see findDefinitions for an example)

524 ↗

(On Diff #124834)

We should return an error this case.

clangd/ClangdUnit.cpp

1062 ↗

(On Diff #124834)

This function should be private.

1096 ↗

(On Diff #124834)

We should not return default-initialized Locations from this function.
Return llvm::Optional<Location> and handle the case when getFileEntryForID returns null by returning llvm::None.

1165 ↗

(On Diff #124834)

Let's add a comment that we don't currently handle macros. It's ok for the first iteration, having documentHighlights for Decls is useful enough to get it in.

clangd/Protocol.h

621 ↗

(On Diff #124834)

Please also compare kind here, to make this operator consistent with operator==.
std::tie(LHS.range, LHS.kind) < std::tie(RHS.range, RHS.kind) should work. If it doesn't, converting enums to int should help: std::tie(LHS.range, int(LHS.kind)) < std::tie(RHS.range, int(RHS.kind))

test/clangd/initialize-params-invalid.test

23 ↗

(On Diff #124834)

NIT: Misindent

test/clangd/initialize-params.test

23 ↗

(On Diff #124834)

NIT: Misindent

[Nebiroth]

Minor code cleanup
getDeclarationLocation now returns llvm::Optional
operator< for DocumentHighlight struct now properly compares the kind field

[malaperle]

very minor comments

clangd/ClangdServer.h

288 ↗

(On Diff #124951)

"for a symbol hovered on."

It doesn't have to be a symbol and the user doesn't have to hover on it. So maybe just "for a given position"

clangd/ClangdUnit.cpp

1088 ↗

(On Diff #124951)

const FileEntry *F = SourceMgr.getFileEntryForID(SourceMgr.getFileID(LocStart));
if (!F)
  return llvm::None;

1098 ↗

(On Diff #124951)

maybe move the check earlier? see comment above.

clangd/Protocol.h

601 ↗

(On Diff #124951)

Use /// like other structs

[malaperle]

clangd/ClangdServer.cpp

526 ↗

(On Diff #124951)

missing return?
I get a warning that looks legit:
./tools/clang/tools/extra/clangd/ClangdServer.cpp:526:7: warning: ignoring return value of function declared with 'warn_unused_result' attribute [-Wunused-result]

llvm::make_error<llvm::StringError>(

[Nebiroth]

Minor code cleanup
unparse and parse methods for JSON are updated

[malaperle]

clangd/ClangdServer.cpp

528 ↗

(On Diff #125228)

Invalid

530 ↗

(On Diff #125228)

It trips here with assertions enabled because we haven't checked the previous "Value" for error before, sigh.
Expected<T> must be checked before access or destruction.
Expected<T> value was in success state. (Note: Expected<T> values in success mode must still be checked prior to being destroyed).

How about this:

std::vector<DocumentHighlight> Result;
llvm::Optional<llvm::Error> Err;
Resources->getAST().get()->runUnderLock([Pos, &Result, &Err,
                                         this](ParsedAST *AST) {
  if (!AST)
    Err = llvm::make_error<llvm::StringError>("Invalid AST",
                                              llvm::errc::invalid_argument);
  Result = clangd::findDocumentHighlights(*AST, Pos, Logger);
});

if (Err)
  return std::move(*Err);

return make_tagged(Result, TaggedFS.Tag);

The tests pass for me with this.

clangd/Protocol.cpp

372 ↗

(On Diff #125228)

Too many new lines. Only keep one.

clangd/Protocol.h

568 ↗

(On Diff #125228)

Use /// like other places

575 ↗

(On Diff #125228)

Use /// like other places

[Nebiroth]

Minor code cleanup
Refactored findDocumentHighlights() to make tests pass when assertions are enabled

[malaperle]

Can you also reformat the code too with clang-format? I think there's a few things in ClangdUnit.h/.cpp

clangd/ClangdServer.cpp

521 ↗

(On Diff #125346)

We should do like findDefinitions here:

if (!Resources)
  return llvm::make_error<llvm::StringError>(
      "findDocumentHighlights called on non-added file",
      llvm::errc::invalid_argument);

528 ↗

(On Diff #125346)

Woops, I forgot the return here if it's an error, so it should be:

if (!AST) {
  Err = llvm::make_error<llvm::StringError>("Invalid AST",
                                            llvm::errc::invalid_argument);
  return;
}

[Nebiroth]

Added error returns in findDocumentHighlights
Ran clang-format on ClangdUnit.h, .cpp and ClangdServer.cpp

[malaperle]

Sorry I forgot to submit this additional comment in my last review pass :(

clangd/Protocol.cpp

365 ↗

(On Diff #125346)

static_cast<int>(DH.kind)

[Nebiroth]

Added static_cast when unparsing

[malaperle]

This looks good to me. @ilya-biryukov, @sammccall Any objections?
I think we can do further iterations later to handle macros and other specific cases (multiple Decls at the position, etc) . It's already very useful functionality.

[ilya-biryukov]

Overall looks good, just a few minor comments.
Let me know if need someone to land this patch for you after you fix those.

clangd/ClangdUnit.cpp

249 ↗

(On Diff #125619)

Mentioning AST in this comment is weird, macros have nothing to do with the AST. We should remove/rephrase the comment.
I'm not sure if multiple occurences of MacroInfo are even possible here, but we could leave the code as is anyway.

clangd/Protocol.h

562 ↗

(On Diff #125619)

NIT: remove this empty comment line and all the others.

581 ↗

(On Diff #125619)

This comparison does not provide a total order.
Please use std::tie(LHS.range, int(LHS.kind)) < std::tie(RHS.range, int(RHS.kind)) instead.

[Nebiroth]

Merged with latest llvm/clang
Minor code cleanup

[Nebiroth]

@ilya-biryukov Need someone to land this.

[ilya-biryukov]

LGTM module a slight tweak to fix compilation. I'll land this today.

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

private:
   onDocumentRangeFormatting(Ctx C,
                             DocumentRangeFormattingParams &Params) override;
   void onDocumentFormatting(Ctx C, DocumentFormattingParams &Params) override;
   void onCodeAction(Ctx C, CodeActionParams &Params) override;
   void onCompletion(Ctx C, TextDocumentPositionParams &Params) override;
   void onSignatureHelp(Ctx C, TextDocumentPositionParams &Params) override;
   void onGoToDefinition(Ctx C, TextDocumentPositionParams &Params) override;
   void onSwitchSourceHeader(Ctx C, TextDocumentIdentifier &Params) override;
+  void onDocumentHighlight(Ctx C, TextDocumentPositionParams &Params) override;
   void onFileEvent(Ctx C, DidChangeWatchedFilesParams &Params) override;
   void onCommand(Ctx C, ExecuteCommandParams &Params) override;
   void onRename(Ctx C, RenameParams &Parames) override;
 
   std::vector<clang::tooling::Replacement>
   getFixIts(StringRef File, const clangd::Diagnostic &D);
 
   JSONOutput &Out;

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

C.reply(json::obj{
                  {"resolveProvider", false},
                  {"triggerCharacters", {".", ">", ":"}},
              }},
             {"signatureHelpProvider",
              json::obj{
                  {"triggerCharacters", {"(", ","}},
              }},
             {"definitionProvider", true},
+            {"documentHighlightProvider", true},
             {"renameProvider", true},
             {"executeCommandProvider",
              json::obj{
                  {"commands", {ExecuteCommandParams::CLANGD_APPLY_FIX_COMMAND}},
              }},
         }}}});
   if (Params.rootUri && !Params.rootUri->file.empty())
     Server.setRootPath(Params.rootUri->file);

 
 void ClangdLSPServer::onSwitchSourceHeader(Ctx C,
                                            TextDocumentIdentifier &Params) {
   llvm::Optional<Path> Result = Server.switchSourceHeader(Params.uri.file);
   std::string ResultUri;
   C.reply(Result ? URI::fromFile(*Result).uri : "");
 }
 
+void ClangdLSPServer::onDocumentHighlight(Ctx C,
+                                          TextDocumentPositionParams &Params) {
+
+  auto Highlights = Server.findDocumentHighlights(
+      Params.textDocument.uri.file,
+      Position{Params.position.line, Params.position.character});
+
+  if (!Highlights) {
+    C.replyError(ErrorCode::InternalError,
+                 llvm::toString(Highlights.takeError()));
+    return;
+  }
+
+  C.reply(json::ary(Highlights->Value));
+}
+
 ClangdLSPServer::ClangdLSPServer(JSONOutput &Out, unsigned AsyncThreadsCount,
                                  bool StorePreamblesInMemory,
                                  const clangd::CodeCompleteOptions &CCOpts,
                                  llvm::Optional<StringRef> ResourceDir,
                                  llvm::Optional<Path> CompileCommandsDir)
     : Out(Out), CDB(/*Logger=*/Out, std::move(CompileCommandsDir)),
       CCOpts(CCOpts), Server(CDB, /*DiagConsumer=*/*this, FSProvider,
                              AsyncThreadsCount, StorePreamblesInMemory,

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

public:
   ///
   /// \p StorePreamblesInMemory defines whether the Preambles generated by
   /// clangd are stored in-memory or on disk.
   ///
   /// Various messages are logged using \p Logger.
   ClangdServer(GlobalCompilationDatabase &CDB,
                DiagnosticsConsumer &DiagConsumer,
                FileSystemProvider &FSProvider, unsigned AsyncThreadsCount,
-               bool StorePreamblesInMemory,
-               clangd::Logger &Logger,
+               bool StorePreamblesInMemory, clangd::Logger &Logger,
                llvm::Optional<StringRef> ResourceDir = llvm::None);
 
   /// Set the root path of the workspace.
   void setRootPath(PathRef RootPath);
 
   /// Add a \p File to the list of tracked C++ files or update the contents if
   /// \p File is already tracked. Also schedules parsing of the AST for it on a
   /// separate thread. When the parsing is complete, DiagConsumer passed in
public:
   /// Get definition of symbol at a specified \p Line and \p Column in \p File.
   llvm::Expected<Tagged<std::vector<Location>>> findDefinitions(PathRef File,
                                                                 Position Pos);
 
   /// Helper function that returns a path to the corresponding source file when
   /// given a header file and vice versa.
   llvm::Optional<Path> switchSourceHeader(PathRef Path);
 
+  /// Get document highlights for a given position.
+  llvm::Expected<Tagged<std::vector<DocumentHighlight>>>
+  findDocumentHighlights(PathRef File, Position Pos);
+
   /// Run formatting for \p Rng inside \p File.
   std::vector<tooling::Replacement> formatRange(PathRef File, Range Rng);
   /// Run formatting for the whole \p File.
   std::vector<tooling::Replacement> formatFile(PathRef File);
   /// Run formatting after a character was typed at \p Pos in \p File.
   std::vector<tooling::Replacement> formatOnType(PathRef File, Position Pos);
   /// Rename all occurrences of the symbol at the \p Pos in \p File to
   /// \p NewName.

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

for (StringRef NewExt : NewExts) {
     llvm::sys::path::replace_extension(NewPath, NewExt.upper());
     if (FS->exists(NewPath))
       return NewPath.str().str();
   }
 
   return llvm::None;
 }
 
+llvm::Expected<Tagged<std::vector<DocumentHighlight>>>
+ClangdServer::findDocumentHighlights(PathRef File, Position Pos) {
+  auto FileContents = DraftMgr.getDraft(File);
+  if (!FileContents.Draft)
+    return llvm::make_error<llvm::StringError>(
+        "findDocumentHighlights called on non-added file",
+        llvm::errc::invalid_argument);
+
+  auto TaggedFS = FSProvider.getTaggedFileSystem(File);
+
+  std::shared_ptr<CppFile> Resources = Units.getFile(File);
+  if (!Resources)
+    return llvm::make_error<llvm::StringError>(
+        "findDocumentHighlights called on non-added file",
+        llvm::errc::invalid_argument);
+
+  std::vector<DocumentHighlight> Result;
+  llvm::Optional<llvm::Error> Err;
+  Resources->getAST().get()->runUnderLock([Pos, &Result, &Err,
+                                           this](ParsedAST *AST) {
+    if (!AST) {
+      Err = llvm::make_error<llvm::StringError>("Invalid AST",
+                                                llvm::errc::invalid_argument);
+      return;
+    }
+    Result = clangd::findDocumentHighlights(*AST, Pos, Logger);
+  });
+
+  if (Err)
+    return std::move(*Err);
+  return make_tagged(Result, TaggedFS.Tag);
+}
+
 std::future<void> ClangdServer::scheduleReparseAndDiags(
     PathRef File, VersionedDraft Contents, std::shared_ptr<CppFile> Resources,
     Tagged<IntrusiveRefCntPtr<vfs::FileSystem>> TaggedFS) {
 
   assert(Contents.Draft && "Draft must have contents");
   UniqueFunction<llvm::Optional<std::vector<DiagWithFixIts>>()>
       DeferredRebuild =
           Resources->deferRebuild(*Contents.Draft, TaggedFS.Value);

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

private:
   /// waiting for rebuild to finish.
   std::shared_ptr<const PreambleData> LatestAvailablePreamble;
   /// Utility class, required by clang.
   std::shared_ptr<PCHContainerOperations> PCHs;
   /// Used for logging various messages.
   clangd::Logger &Logger;
 };
 
-
 /// Get the beginning SourceLocation at a specified \p Pos.
 SourceLocation getBeginningOfIdentifier(ParsedAST &Unit, const Position &Pos,
                                         const FileEntry *FE);
 
 /// Get definition of symbol at a specified \p Pos.
 std::vector<Location> findDefinitions(ParsedAST &AST, Position Pos,
                                       clangd::Logger &Logger);
 
+std::vector<DocumentHighlight>
+findDocumentHighlights(ParsedAST &AST, Position Pos, clangd::Logger &Logger);
+
 /// For testing/debugging purposes. Note that this method deserializes all
 /// unserialized Decls, so use with care.
 void dumpAST(ParsedAST &AST, llvm::raw_ostream &OS);
 
 } // namespace clangd
 } // namespace clang
 #endif

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

 SourceLocation getMacroArgExpandedLocation(const SourceManager &Mgr,
                                            const FileEntry *FE, Position Pos) {
   SourceLocation InputLoc =
       Mgr.translateFileLineCol(FE, Pos.line + 1, Pos.character + 1);
   return Mgr.getMacroArgExpandedLocation(InputLoc);
 }
 
 /// Finds declarations locations that a given source location refers to.
-class DeclarationLocationsFinder : public index::IndexDataConsumer {
-  std::vector<Location> DeclarationLocations;
+class DeclarationAndMacrosFinder : public index::IndexDataConsumer {
+  std::vector<const Decl *> Decls;
+  std::vector<const MacroInfo *> MacroInfos;
   const SourceLocation &SearchedLocation;
   const ASTContext &AST;
   Preprocessor &PP;
 
 public:
-  DeclarationLocationsFinder(raw_ostream &OS,
+  DeclarationAndMacrosFinder(raw_ostream &OS,
                              const SourceLocation &SearchedLocation,
                              ASTContext &AST, Preprocessor &PP)
       : SearchedLocation(SearchedLocation), AST(AST), PP(PP) {}
 
-  std::vector<Location> takeLocations() {
-    // Don't keep the same location multiple times.
+  std::vector<const Decl *> takeDecls() {
+    // Don't keep the same declaration multiple times.
     // This can happen when nodes in the AST are visited twice.
-    std::sort(DeclarationLocations.begin(), DeclarationLocations.end());
-    auto last =
-        std::unique(DeclarationLocations.begin(), DeclarationLocations.end());
-    DeclarationLocations.erase(last, DeclarationLocations.end());
-    return std::move(DeclarationLocations);
+    std::sort(Decls.begin(), Decls.end());
+    auto Last = std::unique(Decls.begin(), Decls.end());
+    Decls.erase(Last, Decls.end());
+    return std::move(Decls);
+  }
+
+  std::vector<const MacroInfo *> takeMacroInfos() {
+    // Don't keep the same Macro info multiple times.
+    std::sort(MacroInfos.begin(), MacroInfos.end());
+    auto Last = std::unique(MacroInfos.begin(), MacroInfos.end());
+    MacroInfos.erase(Last, MacroInfos.end());
+    return std::move(MacroInfos);
   }
 
   bool
   handleDeclOccurence(const Decl *D, index::SymbolRoleSet Roles,
                       ArrayRef<index::SymbolRelation> Relations, FileID FID,
                       unsigned Offset,
                       index::IndexDataConsumer::ASTNodeInfo ASTNode) override {
-    if (isSearchedLocation(FID, Offset)) {
-      addDeclarationLocation(D->getSourceRange());
-    }
+    if (isSearchedLocation(FID, Offset))
+      Decls.push_back(D);
     return true;
   }
 
 private:
   bool isSearchedLocation(FileID FID, unsigned Offset) const {
     const SourceManager &SourceMgr = AST.getSourceManager();
     return SourceMgr.getFileOffset(SearchedLocation) == Offset &&
            SourceMgr.getFileID(SearchedLocation) == FID;
   }
 
-  void addDeclarationLocation(const SourceRange &ValSourceRange) {
-    const SourceManager &SourceMgr = AST.getSourceManager();
-    const LangOptions &LangOpts = AST.getLangOpts();
-    SourceLocation LocStart = ValSourceRange.getBegin();
-    SourceLocation LocEnd = Lexer::getLocForEndOfToken(ValSourceRange.getEnd(),
-                                                       0, SourceMgr, LangOpts);
-    Position Begin;
-    Begin.line = SourceMgr.getSpellingLineNumber(LocStart) - 1;
-    Begin.character = SourceMgr.getSpellingColumnNumber(LocStart) - 1;
-    Position End;
-    End.line = SourceMgr.getSpellingLineNumber(LocEnd) - 1;
-    End.character = SourceMgr.getSpellingColumnNumber(LocEnd) - 1;
-    Range R = {Begin, End};
-    Location L;
-    if (const FileEntry *F =
-            SourceMgr.getFileEntryForID(SourceMgr.getFileID(LocStart))) {
-      StringRef FilePath = F->tryGetRealPathName();
-      if (FilePath.empty())
-        FilePath = F->getName();
-      L.uri = URI::fromFile(FilePath);
-      L.range = R;
-      DeclarationLocations.push_back(L);
-    }
-  }
-
   void finish() override {
     // Also handle possible macro at the searched location.
     Token Result;
     if (!Lexer::getRawToken(SearchedLocation, Result, AST.getSourceManager(),
                             AST.getLangOpts(), false)) {
       if (Result.is(tok::raw_identifier)) {
         PP.LookUpIdentifierInfo(Result);
       }
       IdentifierInfo *IdentifierInfo = Result.getIdentifierInfo();
       if (IdentifierInfo && IdentifierInfo->hadMacroDefinition()) {
         std::pair<FileID, unsigned int> DecLoc =
             AST.getSourceManager().getDecomposedExpansionLoc(SearchedLocation);
         // Get the definition just before the searched location so that a macro
         // referenced in a '#undef MACRO' can still be found.
         SourceLocation BeforeSearchedLocation = getMacroArgExpandedLocation(
             AST.getSourceManager(),
             AST.getSourceManager().getFileEntryForID(DecLoc.first),
             DecLoc.second - 1);
         MacroDefinition MacroDef =
             PP.getMacroDefinitionAtLoc(IdentifierInfo, BeforeSearchedLocation);
         MacroInfo *MacroInf = MacroDef.getMacroInfo();
         if (MacroInf) {
-          addDeclarationLocation(SourceRange(MacroInf->getDefinitionLoc(),
-                                             MacroInf->getDefinitionEndLoc()));
+          MacroInfos.push_back(MacroInf);
         }
       }
     }
   }
 };
 
+/// Finds document highlights that a given list of declarations refers to.
+class DocumentHighlightsFinder : public index::IndexDataConsumer {
+  std::vector<const Decl *> &Decls;
+  std::vector<DocumentHighlight> DocumentHighlights;
+  const ASTContext &AST;
+
+public:
+  DocumentHighlightsFinder(raw_ostream &OS, ASTContext &AST, Preprocessor &PP,
+                           std::vector<const Decl *> &Decls)
+      : Decls(Decls), AST(AST) {}
+  std::vector<DocumentHighlight> takeHighlights() {
+    // Don't keep the same highlight multiple times.
+    // This can happen when nodes in the AST are visited twice.
+    std::sort(DocumentHighlights.begin(), DocumentHighlights.end());
+    auto Last =
+        std::unique(DocumentHighlights.begin(), DocumentHighlights.end());
+    DocumentHighlights.erase(Last, DocumentHighlights.end());
+    return std::move(DocumentHighlights);
+  }
+
+  bool
+  handleDeclOccurence(const Decl *D, index::SymbolRoleSet Roles,
+                      ArrayRef<index::SymbolRelation> Relations, FileID FID,
+                      unsigned Offset,
+                      index::IndexDataConsumer::ASTNodeInfo ASTNode) override {
+    const SourceManager &SourceMgr = AST.getSourceManager();
+    if (SourceMgr.getMainFileID() != FID ||
+        std::find(Decls.begin(), Decls.end(), D) == Decls.end()) {
+      return true;
+    }
+    SourceLocation Begin, End;
+    const LangOptions &LangOpts = AST.getLangOpts();
+    SourceLocation StartOfFileLoc = SourceMgr.getLocForStartOfFile(FID);
+    SourceLocation HightlightStartLoc = StartOfFileLoc.getLocWithOffset(Offset);
+    End =
+        Lexer::getLocForEndOfToken(HightlightStartLoc, 0, SourceMgr, LangOpts);
+    SourceRange SR(HightlightStartLoc, End);
+
+    DocumentHighlightKind Kind = DocumentHighlightKind::Text;
+    if (static_cast<index::SymbolRoleSet>(index::SymbolRole::Write) & Roles)
+      Kind = DocumentHighlightKind::Write;
+    else if (static_cast<index::SymbolRoleSet>(index::SymbolRole::Read) & Roles)
+      Kind = DocumentHighlightKind::Read;
+
+    DocumentHighlights.push_back(getDocumentHighlight(SR, Kind));
+    return true;
+  }
+
+private:
+  DocumentHighlight getDocumentHighlight(SourceRange SR,
+                                         DocumentHighlightKind Kind) {
+    const SourceManager &SourceMgr = AST.getSourceManager();
+    SourceLocation LocStart = SR.getBegin();
+    Position Begin;
+    Begin.line = SourceMgr.getSpellingLineNumber(LocStart) - 1;
+    Begin.character = SourceMgr.getSpellingColumnNumber(LocStart) - 1;
+    Position End;
+    End.line = SourceMgr.getSpellingLineNumber(SR.getEnd()) - 1;
+    End.character = SourceMgr.getSpellingColumnNumber(SR.getEnd()) - 1;
+    Range R = {Begin, End};
+    DocumentHighlight DH;
+    DH.range = R;
+    DH.kind = Kind;
+    return DH;
+  }
+};
+
 } // namespace
 
+llvm::Optional<Location>
+getDeclarationLocation(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;
+  SourceLocation LocEnd = Lexer::getLocForEndOfToken(ValSourceRange.getEnd(), 0,
+                                                     SourceMgr, LangOpts);
+  Position Begin;
+  Begin.line = SourceMgr.getSpellingLineNumber(LocStart) - 1;
+  Begin.character = SourceMgr.getSpellingColumnNumber(LocStart) - 1;
+  Position End;
+  End.line = SourceMgr.getSpellingLineNumber(LocEnd) - 1;
+  End.character = SourceMgr.getSpellingColumnNumber(LocEnd) - 1;
+  Range R = {Begin, End};
+  Location L;
+
+  StringRef FilePath = F->tryGetRealPathName();
+  if (FilePath.empty())
+    FilePath = F->getName();
+  L.uri = URI::fromFile(FilePath);
+  L.range = R;
+  return L;
+}
+
 std::vector<Location> clangd::findDefinitions(ParsedAST &AST, Position Pos,
                                               clangd::Logger &Logger) {
   const SourceManager &SourceMgr = AST.getASTContext().getSourceManager();
   const FileEntry *FE = SourceMgr.getFileEntryForID(SourceMgr.getMainFileID());
   if (!FE)
     return {};
 
   SourceLocation SourceLocationBeg = getBeginningOfIdentifier(AST, Pos, FE);
 
-  auto DeclLocationsFinder = std::make_shared<DeclarationLocationsFinder>(
+  auto DeclMacrosFinder = std::make_shared<DeclarationAndMacrosFinder>(
       llvm::errs(), SourceLocationBeg, AST.getASTContext(),
       AST.getPreprocessor());
   index::IndexingOptions IndexOpts;
   IndexOpts.SystemSymbolFilter =
       index::IndexingOptions::SystemSymbolFilterKind::All;
   IndexOpts.IndexFunctionLocals = true;
 
   indexTopLevelDecls(AST.getASTContext(), AST.getTopLevelDecls(),
-                     DeclLocationsFinder, IndexOpts);
+                     DeclMacrosFinder, IndexOpts);
+
+  std::vector<const Decl *> Decls = DeclMacrosFinder->takeDecls();
+  std::vector<const MacroInfo *> MacroInfos =
+      DeclMacrosFinder->takeMacroInfos();
+  std::vector<Location> Result;
+
+  for (auto Item : Decls) {
+    auto L = getDeclarationLocation(AST, Item->getSourceRange());
+    if (L)
+      Result.push_back(*L);
+  }
+
+  for (auto Item : MacroInfos) {
+    SourceRange SR(Item->getDefinitionLoc(), Item->getDefinitionEndLoc());
+    auto L = getDeclarationLocation(AST, SR);
+    if (L)
+      Result.push_back(*L);
+  }
+
+  return Result;
+}
+
+std::vector<DocumentHighlight>
+clangd::findDocumentHighlights(ParsedAST &AST, Position Pos,
+                               clangd::Logger &Logger) {
+  const SourceManager &SourceMgr = AST.getASTContext().getSourceManager();
+  const FileEntry *FE = SourceMgr.getFileEntryForID(SourceMgr.getMainFileID());
+  if (!FE)
+    return {};
+
+  SourceLocation SourceLocationBeg = getBeginningOfIdentifier(AST, Pos, FE);
+
+  auto DeclMacrosFinder = std::make_shared<DeclarationAndMacrosFinder>(
+      llvm::errs(), SourceLocationBeg, AST.getASTContext(),
+      AST.getPreprocessor());
+  index::IndexingOptions IndexOpts;
+  IndexOpts.SystemSymbolFilter =
+      index::IndexingOptions::SystemSymbolFilterKind::All;
+  IndexOpts.IndexFunctionLocals = true;
+
+  // Macro occurences are not currently handled.
+  indexTopLevelDecls(AST.getASTContext(), AST.getTopLevelDecls(),
+                     DeclMacrosFinder, IndexOpts);
+
+  std::vector<const Decl *> SelectedDecls = DeclMacrosFinder->takeDecls();
+
+  auto DocHighlightsFinder = std::make_shared<DocumentHighlightsFinder>(
+      llvm::errs(), AST.getASTContext(), AST.getPreprocessor(), SelectedDecls);
+
+  indexTopLevelDecls(AST.getASTContext(), AST.getTopLevelDecls(),
+                     DocHighlightsFinder, IndexOpts);
 
-  return DeclLocationsFinder->takeLocations();
+  return DocHighlightsFinder->takeHighlights();
 }
 
 void ParsedAST::ensurePreambleDeclsDeserialized() {
   if (PreambleDeclsDeserialized || !Preamble)
     return;
 
   std::vector<const Decl *> Resolved;
   Resolved.reserve(Preamble->TopLevelDeclIDs.size());

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

struct RenameParams {
   /// The position at which this request was sent.
   Position position;
 
   /// The new name of the symbol.
   std::string newName;
 };
 bool fromJSON(const json::Expr &, RenameParams &);
 
+enum class DocumentHighlightKind { Text = 1, Read = 2, Write = 3 };
+
+/// A document highlight is a range inside a text document which deserves
+/// special attention. Usually a document highlight is visualized by changing
+/// the background color of its range.
+
+struct DocumentHighlight {
+
+  /// The range this highlight applies to.
+
+  Range range;
+
+  /// The highlight kind, default is DocumentHighlightKind.Text.
+
+  DocumentHighlightKind kind = DocumentHighlightKind::Text;
+
+  friend bool operator<(const DocumentHighlight &LHS,
+                        const DocumentHighlight &RHS) {
+    int LHSKind = static_cast<int>(LHS.kind);
+    int RHSKind = static_cast<int>(RHS.kind);
+    return std::tie(LHS.range, LHSKind) < std::tie(RHS.range, RHSKind);
+  }
+
+  friend bool operator==(const DocumentHighlight &LHS,
+                         const DocumentHighlight &RHS) {
+    return LHS.kind == RHS.kind && LHS.range == RHS.range;
+  }
+};
+json::Expr toJSON(const DocumentHighlight &DH);
+
 } // namespace clangd
 } // namespace clang
 
 #endif

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

 }
 
 bool fromJSON(const json::Expr &Params, RenameParams &R) {
   json::ObjectMapper O(Params);
   return O && O.map("textDocument", R.textDocument) &&
          O.map("position", R.position) && O.map("newName", R.newName);
 }
 
+json::Expr toJSON(const DocumentHighlight &DH) {
+  return json::obj{
+      {"range", toJSON(DH.range)},
+      {"kind", static_cast<int>(DH.kind)},
+  };
+}
+
 } // namespace clangd
 } // namespace clang

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

public:
   virtual void onCodeAction(Ctx C, CodeActionParams &Params) = 0;
   virtual void onCompletion(Ctx C, TextDocumentPositionParams &Params) = 0;
   virtual void onSignatureHelp(Ctx C, TextDocumentPositionParams &Params) = 0;
   virtual void onGoToDefinition(Ctx C, TextDocumentPositionParams &Params) = 0;
   virtual void onSwitchSourceHeader(Ctx C, TextDocumentIdentifier &Params) = 0;
   virtual void onFileEvent(Ctx C, DidChangeWatchedFilesParams &Params) = 0;
   virtual void onCommand(Ctx C, ExecuteCommandParams &Params) = 0;
   virtual void onRename(Ctx C, RenameParams &Parames) = 0;
+  virtual void onDocumentHighlight(Ctx C,
+                                   TextDocumentPositionParams &Params) = 0;
 };
 
 void registerCallbackHandlers(JSONRPCDispatcher &Dispatcher, JSONOutput &Out,
                               ProtocolCallbacks &Callbacks);
 
 } // namespace clangd
 } // namespace clang
 
 #endif

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

void clangd::registerCallbackHandlers(JSONRPCDispatcher &Dispatcher,
   Register("textDocument/completion", &ProtocolCallbacks::onCompletion);
   Register("textDocument/signatureHelp", &ProtocolCallbacks::onSignatureHelp);
   Register("textDocument/definition", &ProtocolCallbacks::onGoToDefinition);
   Register("textDocument/switchSourceHeader",
            &ProtocolCallbacks::onSwitchSourceHeader);
   Register("textDocument/rename", &ProtocolCallbacks::onRename);
   Register("workspace/didChangeWatchedFiles", &ProtocolCallbacks::onFileEvent);
   Register("workspace/executeCommand", &ProtocolCallbacks::onCommand);
+  Register("textDocument/documentHighlight",
+           &ProtocolCallbacks::onDocumentHighlight);
 }

clang-tools-extra/trunk/test/clangd/documenthighlight.test

+# RUN: clangd -run-synchronously < %s | FileCheck %s
+# It is absolutely vital that this file has CRLF line endings.
+#
+Content-Length: 125
+
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+
+Content-Length: 479
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"#define MACRO 1\nnamespace ns1 {\nstruct MyClass {\nint xasd;\nvoid anotherOperation() {\n}\nstatic int foo(MyClass*) {\nreturn 0;\n}\n\n};\nstruct Foo {\nint xasd;\n};\n}\nint main() {\nint bonjour;\nbonjour = 2;\nint test1 = bonjour;\nns1::Foo bar = { xasd : 1};\nbar.xasd = 3;\nns1::MyClass* Params;\nParams->anotherOperation();\n}\n"}}}
+
+Content-Length: 156
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":17,"character":2}}}
+# Verify local variable 
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":11,"line":16},"start":{"character":4,"line":16}}},{"kind":3,"range":{"end":{"character":7,"line":17},"start":{"character":0,"line":17}}},{"kind":2,"range":{"end":{"character":19,"line":18},"start":{"character":12,"line":18}}}]}
+
+Content-Length: 157
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":18,"character":17}}}
+# Verify struct highlight
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":11,"line":16},"start":{"character":4,"line":16}}},{"kind":3,"range":{"end":{"character":7,"line":17},"start":{"character":0,"line":17}}},{"kind":2,"range":{"end":{"character":19,"line":18},"start":{"character":12,"line":18}}}]}
+
+Content-Length: 157
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":21,"character":10}}}
+# Verify method highlight
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":14,"line":2},"start":{"character":7,"line":2}}},{"kind":1,"range":{"end":{"character":22,"line":6},"start":{"character":15,"line":6}}},{"kind":1,"range":{"end":{"character":12,"line":21},"start":{"character":5,"line":21}}}]}
+
+Content-Length: 157
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":18,"character":14}}}
+# Verify Read-access of a symbol (kind = 2) 
+# CHECK: {"id":1,"jsonrpc":"2.0","result":[{"kind":1,"range":{"end":{"character":11,"line":16},"start":{"character":4,"line":16}}},{"kind":3,"range":{"end":{"character":7,"line":17},"start":{"character":0,"line":17}}},{"kind":2,"range":{"end":{"character":19,"line":18},"start":{"character":12,"line":18}}}]}
+
+Content-Length: 48
+
+{"jsonrpc":"2.0","id":10000,"method":"shutdown"}
+
+Content-Length: 33
+
+{"jsonrpc":"2.0":"method":"exit"}    
 No newline at end of file

clang-tools-extra/trunk/test/clangd/initialize-params-invalid.test

 # CHECK-NEXT:        "triggerCharacters": [
 # CHECK-NEXT:          ".",
 # CHECK-NEXT:          ">",
 # CHECK-NEXT:          ":"
 # CHECK-NEXT:        ]
 # CHECK-NEXT:      },
 # CHECK-NEXT:      "definitionProvider": true,
 # CHECK-NEXT:      "documentFormattingProvider": true,
+# CHECK-NEXT:      "documentHighlightProvider": true,
 # CHECK-NEXT:      "documentOnTypeFormattingProvider": {
 # CHECK-NEXT:        "firstTriggerCharacter": "}",
 # CHECK-NEXT:        "moreTriggerCharacter": []
 # CHECK-NEXT:      },
 # CHECK-NEXT:      "documentRangeFormattingProvider": true,
 # CHECK-NEXT:      "executeCommandProvider": {
 # CHECK-NEXT:        "commands": [
 # CHECK-NEXT:          "clangd.applyFix"

clang-tools-extra/trunk/test/clangd/initialize-params.test

 # CHECK-NEXT:        "triggerCharacters": [
 # CHECK-NEXT:          ".",
 # CHECK-NEXT:          ">",
 # CHECK-NEXT:          ":"
 # CHECK-NEXT:        ]
 # CHECK-NEXT:      },
 # CHECK-NEXT:      "definitionProvider": true,
 # CHECK-NEXT:      "documentFormattingProvider": true,
+# CHECK-NEXT:      "documentHighlightProvider": true,
 # CHECK-NEXT:      "documentOnTypeFormattingProvider": {
 # CHECK-NEXT:        "firstTriggerCharacter": "}",
 # CHECK-NEXT:        "moreTriggerCharacter": []
 # CHECK-NEXT:      },
 # CHECK-NEXT:      "documentRangeFormattingProvider": true,
 # CHECK-NEXT:      "executeCommandProvider": {
 # CHECK-NEXT:        "commands": [
 # CHECK-NEXT:          "clangd.applyFix"