bug#67152: [PATCH] Fix flymake integration in lua-ts-mode

bug#67152: [PATCH] Fix flymake integration in lua-ts-mode

[jm--- via Bug reports for GNU Emacs, the Swiss army knife of text editors]

If a file has an error on the last line flymake gets an invalid
position for the overlay and raises the error:

  error in process sentinel: Wrong type argument: integer-or-marker-p, nil

bug#67152: [PATCH] Fix flymake integration in lua-ts-mode

[jm--- via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-flymake-integration-in-lua-ts-mode-Bug-67152.patch --]
[-- Type: text/x-diff; name="0001-Fix-flymake-integration-in-lua-ts-mode-Bug-67152.patch", Size: 2087 bytes --]

From ab3ecedb9e4ed4818603249e774dd8a1e6dae28b Mon Sep 17 00:00:00 2001
From: john muhl <jm@pub.pink>
Date: Mon, 13 Nov 2023 16:06:07 -0600
Subject: [PATCH] Fix flymake integration in lua-ts-mode (Bug#67152)

* lisp/progmodes/lua-ts-mode.el (lua-ts-flymake-luacheck): Use
flymake-diag-region to mark highlighted region.
---
 lisp/progmodes/lua-ts-mode.el | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/lisp/progmodes/lua-ts-mode.el b/lisp/progmodes/lua-ts-mode.el
index bb6d5cb8c91..ad753210dd4 100644
--- a/lisp/progmodes/lua-ts-mode.el
+++ b/lisp/progmodes/lua-ts-mode.el
@@ -508,16 +508,18 @@ lua-ts-flymake-luacheck
                                             eol))
                                    nil t)
                             for line = (string-to-number (match-string 1))
-                            for beg = (string-to-number (match-string 2))
-                            for end = (string-to-number (match-string 3))
+                            for (beg . end) = (flymake-diag-region
+                                               source
+                                               (string-to-number (match-string 1))
+                                               (string-to-number (match-string 2)))
                             for msg = (match-string 4)
                             for type = (if (string-match "^(W" msg)
                                            :warning
                                          :error)
                             when (and beg end)
                             collect (flymake-make-diagnostic source
-                                                             (cons line beg)
-                                                             (cons line (1+ end))
+                                                             beg
+                                                             end
                                                              type
                                                              msg)
                             into diags
-- 
2.41.0

bug#67152: [PATCH] Fix flymake integration in lua-ts-mode

[Eli Zaretskii]

> Date: Mon, 13 Nov 2023 22:40:36 +0000
> TLS-Required: No
> From: jm--- via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>

João, is this OK?

> From ab3ecedb9e4ed4818603249e774dd8a1e6dae28b Mon Sep 17 00:00:00 2001
> From: john muhl <jm@pub.pink>
> Date: Mon, 13 Nov 2023 16:06:07 -0600
> Subject: [PATCH] Fix flymake integration in lua-ts-mode (Bug#67152)
> 
> * lisp/progmodes/lua-ts-mode.el (lua-ts-flymake-luacheck): Use
> flymake-diag-region to mark highlighted region.
> ---
>  lisp/progmodes/lua-ts-mode.el | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/lisp/progmodes/lua-ts-mode.el b/lisp/progmodes/lua-ts-mode.el
> index bb6d5cb8c91..ad753210dd4 100644
> --- a/lisp/progmodes/lua-ts-mode.el
> +++ b/lisp/progmodes/lua-ts-mode.el
> @@ -508,16 +508,18 @@ lua-ts-flymake-luacheck
>                                              eol))
>                                     nil t)
>                              for line = (string-to-number (match-string 1))
> -                            for beg = (string-to-number (match-string 2))
> -                            for end = (string-to-number (match-string 3))
> +                            for (beg . end) = (flymake-diag-region
> +                                               source
> +                                               (string-to-number (match-string 1))
> +                                               (string-to-number (match-string 2)))
>                              for msg = (match-string 4)
>                              for type = (if (string-match "^(W" msg)
>                                             :warning
>                                           :error)
>                              when (and beg end)
>                              collect (flymake-make-diagnostic source
> -                                                             (cons line beg)
> -                                                             (cons line (1+ end))
> +                                                             beg
> +                                                             end
>                                                               type
>                                                               msg)
>                              into diags
> -- 
> 2.41.0
> 

bug#67152: [PATCH] Fix flymake integration in lua-ts-mode

[João Távora]

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

On Tue, Nov 14, 2023 at 2:13 PM Eli Zaretskii <eliz@gnu.org> wrote:

> > Date: Mon, 13 Nov 2023 22:40:36 +0000
> > TLS-Required: No
> > From: jm--- via "Bug reports for GNU Emacs,
> >  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>
> João, is this OK?
>

Oooof hard to say.  But no I don't think so.  You see the type
of the object changing in the call to flymake-make-diagnostic?
That could be seriously problematic for non-buffer-local diagnostics of
some  backends.

And another call to flymake-diag-region?

Can you point me to the first message in this thread via a debbugs
URL, or to one where a clear Emacs -Q recipe is given?

Thanks,
João

[-- Attachment #2: Type: text/html, Size: 1547 bytes --]

bug#67152: [PATCH] Fix flymake integration in lua-ts-mode

[João Távora]

On Tue, Nov 14, 2023 at 2:23 PM João Távora <joaotavora@gmail.com> wrote:
>
> On Tue, Nov 14, 2023 at 2:13 PM Eli Zaretskii <eliz@gnu.org> wrote:
>>
>> > Date: Mon, 13 Nov 2023 22:40:36 +0000
>> > TLS-Required: No
>> > From: jm--- via "Bug reports for GNU Emacs,
>> >  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>>
>> João, is this OK?
>
> Oooof hard to say.  But no I don't think so.

Oh no! Silly me, this isn't flymake.el at all!!  I saw a fragment
of a loop and thought it was my code :-), i.e. a framework problem.

Well, then in this case, I think it should be OK, though users should
test. Backends are indeed expected to call flymake-diag-region to
get the region to highlight, as the manual and docstrings explain
 (I think).

João

bug#67152: [PATCH] Fix flymake integration in lua-ts-mode

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Tue, 14 Nov 2023 14:23:29 +0000
> Cc: jm@pub.pink, 67152@debbugs.gnu.org
> 
> Can you point me to the first message in this thread via a debbugs
> URL, or to one where a clear Emacs -Q recipe is given?

https://debbugs.gnu.org/cgi/bugreport.cgi?bug=67152#5

bug#67152: [PATCH] Fix flymake integration in lua-ts-mode

[jm--- via Bug reports for GNU Emacs, the Swiss army knife of text editors]

November 14, 2023 at 2:26 PM, "João Távora" <joaotavora@gmail.com> wrote:

> Oh no! Silly me, this isn't flymake.el at all!! I saw a fragment
> of a loop and thought it was my code :-), i.e. a framework problem.

Right. The problem is the code in lua-ts-mode. Here is the full
recipe just in case (you need the tree-sitter-lua grammar and
luacheck in PATH or set lua-ts-luacheck-program):

  src/emacs test.lua --init-directory=.
  M-x lua-ts-mode

  # test.lua
  print(1)
  print(2

  # init.el
  (add-hook 'lua-ts-mode-hook #'flymake-mode)
  (add-to-list 'treesit-extra-load-path "~/.guix-profile/lib/tree-sitter")
  (setopt lua-ts-luacheck-program "~/.luarocks/bin/luacheck")
  (toggle-debug-on-error)

The error only shows up when the highlighted region is on the
last line.

> Well, then in this case, I think it should be OK,

Thanks for confirming.

> though users should test.

What kind of test do you mean?

> Backends are indeed expected to call flymake-diag-region to
> get the region to highlight, as the manual and docstrings explain
>  (I think).

I agree the docs are clear. I even used the example from the
manual as a template. Now I can’t remember why I originally
deviated from it on the region handling but I’m sure it was
caused by a misunderstanding on my part.

bug#67152: [PATCH] Fix flymake integration in lua-ts-mode

[jm--- via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Updated to remove a compiler warning about the unused 'line' variable.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-flymake-integration-in-lua-ts-mode-Bug-67152.patch --]
[-- Type: text/x-diff; name="0001-Fix-flymake-integration-in-lua-ts-mode-Bug-67152.patch", Size: 2151 bytes --]

From c6a443216075ba2f2f61d52b3261d73fac7d3d9a Mon Sep 17 00:00:00 2001
From: john muhl <jm@pub.pink>
Date: Mon, 13 Nov 2023 16:06:07 -0600
Subject: [PATCH] Fix flymake integration in lua-ts-mode (Bug#67152)

* lisp/progmodes/lua-ts-mode.el (lua-ts-flymake-luacheck): Use
flymake-diag-region to mark highlighted region.
---
 lisp/progmodes/lua-ts-mode.el | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/lisp/progmodes/lua-ts-mode.el b/lisp/progmodes/lua-ts-mode.el
index bb6d5cb8c91..a910d759c83 100644
--- a/lisp/progmodes/lua-ts-mode.el
+++ b/lisp/progmodes/lua-ts-mode.el
@@ -507,17 +507,18 @@ lua-ts-flymake-luacheck
                                             (group (0+ nonl))
                                             eol))
                                    nil t)
-                            for line = (string-to-number (match-string 1))
-                            for beg = (string-to-number (match-string 2))
-                            for end = (string-to-number (match-string 3))
+                            for (beg . end) = (flymake-diag-region
+                                               source
+                                               (string-to-number (match-string 1))
+                                               (string-to-number (match-string 2)))
                             for msg = (match-string 4)
                             for type = (if (string-match "^(W" msg)
                                            :warning
                                          :error)
                             when (and beg end)
                             collect (flymake-make-diagnostic source
-                                                             (cons line beg)
-                                                             (cons line (1+ end))
+                                                             beg
+                                                             end
                                                              type
                                                              msg)
                             into diags
-- 
2.41.0

bug#67152: [PATCH] Fix flymake integration in lua-ts-mode

[João Távora]

Just a note that in the recent patch you provided, (match-string 3)
isn't being used.  If is is a useful "end column" identifier, you
might want to call flymake-diag-region again to get the the
corresponding buffer position.

flymake-diag-region could see some docstring improvements.
It gets a line and optionally a column, whidh defines a
single point.  It then tries to guess a region, because it's
impossible to form a region from just a point.

The guess is based on thing-at-point. This guess _may_ be worse
than whatever the backend supplied in (match-string 3) so passing
that number to flymake-diag-region again to obtain a second point
may be a good idea to compose the two buffer positions to give
to flymake-make-diagnostic.

João

bug#67152: [PATCH] Fix flymake integration in lua-ts-mode

[Eli Zaretskii]

> Date: Wed, 15 Nov 2023 00:09:20 +0000
> TLS-Required: No
> From: jm--- via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> Updated to remove a compiler warning about the unused 'line' variable.

Thanks, installed on the master branch, and closing the bug.