Add window/{did,will}{Create,Rename,Delete}Files - #1074

Have not forgotten you, however I need to push this to the beginning of next week. We have endgame.

Dirk Bäumer (@dbaeumer) understood, np!

Dirk Bäumer (@dbaeumer) I don't suppose you've had chance to look at this yet?

Danny Tuppeny (@DanTup) actually was on my list for this week. Added my comments.

Thanks!

I've done the renames and added create/delete, though I have a few questions:

willRename, didRename

Is there actually any difference between these? Since it's all async, it seems like willRename is going to arrive at the server after the rename has probably occurred (unless using willRenameWaitUntil), so in what case would it be useful for a server to user willRename rather than the other two? Should each operation just be a willXXXWaitUntil and a didXXX?

To be consistent we should also consider adding a dynamicRegistration property. But one for all three is enough. It is the same as for text document synchronization.

The text doc registrations are scoped to text documents (eg. by language ID) but it wasn't clear how this would work here. For example if I say I want renames for just a Dart file, what happens when a I rename a folder? Should it always be sent, or only if it has child Dart files?

That is a good point. It is very unlikely that someone is interested in willRename and willRenameWaitUntil. The problem is that willRename can be implemented as a notification where as willRenameWaitUntil must be implemented as a request/respone.

What we could do have one property willRename that takes the following values: off, event, waitUntil. The opern question would still be if we implement event as a notification or as a request/response as well. For simplicity I would opt for a request/response as well but would document that in the event case the client doesn't need wait for the response since it will be empty.

Regarding

For example if I say I want renames for just a Dart file, what happens when a I rename a folder? Should it always be sent, or only if it has child Dart files?

I think that should be consistent with file events. There a rename of a folder doesn't generate renames for the child files.

That is a good point. It is very unlikely that someone is interested in willRename and willRenameWaitUntil. The problem is that willRename can be implemented as a notification where as willRenameWaitUntil must be implemented as a request/respone.

Right - but what advantage is there having onWillRename versus onDidRename? Both are notifications, and by the time the server receives onWillRename it must assume the rename may have already happened, and therefore it seems like it's identical in function to onDidRename?

I'm thinking that we only need onDidRename and onWillRenameWaitUntil?

I think that should be consistent with file events. There a rename of a folder doesn't generate renames for the child files.

But would it be sent to the server if the server only asked for events for Dart files? (I think it has to - so I'm wondering if we need to specify that folder events will always be sent, regardless of the file/pattern filters on the registration? or there needs to be some way to register for them including folders?)

Regarding dynamic registration: I think there is a misunderstanding. Dynamic registration means that a server later on can register for these events and not only through the initialize result. It doesn't mean that it needs a document filter for registration. For these event I would argue that we either

• only provide a file pattern to make it like the file events.
• make the global.

These events WILL ONLY be fired if a new file is created, rename, deleted using a user action in the UI. They are not a replacement for normal file events.

Regarding which events we need: I agree that a willRename followed by a didRename doesn't make much sense. However I can easily argue that a willRename alone could make sense.

If we make the registration file pattern sensitive (please note that the VS Code API is not that way so we need to implement this ourselves) then I would do the following:

• a will* request/response
• a did* notification

If we make it global I would argue that we should not make all will a request/response since it might result in a performance problem if many server have registered for these events.

I am actually leaning towards the registration on a file pattern and then make the will* always a request/response. The name could simply be willRename.

It doesn't mean that it needs a document filter for registration

Sorry - I misunderstood your comment about copying text document synchronisation.

• only provide a file pattern to make it like the file events.
• make the global

I think the first could be a little confusing - if I say I want just "*.dart" and the user renames a folder with a dart file nested inside it, it's not clear whether I should get the notification or not?

Also - would the pattern be applied to the old URI or new URI in the case of renames?

If we make the registration file pattern sensitive

Should this say case-sensitive? (this might be a can of worms 😬)

make the will* always a request/response. The name could simply be willRename.

So if I understand correctly, we will drop the current willRename (since it's basically the same as didRename and rename the willRenameWaitUntil to willRename in its place?

So we're left with:

• didRename (notification)
• willRename (request/response that waits)

(and the same for create/delete)?

I think this is not that easy because of

If we make it global I would argue that we should not make all will a request/response since it might result in a performance problem if many server have registered for these events.

If we make willRename a request/response that waits and we have 5 servers registered globally for that event VS Code needs to wait for all 5 servers to respond before the actual rename can happen. And it is very likely that at the most one server does something.

In the VS Code API this is different since VS Code talks to one extension host and then calls in memory 5 listeners. If we go for a registration with a pattern we could clearly spec that:

• if a folder renames no events are generated for the files inside the folders.
• for rename the pattern fires for the oldURI.

Unclear is whether we need to allow to specify a pattern denoting a directory.

If we make willRename a request/response that waits and we have 5 servers registered globally for that event VS Code needs to wait for all 5 servers to respond before the actual rename can happen. And it is very likely that at the most one server does something.

Fair point. Though I think most language servers would use didRename, since many implemented updating references before willRename was added to VS Code (I opened the original request, and don't think many other servers needed to know about the rename before it happened). I do agree that having to stall the client waiting for a response makes it less desirable to send events that won't do anything though.

Unclear is whether we need to allow to specify a pattern denoting a directory.

I'm not sure if this means "do we need directory support" or "does it need to be a pattern"? I think directories could probably be just a flag (I suspect most servers will just want them all), but in the interest of allowing it to be controlled (for the same perf reasons mentioned above), maybe it should be two patterns (a file pattern, and a directory pattern)? For Dart, I think what I'm interested in is

files: **/*.dart - any Dart file rename
directories: */ - all directories

How about something like this?

files?: {
	// Req/Resp returning TextEdit[] | null
	willCreate?: FileOperationRegistrationOptions;
	// Notification
	didCreate?: FileOperationRegistrationOptions;

	// Req/Resp returning TextEdit[] | null
	willRename?: FileOperationRegistrationOptions;
	// Notification
	didRename?: FileOperationRegistrationOptions;

	// Req/Resp returning TextEdit[] | null
	willDelete?: FileOperationRegistrationOptions;
	// Notification
	didDelete?: FileOperationRegistrationOptions;
}

interface FileOperationRegistrationOptions {
	documentSelector?: DocumentSelector;
	directorySelector?: DirectorySelector;
}

export type DirectorySelector = DirectoryFilter[];

export interface DirectoryFilter {
	pattern?: string;
}

Dirk Bäumer (@dbaeumer) any thoughts on the comment above?

Danny Tuppeny (@DanTup) was on vacation. Will have a look today.

Dirk Bäumer (@dbaeumer) np! Wasn't sure if the notification may have been overlooked. Thanks!

Danny Tuppeny (@DanTup) I do like the API. However IMO we should not talk about document selectors / filters. At least in VS Code to correctly determine a document selector you need to open the document to determine its language ID. I would argue we should treat these like file events which simply support a glob pattern. See https://github.com/microsoft/vscode-languageserver-node/blob/master/protocol/src/common/protocol.ts#L1419

Then we could say a pattern with a slash at the end matches a directory only.

Dirk Bäumer (@dbaeumer) got it - I'll use globPattern then.

Then we could say a pattern with a slash at the end matches a directory only.

Just to make sure I understand correctly, to support "all .dart files and any folder" I would therefore send two globs, like:

• **/*.dart
• **/

Is that right? If so, I'll update the Pr to match my previous comment above with this change for a proper review. Thanks!

Danny Tuppeny (@DanTup) you can use two registrations or some OR pattern. Something like **{/,*.dart}

Ah cool, even better. I'll get this updated today. Thanks!

Dirk Bäumer (@dbaeumer) I think this is good for a proper look now. Thanks!

Dirk Bäumer (@dbaeumer) are there any notes on how you usually work on changes that span protocol/client? It looks like I need to put the type changes into ./protocol, but the client library seems to be using the one from NPM. Is there an easy way to have it use the local folder while working on it? (this is probably more of an NPM question... in Dart there are dependency_overrides for this).

Edit: Oops, had been looking in the wrong readme. Found this now:

After cloning the repository, run npm install to install dependencies and npm run symlink to point packages in this repository to each other.

😄

Dirk Bäumer (@dbaeumer) I've made a start at trying to implement one of the 6 requests (willCreateFiles) here:

DanTup/vscode-languageserver-node@498507a

Hopefully I'm going in the right direction, but I have some questions:

1. When I save server/client.ts, all of the formatting gets badly messed up (for example tons of blank lines written between all of the imports). I've tried disabling formatOnSave/formatOnPaste/formatOnType and the codeActionsOnSave in the Workspace settings, and yet it continues. I have to use "Save without formatting" every time and it's a real pain. Am I missing something? :(

2. In the case where a server registers statically, is just using a glob of **{/,/*} reasonable? (DanTup/vscode-languageserver-node@498507a#diff-1442f78812d67be25f18d3b5d82e8b4191ae2f939a900eac32d4360222c5d845R1239)

3. Is there any existing code I can use for the glob matching? It seems like the existing code for the text document events uses VS Code's Language.match but it doesn't seem like the glob matching is exposed directly. Should I add the glob NPM package and use it directly? (DanTup/vscode-languageserver-node@498507a#diff-1442f78812d67be25f18d3b5d82e8b4191ae2f939a900eac32d4360222c5d845R1257)

4. Should middleware use VS Code event types or LSP events? (DanTup/vscode-languageserver-node@498507a#diff-1442f78812d67be25f18d3b5d82e8b4191ae2f939a900eac32d4360222c5d845R1267)

Dirk Bäumer (@dbaeumer) I know you're busy (and I'm not helping that 😬), but interested in your thoughts on the above - would love to get more done on this. Thanks! :-)

Really busy week :-). Regarding your questions:

1.) the only thing that is enabled for the Node repository is code action on save. Could it be that you have an extension installed (e.g. prettier) that does this and is not happy with how the Node repository is formatted.
2.) This sounds reasonable to me.
3.) You can copy the code here: https://github.com/microsoft/vscode-eslint/blob/master/client/src/utils.ts#L184 I have written that a while back ago.
4.) The middle ware uses VS Code types for requests going from the client to the server and LSP types for things flowing from the server to the client. This allows incepting the data and modifying it before it is processed.

Dirk Bäumer (@dbaeumer) np, I understand - thank you! I'll work on 2-4.

I also figured 1 out (it happened with every extension disabled). It's the TS "Organize Imports" action - raised here.

Dirk Bäumer (@dbaeumer) as far as I can tell, that pattern matcher doesn't support slashes or stars inside braces:

https://github.com/microsoft/vscode-eslint/blob/master/client/src/utils.ts#L128

Is this something we need to extend it to support, or should I do this another way? (I'm not sure whether this
pattern syntax is well-specified or we can just make the rules up, though I presume there's benefit in being aligned with eslint or VS Code's implementation where possible).

(edit: also, I'm now wondering why have a custom pattern syntax instead of just using the JS regex syntax directly?)

(edit2: passing **/*.txt generates a regex of /^(?:[^\/]|(?:(?:[^\/]\/)+[^\/]))*?\/[^\/]*?\.txt/ which doesn't match test.txt. because the regex requires a separator.. I think this is a bug, so I'll have a go at making this work to my understanding and you can review the changes from the original as part of the PR)

I try to stay away from Regexp in LSP (we have them for snippets but because they are in Textmate). If we start to use them we need to spec which features we support in RegExp in LSP. Glob patterns are well specified.

Actually the eslint matcher only needs to match directories no files. So it might not be a good candidate to use. We have one in VSCode here: https://github.com/microsoft/vscode/blob/master/src/vs/base/common/glob.ts#L446

May be we should just use https://www.npmjs.com/package/glob.

Actually the eslint matcher only needs to match directories no files

Ah, that makes sense. I had started to modify it to support what I needed (including full nesting for the braces), though I'll take a look through the VS Code code to see if that's more easily reusable and does everything required (and if not, I'll try glob). Thanks!

Dirk Bäumer (@dbaeumer)

I was unable to make the VS Code matching or minimatch (which glob uses) behave as I believe we wanted (trailing slashes deciding whether to include only files or only folders), however I (think I) was able to get your es-lint code working with some changes:

DanTup/vscode-languageserver-node@d494cd0

I'll proceed using that for now, but if you think we should do it another way let me know.

The middle ware uses VS Code types for requests going from the client to the server and LSP types for things flowing from the server to the client. This allows incepting the data and modifying it before it is processed.

Let's assume a server says it wants {**/,}*.dart (all folders and all dart files at any level) and a user drags foo.dart and foo.txt in the same action. Should we send the whole event to the server (since one of the files matched) or filter it only to those that matched. If the latter (as I expect), should the middleware get the full list or the filtered list?

Looking at the WillSaveWaitUntil, the middleware is called only if the original event matches the selectors, and any changes it made go straight to the server (so if the middleware changed the document details, it would still go to the server). To be consistent with this, I think we would filter the event before sending it to the middleware, then always send it on to the server (though I don't know if it's strange to filter the files in the event for the middleware?).

Dirk Bäumer (@dbaeumer) I've opened a PR at microsoft/vscode-languageserver-node#687 as a better place to discus the code. It's very incomplete, but I have questions about how best to test so looking for some pointers. Thanks!