bug#66552: 30.0.50; Eglot feature request: handle quirky code actions

[Richard Copley]

On Tue, 17 Oct 2023 at 22:38, João Távora <joaotavora@gmail.com> wrote:
>
> On Tue, Oct 17, 2023 at 8:08 PM Richard Copley <rcopley@gmail.com> wrote:
>
> > > If a servers sends no field, Eglot will
> > > ignore it.  If it sends a wrong one, Eglot won't.
> >
> > > You could work around it with a monkey-patch by advising
> > > eglot--apply-text-edits to always have a null second argument.
> >
> > I'll probably go with that. Thanks for the suggestion. It's a shame
> > that this will affect all languages. It would be nice if I could use a
> > defmethod dispatching on a subclass of eglot-lsp-server! But
> > eglot--apply-text-edits doesn't take a server, so it isn't generic.
>
> For eglot--apply-text-edits to become a generic, it would first
> have to be promoted to Eglot's user API (losing the --), and I
> don't see a very good reason to do that right now.
>
> However, if you wanted a fully "legal" solution, I think you could
> place a server-specific method on
>
>   eglot-handle-request (server your-server) (method workspace/applyEdit)
>
> which removes or massages the `version` cookie in the `edit` keyword
> argument.

In my case the reply to the textDocument/codeAction client request
contains the full WorkspaceEdit, so there is no workspace/applyEdit
server request involved. The edits are applied directly
(eglot-code-actions (interactive) -> eglot--read-execute-code-action
-> eglot-execute -> eglot--apply-workspace-edit). I'm using :before
advice on eglot--read-execute-code-action to massage the list of
actions (both the version number and the title text).

> And even with the monkey-patching approach, you can make something
> server specific by checking the return value of `eglot-current-server`
> before tweaking the value.

I will do that, thanks.

> Another possibility is for Eglot to start interpreting version 0 as
> "any version".  It should be feasible since Eglot controls the
> start of the numbering, which right now is 0 but be increased to 1,
> hopefully without any negative consequences.
>
> > > The second labeling problem is even more bizarre  Again, you
> > > can probably monkey-patch Eglot to work around it.
> >
> > Here I'm not sure I agree. The title is only specified to be "A short,
> > human-readable, title for this code action". In this case the
> > suggestions are computed, and inventing unique friendly names for them
> > isn't feasible. "Apply 'Try this'" seems sensible. It refers to the
> > text of the corresponding diagnostic, "Try this : <newText>".
>
> This newText is hidden deep inside the `edits` field of such messages.
> There's no guarantee it's a 1-element array or even that the edit
> is just an insertion.  So grabbing it consistently to craft a
> user-visible message is just a bad idea

That's true.

> It doesn't make sense for a language server to provide a bunch
> of actions with identical labels.  It isn't something the client
> should have to deal with.  A label, to me, is a piece of information
> for telling one object apart from another object.  If the text of the
> corresponding diagnostic is (presumably unique), why can't
> the label be unique as well?

But these aren't labels, they're titles, and given the specification,
the authors providing code actions won't necessarily foresee their use
as labels, as sensible as it is from the client's point of view. The
message from the diagnostics *could* be used as the title, but I'd
find it hard to argue to the developers that it *should*. That's no
help to you, of course! I agree that my suggestion of building a label
from the newText is a non-starter in the general case. For the time
being it will work for my case, but if Lean 4 starts providing
refactoring support, for example, I'll have to think again.