[clangd] Document highlights for clangd

[Nebiroth]

Summary

Implementation of Document Highlights Request as described in LSP.

[malaperle]

clangd/ClangdUnit.cpp

1029

Last

1069

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

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

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

Still there

clangd/ClangdServer.cpp

513

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

524

We should return an error this case.

clangd/ClangdUnit.cpp

1062

This function should be private.

1096

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

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

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

NIT: Misindent

test/clangd/initialize-params.test

23

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

"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

1087

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

1097

maybe move the check earlier? see comment above.

clangd/Protocol.h

601

Use /// like other structs

[malaperle]

clangd/ClangdServer.cpp

526

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

Invalid

530

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

566

Too many new lines. Only keep one.

clangd/Protocol.h

607

Use /// like other places

614

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

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

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

568

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

961

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

601

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

620

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.