Add textDocument/foldingRange Provider

[billylanchantin]

✅ Status: Ready for Review

This PR is basically where I envisioned it, aka it's ready for review.

Description

This PR creates a folding range provider. See here for more about folding ranges:

https://microsoft.github.io/language-server-protocol/specifications/specification-3-15/#textDocument_foldingRange

Background

I recently raised an issue here:

elixir-lsp/vscode-elixir-ls#170

The tl;dr is that folding ranges stopped working, but as ElixirLS was relying on the default folding ranges implementation to create them, there was no easy way to fix the situation.

@wingyplus made some progress on a home-grown implementation a few months back:

#358

But they did not have the time to devote to finishing the work. I wanna thank them for getting me started -- I took their code as my starting point so I want to make sure they get some credit.

Methodology

High level

I make multiple passes (currently 4) through the source text and create folding ranges from each pass. Then I merge the ranges from each pass to provide the final ranges. Each pass gets a priority to help break ties (the priority is an integer, higher integers win). See:

apps/language_server/lib/language_server/providers/folding_range.ex

Indentation pass (priority: 1)

I use the indentation level -- determined by the column of the first non-whitespace character on each line -- to provide baseline ranges. All ranges from this pass are kind?: :region ranges.

Comment block pass (priority: 2)

I make a "comment block" -- consecutive lines that start with # -- to form regions. All ranges from this pass are kind?: :comment ranges.

Token-pairs pass (priority: 3)

I use pairs of tokens, e.g. do and end, to provide another pass of ranges. All ranges from this pass are kind?: :region ranges.

Special tokens pass (priority: 3)

I find strings (regular and charlist strings and heredocs) and sigils in a pass as they're delimited by a few special tokens. Ranges from this pass are either kind?: :comment if the token signifies a @doc or @moduledoc heredoc or kind?: :region otherwise.

To Do

• Heredoc pass: kind?: :comment | :region
• Comment block pass: kind?: :comment

Any others?

• |> chains: kind?: :region?
• Import blocks: kind?: :imports?

Testing

Run

$ mix cmd --app language_server mix test test/providers/folding_range_test.exs --color

What to look out for when reviewing

• Overall approach: Does the multiple passes with priority makes sense?
• Speed: I tried to make each pass O(n) in the length of the source text. But I don't know how to benchmark my results. There's also some extra Enum.sort()s that we'll want to drop. I just got lazy with the tests. This is fixed now.
• Typespecs: Overkill?
• Idiomatic: Does this PR clash with the style used in the rest of the project?

[billylanchantin]

Hm, older versions of Elixir all seem to be failing on test

test/providers/folding_range_test.exs:462

Any ideas why?

[lukaszsamson]

Any ideas why?

It looks like there is a slight difference in tokenizer output in older versions:
1.9

{:sigil, {2, 14, nil}, 83,
    ["I'm a @moduledoc heredoc.                            # 2\n"], [],
    "\"\"\""},

1.11

{:sigil, {2, 14, nil}, 83,
    ["I'm a @moduledoc heredoc.                            # 2\n"], [], 2,
    "\"\"\""},

[billylanchantin]

@lukaszsamson Thank you for the insight! That seems to have fixed everything.

So here's where we stand:

I think the functionality is basically there. I have a few quibbles that we could address:

• The {:ok, ranges} pattern isn't really used correctly since the sub-range providers default to an [] if there was an issue. Perhaps we want the sub-range-providers to return an {:error, reason} instead so it's clearer what went wrong, then override the result with an [] in FoldingRange.do_provide/1.
• Documentation. Some of the module docs aren't filled out.
• [Optional] Behaviour for the sub-range providers.

We could ignore any or all those changes. Or we could make them in a follow on PR. It's up to how quickly y'all want to get this out the door.

[lukaszsamson]

@lukaszsamson Thank you for the insight! That seems to have fixed everything.

I'm glad I could help. I went through commit history of :elixir_tokenizer' and this seems to be the only significant change between 1.8 and 1.11. There are some heredoc and /r/n` related changes on master though. Let's hope they not break anything.

The {:ok, ranges} pattern isn't really used correctly since the sub-range providers default to an []

Is there any value in returning errors here?

Documentation. Some of the module docs aren't filled out.

IMO it's good enough. We're not likely to need it pushed to hex docs.

[billylanchantin]

There are some heredoc and /r/n related changes on master though. Let's hope they not break anything.

Agreed. Although code folding is broken already so I don't mind patching if more issues come up.

The {:ok, ranges} pattern isn't really used correctly since the sub-range providers default to an []

Is there any value in returning errors here?

No, not really other than debugging and testing. I'd be fine dropping the whole {:ok, ranges} thing and just having all provide_ranges implementations return a list no matter what.

I suppose I pointed it out because when I see {:ok, something}, that signifies to me that it's possible {:error, reason} can happen as well. Since that can't happen here, I was wondering if we should either add errors or drop the tuples.

[axelson]

Thanks for this! I just wanted to drop a note that I am working my way through reviewing this PR, and it's looking great so far so I don't expect to request any major changes.

[axelson]

Looks great, just two very minor comments. Although I will create a PR to switch to doctests for the examples.

[axelson]

Shouldn't we include @typedoc here as well? (and in the matching code)

[axelson]

Here's the PR: billylanchantin#1

[axelson]

Thanks again for this ❤️!

[billylanchantin]

@axelson You all are welcome! Sorry for the delay. I'd wanted to merge your changes in for a while, but I messed up my local build and had trouble getting things working again on my end. That and some life stuff sapped up all my free time.

So glad we got this across the finish line!

[axelson]

No worries! Life happens to all of us (definitely including myself, haha). I'm quite excited for this functionality and I'm hoping to get it out in the next release soon. Maybe next weekend.