Make workspace/willRenameFiles working

[przepompownia]

Fixes #2522
Fixes #2523

[przepompownia]

@dantleech at the moment it seems to work on simple cases but

• we need to verify if we need the part described in workspace/willRenameFiles - destination file must exist before #2522 (currently commented out)
• I'm aware that it's badly designed at the moment - I mean using WorkspaceEdit directly in FileRenamer. Probably we need to define some equivalent of Phpactor\Rename\Model\LocatedTextEditsMap instead and convert it to WorkspaceEdit in the handler.

[przepompownia]

To test it with Neovim (master) I use

local function willRenameFilesHandler(response)
  for clientId, clientResponse in pairs(response) do
    local client = vim.lsp.get_client_by_id(clientId)
    if nil == client then
      vim.notify(('Client %s does not exist'):format(clientId), vim.log.levels.ERROR, {title = 'LSP: workspace/willRenameFiles'})

      return
    end

    vim.lsp.util.apply_workspace_edit(clientResponse.result, client.offset_encoding)
    -- vim.print(clientResponse.result)
  end
end

local function willRenameCurrentBuffer(newPath)
  if not vim.startswith(newPath, '/') then
    newPath = vim.fs.joinpath(vim.uv.cwd(), newPath)
  end
  vim.lsp.buf_request_all(
    0,
    'workspace/willRenameFiles', 
    {
      files = { { oldUri = vim.uri_from_bufnr(0), newUri = vim.uri_from_fname(newPath) } },
    },
    willRenameFilesHandler
  )
end
vim.api.nvim_create_user_command('WillRenameCurrentBuffer', function (opts)
  willRenameCurrentBuffer(opts.args)
end, {nargs = 1, complete = 'file'})

[przepompownia]

@dantleech at the moment it seems to work on simple cases but

* we need to verify if we need the part described in [`workspace/willRenameFiles` - destination file must exist before #2522](https://github.com/phpactor/phpactor/issues/2522) (currently commented out)

* I'm aware that it's badly designed at the moment - I mean using `WorkspaceEdit` directly in `FileRenamer`. Probably we need to define some equivalent of `Phpactor\Rename\Model\LocatedTextEditsMap` instead and convert it to `WorkspaceEdit` in the handler.

At the moment both above things are done:

• replaceDefinition() is needed to edit the source file itself before moving it (in the meaning of WorkspaceEdit changes)
• to avoid reinvention of models and use RenameResult in handler I changed both the adapter and handler into a pattern used in ClassRenamer.

[przepompownia]

Composing the summarized WorkspaceEdit instance from component instances can be controversial. It assumes (relying on LocatedTextEditConverter::toWorkspaceEdit implementation) that only documentChanges field is not empty in each of them and we can ignore other fields.

https://github.com/phpactor/phpactor/pull/2531/files#diff-39ca8889abb439b22964fc403e2e98d48192ef428529ede2bee62f84221faedfR60-R71

[dantleech]

why is this a variadic?

[przepompownia]

Only for enforcing types without using docblock.

[dantleech]

Ok, let's use the docblock then :)

[przepompownia]

No problem, regardless, I'd like to know why you think this is probably bad practice (especially that in practice the constructor gets max two arguments).

[dantleech]

variadics are fine but i'm confused: why is it an array at all and not LocatedTextEdits $edits, RenameResult $result ?

[dantleech]

I'm still confused. The result of the Promise in lib/Rename/Adapter/ClassMover/FileRenamer.php is:

            return new WorkspaceRenameEdits([
                LocatedTextEditsMap::fromLocatedEdits($locatedEdits),
                new RenameResult($from, $to),
            ]);

i.e. why not:

            return new WorkspaceRenameEdits(
                LocatedTextEditsMap::fromLocatedEdits($locatedEdits),
                new RenameResult($from, $to),
            );

It doesn't need to be an array?

[przepompownia]

It doesn't need to be an array?

It's (similarly to WorkspaceEdit's documentChanges elements) the only way to keep the order of operations here.

[dantleech]

Just to clarify though, it's not needed now?

[dantleech]

I mean, we are also passing an aggregate of located text edits, rather than the individual edits. So it's a bit ... weird. If this is gojg to be a list of operations then we should or bably not use the LocatedTextEditsMap here, as that' not an atomic operation.

[przepompownia]

I agree that this separation is far from the best and even weird. I would like to leave some design assumptions to you, but I realize that this may mean a lot of reconstruction (and, in effect, wait probably long time). Please feel free to replace it with something better. I'm not attached in any way the current form of solution. It would be good to have working such LSP request.

[przepompownia]

I'm confused about why phpstan is complaining about changes unrelated to this PR after merge #2614

Edit: most of them were probably secondary to the few unsilenced.

[dantleech]

is sprintf missing a format string? %s %s why do we get the trace from $previous?

[przepompownia]

I copied catching CouldNotRename from RenameHandler to have the same way of error handling and don't know what the trace was needed for in the original place:

https://github.com/phpactor/phpactor/blame/a23130918b61c3f45f4a403abe56b5a06be80de9/lib/Extension/LanguageServerRename/Handler/RenameHandler.php#L93

[dantleech]

I've made a PR on your PR: przepompownia#1 would be great if you can verify that it's all working.