[clangd] Document highlights for clangd

[Nebiroth]

Summary

Implementation of Document Highlights Request as described in LSP.

[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.