Page MenuHomePhabricator

[clangd] Implement textDocument/foldingRange
ClosedPublic

Authored by kbobyrev on Jun 24 2020, 12:10 AM.

Details

Summary

This patch introduces basic textDocument/foldingRange support. It relies on
textDocument/documentSymbols to collect all symbols and uses takes ranges
to create folds.

The next steps for textDocument/foldingRange support would be:

  • Implementing FoldingRangeClientCapabilities and respecting respect client preferences
  • Specifying folding range kind
  • Migrating from DocumentSymbol implementation to custom RecursiveASTVisitor flow that will allow more flexibility
  • Supporting more folding range types: comments, PP conditional regions, includes and other code regions (e.g. public/private/protected sections of classes, control flow statement bodies)

Tested: (Neo)Vim (coc-clangd) and VSCode.

Related issue: https://github.com/clangd/clangd/issues/310

Diff Detail

Event Timeline

kbobyrev created this revision.Jun 24 2020, 12:10 AM
sammccall added inline comments.Jun 30 2020, 4:15 PM
clang-tools-extra/clangd/ClangdLSPServer.cpp
975

one we fix the signature, you should just be able to pass the callback through.

977

unused capture (and variable)

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

this should return (as callback) vector<FoldingRange>.
You got unlucky: it's sandwiched between onDocumentSymbol and onCodeAction which both have to be dynamically typed as they return different structures depending on capabilities.

clang-tools-extra/clangd/FindSymbols.cpp
281 ↗(On Diff #272922)

How useful are folding ranges when symbols start/end on the same line? Do we really want to emit them?

297 ↗(On Diff #272922)

I'm not sure whether this is the right foundation, vs RecursiveASTVisitor.
How would we generalize to compoundstmts etc without RAV, and if we do use RAV for those, why would we still use getDocSymbols for anythnig?

clang-tools-extra/clangd/FindSymbols.h
50 ↗(On Diff #272922)

"using Document Symbols" seems like an implementation detail here, I'd keep it out of the first line at least.

Maybe separate out explicitly in a second line: Currently this includes the ranges of multi-line symbols (functions etc)

52 ↗(On Diff #272922)

This seems like a surprising place to find this function - it only really makes sense when considering the current implementation (which will presumably grow).
I think it'd be a more natural fit in SemanticSelection, maybe.

kbobyrev updated this revision to Diff 274741.Jul 1 2020, 3:35 AM
kbobyrev marked 7 inline comments as done.

Address review comments.

clang-tools-extra/clangd/FindSymbols.cpp
281 ↗(On Diff #272922)

I think they are useful from the LSP perspective, if characters didn't matter in the ranges they wouldn't be included in the protocol at all and there wouldn't be any way for clients to specify their preferences.

I don't think it's a common use case, but I do think it's a totally valid one. Maybe those should be the first candidates for excluding when there is a limit and maybe we could completely cut them for performance. But from the LSP perspective it seems logical to have those.

What do you think?

297 ↗(On Diff #272922)

Sure, I agree. The thing is I wanted to do folding ranges in an incremental way while gradually building the right foundations. RAV + TokenBuffers certainly seems right for the final implementation but it'd be much more code and tests and I didn't want to push a big patch since it is harder to review and also harder to feel the real progress. Pushing this patch allows me to parallelize the work, too, since there are several improvements on the LSP side which wouldn't touch implementation.

I think it'd be better to leave the current implementation and simply replace it in the next patch, do you see any reasons why this wouldn't be a good option?

kbobyrev updated this revision to Diff 274743.Jul 1 2020, 3:37 AM

Remove unused variable.

I think this looks reasonable, but I'd like to make sure we have a plan going forward otherwise the behavior/assumptions tend to calcify.
I think RAV and adding other region types are clear, but maybe we should discuss lines/filtering behavior offline a bit.

clang-tools-extra/clangd/FindSymbols.cpp
281 ↗(On Diff #272922)

Well, I don't think "the protocol allows regions within a line, therefore we must emit them" is a great argument. You mention a valid use case, but you haven't *described* any use cases - what specific C++ intra-line region is the user likely to fold in a way that's useful?

I think that it's hard to point to concrete benefits, but the costs are clear enough:

  • responses will be (much) bigger, which we know some clients don't deal well with
  • clients are not likely to do any smart filtering, so I think this will lead to performance/ux problems in practice (especially if most implementations only support folding blocks etc)
  • it makes decisions on what folding regions to emit harder, e.g. should we fold function calls, not fold them, or fold them only if start/end are on different lines?
  • we need two different modes, to handle clients that support line-folds vs clients that support char-folds. (If we only emit multi-line folds we could also only emit the inner (or the outer) fold for a pair of lines, and the result would be suitable for both types of editors)

when there is a limit

It's pretty important that this feature behaves consistently, for it to be useful. If we're going to only-sometimes enable folding of certain constructs, it better be for a reason that's pretty obvious to the user (I believe single vs multiline qualifies, but "the document complexity exceeds X" certainly doesn't.) For this reason I don't think we should implement that capabilities feature (or should do it in some totally naive and unobtrusive way, like truncation)

297 ↗(On Diff #272922)

That sounds fair enough - it is very simple. I'd suggest leaving a slightly more specific fixme: like "getDocumentSymbols() is conveniently available but limited (e.g. doesn't yield blocks inside functions). Replace this with a more general RecursiveASTVisitor implementation instead."

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

This doesn't really specify what is to be fixed and why.
Vs FIXME: add FoldingRangeClientCapabilities so we can support per-line-folding editors.

(wouldn't this fixme belong on the *request* rather than the response?)

clang-tools-extra/clangd/SemanticSelection.cpp
100

nit: comments
I think PP conditional regions and includes are maybe more relevant than definitions?
Maybe reference the bug here.

nridge added a subscriber: nridge.Jul 2 2020, 6:46 PM
kbobyrev updated this revision to Diff 275929.Jul 7 2020, 12:40 AM
kbobyrev marked 7 inline comments as done.

Hide FoldingRanges feature behind the flag.

kbobyrev added inline comments.Jul 7 2020, 2:07 AM
clang-tools-extra/clangd/FindSymbols.cpp
281 ↗(On Diff #272922)

As discussed online, we should emit single-line ranges.

Tests :-)

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

nit: =0 (and on endLine)

clang-tools-extra/clangd/SemanticSelection.cpp
107

I think this should mention the contents-of-blocks vs whole-nodes issue too.

clang-tools-extra/clangd/SemanticSelection.h
28

I think "main file" section is just a for-now implementation detail, I'd leave this out.

clang-tools-extra/clangd/tool/ClangdMain.cpp
300

rangees -> ranges

kbobyrev marked an inline comment as done.Jul 9 2020, 2:14 PM

Tests :-)

I was hoping glorious DocumentSymbols super tested API would shield me from that :P Didn't think of reasonable testing strategy but fair enough, will do!

clang-tools-extra/clangd/tool/ClangdMain.cpp
300

My broken keyboard puts multiple symbols all over the place :( Catching them all is hard, sorry, the other patch also had a couple of those.

kbobyrev planned changes to this revision.Jul 9 2020, 2:24 PM

Tests :-)

I was hoping glorious DocumentSymbols super tested API would shield me from that :P

While the implementation is shared, a very small set of tests is fine. We should at least include some of the ranges that are "incorrect" that we want to fix.

Didn't think of reasonable testing strategy but fair enough, will do!

I think you can literally just use an Annotations marked with all the ranges you expect.

kbobyrev updated this revision to Diff 277357.Jul 13 2020, 2:45 AM
kbobyrev marked 4 inline comments as done.

Add tests and fix DocumentSymbol ranges.

kbobyrev updated this revision to Diff 277359.Jul 13 2020, 2:48 AM

Updated a couple of comments.

I'd suggest splitting out the DocumentSymbol changes into a separate patch from the FoldingRanges patches.

clang-tools-extra/clangd/FindSymbols.cpp
140 ↗(On Diff #277359)

this whole block seems likely to be obsolete if you switch to toHalfOpenFileRange

167 ↗(On Diff #277359)

As far as I can tell, this line is just always wrong: EndLoc points at the *start* of the last token in the range, so the last token will be omitted from the range in all cases.

168 ↗(On Diff #277359)

I can't see any conceptual reason for this to be conditional.

clang-tools-extra/clangd/SemanticSelection.h
28

Can we still have a high-level description of what this function does?

Returns a list of ranges whose contents might be collapsible in an editor.
This should include large scopes, preprocessor blocks etc.
clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
203

nit, the test is usually named after the API/feature, so this should be TEST(FoldingRanges, All) or so

kbobyrev edited the summary of this revision. (Show Details)Jul 13 2020, 4:06 AM
kbobyrev updated this revision to Diff 277381.Jul 13 2020, 4:25 AM
kbobyrev marked 5 inline comments as done.

Isolate DocumentSymbol range changes into D83668.

kbobyrev updated this revision to Diff 277382.Jul 13 2020, 4:27 AM

Rebase on top of master.

kbobyrev edited the summary of this revision. (Show Details)Jul 13 2020, 4:27 AM
sammccall accepted this revision.Jul 13 2020, 6:01 AM

Nice, thanks!

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

for outgoing fields that are optional in the protocol but we always set, we tend to omit Optional. The protocol is worded in a way that suggests servers may continue to set the character fields even for line-oriented editors, and I think we should do this, dropping Optional here.

This revision is now accepted and ready to land.Jul 13 2020, 6:01 AM
kbobyrev updated this revision to Diff 277683.Jul 14 2020, 12:27 AM
kbobyrev marked an inline comment as done.

Remove llvm::Optional from character fields.

This revision was automatically updated to reflect the committed changes.