Add textDocument/foldingRange capability to LSP client

[w4v3]

Will resolve #12836. Ready for comments now.

[w4v3]

I have a few questions:

1. What is the desired way in which the user should interact with this feature? Currently, vim.lsp.buf.document_fold() issues a textDocument/foldingRange request, which upon completion sets the foldexpr and foldmethod to update the folds. The function vim.lsp.buf.foldexpr() exposes the data from the last such request in the form required for foldexpr. This means that setting foldexpr to vim.lsp.buf.foldexpr() directly is relatively useless since it does not issue a request to receive the updated folds, and also vim.lsp.buf.document_fold() blindly overrides the window-local user settings of foldexpr/foldmethod. For me this leads to the experience I would expect, at least for vim.lsp.buf.document_fold(), but there might be better ways to handle this.
2. https://github.com/prabirshrestha/vim-lsp/blob/master/autoload/lsp/ui/vim/folding.vim uses textprops to remember the locations of the lines at the time the request was issued, in case the user makes changes before the request is completed. I have not implemented this; should I? Personally I just bind vim.lsp.buf.document_fold() to an autocmd InsertLeave and don't care about temporarily incorrect folds since they are inevitable anyways.
3. Is it necessary to deal with the possibility that the request is used on multiple servers, returning different fold ranges?

[theHamsta]

Just out of curiosity, doesn't this have the same problem as we at nvim-treesitter have with tree-sitter folding, that we can only fold on LSP/tree-sitter buffers but foldexpr/foldmethod is a window option and the window might switch to a non-LSP/tree-sitter buffer.

[w4v3]

The fold information is stored with the buffer number of the buffer the document_fold method was called on. The options are then set for all windows associated with the buffer, but the foldexpr only returns non-zero values if there are entries for the current buffer. Unless I'm missing something, the only problem arising from this is that when the user switches to a different buffer, the foldmethod is still set to "expr" rather than what it was before. Or can it happen somehow that vim.lsp.buf is not available for some newly opened buffer?

[theHamsta]

foldmethod is still set to "expr" rather than what it was before.

This is the issue I was referring to. But this is rather an inherent problem of foldmethod

[mjlbach]

Couldn't we add an autocommand or something to change the fold method based on buffer id? I guess the solution would be to make foldmethod buffer local in the long term.

[w4v3]

What should that look like exactly? One possibility would be to add an option for the user to make these options generally window local or buffer local. This would be compatible and keep the behavior consistent, but also a somewhat strange option to have. Or do you mean that it should only be buffer local if LSP folding is used? One could also add "lsp" as a possible value to foldmethod, in which case foldexpr should be ignored (and therefore the user setting of foldexpr is preserved) and the user would trigger folding by setlocal foldmethod=lsp, so that control over the locality of these options is again in their hand. It would also eliminate the need to expose the LSP foldexpr, which should not be used directly anyways, and one could integrate the invocations of the LSP folding procedure more tightly.

[w4v3]

So what should I do next; should I explicitly request a review and if so, how?

[w4v3]

Maybe since @tjdevries is assigned to the issue, can you review my code or otherwise direct me?

[mjlbach]

I'm ok merging this with the limitations of foldmethod in mind, but I think we should create a new tracking issue for a buffer local implementation of foldmethod. We could hypothetically unmap foldexpr on BufLeave. Basically the logic would be:

if BufLeave, set up an autocmd on BufEnter that unmaps the foldexpr for the window if the same language server is not attached to the window.

[w4v3]

@mjlbach I'm done then, could you merge it please? I've been happily using this feature for almost a year now but it would be great if I could have this without having to keep maintaining my fork.

[mjlbach]

Let me tag Mathias for review, I think the logic looks ok. I'm a little bit concerned about:

• the aforementioned conflict between windows, buffers, foldexpr (and my possible workaround)
• multi-server conflicts
• the caching mechanism that reset foldexpr on each evaluation

[mfussenegger]

I think at the very least this would require some tests.

But I'm also not sure if it is good to add 4 new public methods to the API. Ideally it would only expose the foldexpr and keep the remainder private.

Not entirely sure how to make that happen. foldexpr could keep a cache internally and make a sync request the first time + register some autocmds to keep it updated async afterwards.
Or depending on how often the foldexpr gets evaluated maybe it is even feasible to make sync requests all the time if the buffer got dirty inbetween?

And I think we should consider adding a buflocal foldexpr first to solve the mentioned issues with the window based foldexpr.

[w4v3]

Let me tag Mathias for review, I think the logic looks ok. I'm a little bit concerned about:

• the aforementioned conflict between windows, buffers, foldexpr (and my possible workaround)

Couldn't the user put your workaround into their init.vim for now if they are unhappy with a window-local foldmethod? And what would be the benefit of solving this conflict before integrating this feature given that the conflict persists without the feature?

• multi-server conflicts

Is there a preferred way for how I should deal with such conflicts?

• the caching mechanism that reset foldexpr on each evaluation

Do you have an alternative suggestion for how to achieve the folds being updated once the response is received? How would you feel about the aforementioned possibility of an lsp option to foldmethod, which would ignore foldexpr and continuously and asynchronously update the folds?

[w4v3]

I think at the very least this would require some tests.

Could you point me to where I would implement such tests? I can't find the tests for the other LSP handlers in the codebase.

But I'm also not sure if it is good to add 4 new public methods to the API. Ideally it would only expose the foldexpr and keep the remainder private.

I think the user should be able to explicitly trigger document folding requests via vim.lsp.buf.document_fold()—I'd rather keep vim.lsp.buf.foldexpr() private since it's not automatically updated. I was also under the impression that vim.lsp.util contains many public functions that would be considered implementation details, so I didn't think providing those utilities would be problematic. Are you suggesting I move the logic from there into vim.lsp.handlers, from where I am currently calling these functions?

Not entirely sure how to make that happen. foldexpr could keep a cache internally and make a sync request the first time + register some autocmds to keep it updated async afterwards. Or depending on how often the foldexpr gets evaluated maybe it is even feasible to make sync requests all the time if the buffer got dirty inbetween?

In my experience making sync requests with foldexpr is way too slow for large files. I will look into implementing your first suggestion though.

And I think we should consider adding a buflocal foldexpr first to solve the mentioned issues with the window based foldexpr.

See my previous comment. Does this mean no extensions or changes to any part of neovim's folding mechanisms should be suggested before this issue is solved or in what way is this PR particularly affected by it?

[mfussenegger]

Could you point me to where I would implement such tests? I can't find the tests for the other LSP handlers in the codebase.

Most of the tests for the lsp module are in ./test/functional/plugin/lsp_spec.lua

I think the user should be able to explicitly trigger document folding requests via vim.lsp.buf.document_fold()—I'd rather keep vim.lsp.buf.foldexpr() private since it's not automatically updated

I'm not sure if it wouldn't be surprising that document_fold() implicitly changes the foldexpr as a side effect.

I was also under the impression that vim.lsp.util contains many public functions that would be considered implementation details, so I didn't think providing those utilities would be problematic

This is currently the case to some degree, but we're trying to reduce that as people end up using this functions and are then unhappy if we later change them.

In my experience making sync requests with foldexpr is way too slow for large files. I will look into implementing your first suggestion though.

I suspected as much. That was the reason why I mentioned the caching.

Does this mean no extensions or changes to any part of neovim's folding mechanisms should be suggested before this issue is solved or in what way is this PR particularly affected by it?

No it doesn't mean that - but it means that we shouldn't merge something that we know has flaws which in the worst case may require breaking changes later on.
We should first lay the groundwork to solve these flaws.

[tom-anders]

Is there a tracking issue for making foldexpr/foldmethod buffer-local? I'd possibly be interested in trying to implement this. Seems like it is the main blocker for this PR

[lewis6991]

foldexpr is already window local and there are no plans to change this. Note window local options can be local to a specific buffer in a specific window (via setlocal) so they are already buffer local.

[tom-anders]

foldexpr is already window local and there are no plans to change this. Note window local options can be local to a specific buffer in a specific window (via setlocal) so they are already buffer local.

Ah okay, then I misunderstood @mjlbach's comment here:

I'm ok merging this with the limitations of foldmethod in mind, but I think we should create a new tracking issue for a buffer local implementation of foldmethod.

@w4v3 are you still working on this PR? Or should someone else take over possibly?

[w4v3]

@w4v3 are you still working on this PR? Or should someone else take over possibly?

I'm happy to work on getting this merged (i.e. resolving conflicts, writing tests, and a bit of restructuring) but I also thought that the window-local nature of foldexpr would stand in the way of this PR. I suppose I'll finish it up and then see what the maintainers decide.

[igorlfs]

As a reference, the plugin nvim-ufo implements support for this capability.

[w4v3]

I'm once again ready for comments. I've implemented the following changes:

• only foldexpr() and refresh_folds() are now public; foldexpr() only returns the cashed fold levels, and refresh_folds() issues the requests to the server
• the response handler does not change foldmethod/foldexpr
• the logic calculating the foldlevels is quite different now and tries to do what :fold/zf do in case of overlapping folds (note that this makes the outcome dependent on the order of the FoldingRange entries in the server response)
• added tests, let me know if they're in the right place and sufficient (test/functional/plugin/lsp/utils_spec.lua)

Since the implementation does not touch foldmethod/foldexpr, updating the folds may require forcing the recomputation of the foldexpr using zx. I think this is as unobtrusive as it gets. foldexpr() does not have side-effects and refresh_fold() can be bound by the user to autocmds. Of course, we could also have foldexpr() set up those autocmds as @mfussenegger suggested, although this removes control over how often requests are issued from the user, which I personally find undesirable.

We could also issue the request inside foldexpr(), but this seems to perform quite poorly. The nvim-ufo plugin pointed out by @igorlfs implements this functionality using foldmethod=manual and issuing :fold commands from the server responses. This way one can avoid the quirks coming with foldexpr. Should we do it like this as well perhaps?

[lewis6991]

We could also issue the request inside foldexpr(), but this seems to perform quite poorly.

I would highly advocate we do this and remove refresh_folds(). If this has issues, then these issues should be at least quantified before we move forward.

Of course, we could also have foldexpr() set up those autocmds as @mfussenegger suggested, although this removes control over how often requests are issued from the user, which I personally find undesirable.

This is not something the user should need to think about.

[w4v3]

I would highly advocate we do this and remove refresh_folds(). If this has issues, then these issues should be at least quantified before we move forward.

One problem with this is that then every time the foldexpr is evaluated, the cached values are returned after a new request to update the cache is issued, so foldexpr will never return the most up to date values unless the user forces a recomputation before making further changes to the document. If the evaluation of the foldexpr could somehow be performed async so that it could wait for the results of the request without blocking nvim then this would be much cleaner, but this is not an option, right?

This is not something the user should need to think about.

In that case, under which circumstances would you say the cache should be updated? Currently I'm using this with an InsertLeave autocmd. Often, the foldexpr is evaluated when leaving insert mode as well, but due to the async request the updated results are usually not available yet at that point, making zx necessary.

I'm starting to tend towards thinking that the foldmethod=manual route would be better suited. It would just need a document_fold() function the user can trigger whenever they want which performs the request and then sets the folds when the request is completed. The mode of operation of foldexpr just doesn't seem to play well with the way the LSP client works.