GNU bug report logs - #29193
26.0.90; Using (thing-at-point 'sexp) in flymake-diag-region might be suboptimal

Previous Next

Package: emacs;

Reported by: Dmitry Gutov <dgutov <at> yandex.ru>

Date: Tue, 7 Nov 2017 15:29:01 UTC

Severity: minor

Found in version 26.0.90

Done: Dmitry Gutov <dgutov <at> yandex.ru>

Bug is archived. No further changes may be made.

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 29193 in the body.
You can then email your comments to 29193 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to bug-gnu-emacs <at> gnu.org:
bug#29193; Package emacs. (Tue, 07 Nov 2017 15:29:01 GMT) Full text and rfc822 format available.

Acknowledgement sent to Dmitry Gutov <dgutov <at> yandex.ru>:
New bug report received and forwarded. Copy sent to bug-gnu-emacs <at> gnu.org. (Tue, 07 Nov 2017 15:29:01 GMT) Full text and rfc822 format available.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: bug-gnu-emacs <at> gnu.org
Subject: 26.0.90; Using (thing-at-point 'sexp) in flymake-diag-region might be
 suboptimal
Date: Tue, 7 Nov 2017 17:28:06 +0200
Example:

Rubocop can report warning when 'end' is at wrong column. It just
reports the beginning column, of course.

In ruby-mode, (thing-at-point 'sexp) signals an error at this position.
I'm not sure exactly whether it's a problem in ruby-mode.

But Flycheck uses (thing-at-poing 'symbol) for the same purpose, and the
whole 'end' token gets highlighted (which is probably what we expect).

In contrast, Flymake only highlights its first character ('e').

++

In GNU Emacs 26.0.90 (build 5, x86_64-pc-linux-gnu, GTK+ Version 3.22.11)
 of 2017-11-07 built on zappa
Repository revision: ca2d94ba61dee678f85bfc7299d167e7219e6d8f
Windowing system distributor 'The X.Org Foundation', version 11.0.11903000
System Description:	Ubuntu 17.04




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#29193; Package emacs. (Sat, 12 Dec 2020 11:40:01 GMT) Full text and rfc822 format available.

Message #8 received at 29193 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 29193 <at> debbugs.gnu.org
Subject: Re: bug#29193: 26.0.90; Using (thing-at-point 'sexp) in
 flymake-diag-region might be suboptimal
Date: Sat, 12 Dec 2020 12:39:16 +0100
Dmitry Gutov <dgutov <at> yandex.ru> writes:

> Rubocop can report warning when 'end' is at wrong column. It just
> reports the beginning column, of course.
>
> In ruby-mode, (thing-at-point 'sexp) signals an error at this position.
> I'm not sure exactly whether it's a problem in ruby-mode.
>
> But Flycheck uses (thing-at-poing 'symbol) for the same purpose, and the
> whole 'end' token gets highlighted (which is probably what we expect).
>
> In contrast, Flymake only highlights its first character ('e').

I had a look at the current `flymake-diag-region', and it does not use
(thing-at-point 'sexp) at present.  It does use (end-of-thing 'sexp),
though.

So is this problem still present?  There was no recipe for reproduction,
so it's not immediately clear.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#29193; Package emacs. (Sat, 12 Dec 2020 23:57:02 GMT) Full text and rfc822 format available.

Message #11 received at 29193 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 29193 <at> debbugs.gnu.org
Subject: Re: bug#29193: 26.0.90; Using (thing-at-point 'sexp) in
 flymake-diag-region might be suboptimal
Date: Sun, 13 Dec 2020 01:56:13 +0200
On 12.12.2020 13:39, Lars Ingebrigtsen wrote:
> Dmitry Gutov <dgutov <at> yandex.ru> writes:
> 
>> Rubocop can report warning when 'end' is at wrong column. It just
>> reports the beginning column, of course.
>>
>> In ruby-mode, (thing-at-point 'sexp) signals an error at this position.
>> I'm not sure exactly whether it's a problem in ruby-mode.
>>
>> But Flycheck uses (thing-at-poing 'symbol) for the same purpose, and the
>> whole 'end' token gets highlighted (which is probably what we expect).
>>
>> In contrast, Flymake only highlights its first character ('e').
> 
> I had a look at the current `flymake-diag-region', and it does not use
> (thing-at-point 'sexp) at present.  It does use (end-of-thing 'sexp),
> though.

The behavior is the same: when 'end' is misindented, only 'e' in 'end' 
is highlighted.

Whereas the message is:

Layout/EndAlignment: ‘end‘ at 23, 3 is not aligned with ‘class‘ at 5, 4.

That highlighting is not too hard to interpret, but we could do better, 
probably.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#29193; Package emacs. (Sun, 13 Dec 2020 12:53:01 GMT) Full text and rfc822 format available.

Message #14 received at 29193 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 29193 <at> debbugs.gnu.org,
 João Távora <joaotavora <at> gmail.com>
Subject: Re: bug#29193: 26.0.90; Using (thing-at-point 'sexp) in
 flymake-diag-region might be suboptimal
Date: Sun, 13 Dec 2020 13:51:56 +0100
Dmitry Gutov <dgutov <at> yandex.ru> writes:

>> I had a look at the current `flymake-diag-region', and it does not
>> use
>> (thing-at-point 'sexp) at present.  It does use (end-of-thing 'sexp),
>> though.
>
> The behavior is the same: when 'end' is misindented, only 'e' in 'end'
> is highlighted.
>
> Whereas the message is:
>
> Layout/EndAlignment: ‘end‘ at 23, 3 is not aligned with ‘class‘ at 5, 4.
>
> That highlighting is not too hard to interpret, but we could do
> better, probably.

OK, adding João to the Cc's.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#29193; Package emacs. (Sun, 13 Dec 2020 13:20:01 GMT) Full text and rfc822 format available.

Message #17 received at 29193 <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 29193 <at> debbugs.gnu.org, Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#29193: 26.0.90; Using (thing-at-point 'sexp) in
 flymake-diag-region might be suboptimal
Date: Sun, 13 Dec 2020 13:19:08 +0000
Lars Ingebrigtsen <larsi <at> gnus.org> writes:

> OK, adding João to the Cc's.

Not sure why 'symbol and 'sexp don't reference the same "thing", or at
least stuff consistent in terms of nesting.  In my testing
(thing-at-point 'sexp) at the "e" got me nothing, but at the "n" or "d"
got me the whole sexp.  Similar strange situation with "begin".  "b"
sees the whole sexp, any of "egin" doesn't.  This seems to resist
different indentations.

In the end flymake-diag-region tries its best to guess a region from
limited line/col stuff and asks thingatpt.el for help, which in turn
will probably ask the major-mode for syntactic navigation.

So I wouldn't say the problem is in flymake.el, but in whom is
untimately providing the (broken) goods.  Then again maybe this untested
patch would work and not break much stuff for other backends...

diff --git a/lisp/progmodes/flymake.el b/lisp/progmodes/flymake.el
index efa7b2ffbf..6c3e0a1981 100644
--- a/lisp/progmodes/flymake.el
+++ b/lisp/progmodes/flymake.el
@@ -437,7 +437,8 @@ flymake-diag-region
               (if (and col (cl-plusp col))
                   (let* ((beg (progn (forward-char (1- col))
                                      (point)))
-                         (sexp-end (ignore-errors (end-of-thing 'sexp)))
+                         (sexp-end (or (ignore-errors (end-of-thing 'sexp))
+                                       (ignore-errors (end-of-thing 'symbol))))
                          (end (or (and sexp-end
                                        (not (= sexp-end beg))
                                        sexp-end)


Other than that, I don't have much to add.  I don't have the resources
to debug ruby-mode and/or install rubocop right now.  Good luck.

João








Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#29193; Package emacs. (Sun, 13 Dec 2020 20:56:01 GMT) Full text and rfc822 format available.

Message #20 received at 29193 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: João Távora <joaotavora <at> gmail.com>,
 Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 29193 <at> debbugs.gnu.org
Subject: Re: bug#29193: 26.0.90; Using (thing-at-point 'sexp) in
 flymake-diag-region might be suboptimal
Date: Sun, 13 Dec 2020 22:55:42 +0200
On 13.12.2020 15:19, João Távora wrote:
> -                         (sexp-end (ignore-errors (end-of-thing 'sexp)))
> +                         (sexp-end (or (ignore-errors (end-of-thing 'sexp))
> +                                       (ignore-errors (end-of-thing 'symbol))))

That seems to fix it, thank you.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#29193; Package emacs. (Mon, 14 Dec 2020 11:04:01 GMT) Full text and rfc822 format available.

Message #23 received at 29193 <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, 29193 <at> debbugs.gnu.org
Subject: Re: bug#29193: 26.0.90; Using (thing-at-point 'sexp) in
 flymake-diag-region might be suboptimal
Date: Mon, 14 Dec 2020 11:03:13 +0000
On Sun, Dec 13, 2020 at 8:55 PM Dmitry Gutov <dgutov <at> yandex.ru> wrote:
>
> On 13.12.2020 15:19, João Távora wrote:
> > -                         (sexp-end (ignore-errors (end-of-thing 'sexp)))
> > +                         (sexp-end (or (ignore-errors (end-of-thing 'sexp))
> > +                                       (ignore-errors (end-of-thing 'symbol))))
>
> That seems to fix it, thank you.

Feel free to push if you can't come up with anything better in ruby-mode.

João




Reply sent to Dmitry Gutov <dgutov <at> yandex.ru>:
You have taken responsibility. (Wed, 16 Dec 2020 00:57:01 GMT) Full text and rfc822 format available.

Notification sent to Dmitry Gutov <dgutov <at> yandex.ru>:
bug acknowledged by developer. (Wed, 16 Dec 2020 00:57:02 GMT) Full text and rfc822 format available.

Message #28 received at 29193-done <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: João Távora <joaotavora <at> gmail.com>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, 29193-done <at> debbugs.gnu.org
Subject: Re: bug#29193: 26.0.90; Using (thing-at-point 'sexp) in
 flymake-diag-region might be suboptimal
Date: Wed, 16 Dec 2020 02:55:04 +0200
On 14.12.2020 13:03, João Távora wrote:
> Feel free to push if you can't come up with anything better in ruby-mode.

Pushed, and closing, thanks.

Regarding "doing the fix in the proper place", the alternative would be 
changing the implementation of thing-at-point--end-of-sexp.

But I'm not sure it's a well-defined function. It clearly special-cases 
characters with class "closing paren". And there's no clear counterpart 
for 'end' (even though it's a sexp closer in ruby-mode's syntax).




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#29193; Package emacs. (Thu, 17 Dec 2020 23:26:01 GMT) Full text and rfc822 format available.

Message #31 received at 29193 <at> debbugs.gnu.org (full text, mbox):

From: Glenn Morris <rgm <at> gnu.org>
To: 29193 <at> debbugs.gnu.org
Cc: Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#29193: 26.0.90;
 Using (thing-at-point 'sexp) in flymake-diag-region might be
 suboptimal
Date: Thu, 17 Dec 2020 18:25:09 -0500
fda9a2 causes a test failure.

Ref eg: https://hydra.nixos.org/build/132986117

(Are people trying to run foo-tests.el before pushing changes to foo.el?)




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#29193; Package emacs. (Fri, 18 Dec 2020 02:07:02 GMT) Full text and rfc822 format available.

Message #34 received at 29193 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Glenn Morris <rgm <at> gnu.org>, 29193 <at> debbugs.gnu.org
Subject: Re: bug#29193: 26.0.90; Using (thing-at-point 'sexp) in
 flymake-diag-region might be suboptimal
Date: Fri, 18 Dec 2020 04:06:05 +0200
On 18.12.2020 01:25, Glenn Morris wrote:
> fda9a2 causes a test failure.
> 
> Ref eg:https://hydra.nixos.org/build/132986117

Thanks! It was a curious bug: one would expect that some highlighting's 
boundaries might change. What changed, however, is one's _type_.

I've pushed a fix, and I'll let the Flymake's maintainer sort out at his 
leisure whether flymake-diag-region can be relied on not to change match 
data, as several of its clients seem to assume.

> (Are people trying to run foo-tests.el before pushing changes to foo.el?)

Some of us are spoiled enough to expect an email from the CI when some 
breakage occurs.

(Sorry.)




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#29193; Package emacs. (Fri, 18 Dec 2020 11:43:02 GMT) Full text and rfc822 format available.

Message #37 received at 29193 <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: Glenn Morris <rgm <at> gnu.org>, 29193 <at> debbugs.gnu.org
Subject: Re: bug#29193: 26.0.90; Using (thing-at-point 'sexp) in
 flymake-diag-region might be suboptimal
Date: Fri, 18 Dec 2020 11:42:02 +0000
On Fri, Dec 18, 2020 at 2:07 AM Dmitry Gutov <dgutov <at> yandex.ru> wrote:
>
> On 18.12.2020 01:25, Glenn Morris wrote:
> > fda9a2 causes a test failure.
> >
> > Ref eg:https://hydra.nixos.org/build/132986117
>
> Thanks! It was a curious bug: one would expect that some highlighting's
> boundaries might change. What changed, however, is one's _type_.
>
> I've pushed a fix, and I'll let the Flymake's maintainer sort out at his
> leisure whether flymake-diag-region can be relied on not to change match
> data, as several of its clients seem to assume.

I think this is rather a thingatpt.el issue, which makes no mention of
match-data destruction.  Regardless, it's decent enough to do that in
flymake-diag-region so I pushed save-match-data a bit higher.

Thanks,
João




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#29193; Package emacs. (Fri, 18 Dec 2020 15:23:01 GMT) Full text and rfc822 format available.

Message #40 received at 29193 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: João Távora <joaotavora <at> gmail.com>
Cc: Glenn Morris <rgm <at> gnu.org>, 29193 <at> debbugs.gnu.org
Subject: Re: bug#29193: 26.0.90; Using (thing-at-point 'sexp) in
 flymake-diag-region might be suboptimal
Date: Fri, 18 Dec 2020 17:22:18 +0200
On 18.12.2020 13:42, João Távora wrote:

> I think this is rather a thingatpt.el issue, which makes no mention of
> match-data destruction.  Regardless, it's decent enough to do that in
> flymake-diag-region so I pushed save-match-data a bit higher.

By default we assume that any function can destroy match-data (and only 
a certain set doesn't).

But it's an easy mistake to make, especially given the fairly opaque 
implementation of (end-of-thing 'symbol).




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#29193; Package emacs. (Fri, 18 Dec 2020 15:27:01 GMT) Full text and rfc822 format available.

Message #43 received at 29193 <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: Glenn Morris <rgm <at> gnu.org>, 29193 <at> debbugs.gnu.org
Subject: Re: bug#29193: 26.0.90; Using (thing-at-point 'sexp) in
 flymake-diag-region might be suboptimal
Date: Fri, 18 Dec 2020 15:26:26 +0000
On Fri, Dec 18, 2020 at 3:22 PM Dmitry Gutov <dgutov <at> yandex.ru> wrote:
>
> On 18.12.2020 13:42, João Távora wrote:
>
> > I think this is rather a thingatpt.el issue, which makes no mention of
> > match-data destruction.  Regardless, it's decent enough to do that in
> > flymake-diag-region so I pushed save-match-data a bit higher.
>
> By default we assume that any function can destroy match-data (and only
> a certain set doesn't).

By that reasoning the problem should be solved in whoever called
flymake-diag-region and wrongly assumed it would keep match data.
Doesn't a seem very friendly API tho (and no idea who it was, anyway).




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#29193; Package emacs. (Fri, 18 Dec 2020 15:30:02 GMT) Full text and rfc822 format available.

Message #46 received at 29193 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: João Távora <joaotavora <at> gmail.com>
Cc: Glenn Morris <rgm <at> gnu.org>, 29193 <at> debbugs.gnu.org
Subject: Re: bug#29193: 26.0.90; Using (thing-at-point 'sexp) in
 flymake-diag-region might be suboptimal
Date: Fri, 18 Dec 2020 17:29:48 +0200
On 18.12.2020 17:26, João Távora wrote:
> On Fri, Dec 18, 2020 at 3:22 PM Dmitry Gutov <dgutov <at> yandex.ru> wrote:
>>
>> On 18.12.2020 13:42, João Távora wrote:
>>
>>> I think this is rather a thingatpt.el issue, which makes no mention of
>>> match-data destruction.  Regardless, it's decent enough to do that in
>>> flymake-diag-region so I pushed save-match-data a bit higher.
>>
>> By default we assume that any function can destroy match-data (and only
>> a certain set doesn't).
> 
> By that reasoning the problem should be solved in whoever called
> flymake-diag-region and wrongly assumed it would keep match data.

Yes.

> Doesn't a seem very friendly API tho (and no idea who it was, anyway).

Also true. So we can document that flymake-diag-region does indeed 
preserve match data.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#29193; Package emacs. (Fri, 18 Dec 2020 15:40:01 GMT) Full text and rfc822 format available.

Message #49 received at 29193 <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: Glenn Morris <rgm <at> gnu.org>, 29193 <at> debbugs.gnu.org
Subject: Re: bug#29193: 26.0.90; Using (thing-at-point 'sexp) in
 flymake-diag-region might be suboptimal
Date: Fri, 18 Dec 2020 15:39:07 +0000
On Fri, Dec 18, 2020 at 3:29 PM Dmitry Gutov <dgutov <at> yandex.ru> wrote:
> > Doesn't a seem very friendly API tho (and no idea who it was, anyway).
> Also true. So we can document that flymake-diag-region does indeed
> preserve match data.

Done




bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Sat, 16 Jan 2021 12:24:08 GMT) Full text and rfc822 format available.

This bug report was last modified 3 years and 73 days ago.

Previous Next


GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.