Adding fix suggestions to Flymake diagnostics

Adding fix suggestions to Flymake diagnostics

[Eshel Yaron]

[-- Attachment #1: Type: text/plain, Size: 1058 bytes --]

Hi Spencer, João, and all,

I've been playing around a bit with adding fix suggestions to Flymake.
The idea is to provide backends with a standard API for associating fix
suggestions with diagnostics, and to provide users with a consistent UI
for applying such suggestions.

Some backends already use various tricks to suggest fixes in one way or
another.  For example, Eglot associates LSP "quickfix" actions with some
diagnostics.  What do you think about standardizing this concept?

I'm attaching a patch with an initial implementation that extends the
Flymake API and adapts Eglot to provide fix suggestions in this manner.
This lets you apply fix suggestions via context-menu-mode right-click
menus, and via a new command flymake-fix that looks for a fix for the
diagnostic at point.  So far I tested it with clangd and gopls.

Note that this patch uses the function refactor-apply-edits from my
refactor.el library (attached as well) in order to apply the fix
suggestions, so you'll need that too if you want to give it a spin.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-fix-suggestions-to-Flymake-diagnostics.patch --]
[-- Type: text/x-patch, Size: 10258 bytes --]

From b68c6013693a163462ba1ff49494bfbe68d3eb6c Mon Sep 17 00:00:00 2001
From: Eshel Yaron <me@eshelyaron.com>
Date: Sun, 26 May 2024 14:41:03 +0200
Subject: [PATCH] Add fix suggestions to Flymake diagnostics

---
 lisp/progmodes/eglot.el   | 81 ++++++++++++++++++++++++++++++---------
 lisp/progmodes/flymake.el | 54 ++++++++++++++++++++++++--
 2 files changed, 112 insertions(+), 23 deletions(-)

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index bb915fb4a91..4daa1f213de 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -2382,6 +2382,65 @@ eglot--TextDocumentIdentifier-cache
 server on textDocument/didOpen and similar calls.  TRUENAME is the
 expensive cached value of `file-truename'.")
 
+(defun eglot--flymake-fix (data)
+  "Return fix suggestions for Flymake diagnostic with DATA."
+  (eglot--dbind ((Diagnostic) range) (alist-get 'eglot-lsp-diag data)
+    (pcase-let ((`(,beg . ,end) (eglot--diag-range-region range)))
+      (let ((actions (eglot-code-actions beg end "quickfix")))
+        (seq-keep
+         (eglot--lambda ((CodeAction) title edit)
+           (eglot--dbind
+               ((WorkspaceEdit) changes documentChanges)
+               edit
+             (let ((prepared
+                    (mapcar
+                     (eglot--lambda ((TextDocumentEdit) textDocument edits)
+                       (eglot--dbind ((VersionedTextDocumentIdentifier)
+                                      uri version)
+                           textDocument
+                         (list (eglot-uri-to-path uri) edits version)))
+                     documentChanges)))
+               (unless (and changes documentChanges)
+                 (cl-loop for (uri edits) on changes by #'cddr
+                          do (push (list (eglot-uri-to-path uri) edits)
+                                   prepared)))
+               (when prepared
+                 (list title
+                       (mapcar
+                        (pcase-lambda (`(,file ,edits . ,_))
+                          (let ((buf (find-file-noselect file)))
+                            (cons buf
+                                  (seq-map (eglot--lambda ((TextEdit)
+                                                           range newText)
+                                             (pcase (with-current-buffer buf
+                                                      (eglot-range-region range))
+                                               (`(,beg . ,end)
+                                                (list beg end newText))))
+                                           edits))))
+                        prepared))))))
+         actions)))))
+
+(defun eglot--diag-range-region (range)
+  (pcase-let ((`(,beg . ,end) (eglot-range-region range)))
+    ;; Fallback to `flymake-diag-region' if server
+    ;; botched the range
+    (when (= beg end)
+      (if-let* ((st (plist-get range :start))
+                (diag-region
+                 (flymake-diag-region
+                  (current-buffer) (1+ (plist-get st :line))
+                  (plist-get st :character))))
+          (setq beg (car diag-region) end (cdr diag-region))
+        (eglot--widening
+         (goto-char (point-min))
+         (setq beg
+               (eglot--bol
+                (1+ (plist-get (plist-get range :start) :line))))
+         (setq end
+               (line-end-position
+                (1+ (plist-get (plist-get range :end) :line)))))))
+    (cons beg end)))
+
 (cl-defmethod eglot-handle-notification
   (server (_method (eql textDocument/publishDiagnostics)) &key uri diagnostics
            &allow-other-keys) ; FIXME: doesn't respect `eglot-strict-mode'
@@ -2413,24 +2472,7 @@ eglot-handle-notification
                        diag-spec
                      (setq message (mess source code message))
                      (pcase-let
-                         ((`(,beg . ,end) (eglot-range-region range)))
-                       ;; Fallback to `flymake-diag-region' if server
-                       ;; botched the range
-                       (when (= beg end)
-                         (if-let* ((st (plist-get range :start))
-                                   (diag-region
-                                    (flymake-diag-region
-                                     (current-buffer) (1+ (plist-get st :line))
-                                     (plist-get st :character))))
-                             (setq beg (car diag-region) end (cdr diag-region))
-                           (eglot--widening
-                            (goto-char (point-min))
-                            (setq beg
-                                  (eglot--bol
-                                   (1+ (plist-get (plist-get range :start) :line))))
-                            (setq end
-                                  (line-end-position
-                                   (1+ (plist-get (plist-get range :end) :line)))))))
+                         ((`(,beg . ,end) (eglot--diag-range-region range)))
                        (eglot--make-diag
                         (current-buffer) beg end
                         (eglot--diag-type severity)
@@ -2439,7 +2481,8 @@ eglot-handle-notification
                                     (cl-loop for tag across tags
                                              when (alist-get tag eglot--tag-faces)
                                              collect it)))
-                          `((face . ,faces))))))
+                          `((face . ,faces)))
+                        #'eglot--flymake-fix)))
            into diags
            finally (cond ((and
                            ;; only add to current report if Flymake
diff --git a/lisp/progmodes/flymake.el b/lisp/progmodes/flymake.el
index 2e602658ea7..edf9e95063b 100644
--- a/lisp/progmodes/flymake.el
+++ b/lisp/progmodes/flymake.el
@@ -368,7 +368,7 @@ flymake-error
   locus beg end type text backend data overlay-properties overlay
   ;; FIXME: See usage of these two in `flymake--highlight-line'.
   ;; Ideally they wouldn't be needed.
-  orig-beg orig-end)
+  orig-beg orig-end fix-function)
 
 ;;;###autoload
 (defun flymake-make-diagnostic (locus
@@ -377,7 +377,8 @@ flymake-make-diagnostic
                                 type
                                 text
                                 &optional data
-                                overlay-properties)
+                                overlay-properties
+                                fix-function)
   "Make a Flymake diagnostic for LOCUS's region from BEG to END.
 LOCUS is a buffer object or a string designating a file name.
 
@@ -396,14 +397,24 @@ flymake-make-diagnostic
 OVERLAY-PROPERTIES is an alist of properties attached to the
 created diagnostic, overriding the default properties and any
 properties listed in the `flymake-overlay-control' property of
-the diagnostic's type symbol."
+the diagnostic's type symbol.
+
+FIX-FUNCTION, if non-nil, is a function that takes DATA and returns a
+list of fix suggestions for this diagnostic.  Each fix suggestion is a
+list (TITLE EDITS), where TITLE is a string describing the fix and EDITS
+is a list of (FILE-OR-BUFFER . CHANGES) cons cells, where FILE-OR-BUFFER
+is the file name or buffer to edit, and CHANGES is a list of changes to
+perform in FILE-OR-BUFFER.  Each element of CHANGES is in turn a
+list (BEG END STR), where BEG and END are buffer positions to delete and
+STR is the string to insert at BEG afterwards."
   (when (stringp locus)
     (setq locus (expand-file-name locus)))
   (flymake--diag-make :locus locus :beg beg :end end
                       :type type :text text :data data
                       :overlay-properties overlay-properties
                       :orig-beg beg
-                      :orig-end end))
+                      :orig-end end
+                      :fix-function fix-function))
 
 ;;;###autoload
 (defun flymake-diagnostics (&optional beg end)
@@ -849,6 +860,40 @@ flymake--update-eol-overlays
             (overlay-put o 'before-string (flymake--eol-overlay-summary src-ovs))
           (delete-overlay o))))))
 
+(defun flymake-diagnostic-context-menu (menu click)
+  "Extend MENU with fix suggestions for diagnostic at CLICK."
+  (when-let ((diag (mouse-posn-property (event-start click)
+                                        'flymake-diagnostic))
+             (fix-fun (flymake--diag-fix-function diag))
+             (fixes (funcall fix-fun (flymake--diag-data diag)))
+             (i 1))
+    (dolist (fix fixes)
+      (define-key menu (vector (intern (format "flymake-fix-%d" i)))
+                  `(menu-item ,(format "Fix: %s" (car fix))
+                              ,(lambda ()
+                                 (interactive)
+                                 (refactor-apply-edits (cadr fix)))
+                              ,@(cddr fix)))
+      (cl-incf i)))
+  menu)
+
+(defun flymake-fix (pos)
+  "Fix Flymake diagnostic at POS."
+  (interactive "d")
+  ;; TODO - fix _all_ diagnostics at point.
+  (if-let ((diag (car (flymake-diagnostics pos))))
+      (if-let ((fix-fun (flymake--diag-fix-function diag))
+               (fixes (funcall fix-fun (flymake--diag-data diag))))
+          (refactor-apply-edits
+           (car (if (cdr fixes)
+                    (alist-get
+                     (completing-read (format-prompt "Fix" (caar fixes))
+                                      fixes nil t nil nil (caar fixes))
+                     fixes nil nil #'string=)
+                  (cdar fixes))))
+        (message "No fix available for this diagnostic"))
+    (user-error "No diagnostic at this position")))
+
 (cl-defun flymake--highlight-line (diagnostic &optional foreign)
   "Attempt to overlay DIAGNOSTIC in current buffer.
 
@@ -956,6 +1001,7 @@ flymake--highlight-line
              (flymake-diagnostics pos)
              "\n"))))
       (default-maybe 'severity (warning-numeric-level :error))
+      (default-maybe 'context-menu-functions '(flymake-diagnostic-context-menu))
       ;; Use (PRIMARY . SECONDARY) priority, to avoid clashing with
       ;; `region' face, for example (bug#34022).
       (default-maybe 'priority (cons nil (+ 40 (overlay-get ov 'severity)))))
-- 
2.45.0


[-- Attachment #3: refactor.el --]
[-- Type: application/emacs-lisp, Size: 10547 bytes --]

Re: Adding fix suggestions to Flymake diagnostics

[João Távora]

I think this is the wrong way to approach it.  Refactorings can be, but are not
necessarily, associated with diagnostics.  A LSP rename, an extraction of a
number of statements into a function, are just two examples of edits that are
not associated with diagnostics.

The right way, as I think I've said before, is to start by lifting, and
perhaps generalizing, Eglot's existing refactoring support into a
refactor.el that
enables Eglot to keep working as it has been before and then take it from there,
seein where this new refactor.el can help other refactor providers.

That would be the greatest value.  Maybe Flymake adjustments can be
made.  That's
up to Spencer.  I just note that Flymake as a library is already flexible enough
to make these context menusm and I don't immediately see the need to bring new
concepts such as "code edits" of a specific format into Flymake's API.
Just seems
to increase coupling and brittleness.  At most, I would create some
utils (in some
other library, maybe refactor.el itself) that simplify the creation of
these menus.
But again, that's up to Spencer.

Maybe your refactor.el is promising (I had some unfinished version left around
somewhere, too), but I don't see eglot.el requiring refactor.el, or expressing
eglot-rename or eglot-code-actions in terms of refactor-rename or
refactor-code-actions.
That's my starting point: any refactor.el worth considering has to be able to
remove much of Eglot's code from eglot.el that is not LSP-specific
*and* keep the
same Eglot UX for its LSP-aided refactorings, since everybody's
accustomed to that UX.

Of course if you want to design some other refactor.el to serve
primarily some other
backend (say, an Elisp backend) Eglot can happily keep using its
original longstanding
hand-rolled refactoring support.

João

Re: Adding fix suggestions to Flymake diagnostics

[Eshel Yaron]

João Távora <joaotavora@gmail.com> writes:

> I think this is the wrong way to approach it.  Refactorings can be, but are not
> necessarily, associated with diagnostics.

Right, but this proposal is not about refactoring, rather it's about fix
suggestions for diagnosed issues :)

> [...]
> I just note that Flymake as a library is already flexible enough to
> make these context menu

Yes, Flymake lets backends associate arbitrary overlay properties with
diagnostics, but that requires backends to worry about UI stuff.  My
suggestion is to give backends a standard way to provide the relevant
data, and deal with the UI separately and consistently across backends.

> and I don't immediately see the need to bring new concepts such as
> "code edits" of a specific format into Flymake's API.  Just seems to
> increase coupling and brittleness.

That's a valid concern.  It would be nice to use a generic "code edit"
object/format that's not specific to Flymake.  Maybe we already have
something like that somewhere else in Emacs?

> [...]
> Maybe your refactor.el is promising (I had some unfinished version left around
> somewhere, too), but I don't see eglot.el requiring refactor.el, or expressing
> eglot-rename or eglot-code-actions in terms of refactor-rename or
> refactor-code-actions.

I only mentioned my refactor.el because I had refactor-apply-edits at
hand and it made it easier to demonstrate how fix suggestions can work,
I'm not suggesting to change anything about eglot-rename and friends.
FWIW, I use this refactor-rename both with (a modified) Eglot as the
backend instead of eglot-rename, as well as with other backends in modes
that don't have LSP support.  Similarly, with this fix suggestions
proposal, what I'd like to have is a consistent way of saying "fix this"
when presented with a diagnostic, regardless of whether I'm using LSP.


Best,

Eshel

Re: Adding fix suggestions to Flymake diagnostics

[João Távora]

On Sun, May 26, 2024 at 4:56 PM Eshel Yaron <me@eshelyaron.com> wrote:
ntext menu
>
> Yes, Flymake lets backends associate arbitrary overlay properties with
> diagnostics, but that requires backends to worry about UI stuff.

100% Agree.  And indeed Eglot would like not to worry about UI, but it has
to on more than one occasion.  So if you want to lift Eglot's diagnostic-fixing
things out of there, I suggest lifting them into another library that is *not*
Flymake.  That library would tend to be the new refactor.el, which would
always need an "edit" concept of its own (with more or less similarities
to LSP).

Ideally refactor.el would give eglot.el a function (or more likely a macro)
to simplify what is currently done ca. lines 2287 of eglot.el ("Flymake
customization") and also a drop-in replacement (or near-drop-in) for
eglot-code-actions-at-mouse.

Flymake.el would remain ignorant of what an edit is, which is a good thing
in my book.

> I'm not suggesting to change anything about eglot-rename and friends.

Well, I'm encouraging you to :-) I gave your refactor.el a *very* cursory
read but it seems to be at least more or less the length and complexity
I envisioned such a library having.  It has its own concept of edit, seems
to know how to present things as diffs, all good things.  The tricky part
if I remember was how to talk to the backend about the kinds of actions
available.  Maybe I'm misremembering.  Anyway, refactor.el is definitely
where I would put all this with an eye to slashing great as much Eglot UI
code as possible.

João

Re: Adding fix suggestions to Flymake diagnostics

[Eshel Yaron]

Hi João,

João Távora <joaotavora@gmail.com> writes:

> On Sun, May 26, 2024 at 4:56 PM Eshel Yaron <me@eshelyaron.com> wrote:
>>
>> Yes, Flymake lets backends associate arbitrary overlay properties with
>> diagnostics, but that requires backends to worry about UI stuff.
>
> 100% Agree.  And indeed Eglot would like not to worry about UI, but it has
> to on more than one occasion.  So if you want to lift Eglot's diagnostic-fixing
> things out of there

That's not strictly my goal, although it could be a nice side effect.
What's important is to have a good, backend-agnostic, experience for
fixing diagnostics.  Such a standard UI may subsume some of Eglot's
bespoke UI, but it may also be complementary and the two can coexist,
perhaps with some LSP-specific use cases only provided by Eglot.

> , I suggest lifting them into another library that is *not* Flymake.
> That library would tend to be the new refactor.el, which would always
> need an "edit" concept of its own (with more or less similarities to
> LSP).

I don't think of fixing diagnostics as a form of refactoring, rather as
a separate activity/use case, even though there's some technical overlap
in that both involve applying edits to code.  You fix diagnosed issues
in response to their detection, while a refactor operation (rename,
extract, ...) is something you mostly do unprompted.  Therefore
refactor.el is not the best place to put something like my proposed
flymake-fix, IMO.  We could have a separate library, say flyfix.el, and
put the fix-diagnostics UI there.  We'd still need Flymake backends to
communicate with this new library, since these backends are best
positioned for providing fixes for the issues they diagnose, but this
communication can be transparent to Flymake--for example, it could rely
on some overlay property that backends would add to diagnostics and the
UI of the new library would look for.  If we really don't want to teach
Flymake about fixes, I think that could work.  WDYT?

> I gave your refactor.el a *very* cursory read but it seems to be at
> least more or less the length and complexity I envisioned such a
> library having.  It has its own concept of edit, seems to know how to
> present things as diffs, all good things.  The tricky part if I
> remember was how to talk to the backend about the kinds of actions
> available.

That's definitely a challenge.  Currently I tried to keep things pretty
flexible: there are standard operations (so far only "rename", but I'm
planning to add "extract", "inline", and "organize" for imports) and
custom operations.  Backends can say which operations they can
facilitate, and that can include both standard and custom operations.
For standard operations, the library (refactor.el) takes care of all the
UI consideration and the backend only needs to provide data in a
standard format, while for custom actions the library gives control back
to the backend.


Eshel

Re: Adding fix suggestions to Flymake diagnostics

[João Távora]

On Mon, May 27, 2024 at 1:15 PM Eshel Yaron <me@eshelyaron.com> wrote:

> That's not strictly my goal, although it could be a nice side effect.
> What's important is to have a good, backend-agnostic, experience for
> fixing diagnostics.  Such a standard UI may subsume some of Eglot's
> bespoke UI, but it may also be complementary and the two can coexist,
> perhaps with some LSP-specific use cases only provided by Eglot.

I don't think there's anything complex in Eglot's diagnostic-fixing UI that
can't be subsumed by any reasonable interface.

> for example, it could rely
> on some overlay property that backends would add to diagnostics and the
> UI of the new library would look for.  If we really don't want to teach
> Flymake about fixes, I think that could work.  WDYT?

What you're describing is similar, if not exactly identical to what
happens today

1. The overlay property you suggest exists.  It is 'keymap'
2. it's added to flymake diagnostics overlays via the diagnostic type property
   'flymake-overlay-control', part of Flymake's generic API for adding things
   to the overlay diagnostic.
3. The overlay _value_ is a simple keymap mapping the '[mouse-2]' binding to
   a function.  I think some users add more mappings to the keymap, which is
   bound to eglot-diagnostics-map.
4. The function is Eglot's.  It mixes UI and LSP logic, alas.
4.1 it expects the diagnostic at the mouse point to have a way to "produce"
    code edits (at first I thought this was async, but you're in luck: it's
    sync, which greatly simplified matters).
4.2 calls upon this logic to gather said edits
4.3 presents them to the user and applies them.  This is UI.

If you're suggesting to wrap all this setup in some function/macro of
newlibrary.el (the file name for your "new library") so that only the
complexity of 4.2 remains in eglot.el, then that's exactly what I'm
suggesting.

I just note that  newlibrary.el _will_ have to know about "edit-producing
diagnostics", which means knowing about edits, or at least a way to
apply them.
It will likely have to require 'refactor.el',  which is where the "edit" format
will live, and call on it to present and eventually apply those edits.

In fact, in my mind, newlibrary.el is so short that there's little
reason it shouldn't just be a small section of refactor.el.

But if it motivates you to start with a separate file, then newlibrary.el
(or whatever name you want to give it) is absolutely fine.

João

Re: Adding fix suggestions to Flymake diagnostics

[Eshel Yaron]

João Távora <joaotavora@gmail.com> writes:

> On Mon, May 27, 2024 at 1:15 PM Eshel Yaron <me@eshelyaron.com> wrote:
>
>> for example, it could rely on some overlay property that backends
>> would add to diagnostics and the UI of the new library would look
>> for.  If we really don't want to teach Flymake about fixes, I think
>> that could work.  WDYT?
>
> What you're describing is similar, if not exactly identical to what
> happens today

I'm describing/requesting a backend-agnostic UI for fixing diagnostics.
What happens in Eglot today is similar in that it's one example of a UI
for fixing diagnostics, but it's definitely a not generic one
(i.e. usable with other backends).

> 1. The overlay property you suggest exists.  It is 'keymap'
> 2. it's added to flymake diagnostics overlays via the diagnostic type property
>    'flymake-overlay-control', part of Flymake's generic API for adding things
>    to the overlay diagnostic.
> 3. The overlay _value_ is a simple keymap mapping the '[mouse-2]' binding to
>    a function.  I think some users add more mappings to the keymap, which is
>    bound to eglot-diagnostics-map.
> 4. The function is Eglot's.  It mixes UI and LSP logic, alas.
> [...]
> If you're suggesting to wrap all this setup in some function/macro of
> newlibrary.el (the file name for your "new library") so that only the
> complexity of 4.2 remains in eglot.el, then that's exactly what I'm
> suggesting.

More or less, yes.  Except the overlay property wouldn't be keymap but
something that just holds a function that produces the fix suggestion,
so as not to commit to a specific UI, like the fix-function argument of
flymake-make-diagnostic from my patch.

And again, I'm not especially interested in simplifying Eglot: that can
be a nice bonus, but it's fine IMO if Eglot ends up keeping its code
while also working with the standard API.

> I just note that newlibrary.el _will_ have to know about
> "edit-producing diagnostics", which means knowing about edits, or at
> least a way to apply them.  It will likely have to require
> 'refactor.el', which is where the "edit" format will live, and call on
> it to present and eventually apply those edits.
>
> In fact, in my mind, newlibrary.el is so short that there's little
> reason it shouldn't just be a small section of refactor.el.

Right.  We also needs to know about Flymake diagnostics and their
overlays, though, so the same argument suggests putting it in Flymake,
as I did in my draft implementation :)

But I think that we agree on the essence, and where the code lives isn't
that crucial anyway, so I don't mind moving this stuff to refactor.el or
indeed to a new library if that's preferable.

Re: Adding fix suggestions to Flymake diagnostics

[João Távora]

On Mon, May 27, 2024 at 6:40 PM Eshel Yaron <me@eshelyaron.com> wrote:

> More or less, yes.  Except the overlay property wouldn't be keymap but
> something that just holds a function that produces the fix suggestion,

Overlays are for storing overlay known properties,  if you want to attach
arbitrary metadata to a diagnostic, I'd use the its `data` field.

> And again, I'm not especially interested in simplifying Eglot: that can
> be a nice bonus, but it's fine IMO if Eglot ends up keeping its code
> while also working with the standard API.

That's a shame, but your call. Eglot will have no more code to do the
same it already does.  I hope you've gotten my input of how I'd do it,
you're free to do it as you wish and I'm free to ignore it.

> > I just note that newlibrary.el _will_ have to know about
> > "edit-producing diagnostics", which means knowing about edits, or at
> > least a way to apply them.  It will likely have to require
> > 'refactor.el', which is where the "edit" format will live, and call on
> > it to present and eventually apply those edits.
> >
> > In fact, in my mind, newlibrary.el is so short that there's little
> > reason it shouldn't just be a small section of refactor.el.
>
> Right.  We also needs to know about Flymake diagnostics and their
> overlays, though, so the same argument suggests putting it in Flymake,
> as I did in my draft implementation :)

It's not a symmetrical situation.  It's quite different to use the
longstanding battle-tested, 4-major-version-old concept of "diagnostic"
in a new library and taiting a longstanding stable library with a
duplicated, brittle,  untested new concept that is completely foreign
to its genesis.

Re: Adding fix suggestions to Flymake diagnostics

[Eshel Yaron]

[-- Attachment #1: Type: text/plain, Size: 2646 bytes --]

Hi,

João Távora <joaotavora@gmail.com> writes:

> On Mon, May 27, 2024 at 6:40 PM Eshel Yaron <me@eshelyaron.com> wrote:
>
>> And again, I'm not especially interested in simplifying Eglot: that can
>> be a nice bonus, but it's fine IMO if Eglot ends up keeping its code
>> while also working with the standard API.
>
> That's a shame, but your call. Eglot will have no more code to do the
> same it already does.  I hope you've gotten my input of how I'd do it,
> you're free to do it as you wish and I'm free to ignore it.

I got your input and I appreciate your time and thought.  Thanks.

To avoid getting too caught up on the specifics of Eglot, we can use
another Flymake backend to demonstrate how a standard fix suggestions
feature might look.  I'm attaching below a new patch that adds fix
suggestions for shellcheck diagnostics in sh-mode via the same API.

Here's an example shell script you can use to see how it works:

--8<---------------cut here---------------start------------->8---
#!/bin/bash

echo foo/$1/baz
--8<---------------cut here---------------end--------------->8---

Shellcheck complains that $1 better be quoted, and we see that via a
Flymake diagnostic.  Go to that diagnostic and say M-x flymake-fix RET
to fix it.  (This still uses refactor.el for applying the code edits.)

>> > I just note that newlibrary.el _will_ have to know about
>> > "edit-producing diagnostics", which means knowing about edits, or at
>> > least a way to apply them.  It will likely have to require
>> > 'refactor.el', which is where the "edit" format will live, and call on
>> > it to present and eventually apply those edits.
>> >
>> > In fact, in my mind, newlibrary.el is so short that there's little
>> > reason it shouldn't just be a small section of refactor.el.
>>
>> Right.  We also needs to know about Flymake diagnostics and their
>> overlays, though, so the same argument suggests putting it in Flymake,
>> as I did in my draft implementation :)
>
> It's not a symmetrical situation.  It's quite different to use the
> longstanding battle-tested, 4-major-version-old concept of "diagnostic"
> in a new library and taiting a longstanding stable library with a
> duplicated, brittle,  untested new concept that is completely foreign
> to its genesis.

Flymake communicates with programs that produce diagnostics, along with
fixes that describe suggested code edits.  I agree that code edits are
not the core business of Flymake, but characterizing code edits as
"completely foreign to Flymake's genesis" seems somewhat exaggerated.

Anyway, here's that new patch:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: shellcheck-fix.patch --]
[-- Type: text/x-patch, Size: 6232 bytes --]

diff --git a/lisp/progmodes/flymake.el b/lisp/progmodes/flymake.el
index 2e602658ea7..cf5349765db 100644
--- a/lisp/progmodes/flymake.el
+++ b/lisp/progmodes/flymake.el
@@ -368,7 +368,7 @@ flymake-error
   locus beg end type text backend data overlay-properties overlay
   ;; FIXME: See usage of these two in `flymake--highlight-line'.
   ;; Ideally they wouldn't be needed.
-  orig-beg orig-end)
+  orig-beg orig-end fix-function)
 
 ;;;###autoload
 (defun flymake-make-diagnostic (locus
@@ -377,7 +377,8 @@ flymake-make-diagnostic
                                 type
                                 text
                                 &optional data
-                                overlay-properties)
+                                overlay-properties
+                                fix-function)
   "Make a Flymake diagnostic for LOCUS's region from BEG to END.
 LOCUS is a buffer object or a string designating a file name.
 
@@ -396,14 +397,24 @@ flymake-make-diagnostic
 OVERLAY-PROPERTIES is an alist of properties attached to the
 created diagnostic, overriding the default properties and any
 properties listed in the `flymake-overlay-control' property of
-the diagnostic's type symbol."
+the diagnostic's type symbol.
+
+FIX-FUNCTION, if non-nil, is a function that takes DATA and returns a
+list of fix suggestions for this diagnostic.  Each fix suggestion is a
+list (TITLE EDITS), where TITLE is a string describing the fix and EDITS
+is a list of (FILE-OR-BUFFER . CHANGES) cons cells, where FILE-OR-BUFFER
+is the file name or buffer to edit, and CHANGES is a list of changes to
+perform in FILE-OR-BUFFER.  Each element of CHANGES is in turn a
+list (BEG END STR), where BEG and END are buffer positions to delete and
+STR is the string to insert at BEG afterwards."
   (when (stringp locus)
     (setq locus (expand-file-name locus)))
   (flymake--diag-make :locus locus :beg beg :end end
                       :type type :text text :data data
                       :overlay-properties overlay-properties
                       :orig-beg beg
-                      :orig-end end))
+                      :orig-end end
+                      :fix-function fix-function))
 
 ;;;###autoload
 (defun flymake-diagnostics (&optional beg end)
@@ -849,6 +860,42 @@ flymake--update-eol-overlays
             (overlay-put o 'before-string (flymake--eol-overlay-summary src-ovs))
           (delete-overlay o))))))
 
+(defun flymake-diagnostic-context-menu (menu click)
+  "Extend MENU with fix suggestions for diagnostic at CLICK."
+  (when-let ((diag (seq-find #'flymake--diag-fix-function
+                             (flymake-diagnostics
+                              (posn-point (event-start click)))))
+             (fixes (funcall (flymake--diag-fix-function diag)
+                             (flymake--diag-data diag)))
+             (i 1))
+    (dolist (fix fixes)
+      (define-key menu (vector (intern (format "flymake-fix-%d" i)))
+                  `(menu-item ,(format "Fix: %s" (car fix))
+                              ,(lambda ()
+                                 (interactive)
+                                 (refactor-apply-edits (cadr fix)))
+                              ,@(cddr fix)))
+      (cl-incf i)))
+  menu)
+
+(defun flymake-fix (pos)
+  "Fix Flymake diagnostic at POS."
+  (interactive "d")
+  ;; TODO - fix _all_ diagnostics at point.
+  (if-let ((diags (flymake-diagnostics pos)))
+      (if-let ((diag (seq-find #'flymake--diag-fix-function diags))
+               (fixes (funcall (flymake--diag-fix-function diag)
+                               (flymake--diag-data diag))))
+          (refactor-apply-edits
+           (car (if (cdr fixes)
+                    (alist-get
+                     (completing-read (format-prompt "Fix" (caar fixes))
+                                      fixes nil t nil nil (caar fixes))
+                     fixes nil nil #'string=)
+                  (cdar fixes))))
+        (message "No fix available for this diagnostic"))
+    (user-error "No diagnostic at this position")))
+
 (cl-defun flymake--highlight-line (diagnostic &optional foreign)
   "Attempt to overlay DIAGNOSTIC in current buffer.
 
@@ -956,6 +1003,7 @@ flymake--highlight-line
              (flymake-diagnostics pos)
              "\n"))))
       (default-maybe 'severity (warning-numeric-level :error))
+      (default-maybe 'context-menu-functions '(flymake-diagnostic-context-menu))
       ;; Use (PRIMARY . SECONDARY) priority, to avoid clashing with
       ;; `region' face, for example (bug#34022).
       (default-maybe 'priority (cons nil (+ 40 (overlay-get ov 'severity)))))
diff --git a/lisp/progmodes/sh-script.el b/lisp/progmodes/sh-script.el
index a348e9ba6fd..fccee14af82 100644
--- a/lisp/progmodes/sh-script.el
+++ b/lisp/progmodes/sh-script.el
@@ -3195,6 +3195,21 @@ sh-shellcheck-arguments
 
 (defvar-local sh--shellcheck-process nil)
 
+(defun sh-shellcheck-fix (data)
+  "Format DATA, a cons cell (TITLE . FIX), as a Flymake fix suggestion."
+  `((,(car data)
+     ((,(current-buffer)
+       ,@(mapcar
+          (lambda (rep)
+            (let-alist rep
+              (without-restriction
+                (save-excursion
+                  (goto-char (point-min))
+                  (list (1- (+ (pos-bol .line) .column))
+                        (1- (+ (pos-bol .endLine) .endColumn))
+                        .replacement)))))
+          (alist-get 'replacements (cdr data))))))))
+
 (defun sh-shellcheck-flymake (report-fn &rest _args)
   "Flymake backend using the shellcheck program.
 Takes a Flymake callback REPORT-FN as argument, as expected of a
@@ -3236,7 +3251,9 @@ sh-shellcheck-flymake
                                 ("error" :error)
                                 ("warning" :warning)
                                 (_ :note))
-                              (format "SC%s: %s" .code .message)))))
+                              (format "SC%s: %s" .code .message)
+                              (cons .message .fix) nil
+                              (when (consp .fix) #'sh-shellcheck-fix)))))
                         (funcall report-fn))))
                 (kill-buffer (process-buffer proc)))))))
     (unless dialect

Re: Adding fix suggestions to Flymake diagnostics

[João Távora]

On Fri, May 31, 2024 at 8:15 AM Eshel Yaron <me@eshelyaron.com> wrote:

> not the core business of Flymake, but characterizing code edits as
> "completely foreign to Flymake's genesis" seems somewhat exaggerated.

How so?  It's pretty accurate.  The genesis of Flymake in its current
form some was ~7/8 years ago and it had 0 lines of buffer-changing
code. Even before that, in fact. So if you agree it's not its "core
business", just use one of many Lisp techniques to invert the dependency.
Put the new thing in the new library and make it use the old stable
thing too. Get the same, but cleaner.

Re: Adding fix suggestions to Flymake diagnostics

[Eshel Yaron]

Hi João,

João Távora <joaotavora@gmail.com> writes:

> Put the new thing in the new library and make it use the old stable
> thing too. Get the same, but cleaner.

Yes, adding diagnostic fixes via a new library might work, that's what
I suggested upthread.  As you pointed out, that new library turns out
very small, so I'm not really sure it's worth adding a whole new file.

Spencer, do you perhaps have a preference either way?  Other thoughts?


Thanks,

Eshel