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

[Eshel Yaron]

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?

> - 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.

> - 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.

> - 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

> 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.

> - 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.

> - 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.


Eshel