Format on type adds bracket incorrectly

[Waqar144]

Hi,

I am not a rust developer or a active rust-analyzer user but I work on a lsp client (Kate editor) and we got this bug report: https://bugs.kde.org/show_bug.cgi?id=452047

Consider the following simple rust code:

fn main() {
    |println!("hello");
}

| indicates caret position. If you have "Format on Type" enabled and ou press backspace enought times so that the lines wraps into the previous line and touches the { of the previous line, rust-analzyer suggests to add a bracket at the end of line resulting in

fn main() {|println!("hello");}
}

This led to an infinite loop in Kate (which has been fixed), but still it doesn't seem right that the LSP suggests to a bracket in that place.

rust-analyzer version: rust-analyzer bc08b8e 2022-03-28 stable

rustc version: rustc 1.51.0 (2fd73fabe 2021-03-23)

[nemethf]

Do I understand correctly that the user only types backspaces in the example? If so, the LSP client should never send a textDocument/onTypeFormatting request. According to the specification the client can send this request only after the user types a trigger character. Backspace is not a trigger character for rust-analyzer. At least that's my understanding. Nevertheless, looking at the code of rust-analyzer it seems it does nothing if the char_typed is not in the trigger_chars.

[Waqar144]

we send formatting request because of the preceding {, not backspace. The server should be able to handle this correctly in my understanding.

[nemethf]

So what's the ch parameter of the onTypeFormatting request?

[Waqar144]

it is {

[lnicola]

From https://microsoft.github.io/language-server-protocol/specifications/specification-current/#textDocument_onTypeFormatting:

The character that has been typed.

Unless I'm missing something, I wouldn't interpret deleting an indent as typing the opening brace on the previous line.

[Waqar144]

Perhaps you are right, but shouldn't the server just do the right thing in any case? The server has no idea whether the user typed or not, it has to respond correctly to a request or ignore it if it can't

[lnicola]

So if I have

fn main() <|>println!("hello");
}

Type a { yields:

fn main() {println!("hello");}
}

, which might not be ideal, but appears to work as intended (it surrounds with braces the current expression). I could potentially go on and type if true { and get:

fn main() { if true {println!("hello");}
}

, which is fine (though I'm not sure why it doesn't add another } this time).

The server has no idea whether the user typed or not

Doesn't textDocument/onTypeFormatting mean the user has typed something? What does it mean in Kate?

[flodiebold]

Maybe we should check whether the braces were balanced before inserting another one.

[Waqar144]

, which might not be ideal, but appears to work as intended (it surrounds with braces the current expression).

Since I am no rust expert I can't say whether that is correct or not but my general programming instinct says the brackets should stay balanced.

Doesn't textDocument/onTypeFormatting mean the user has typed something? What does it mean in Kate?

It means the same, though in Kate any "text change" goes through the onTypeFormatting code path and a request gets sent if the character preceding the current cursor position is a trigger char. My point is that whether the user has typed is a client side thing, the server cannot rely on this or any particular "typing behaviour" since for the server its just any other request and thus it should be more robust in handling this imo.

[lnicola]

I think you want onTypeFormatting to be idempotent, so sending the same request multiple times does not result in further edits.

In a quick test, in Code onTypeFormatting actually means that the user has typed something. It's not sent on paste (even if you paste a single character), and it's not sent when an edit touches a trigger character (does removing the a in {a send onTypeFormatting in Kate?).

Kate seems to have a different interpretation of it than what the spec seems to imply and what Code (and presumably other clients) does. We might still improve this feature to keep braces balanced in more cases, but if my (limited) understanding of LSP is correct, Kate appears to be non-compliant.

[Veykril]

Kate seems to have a different interpretation of it than what the spec seems to imply and what Code (and presumably other clients) does. We might still improve this feature to keep braces balanced in more cases, but if my (limited) understanding of LSP is correct, Kate appears to be non-compliant.

The docmentation actually doesn't say much about that if I see this right, https://microsoft.github.io/language-server-protocol/specifications/specification-current/#textDocument_onTypeFormatting
Though to me the behaviour of Kate sounds wrong.

Nevertheless balancing the braces sounds unfeasible as the on typing handler has to be very fast since it is synchronous. So going through the entire file to check for balancedness is way too slow, unless I am missing something obvious here.

[lnicola]

The docmentation actually doesn't say much about that if I see this right

Well, if I have {a, select the a and type b, I'd expect an onTypeFormatting request for b, not for { just because my edit was adjacent to it. The request has a single character, and it's supposed to have been "typed".

[Veykril]

Right, I was more focusing on deleting a character as thats where Kate invokes it if I understood this right. But ye replacing a selection of chars is the same case, which you are also right, it shouldn't invoke the handler at all either.

[Waqar144]

does removing the a in {a send onTypeFormatting in Kate?).

Haven't tried it, but most probably yes.

Also, I have opened a parallel discussion in Kate about this issue.

Nevertheless balancing the braces sounds unfeasible as the on typing handler has to be very fast since it is synchronous

The way we do balancing in Kate is by providing an upper limit of lines to check (which is 250 I think). This gives reasonable performance and we haven't had any bugs about it in the past 20 years.

Well, if I have {a, select the a and type b, I'd expect an onTypeFormatting request for b, not for {

It depends on the editor. In Kate for e.g, this is a remove + insert behind the scenes. So it could be that you get two onTypeFormatting requests, one for "{" and one for "b" (assuming both are trigger chars)