joaotavora / eglot

A client for Language Server Protocol servers
GNU General Public License v3.0
2.27k stars 200 forks source link

The use of `tab-width` in `eglot-format` #157

Closed casouri closed 5 years ago

casouri commented 5 years ago

https://github.com/joaotavora/eglot/blob/604c1b0c31f7202f83373dd97f620dbc2dddfa52/eglot.el#L1510

According to the documentation of tab-width:

NOTE: This controls the display width of a TAB character, and not the size of an indentation step. This should be an integer greater than zero.

I think something else should be used for indentation.

For example, js-indent-level for javascript, python-indent for python.

I can look into this if I find some time, but I can't promise anything.

terlar commented 5 years ago

You can check the editorconfig package to see which modes goes to which indent variables: https://github.com/editorconfig/editorconfig-emacs/blob/master/editorconfig.el#L141

Also JS can be either js2-mode or js-mode etc. It is quite intricate.

joaotavora commented 5 years ago

But the tabSize property in the spec also doesn't seem to be the size of an indentation step. It's, according to https://microsoft.github.io/language-server-protocol/specification#textDocument_formatting, the "size of a tab stop in spaces".

This is very close to Emacs' "size of a tab stop in columns". It's basically the same.

casouri commented 5 years ago

Although looking similar, I do think they mean different things:

tab-width in Emacs denotes to "size of a tab stop". A tab stop is not like tabs used in programming:

Quoting practical typography:

On typewriters, the tab key moved the carriage to a fixed horizontal position, marked with a tab stop. This allowed typists to create columns of text or numbers, also known as tabular layouts (hence the name tab).

Using tab in fundamental-mode:

hi      good    nihao   pepper  looong  short   ii      align
good    nihao   loooong short   align   ii      hi      bye

As you see, they are aligned to "tab stops", which are 8 columns apart from each other. As the doc says "it's not indent step".

Other the other hand, since LSP is all about programming rather than page layout, I think it is safe to assume that they mean "size of a tab (not tab stop) in spaces" by "how many spaces does a tab translates to". That is what variables like js-indent-level and python-indent used for.

mkcms commented 5 years ago

@casouri Do you know if any servers actually use this information? I know that clangd uses settings from .clang-format file and ignores this, pyls uses autopep8 or yapf settings, eclipse server uses it's own. I haven't tested this much, so maybe I'm wrong.

joaotavora commented 5 years ago

@casouri

As you see, they are aligned to "tab stops", which are 8 columns apart from each other. As the doc says "it's not indent step".

I don't think I said that anywhere anything to the contrary.

That is what variables like js-indent-level and python-indent used for.

It is possible that you are mixing up two different things.

python-indent and js-indent-level or c-basic-offset have nothing to do with tab, or tab-width.

Here is an example. You can quite safely edit a C with c-basic-offset = 4 and tab-width=256, for example

void main (){
    printf("Hello");    printf(", world");
}
/* Local Variables: */
/* c-basic-offset: 4 */
/* tab-width: 256 */
/* End: */

Looks like this:

image

The TAB character occupies 1 byte, but is is displayed as (slightly less) than 256 columns, and that is why you don't see the second printf unless your screen is very wide.

When you try to edit (or reformat) that file with LSP, it is important that LSP knows that your meant 256 columns so that it can insert (slightly less) than 256 spaces in case you provided also the insertSpaces option set to true.

casouri commented 5 years ago

@joaotavora Right, I am mixing the two. I assumed tabSize in LSP means indent step, while you consider it the width of a tab in spaces. I think your decision is correct. (use tab-width for tabSize)

Now I think the spec has some problems: It assumes the indent level to be a tab — if insertSpace turned on, tabSpace × space.

I think they should add an indentLevel property. What do you think? If you guys think this is a good idea, I'll go and open an issue on lsp repo.


@mkcms typescript language server uses it. and my indent level becomes 8 spaces after I reformat.

joaotavora commented 5 years ago

Now I think the spec has some problems:

Now we're getting somewhere... :wink:

casouri commented 5 years ago

I asked LSP maintainer, here is his response: Microsoft/language-server-protocol#610.

Basically:

  1. tabWidth is indentation step
  2. LSP doesn't spec what does the backend do regarding a tab in the middle of a line.
joaotavora commented 5 years ago

@casouri

  1. I think the "we won't spec this" in the maintainer's response is the right approach. I.e. _do_provide a way for tabWidth to be communicated to the server, but don't be too strict on what the server is supposed to do with it. I think this is the right approach because spec'ing it would be hard, not impossible, and it's better to underspec than misspec.

  2. On the other hand, I think that mixing up tab width with indentation width is a huge blunder.

There's the possibity to look at the problem this way: tabWidth is just a really bad name for indentationWidth. In that case Eglot still won't add any logic to put mode-specific values in that field:

casouri commented 5 years ago

In that case Eglot still won't add any logic to put mode-specific values in that field

That's the right thing to do.

You can ask Emacs to add a uniform interface for that. Then Eglot could use that interface

I can report a bug on that. But that's not going to be picked up anytime soon (I think).

You can something in eglot--managed-mode-hook to set tabWidth to some uniform value. If your server is going to mess up your tabs, you probably won't be using M-x tabify and such anyway.

I can write a simple function to return the indentation step of the current major mode. I'm not very comfortable on this, tho, because tab-width is /not/ indentation step.

We could add a third variable eglot-indentation-step, but I'm not really convinced yet.

Maybe we can add it but set it to nil by default. And only let it override tab-width if it is set? That way I can use the previous method and stop worrying about side effects of changing tab-width.

Come thing of it, it is kind of like "a uniform interface", it's just I'm providing it instead of Emacs, and it only works for eglot.