emacs-lsp / lsp-mode

Emacs client/library for the Language Server Protocol
https://emacs-lsp.github.io/lsp-mode
GNU General Public License v3.0
4.76k stars 874 forks source link

textDocument/onTypeFormatting is seriously interfering with major mode indentation. #1971

Open kiennq opened 4 years ago

kiennq commented 4 years ago

A simple snippet to demonstrate problem using clangd with

(setq lsp-clients-clangd-args
        `("--fallback-style=Microsoft" "--clang-tidy" "--background-index" "-header-insertion-decorators=0"))
int main()
{
    if (1 < 2) {<|>
    return 0;
}

The <|> is cursor position. Now if we press Enter, the onTypeFormatting is kicked in and resulted in something like

int main()
{
    if (1 < 2)
    {
                <|>
    return 0;
}

Note that the cursor position on the next line is over-indented. The server trace

[Trace - 12:33:40 AM] Received response 'textDocument/onTypeFormatting - (128)' in 12ms.
Result: [
  {
    "newText": "\n    ",
    "range": {
      "end": {
        "character": 15,
        "line": 2
      },
      "start": {
        "character": 14,
        "line": 2
      }
    }
  },
  {
    "newText": "\n        ",
    "range": {
      "end": {
        "character": 0,
        "line": 3
      },
      "start": {
        "character": 16,
        "line": 2
      }
    }
  }
]

So you can se that the second text edit from server already contained indenting information, plus the indentation done by indent-line-function, that makes the new line is doubled indented.

yyoncho commented 4 years ago

Seems like we are applying text edits that are incompatible with the already applied changes to the buffer. We may consider changes on the current line as something incompatible(or check if (> end first-edited) shouldn't be changed to (>= end first-edited). We may also revert and keep on type formatting as tick.

yyoncho commented 4 years ago

I also wonder in general, what is the correct way to handle this case, becase apparently the same problem will exist in vscode as well.

yyoncho commented 4 years ago

I think that it should be better to rever the latest changes related to onTypeFormatting until we sort that out - soon we will start getting issue reports for that.

kiennq commented 4 years ago

Ok, I will put it back to tick. Before we never applied on type formatting on Enter since the text will be changed by indent-line-function. The latest change on onTypeFormatting allowing text to be applied, since it's compatible (really, the indent-line-function change text after \n, while the onTypeFormatting change text before and at \n).

yyoncho commented 4 years ago

Ok, I will put it back to tick. Before we never applied on type formatting on Enter since the text will be changed by indent-line-function. The latest change on onTypeFormatting allowing text to be applied, since it's compatible (really, the indent-line-function change text after \n, while the onTypeFormatting change text before and at \n).

Yes.