Add "textDocument/foldingRanges"

[rockbruno]

Hey there, I was reading about the LSP yesterday and wanted to take a shot at implementing the folding requests 🙂

Although I'm unsure how Xcode implemented it, some spelunking makes me believe you guys are using thebodyoffsets from editor.open requests, so that's how I tackled it.

The first commit works for perfectly idented bodies but stills fails for weird syntaxes like:

func foo() {

return }

Since Xcode can treat these, I'll implement the full request to treat the edge cases.

[rockbruno]

I've removed the WIP tag and rebased the commits - It's now supporting all bodies and comments recognized by SourceKit. My only pet peeve is that it acts slightly different than the default folding behaviour from VS Code. While VSCode will fold something with the bracket on the next line:

func foo() { ...
}

...this implementation will get rid of it:

func foo() { ...

It does seem to be in line with how Xcode folds it though, and VS Code fails to fold the edge case I first mentioned which makes me think this might not be the worst approach after all.

[benlangmuir]

Hey, thanks for working on this! I've left a bunch of feedback on the changes.

The bigger question in my mind is about the approach taken to find the ranges. What you're doing here - opening a new document in syntax-only mode for every query - has the advantage of being simple to implement, but it means we do more work than necessary when the document is already returning these ranges after every document open and edit.

An alternative approach would be to keep track of that syntax map and structure information during open/edit so that you can return it here without doing another sourcekitd query. That's less expensive, but more complex since the syntax map changes during an edit request are returned as a diff, not all the results at once. This is also a more scalable approach than opening a new document since we will want to add more queries that need the same information.

A third approach would be to integrate the swift syntax tree and build the ranges from that. This is the right way to do it in the long term, but there's a lot going on with swift syntax right now and I haven't evaluated what the effects of adding a dependency on swift syntax are right now.

I need to think about this more, but my current feeling is to accept the current (open a new document ever time) approach and move to swift syntax in the future. /cc @akyrtzi for his thoughts on this question.

[benlangmuir]

defaults to the length of the start line.

This is what the spec says, but is it actually correct, or should it be from the beginning of the line? What is vscode doing here?

Please also rename "character" to "utf16index" in these properties and then customize the CodingKeys to keep serialization working. See Position for an example. I've been trying to keep places that need utf16 conversions really clear.

[rockbruno]

The builtin behavior (in .swift files at least) appears to be to fold everything in the lines after and before the braces:

func foo() { 1111 //intact
    2222 //folded
3333 } //intact

With SourceKit setting startLine to 1 and endLine to 3, the behavior is:

func foo() { 1111 //intact
    2222 //folded
3333 } //folded

Which is the behavior the comments define. The problem is that they appear to be ignoring the character properties, as I was unable to make func foo() { stay intact but fold 1111. I'm investigating if that's really the case.

[akyrtzi]

I need to think about this more, but my current feeling is to accept the current (open a new document ever time) approach and move to swift syntax in the future. /cc @akyrtzi for his thoughts on this question.

Using sourcekitd for syntactic functionality is an evolutionary dead-end, we'll adopt SwiftSyntax at some point. So I wouldn't worry about making it efficient, keep it simple.
The tests you'll add for this will be useful to validate that SwiftSyntax integration works as expected later on, make sure you have tests that check the behavior when edits occur as well.

[benlangmuir]

make sure you have tests that check the behavior when edits occur as well.

Folding ranges doesn't have any real interaction with edits, so it doesn't seem like the right place to test this to me. When we add swift syntax support I expect we'll want to test the more specific goal of keeping a shared syntax tree in sync with edit requests.

[akyrtzi]

make sure you have tests that check the behavior when edits occur as well.

Folding ranges doesn't have any real interaction with edits, so it doesn't seem like the right place to test this to me. When we add swift syntax support I expect we'll want to test the more specific goal of keeping a shared syntax tree in sync with edit requests.

SGTM.

[rockbruno]

Thank you for your inputs @benlangmuir and @akyrtzi. I was under the impression that SourceKit had some form of internal cache given that I get a few duplicate editor.open requests after booting a single-file Xcode project, so I blindly went with the always-request approach. I'll implement the necessary changes shortly.

[benlangmuir]

I'm going to agree with @akyrtzi that we should keep this simple until we can switch to swift-syntax, so let's keep your current approach of opening a new document. I have a few more specific comments, but otherwise this is looking good.

[benlangmuir]

I don't think this subtraction should be necessary. Both LSP and sourcekitd use half-open ranges (the range includes the start position and goes up to but not including the end position). They should both expect the end offset to be one-past the last character of the comment AFAICT.

Heres what the spec says about the Range type:

A range in a text document expressed as (zero-based) start and end positions. A range is comparable to a selection in an editor. Therefore the end position is exclusive.

And the comments in the spec about FoldingRange's endCharacter says that it defaults to the line length, which is also one-past the end.

[rockbruno]

The reason I added that is because a simple comment like //foo will end up with a different endLine, as the "end" of the comment here is the start of the next line. It was causing VSCode to fold whatever was on the next line even though the comment only really covers a single line. It might have to do with the supposed ignored properties I mentioned earlier in this case, but I wasn't sure how to stop it other than subtracting it. What do you think?

[benlangmuir]

Hmm, in that case subtracting 1 before finding the Position doesn't seem like a good fix, since it only does the right thing if the end position is on a different line. Maybe you could compute the correct Position, but then check if the end of the range is at column 0, and in that case change it to the previous line but with column number = line length? It's the same position in an abstract sense, but if something is only looking at the line number and not the column it will be more correct.

[benlangmuir]

To clarify, I mean change Position(line: N, utf16Index: 0) to Position(line: N-1, utf16Index: <length of line N-1>). You can get the line length from the snapshot's LineTable.

[rockbruno]

Definitely sounds better! To be fair, I just noticed the same problem exists if you have something right after a brace, like a promise:

foo().then {
   //
}.then {
}

Folding the first block would also fold the second then if the client only supports folding whole lines. It's the same problem, although the comment one definitely looks weirder. If you're right about the end position being expected to be one character past then perhaps we don't need to treat this at all since the problem will always exist for bodies.

[rockbruno]

Alright! I think I finally cracked the issue this time @benlangmuir . It seems that VSCode really only supports line folding:

    fillClientCapabilities(capabilites) {
        let capability = ensure(ensure(capabilites, 'textDocument'), 'foldingRange');
        capability.dynamicRegistration = true;
        capability.rangeLimit = 5000;
        capability.lineFoldingOnly = true;
    }

Their models don't even check the offsets.

In regards to why the last line was getting folded, their tests seem to show that they're not supposed to be part of the range. I wasn't sure if that's how the LSP is supposed to work but I've coded the feature around it just in case.

I've removed the code that was subtracting the offsets and instead starting passing the client's capabilities to the server. If they only support line folding, I ignore the last line, otherwise I send the real lines and offsets. This makes it work exactly like the built-in behaviour in VSCode, although I'm still not sure if it would behave correctly in an IDE that can fold arbitrary ranges (as in, do we need to ignore the last character?)

I also added support to doc comments, but since SourceKit breaks them into several components I've had to make the response's merge successive comments into one. I took a bit of freedom to make it work so I think you'll want to take a look at it again.

[benlangmuir]

LGTM! Congratulations on landing our first new request since we open-sourced! 🎉

[rockbruno]

Thanks for the patience @benlangmuir! Feel free to ping me once SwiftSyntax gets in the picture, I would love to assist in porting this and other features in any way I can.

[benlangmuir]

@rockbruno we should update the README status table. Would you like to do the honours?

[rockbruno]

Absolutely! #45 🎉