Stream: t-compiler/wg-rls-2.0

Topic: Enhanced typing


std::Veetaha (Feb 19 2020 at 20:46, on Zulip):

I think this is a bug. I don't get automatic /// when I type enter after an empty line in the middle of the comment.
bug.gif

Laurențiu Nicola (Feb 19 2020 at 20:59, on Zulip):

You need to move the cursor to the right.

Laurențiu Nicola (Feb 19 2020 at 20:59, on Zulip):

It works after /// , but not after ///

Laurențiu Nicola (Feb 19 2020 at 21:04, on Zulip):

https://github.com/rust-analyzer/rust-analyzer/blob/master/crates/ra_ide/src/typing.rs#L49-L52 this check seems suspicious

Laurențiu Nicola (Feb 20 2020 at 07:40, on Zulip):

The one above it, actually

std::Veetaha (Feb 20 2020 at 09:04, on Zulip):

Yeah, I wonder why we add +1 there...

std::Veetaha (Feb 20 2020 at 09:06, on Zulip):

PR is called Don't continue line comments, @matklad maybe you can tell?

Laurențiu Nicola (Feb 20 2020 at 09:06, on Zulip):

Well, the prefix is ///, so the condition looks all right to me because it's using <

Laurențiu Nicola (Feb 20 2020 at 09:06, on Zulip):

I tried to remove the + 1 and it works, but inserts an extra space on the next line

Laurențiu Nicola (Feb 20 2020 at 09:07, on Zulip):

(which is normal, but annoying)

Laurențiu Nicola (Feb 20 2020 at 09:08, on Zulip):

The code below adds the prefix and a space, so I guess we should only add the space if the part to the right of the cursor doesn't start with a space

std::Veetaha (Feb 20 2020 at 09:09, on Zulip):

I don't think this is annoying. Writing comments without putting spaces after /// is generally a bad practice

Laurențiu Nicola (Feb 20 2020 at 09:10, on Zulip):

That's not what I meant. If I press enter here: ///<|> hello, the added line will say /// <|> hello because it copies over the text to the right of the cursor but also adds the space. It should say /// <|> hello or ///<|> hello. This is after removing that + TextUnit::from(1), of course.

std::Veetaha (Feb 20 2020 at 09:10, on Zulip):

An annoyance that I see is that on_enter bails when you don't have that space after /// which is not rare when you have "files.trimTrailingWhitespace": true,

std::Veetaha (Feb 20 2020 at 09:12, on Zulip):

Laurențiu Nicola said:

That's not what I meant. If I press enter here: ///<|> hello, the added line will say /// <|> hello because it copies over the text to the right of the cursor but also adds the space. It should say /// <|> hello or ///<|> hello.

Hmm, that is controversial

std::Veetaha (Feb 20 2020 at 09:13, on Zulip):

Actually typescript on_enter does the same

std::Veetaha (Feb 20 2020 at 09:14, on Zulip):

When you press enter in doc comment it does unconditionally add that space

Laurențiu Nicola (Feb 20 2020 at 09:14, on Zulip):

TypeScript has block comments? :sweat_smile:

Laurențiu Nicola (Feb 20 2020 at 09:14, on Zulip):

Yes, it does

std::Veetaha (Feb 20 2020 at 09:14, on Zulip):

They are not in ECMAScript standard. It is a pervasive convevntion to write

/**
 *
 */
std::Veetaha (Feb 20 2020 at 09:15, on Zulip):

And I do like this style of comments more, Rust even supports it...

Laurențiu Nicola (Feb 20 2020 at 09:17, on Zulip):

Ah, actually I think it's correct to remove it. Sorry, bit tired. offset < start + len means that the cursor is inside the prefix. They'll be equal when the cursor is on ///<|>

std::Veetaha (Feb 20 2020 at 09:19, on Zulip):

Sure, tho let's get an approval from the author of this function anyway

std::Veetaha (Feb 20 2020 at 22:46, on Zulip):

Also, one more flaw that I have noticed is that auto comment insertion doesn't work with multi-cursors:

std::Veetaha (Feb 20 2020 at 22:48, on Zulip):

multi-cursor-enter.gif

std::Veetaha (Feb 20 2020 at 22:50, on Zulip):

This is how it works in TS for comparison:
multi-cursor-enter-ts.gif

std::Veetaha (Feb 20 2020 at 22:56, on Zulip):

I'll try to look at TypeScript implementation and see what we can do here

std::Veetaha (Feb 20 2020 at 22:56, on Zulip):

Because enhanced typing has aggregated quite some controversy already...

std::Veetaha (Feb 20 2020 at 23:11, on Zulip):

This is where they do it

std::Veetaha (Feb 20 2020 at 23:11, on Zulip):

This happens not on lsp side...

std::Veetaha (Feb 20 2020 at 23:15, on Zulip):

@matklad , should we do the same on our TypeScript extension? Since I heard LSP doesn't support pushing edits anyway... Otherwise users have a lot of problems with our custom on_enter implementation (one, two)...

Laurențiu Nicola (Feb 20 2020 at 23:28, on Zulip):

That's quite nifty

matklad (Feb 20 2020 at 23:28, on Zulip):

We shall not use regular expressions for parsing Rust.

Laurențiu Nicola (Feb 20 2020 at 23:29, on Zulip):

It won't work in raw strings, but it's not so bad otherwise

std::Veetaha (Feb 20 2020 at 23:37, on Zulip):

That is the API provided by VSCode, if you don't want native Regexp, we can implement our own since this is just an interface

std::Veetaha (Feb 20 2020 at 23:44, on Zulip):

@matklad , RLS does the same thing...

matklad (Feb 20 2020 at 23:51, on Zulip):

I do strongly believe that using regular expressions in any capacity for analyzing a programming language is a step back. if we can't user the proper technology (incremental parser), we should push for removing the blockers for using the proper technology, and not implement a workaround.

That's a tad too idealistic, and I agree that, to derive the most value to the most users now we should just go with what's default for VS Code (and, for example, completely ignore syntax highlighting until it is stabilized). I don't want to do that partially just because it goes against my personal value, and paritally becaues I think pushing for the right thing will lead to the more value overall.

matklad (Feb 20 2020 at 23:55, on Zulip):

Like, that's literary the original story behind rust-analyzer:

I've implemented the "Run this test" in RLS. I had to use regular expression, because I didn't have access to the parser. As I had a half-written Rust parser for libsyntax2 at that moment, I've decided that it would be a good use of my time to push that parser to 80% ready state and to add an LSP on top of it¸just so that I can have one-shortcut test running functionality, based on a real parser:

https://github.com/rust-analyzer/rust-analyzer/commit/57518153147ad53639f16cc940d219dc582c550a

matklad (Feb 21 2020 at 00:01, on Zulip):

Hm, I think the relevant commit is https://github.com/rust-analyzer/rust-analyzer/commit/36bd28633baf6015b767e9e70d2d53185271db50

Don't remember how they came along actually. Just re-learned that ruts-analyzer actually started as a native plugin for NodeJS, and not as a language server

std::Veetaha (Feb 21 2020 at 00:09, on Zulip):

What is libsyntax2?

std::Veetaha (Feb 21 2020 at 00:10, on Zulip):

Anyway, I won't go against anybody's personal value...

std::Veetaha (Feb 21 2020 at 00:25, on Zulip):

Another option would be to listen for OnDidChangeTextDocument event, but this is emitted when the change is already applied... I'll try to see what we can do with this once I have time... Also note that multicursor support should be added, but later after we fix current issues

std::Veetaha (Feb 24 2020 at 08:53, on Zulip):

I am genuinely stuck. We cannot implemented onEnter through OnDidChangeTextDocument event, since it is emitted when the document already contains the inserted newline character, moreover VSCode automatically prepends some whitespace for auto indentation which doesn't help at all...

std::Veetaha (Feb 24 2020 at 08:54, on Zulip):

Moreover it is async, so that the user may outtype our server in theory

std::Veetaha (Feb 24 2020 at 08:56, on Zulip):

I looked over the native vscode source that deals with those regexps and they do it using private access to DOM, since extension host has no direct access to it

std::Veetaha (Feb 24 2020 at 09:03, on Zulip):

Another way to fix this would be to stay using the keybinding approach (or overriding the "default:type" command) and adding workarounds for each reported bug in isolation

std::Veetaha (Feb 24 2020 at 09:06, on Zulip):

(By the way, even our current approach uses async code so that the onEnter handler is not reentrant in theory, which may cause data races)

Laurențiu Nicola (Feb 24 2020 at 09:30, on Zulip):

My "vote" would go towards the regex solution. It's less elegant, but is better integrated with the editor, and should work fine most of the time (e.g. even if you're typing a doc comment in a raw string, maybe you actually want the comment to be automatically extended). We're not parsing the full language, only recognizing comments. We already have a bunch of heuristics for other things. And, uh, you can parse HTML with regexes :slight_smile:.

std::Veetaha (Feb 24 2020 at 10:29, on Zulip):

@matklad , what if we copy the regexeps setup from the RLS source, keep our on_enter implementation in typing.rs handler for better times and create an issue at VSCode to request a better support for our use case?
This way we fix the bugs and even get the feature of multiple cursors and support for star-like block comments, i.e.:

/**
 *
 */

which do not work with our impl now...
I am very sorry to ask you for this once again, though I am not standing for this...

matklad (Feb 24 2020 at 10:36, on Zulip):

I'd rather not add regexes .

matklad (Feb 24 2020 at 11:18, on Zulip):

More generally, all language specific language should be implemented on the server side.

std::Veetaha (Feb 24 2020 at 11:33, on Zulip):

Yeah, this is in ideal world, real world is complicated...

Jeremy Kolb (Feb 24 2020 at 12:50, on Zulip):

Custom LSP request?

Jeremy Kolb (Feb 24 2020 at 12:50, on Zulip):

though i think we have that for on enter

Florian Diebold (Feb 24 2020 at 13:18, on Zulip):

is it really impossible to handle this asynchronously? wouldn't it be possible to insert whatever RA returns even if the user has typed a few more characters since then?

Florian Diebold (Feb 24 2020 at 13:19, on Zulip):

it seems to me that that + doing nothing on error would alleviate most problems

Laurențiu Nicola (Feb 24 2020 at 13:19, on Zulip):

I think VS extensions run asynchronously, but I'm not sure if they can do edits like this. The issue is that you need to handle conflict resolution with later edits from the user.

Florian Diebold (Feb 24 2020 at 13:19, on Zulip):

(+ some timeout, say 100ms, after which it doesn't really make sense to still insert it)

Florian Diebold (Feb 24 2020 at 13:20, on Zulip):

The issue is that you need to handle conflict resolution with later edits from the user.

I know, what I'm saying is that maybe that's not really too hard in this specific case?

Laurențiu Nicola (Feb 24 2020 at 13:21, on Zulip):

Maybe "don't do anything if the user typed something", but can we even detect it?

Florian Diebold (Feb 24 2020 at 13:23, on Zulip):

maybe keeping track of the cursor position where the newline was inserted, and replacing that afterwards? Of course checking that it's still a newline at that time

Florian Diebold (Feb 24 2020 at 13:24, on Zulip):

I doubt users will be able to type anything that actually conflicts if we respond reasonably fast; usually they will type a few more letters after the newline

Laurențiu Nicola (Feb 24 2020 at 13:24, on Zulip):

I'm also worried about the response time when the system is busy, It's annoying enough that my mouse cursor lags when I'm compiling something, but to be fair that's a Gnome thing, and rust-analyzer's enhanced typing is relatively snappy even in those circumstances.

Florian Diebold (Feb 24 2020 at 13:25, on Zulip):

of course we could also just put a hard timeout of 16ms on the request, if it really has to be sychronous

Florian Diebold (Feb 24 2020 at 13:26, on Zulip):

I think the idea is that it'll always be very fast. It'll certainly be pretty useless if it's not, so we can just skip it in those situations. But if we do it synchronously, the typing would still be slowed down by our timeout

Laurențiu Nicola (Feb 24 2020 at 13:26, on Zulip):

Agreed. Ideally it's going to be fast, but that's sometimes outside of our control.

Laurențiu Nicola (Feb 24 2020 at 13:31, on Zulip):

I can't find it, but there's an older issue about onEnter where somebody linked a related Code issue about extensions slowing down typing like this.

Laurențiu Nicola (Feb 24 2020 at 13:32, on Zulip):

https://github.com/rust-analyzer/rust-analyzer/issues/2331#issuecomment-556993009

Laurențiu Nicola (Feb 24 2020 at 13:33, on Zulip):

That's pretty bad, if true

matklad (Feb 24 2020 at 13:36, on Zulip):

is it really impossible to handle this asynchronously?

In theory, you can do very cool stuff here with operational transofmation, where server-generated edits race with user-initiated ones. In practice, I've just fixed two huge bugs with this feature, where we've blocked onEnter altogether if there were server errors, and that should alleviate most of the problems I think

Laurențiu Nicola (Feb 24 2020 at 13:37, on Zulip):

@matklad what do you think about DanTup's comment?

Laurențiu Nicola (Feb 24 2020 at 13:39, on Zulip):

that also means that any other extensions that have code handlers firing from that key press

Not only that, but I imagine it could be any handler running in a different extension

matklad (Feb 24 2020 at 13:41, on Zulip):

Could our new infra with onEnter keypress alleviate this issue?

matklad (Feb 24 2020 at 13:43, on Zulip):

Ultimately, I think this is an editor issue. VS Code simply does not have a proper architecture for synchroneous semantic things, like join/split lines, indentation-aware caret placement, etc. Ideally, it should be handled by either allowing truly synchronous actions, or by using OT-style async edit.

matklad (Feb 24 2020 at 13:44, on Zulip):

For the time being, I am not too worried about that: as long as we do the right thing on the server, we should be able to catch up with whatever editor advencement happen

Laurențiu Nicola (Feb 24 2020 at 13:44, on Zulip):

Do you mean #3289?

Laurențiu Nicola (Feb 24 2020 at 13:46, on Zulip):

I don't think so. After all, when there's no onEnter handler installed, Code can do its regex thing synchronously. If even a single extension sets one up, the editor will have to wait after the extension host to process everything. I wouldn't expect things to improve soon.

std::Veetaha (Feb 24 2020 at 13:51, on Zulip):

VSCode does place indentation even without our onenter

std::Veetaha (Feb 24 2020 at 13:51, on Zulip):

It just doesn't place comment prefixes automatically

std::Veetaha (Feb 24 2020 at 13:53, on Zulip):

Also I do have a problem on startup, it's mostly perceivable in debug build. Some time before the bootstrap is finished my enter key presses don't put newlines and after some lag time they appear all at once

Laurențiu Nicola (Feb 24 2020 at 13:54, on Zulip):

Yeah, I've seen that (in release builds)

matklad (Feb 24 2020 at 13:55, on Zulip):

VSCode does place indentation even without our onenter

It does it wrong

std::Veetaha (Feb 24 2020 at 13:55, on Zulip):

I agree this is an editor issue. VSCode decided to use process per extension approach and as less synchronicity as possible in order to keep the editor responsive disregarding bad designed extensions

Laurențiu Nicola (Feb 24 2020 at 13:58, on Zulip):

We measured the time taken for an ordinary (non-overridden) execution of the type command (which was roughly 20ms on my machine), vs. then calling 'default: type' command, which was always roughly 10x slower. Even though I am not familiar with the extensions API, this is puzzling, since these two calls should be doing exactly the same thing, yet calling executeCommand('default: type', ...) results in a much slower execution time of the default type command.

Laurențiu Nicola (Feb 24 2020 at 13:58, on Zulip):

Ugh.

std::Veetaha (Feb 24 2020 at 14:00, on Zulip):

Yeah, people do like to override the onInput handler https://github.com/microsoft/vscode/issues/65876

std::Veetaha (Feb 25 2020 at 19:41, on Zulip):

Tried setting this condition for on_enter keybinding:

"when": "editorLangId == rust && !suggestWidgetVisible && (!vim.active || vim.mode == 'Insert')"

But Vscode either doesn't support parentheses here or has a bug, since when I run the extension the condition get's transformed into an invalid expression:
image.png

std::Veetaha (Feb 25 2020 at 20:13, on Zulip):

Created an issue at vscode repo regrading that: https://github.com/microsoft/vscode/issues/91473

Laurențiu Nicola (Feb 25 2020 at 20:35, on Zulip):

So.. editorLangId == rust && !suggestWidgetVisible && !vim.active || editorLangId == rust && !suggestWidgetVisible && vim.mode == 'Insert'?

std::Veetaha (Feb 25 2020 at 20:50, on Zulip):

Yeah also thought about that, I guess this is the only possible workaround...

Laurențiu Nicola (Feb 25 2020 at 20:51, on Zulip):

But does it work?

std::Veetaha (Feb 25 2020 at 20:51, on Zulip):

Gonna try now

std::Veetaha (Feb 25 2020 at 20:55, on Zulip):

Yes it does. Though I am not 100% sure it will fix all vscode-vim issues. One user warned us that vim extension provides text searching in insert mode which should also not trigger the newline...

std::Veetaha (Feb 25 2020 at 20:58, on Zulip):

But I am a 0% vim user so won't bother that much...

Laurențiu Nicola (Feb 25 2020 at 21:04, on Zulip):

Ugh, how do I edit that condition? I tried to right-click and choose "Change When Expression", I can type (well, paste) it, but it doesn't seem to get committed on Enter

std::Veetaha (Feb 25 2020 at 21:05, on Zulip):

Try this image.png

Laurențiu Nicola (Feb 25 2020 at 21:07, on Zulip):

What's -rust-analyzer.onEnter?

Laurențiu Nicola (Feb 25 2020 at 21:08, on Zulip):

image.png

Laurențiu Nicola (Feb 25 2020 at 21:08, on Zulip):

Not sure what happened :sweat_smile:

std::Veetaha (Feb 25 2020 at 21:09, on Zulip):

That's weird. I suppose that minus prefix for the command name disables the keybinding

std::Veetaha (Feb 25 2020 at 21:11, on Zulip):

I have only one onEnter in my Default keybindnigs file (note that there are two of them: default an user-defined).

Laurențiu Nicola (Feb 25 2020 at 21:24, on Zulip):

Yeah, that's from my user-defined one

Laurențiu Nicola (Feb 25 2020 at 21:25, on Zulip):

I wanted to add editorTextFocus to that condition, but it doesn't seem to do much for the search case

Laurențiu Nicola (Feb 25 2020 at 21:27, on Zulip):

Also, this is very frustrating. I'm a mediocre Vim user, but the things I do in Vim don't work with this extension.

std::Veetaha (Feb 25 2020 at 21:27, on Zulip):

I already created a PR, or doesn't it actually work with the search mode?

Laurențiu Nicola (Feb 25 2020 at 21:28, on Zulip):

In the search mode. Have you tried it?

std::Veetaha (Feb 25 2020 at 21:29, on Zulip):

I barely managed to get to the insert mode from the normal mode, are you kiddin?

Laurențiu Nicola (Feb 25 2020 at 21:29, on Zulip):

lol

Laurențiu Nicola (Feb 25 2020 at 21:29, on Zulip):

Press /, then type the text to search

std::Veetaha (Feb 25 2020 at 21:31, on Zulip):

So this is why I couldn't write a comment...

std::Veetaha (Feb 25 2020 at 21:33, on Zulip):

Hmm, when I type kyrillic letters in normal mode it doesn't enter the insert mode...

std::Veetaha (Feb 25 2020 at 21:34, on Zulip):

But you can enter the search input only from normal mode, right? So it does work in this case...

Laurențiu Nicola (Feb 25 2020 at 21:34, on Zulip):

lol again

Laurențiu Nicola (Feb 25 2020 at 21:34, on Zulip):

You need to press i to go from normal to insert mode, then ESC to get back to normal mode

Laurențiu Nicola (Feb 25 2020 at 21:35, on Zulip):

/ for search only works in normal mode

std::Veetaha (Feb 25 2020 at 21:36, on Zulip):

Lol, when I type / character it types the character, but then deletes it and the latency is pretty noticeable (especially when you hold the key for some time)
vim-bizzare.gif

Laurențiu Nicola (Feb 25 2020 at 21:37, on Zulip):

What? Does it go from normal to insert mode automatically?

std::Veetaha (Feb 25 2020 at 21:37, on Zulip):

Yeah

Laurențiu Nicola (Feb 25 2020 at 21:37, on Zulip):

Try.. restarting Code? Or dunno. It's i on an US English keyboard, but I don't even know if you have that key...

Laurențiu Nicola (Feb 25 2020 at 21:38, on Zulip):

Maybe try changing your keyboard layout

std::Veetaha (Feb 25 2020 at 21:39, on Zulip):

Of course it does!

std::Veetaha (Feb 25 2020 at 21:39, on Zulip):

IMG_20200225_233818.jpg

Laurențiu Nicola (Feb 25 2020 at 21:40, on Zulip):

That's... honestly better than the non-QWERTY layouts I've had to use.

std::Veetaha (Feb 25 2020 at 21:49, on Zulip):

Anyway, I believe my amendment does work, since you can trigger the search only from the normal mode, right?

std::Veetaha (Feb 28 2020 at 20:48, on Zulip):

@matklad can't we use formatting on-type feature to implement the on-enter functionality?

matklad (Feb 28 2020 at 20:53, on Zulip):

Last time I checked, no

Last update: May 29 2020 at 17:00UTC