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

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

[Dmitry Gutov]

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

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

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@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

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

[Dmitry Gutov]

On 12.12.2020 13:39, Lars Ingebrigtsen wrote:
> Dmitry Gutov <dgutov@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.

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

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@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

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

[João Távora]

Lars Ingebrigtsen <larsi@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

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

[Dmitry Gutov]

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.

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

[João Távora]

On Sun, Dec 13, 2020 at 8:55 PM Dmitry Gutov <dgutov@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

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

[Dmitry Gutov]

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

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

[Glenn Morris]

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

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

[Dmitry Gutov]

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

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

[João Távora]

On Fri, Dec 18, 2020 at 2:07 AM Dmitry Gutov <dgutov@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

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

[Dmitry Gutov]

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

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

[João Távora]

On Fri, Dec 18, 2020 at 3:22 PM Dmitry Gutov <dgutov@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).

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

[Dmitry Gutov]

On 18.12.2020 17:26, João Távora wrote:
> On Fri, Dec 18, 2020 at 3:22 PM Dmitry Gutov <dgutov@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.

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

[João Távora]

On Fri, Dec 18, 2020 at 3:29 PM Dmitry Gutov <dgutov@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