microsoft / vscode-languageserver-node

Language server protocol implementation for VSCode. This allows implementing language services in JS/TS running on node.js
MIT License
1.45k stars 325 forks source link

Language client does not automanticly send file operation notification/reqiuest when working on certain path #1215

Closed GeForceLegend closed 11 months ago

GeForceLegend commented 1 year ago

My language client does not automanticly send file operation notification/reqiuest (the 6 things in ServerCapabilities.workspace.file_operations, didCreate and willCreate, etc) on file change. Did I forget something in client startup?

More detailed information is provided in discussion in tower-lsp repo and stackoverflow. Source code is here. Looks like this is not a server-side issue, since force sending a didRename notification works, and capabilities in initialize result is just like expected.

dbaeumer commented 1 year ago

To clarify: these operations are only send when the user triggers these operation from within the VS Code user interface or via applying a txt edit.

In addition your server needs to register for these.

I tested it locally and the events fire for me for a registration like this:

                    fileOperations: {
                        willRename: {
                            filters: [ { scheme: 'file', pattern: { glob: '**/*.ts', matches: 'file' } } ]
                        }
                    }

The testbed code is here: https://github.com/microsoft/vscode-languageserver-node/tree/dbaeumer/onWillRenameFiles

If you provide me with a GitHub repository I can clone that demos the missing events in a TS/JS server setup I will have a look.

GeForceLegend commented 1 year ago

Github repository and some detailed infomation is provided in the links given when I opened this issue. If you havnt see them (or for some reason dont know they are links), here are the direct url: My own project repo: https://github.com/GeForceLegend/vscode-mcshader/tree/rename_feature Discussion opened by me in the framework repo of my server: https://github.com/ebkalderon/tower-lsp/discussions/387; Stack overflow question though no one answered: https://stackoverflow.com/questions/75781429/unable-to-get-file-operation-willrename-etc-notifications-in-language-server Server side is running on Rust, but testing result looks nothing related to server.

dbaeumer commented 1 year ago

I saw the links, but I am not an Rust expert nor do I maintain any Rust specific code. So I can only talk about the LSP client code and everything works as expected in my test setup (see my branch). If you want me to dig deeper I would like to ask you to provide me with a setup were the issue is reproducible in a TS/JS setup.

GeForceLegend commented 1 year ago

Can I provide a packuped vsix extension or a executable server file so you can run and test it without a rust setup, focus on the client side?

dbaeumer commented 1 year ago

Have you looked at my example and ensured that your setup looks the same. Especially the glob pattern needs to match directory paths. A pattern like *.ts only matches for example TypeScript file in the workspace root.

Best would be for me if you provide me with a 10 line server it TypeScript that registers a will rename handler that is not notified. Then I will definitely have a look and debug it.

GeForceLegend commented 1 year ago

I'm wondering if this is related to certain name in path. Below provides log result of your client and your server running with 2 different work space pathes. I created test.ts and renamed them to te1st.ts on the root of both. We can see the first one can correctly recieve notification, but the second one cant.

Debugger listening on ws://127.0.0.1:6012/3a976b18-bd40-4305-a041-8efe69f8d4df
For help, see: https://nodejs.org/en/docs/inspector
[server]test
lsp-sample file:///d%3A/Documents/GitHub/Microsoft/vscode-extension-samples/lsp-sample
[Error - 18:59:34] Notification testbed/notification defines parameters by name but received parameters by position
Is array: true
Get workspace folders: lsp-sample file:///d%3A/Documents/GitHub/Microsoft/vscode-extension-samples/lsp-sample
Configuration received
{
  files: [
    {
      oldUri: 'file:///d%3A/Documents/GitHub/Microsoft/vscode-extension-samples/lsp-sample/server/test.ts',
      newUri: 'file:///d%3A/Documents/GitHub/Microsoft/vscode-extension-samples/lsp-sample/server/te1st.ts'
    }
  ]
}
Debugger listening on ws://127.0.0.1:6012/cf890bd3-d5ca-48d0-819c-5a9ddec46d23
For help, see: https://nodejs.org/en/docs/inspector
[server]test
CoreProfileGbuffersTerrain file:///c%3A/Users/L/AppData/Roaming/.minecraft/shaderpacks/CoreProfileGbuffersTerrain
[Error - 19:00:23] Notification testbed/notification defines parameters by name but received parameters by position
Is array: true
Get workspace folders: CoreProfileGbuffersTerrain file:///c%3A/Users/L/AppData/Roaming/.minecraft/shaderpacks/CoreProfileGbuffersTerrain
Configuration received
dbaeumer commented 1 year ago

Are you saying you are opening a workspace folder under .../AppData/Roaming?

GeForceLegend commented 1 year ago

Yes, since this is the place of Minecraft itself

GeForceLegend commented 1 year ago

After testing for more situation, I found there is nothing to do with .../AppData/Roaming. It's mostly caused by the point in .minecraft. Folders without . perfix are all working fine, and another folder with this perfix .minecraft_dungeons will suffer this issue too.

dbaeumer commented 1 year ago

That explains it and I was able to reproduce it. The reason is that glob (that is the matching algorithm we use) be default doesn't match files/folders starting with a dot to be Linux glob complaint (see https://linux.die.net/man/7/glob).

We should add an options that allows matching names with . to the registration options.

@DanTup what I noticed while debugging this is that we match against the full FS path. What was the reason to not only match against the workspace relative path since we only send these events for things inside the workspace?

dbaeumer commented 1 year ago

We could also think about making dot matching the default. Need to check if this violates any spec or other things (VS Code behavior)

GeForceLegend commented 1 year ago

Directly use the vscode API like vscode.workspace.onWillRenameFiles in client, or certain file operation features like workspace/didChangeWatchedFiles can correctly handle file events inside such path. Do they expected to run in this way?

dbaeumer commented 1 year ago

The file operation events (like onWillRenameFiles) work different in LSP than VS Code. In VS Code you get all renames without registration whereas in LSP you need to register for them for performance reasons.

I check and document selectors in LSP / VS Code match dot files / folders and they use glob specification as well. So we should simply enable the dot option in the matching library

dbaeumer commented 1 year ago

Created https://github.com/microsoft/vscode-languageserver-node/pull/1221

DanTup commented 1 year ago

@DanTup what I noticed while debugging this is that we match against the full FS path. What was the reason to not only match against the workspace relative path since we only send these events for things inside the workspace?

I'm not sure. I looked through https://github.com/microsoft/vscode-languageserver-node/pull/687 and https://github.com/microsoft/language-server-protocol/pull/1074 but couldn't find any specific discussion about this so it's possible there was no reason other than the code already had the full path.

Although, I think the workspace relative path is also ambiguous if a file is included via multiple workspace roots? So if it's going to only be matched on that, that might need specifying (as the closest/shortest path?).