Filter reported for workspace/willRename excludes folders/packages

[mikehaertl]

#3127 introduced a filter for workspace/willRename operations:

[{
    pattern = {
      glob = "**/*.java",
      matches = "file"
    },
    scheme = "file"
} ]

This filter seems to exclude directories. So for some clients this breaks renaming a package - unless they ignore the filter (tested with nvim-lsp-file-operations).

So the reported filter should probably be extended:

[
{
    pattern = {
      glob = "**/*.java",
      matches = "file"
    },
    scheme = "file"
},
{
    pattern = {
      glob = "**",
      matches = "folder"
    },
    scheme = "file"
}
 ]

[rgrunber]

I'd have to check if this was behaviour that actually regressed. If we could support renaming all package declarations of source files when a particular folder (representing a part of the package declaration) is renamed, then we should definitely provide the support.

[mikehaertl]

Isn't this what this test is about?

eclipse.jdt.ls/org.eclipse.jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/handlers/FileEventHandlerTest.java

Lines 142 to 144 in bb26c98

	// Test renaming package from "parent.pack2" to "parent.newpack2"
	@Test
	public void testRenamePackage() throws JavaModelException, BadLocationException {

[rgrunber]

Those tests would validate the language server's side of renaming of packages but not the mechanism by which the client detects the need for the renaming. For example, VS Code doesn't rely on workspace/willRenameFiles because it wants to present a UI prior to applying the changes (preview mode). The current spec has no way to allow for that, so it has to be done manually.

[mikehaertl]

Those tests would validate the language server's side of renaming of packages but not the mechanism by which the client detects the need for the renaming.

Hmm, but isn't jdt-ls all about server side? I wonder why it matters how a specific client behaves. And if VS Code doesn't even rely on willRenameFiles why is it then relevant at all?

TBH I also don't quite understand what you mean by "the need for the renaming": If a client supports it, it can send a willRename whenever it detects the renaming of a file/directory that matches the filter. The filter is clearly restricted to willRename.

[rgrunber]

The language server has many different ways a client may configure the behaviour it expects from the server. For example, VS Code wants to get the result of workspace/willRenameFiles without actually applying them. It wants to create a UI around those options to allow a user to select/preview what would occur if they're applied. However, the language server spec has no way of doing that. As a result, VS Code does not send the capabilities mentioned. Instead it implements this manually. https://github.com/redhat-developer/vscode-java/blob/master/src/fileEventHandler.ts#L140-L174 .

By "the need for renaming" I meant that a client can manually implement detection for rename using its own API and then just manually send workspace/willRenameFiles.

Does introducing a pattern for folders fix the issue for nvim ?

[mikehaertl]

Interesting, but then you basically say that VS Code has solved it in a different way anway. So still not sure, how it's related to this issue here.

I just did a test now and hardcoded the filter into the nvim-file-operations plugin. With that in place I can rename a folder and it will update all classes. Actually that's what I expected: The filter is only used internally by the plugin to decide whether to send a workspace/willRename request to the LSP server or not.

https://github.com/antosha417/nvim-lsp-file-operations/blob/223aca86b737dc66e9c51ebcda8788a8d9cc6cf2/lua/lsp-file-operations/will-rename.lua#L36-L41

I previously disabled the filter check completely and this also worked.

For completeness here's how I tested:

    if will_rename ~= nil then
      -- local filters = will_rename.filters or {}
      local filters = {
        {
          pattern = {
            glob = '**/*.java',
            matches = 'file'
          },
          scheme = 'file'
        },
        {
          pattern = {
            glob = '**',
            matches = 'folder'
          },
          scheme = 'file'
        }
      }
      if utils.matches_filters(filters, data.old_name) then
        local edit = getWorkspaceEdit(client, data.old_name, data.new_name)
        if edit ~= nil then
          log.debug("Going to apply workspace/willRename edit", edit)
          vim.lsp.util.apply_workspace_edit(edit, client.offset_encoding)
        end
      end
    end

[rgrunber]

Interesting, but then you basically say that VS Code has solved it in a different way anway. So still not sure, how it's related to this issue here.

It isn't related. It's an example I gave on how clients can configure the language server behaviour differently depending on if the existing workflow doesn't match what they'd like to do. That was in response to a question you had about why an existing testcase doesn't capture the client renaming a folder, and the more general question of why the client-side makes any difference. And that was because I wasn't sure if the renaming of a folder on the client triggered the rename or whether it had to be done strictly in a package declaration.

I just did a test now and hardcoded the filter into the nvim-file-operations plugin. With that in place I can rename a folder and it will update all classes. Actually that's what I expected: The filter is only used internally by the plugin to decide whether to send a workspace/willRename request to the LSP server or not.

Ok, then we can definitely add the filter to preserve this behaviour. CC'ing @angelozerr for awareness.

[rgrunber]

You should be able to try a nightly of JDT-LS at https://download.eclipse.org/jdtls/snapshots/ once a newer one appears and it should have the fix.