expand-snippet: indent according to the line where snippet is expanded

[kiennq]

Doing what's suggest in #1935 to simply align the expanded snippet with the indentation level of where snippet is expanded.
This should be fairly fast and could be used even in org-mode.

[kiennq]

@yyoncho Can you check if this change still works fine with your snippets?

[yyoncho]

When I tested it I noticed that the snippets that come from jdtls are not indented with the proper indent level, e. g.

package temp;

class App {
    public static void main(final String[] args) {
        new Runnable(){
             |
        };
    }

}

The completion is "newText": "Runnable(){\n\t${0}\n}". When using auto the code is indented correctly with 4 spaces. With proposed change it is indented using \t.

[kiennq]

When I tested it I noticed that the snippets that come from jdtls are not indented with the proper indent level, e. g.

package temp;

class App {
    public static void main(final String[] args) {
        new Runnable(){
             |
        };
    }

}

The completion is "newText": "Runnable(){\n\t${0}\n}". When using auto the code is indented correctly with 4 spaces. With proposed change it is indented using \t.

The snippet is indeed indented with \t as it's specified in snippet.
It looks like your jdtls is set to format with tab instead of spaces? Won't that cause problem with indentation when reformat via lsp-format-buffer?

I found this redhat-developer/vscode-java#557 (comment), it seems that setting .settings/org.eclipse.jdt.core.prefs with following would fix the problem?

org.eclipse.jdt.core.formatter.tabulation.char=space
org.eclipse.jdt.core.formatter.tabulation.size=4

I can tabify/untabify if needed, but it seems that we should setting server options here.
One reason is that while lsp-format-buffer highly goes with server setting, client side format may go differently, it would be better to strictly use server returns as source of truth for indentation.

[yyoncho]

One reason is that while lsp-format-buffer highly goes with server setting, client side format may go differently, it would be better to strictly use server returns as source of truth for indentation.

In general, this is true, but practically we follow what vscode does. The thing is that there is nothing in the spec that says that the server should format the snippets properly. Also, vscode ATM is performing client-side formatting (I verified that it is also getting \t which is converted to 4 spaces). I am not sure on what part of the spec this PR is based on.

[kiennq]

In general, this is true, but practically we follow what vscode does. The thing is that there is nothing in the spec that says that the server should format the snippets properly. Also, vscode ATM is performing client-side formatting (I verified that it is also getting \t which is converted to 4 spaces). I am not sure on what part of the spec this PR is based on.

I think they're still on discussion about that microsoft/language-server-protocol#83.
And yes, VsCode is doing magic indentation on client side.
Again for tabs/spaces conversion I can do a simple conversion here so we will have a fairly quick indentation function that doesn't affect org-mode performance so we still have snippets indetented on lsp-org. @yyoncho WDYT?

Also I wonder if range formatting in jdtls overwrite spaces with tabs?
There was an issue on java code fixes redhat-developer/vscode-java#557 (comment) that mentions code fixes force tab for indentation and they need to change server prefs so server can return the correct indentation. Which I think is reasonable change, given that server will use tabs/spaces for a lot of other place (formatting, quick fixes, text edit ...) than just snippets.

[yyoncho]

performance so we still have snippets indetented on lsp-org. @yyoncho WDYT?

It will be an improvement for org-mode integration.

Which I think is reasonable change, given that server will use tabs/spaces for a lot of other place (formatting, quick fixes, text edit ...) than just snippets.

Most likely we should fix formatting after applying text edits as well. FWIW vscode hasn't implemented the client-side formatting for snippets only for JDTLS. It is very likely that there are other servers relying on this undocumented behaviour.

[kiennq]

Most likely we should fix formatting after applying text edits as well.

I'm a little bit skeptical about this, as formatting in elisp is fairly slow, and the indentation may completely different from what language server suggested.
Especially for some old mode (example : c++-mode) that not aware of new syntax / define and will indent code differently/incorrectly.

[yyoncho]

Most likely we may use rangeFormattingProvider if present, but yes, we should try to avoid elisp formatting.

[kiennq]

If we use rangeFormattingProvider, I think this PR alone would be enough. Since server returned snippet should have same format with rangeFormattingProvider.
Anyway, I've pushed a new patch that will properly convert tab/spaces depends on client configuration. Can you help checking?

[yyoncho]

If we use rangeFormattingProvider, I think this PR alone would be enough. Since server returned snippet should have same format with rangeFormattingProvider.

According to microsoft/language-server-protocol#221 and microsoft/language-server-protocol#83 the default will be relativeIndent = off but in this PR relative indentation is promoted as the default behaviour. Can you point me a scenario with real language server in which the proposed behaviour is better than the 'auto behaviour of yasnippet?

[kiennq]

According to microsoft/language-server-protocol#221 and microsoft/language-server-protocol#83 the default will be relativeIndent = off but in this PR relative indentation is promoted as the default behaviour.

From what I see in https://github.com/microsoft/language-server-protocol/pull/221/files, the default behavior is disableRelativeIndent?: boolean, that's means the relative indentation is default behavior.
Another code in https://github.com/Microsoft/vscode/blob/25589f6cc4aa3450450fa0f49979fcf7835f52d4/src/vs/vscode.proposed.d.ts#L16-L39 also state that KeepWhitespace (relative indentation off) is off by default.

Can you point me a scenario with real language server in which the proposed behaviour is better than the 'auto behaviour of yasnippet?

The auto behavior of yasnippet depends on indent-line-function to indent each inserted new line. Thus it's completely major mode indentation (lisp code) that taking effect.
While I'm not being able to pin point to a real language server that may cause problem (partly due to major-mode are quite good already and server that returns snippets are not that common), the proposed approach is

• has better perf
• be able to work even for org-mode

Here is some benchmark results

(benchmark-run 1000
  (with-temp-buffer
    (yas-minor-mode +1)
    (let ((yas-indent-line 'auto)
          (indent-line-function 'rust-mode-indent-line))
      (yas-expand-snippet "match list {\n    {1:_}: {0},\n}" 1 1))))
(0.308898 0 0.0)

(benchmark-run 1000
  (with-temp-buffer
    (yas-minor-mode +1)
    (let* ((inhibit-field-text-motion t)
           (offset (save-excursion
                     (goto-char 1)
                     (back-to-indentation)
                     (buffer-substring-no-properties
                      (line-beginning-position)
                      (point))))
           (yas-indent-line 'auto)
           (yas-also-auto-indent-first-line nil)
           (indent-line-function (lambda () (save-excursion
                                              (forward-line 0)
                                              (insert offset)
                                              (if indent-tabs-mode
                                                  (tabify (line-beginning-position) (line-end-position))
                                                (untabify (line-beginning-position) (line-end-position)))))))
      (yas-expand-snippet "match list {\n    {1:_}: {0},\n}" 1 1))))
(0.208318 0 0.0)

[yyoncho]

While I'm not being able to pin point to a real language server that may cause problem

I can point servers with problems with proposed implementation - for example, replacing \t with spaces won't be sufficient since indentation in cc-mode based languages like java-mode is controlled by c-basic-offset, not by tab-width. After all, the servers expect the indentation to be performed according to major mode and we cannot replace it relative indentation and expect that to work. We can keep it for org-mode but I don't see this as an improvement for general usage (performance gain is negligible since it is one-time operation). After all, #1805 is about covering lsp-mode - vscode discrepancies and 'auto seems to be a better match for that - or at least there is no evidence indicating the opposite.

[kiennq]

After all, the servers expect the indentation to be performed according to major mode and we cannot replace it relative indentation and expect that to work.

I don't think this is entirely correct, since the server has settings itself to indicate the format.
In lsp-mode we even have lsp-enable-indentation to indent regions using the file formatting functionality provided by the language server.

If we have server formatting functionality, I can't see why we shouldn't use server relative indentation here in this case too.
I can add a check to only enable relative indentation if server has formatting capability and lsp-enable-indentation is on, WDYT?

[kiennq]

@yyoncho I found a problem in real language server (clangd-10.0.0) that the current behavior (using Emacs indentation instead of relative indentation) not working.
Here is an example:

The cases and following bracket is wrongly indented.
With proposed relative indent, that problem is not happen.
The snippet in this case is switch (${1:condition}) {\n${0:cases}\n}.

Also, more information in this microsoft/language-server-protocol#83 (comment) and microsoft/language-server-protocol#880 (comment), it seems that new property keepWhitespace will be introduced in next LSP release.

Come back to my proposal, do you think it would be ok to use relative indentation when:

• server supports formatting
• lsp-enable-indentation is on
• keepWhitespace is nil (default behavior)

[yyoncho]

• server supports formatting
• lsp-enable-indentation is o

Can you elaborate on that? I would like to avoid calling the server after expanding the server. Although, we may use the same strategy as the additionalTextEdits but IMO it will be like too much. Especially, with the poor primitives for async programming that we currently have. If we start using deferred/promises it will be easier to compose async calls.

[kiennq]

I mean this kind of check

(and lsp-enable-indentation
                                            (or (lsp-feature? "textDocument/rangeFormatting")
                                                (lsp-feature? "textDocument/formatting")))

[yyoncho]

I mean this kind of check

(and lsp-enable-indentation
                                            (or (lsp-feature? "textDocument/rangeFormatting")
                                                (lsp-feature? "textDocument/formatting")))

I understand the checks but I do not understand what we will do in then/else clauses

[kiennq]

I understand the checks but I do not understand what we will do in then/else clauses

If the check passed, we will use relative indentation as in https://github.com/microsoft/vscode/blob/master/src/vs/vscode.d.ts#L3865-L3869, since the server can provide format.

If the check is not passing, we fallback to Emacs indentation function (same with setting yas-indent-line to auto and not tinkering with indent-line-function)

[yyoncho]

If the check passed, we will use relative indentation as in

I do not like this heuristic. Some server like intelephense allow user defined
snippets and I doubt that the server is that smart to indent client defined
snippets server side.

ATM both relative and 'auto do not cover all of the cases and fail to handle
snippets. The issue with clangd is that c++-mode fails to handle invalid C++ code.

[kiennq]

I do not like this heuristic. Some server like intelephense allow user defined
snippets and I doubt that the server is that smart to indent client defined
snippets server side.

Then that's server side issue, isn't it?
The relative indentation is quite powerful, I think it will work even for this case.

ATM both relative and 'auto do not cover all of the cases and fail to handle
snippets.

Which case it doesn't cover?

The issue with clangd is that c++-mode fails to handle invalid C++ code.

It's because we're using c++-mode indentation function, thus it failed to handle those snippets.
Again, the relative indent is working fine for this case.

My main argument is that snippets are normally defined with correct indentation already, but since we're applying snippets with a certain level of offset, we need to readjust it.

Using client side indentation is mostly wasteful, and sometimes may not working well.
We've already provide formatting functionality by server, I don't see why we don't trust server with its snippet indentation too.

[yyoncho]

kiennq writes:

Then that's server side issue, isn't it?

No, it is not - the formatting settings are passed when you call textDocument/format* methods so there is **no** way for the server to guess them and format the snippet properly because it doesn't know what are the client settings. So, it is guaranteed that the server will provide wrongly formatted snippets unless there is some magical/language spefic way to find the indent size, right?

Which case it doesn't cover?

Relative indentation does not work with JDTLS as I described in previous comments. And generally, relative indentation will **always** fail ATM just do ``` (setq tab-width 2) ``` ``` (setq tab-width 4) ``` and expand the snippet 2 times.

[kiennq]

Ok, that makes sense.
However, for mode language server that provides alternative way to format settings (ex: .clang-format for clangd), I still think that using relative indentation is better. At least, language server has more information insight about language than any major mode.

How about this, I will create a local variable that user can set (via :after-open-fn) to allow using relative indentation if they want. That would provide maximum flexibility. Also relative indentation will become default for org-mode. WDYT @yyoncho ?
Note that the tabify/untabify code has been removed.

[yyoncho]

Ok, let's have that as an optional feature and later bind it to the new property in CompletionItem.

How about this, I will create a local variable that user can set (via :after-open-fn)

We should rather introduce server-specific hook e. g. lsp-jdtls-after-open-hook

[kiennq]

We should rather introduce server-specific hook e. g. lsp-jdtls-after-open-hook

Can you elaborate more on this?

[yyoncho]

I mean that if we do something like:

(run-hooks (intern (format "lsp-%s-after-open-hook" 
                           (lsp--client-server-id (lsp--workspace-client workspace)))))

And then use that hook we will allow clients to turn off/on a feature. If we use after-open-fn users will have to modify internal structure to achieve what they want.

[kiennq]

I mean that if we do something like:

(run-hooks (intern (format "lsp-%s-after-open-hook" 
                           (lsp--client-server-id (lsp--workspace-client workspace)))))

And then use that hook we will allow clients to turn off/on a feature. If we use after-open-fn users will have to modify internal structure to achieve what they want.

Interesting, do you have any suggestion how can we define that hook?
Manually adding new hook for all clients seems a bit too much changes, I'm thinking of either changing lsp-register-client to become macro with defvar inside of it, or manually make a new hook symbol via intern and set properties to it so it can showing up documentation.

[yyoncho]

Interesting, do you have any suggestion how can we define that hook?

FTR it will work without defining it.

(run-hooks 'anything)

works just fine.

Changing lsp-register-client to be a macro is an old idea, we should do it at some point. And not only about the hook but priority, binary name + args(so we can unify it), etc. But IMO better to introduce a new method or keep the method and call the macro inside of it otherwise we will break the binary compatibility and again that will cause a lot of questions/issues.

[yyoncho]

Thinking more about that it is better not to have that as a macro at all. This means that every time we change the macro the code that is bytecompiled agaist that macro will break... Also, when users update all packages package.el does not update the packages in the correct order(e. g. from the root package to the other). This again will cause breakages.

[kiennq]

@yyoncho Done, I've added new hooks for every client that use lsp-register-client

[yyoncho]

Looks good!

We should probably think of a way to mark the in which the property was added to the protocol. Because atm we have some experimental one.

[yyoncho]

This is a bit weird structure - can you refactor to (let ((client ..) and then the lambda block?

[kiennq]

Ok