bug#71504: 30.0.50; FR: Fix suggestions ("quick fix") for Flymake diagnostics

[Spencer Baugh <sbaugh@janestreet.com>]

Eshel Yaron <me@eshelyaron.com> writes:

> Hi Spencer,
>
> Spencer Baugh <sbaugh@janestreet.com> writes:
>
>> Eshel Yaron <me@eshelyaron.com> writes:
>>> Eli Zaretskii <eliz@gnu.org> writes:
>>>>
>>>> I'm asking what is the overall idea of the proposed implementation.  I
>>>> think it's worthwhile to present it, so we could see if we all agree
>>>> with that idea and the details of the proposed implementation.
>>>
>>> Thanks.  To clarify, ideally Spencer will implement this feature request
>>> however he sees fit.  I'm offering my implementation as a reference, but
>>> I'm not advocating for it over other alternatives that may come up.
>>>
>>> The idea of my implementation is to allow Flymake backends to associate
>>> fixes with some of the diagnostics they create, and to add a command
>>> that tries to apply a fix for the diagnostic at point.  For the details,
>>> see below the same patch I attached to this message:
>>> https://lists.gnu.org/archive/html/emacs-devel/2024-05/msg01318.html
>>
>> A few issues:
>>
>> - At an immediate glance, fix-function should return a cl-defstruct
>> instead of a list, to match the rest of the flymake API.
>
> The fix-function returns any number of fixes, as a list of fixes.  I
> guess that's not the list you're referring to, because I don't see how
> this list (whose number of elements vary) can be replaced by a
> cl-defstruct.  Each individual fix is also represented as a list, with
> exactly two elements: TITLE and EDITS.  Is this what you'd like to see
> replaced with a cl-defstruct?

Yes.

>> - An implementation of this feature in flymake absolutely must come with
>> support for Eglot, as the main user of this feature.  Which, if the
>> Eglot maintainer doesn't want to merge that, may mean we can't move
>> forward.
>
> I agree that Eglot support is important (it was naturally the first
> backend I adapted), and that it'll be great to have João on board.
> I don't think it's a blocker, but it's your call.

I don't want to be obstructionist, but it seems to me that if we land
this in flymake, eventually it will just *have* to be supported in
Eglot.  I don't want to abuse that as a way to work around what Joao
prefers.

>> - Your patch adds support in shellcheck for fixes.  That's
>> uncontroversially useful and good.  Could we add this support in a
>> shellcheck-specific way before finalizing the flymake fix API?  (Taking
>> care to add it in a way that can be supported by a later unified UI, of
>> course)
>
> Maybe, but it would be less useful: it wouldn't help other backends,
> including third-party backends, to provide fixes.  Also, I don't think
> there's anything final here, there's plenty of room for developing the
> API further if more/different requirements arise.

Of course that's all true, but also it doesn't seem harmful to just add
some shellcheck-apply-fix command, which contains all the
shellcheck-specific logic you already need to add.  It will have a tiny
bit of duplication, but we can get rid of that after adding a unified
UI.

>> - Likewise, you mentioned adding support for fixes to checkdoc, although
>> I'm not sure where that patch is.
>
> It's on my development branch, namely this commit:
>
> http://git.eshelyaron.com/gitweb/?p=emacs.git;a=commitdiff;h=650c2a056af8df85065b2851d3513c1e3d62c60c

Is this server down right now?

>> That sounds also extremely useful, could it also be added first?
>
> First as in not via Flymake?  Note that checkdoc already has its own fix
> support, the point here is to add something that'll work consistently
> across backends.

Ah, I'll have to judge when I see the commit.

>> - Do you hope to have a default binding for the fix-applying command you
>> mention?  Certainly I would like that, and I think it's worth talking
>> about now.
>
> IMO it's fine to leave it unbound at first, and see how users bind it.
> But if you find an appropriate available binding, why not? :)
>
>> More broadly, I'm still a bit unsure about this whole approach.
>>
>> - What UI, exactly, do you want to build on top of this?  Can we see an
>> example of this UI?  Or is it really just this one command?
>
> A simple command is a good start.  Once diagnostics are enriched with
> fix suggestions, folks can add more bells and whistles if they like to.
>
> FWIW, two further small enhancements that I'm experimenting with are:
> 1. Apply fixes from the diagnostics list buffer.
> 2. Apply fixes to one or more diagnostics that you choose with completion.
>
>> - If it's just the one command, and your hope is to give this some
>> default binding, what exactly is the problem with doing that via a
>> keymap overlay as Joao suggests?  What do you want to do which can't be
>> done with a keymap overlay bound to a backend-specific function?
>
> As we've already established, individual backends can do anything at all
> via overlay properties and other methods.  But that puts extraneous
> burden on backends to deal with frontend concerns, and it doesn't yield
> a consistent UI across backends (one command that works everywhere).
>
>> - Could this UI also support spell-checking, via ispell or flyspell?  It
>> seems like "fix the spelling of a typo'd word" is something that would
>> very naturally fit this whole idea.
>
> Sure.

My implicit point here is that ispell/flyspell aren't flymake-based, so
something tied to flymake may make this harder.  (But maybe
flymake-based spellcheckers are better anyway?  I don't actually use
flyspell or any other on-the-fly spellchecker, so I can't really say)

>> - Could we implement all this outside of flymake, I wonder, with some
>> kind of refactor-backend-functions buffer-local?
>
> Dunno, probably, somehow.  But there's no refactor.el in core, and,
> however we do it, it'll need to interact with Flymake diagnostics that
> come from Flymake backends, so flymake.el seems like a natural choice.
> Crucially, in flymake.el we can extend flymake-make-diagnostic to make
> it as easy as possible for backends to provide fixes.

Well, sure, flymake is important.  But if we did it with some kind of
buffer-local list of backend functions, it could also have backends
which aren't flymake.  IDK if there are such backends though.