all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* bug#29193: 26.0.90; Using (thing-at-point 'sexp) in flymake-diag-region might be suboptimal
@ 2017-11-07 15:28 Dmitry Gutov
  2020-12-12 11:39 ` Lars Ingebrigtsen
       [not found] ` <handler.29193.D29193.160808016725606.notifdone@debbugs.gnu.org>
  0 siblings, 2 replies; 15+ messages in thread
From: Dmitry Gutov @ 2017-11-07 15:28 UTC (permalink / raw)
  To: 29193

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





^ permalink raw reply	[flat|nested] 15+ messages in thread

* bug#29193: 26.0.90; Using (thing-at-point 'sexp) in flymake-diag-region might be suboptimal
  2017-11-07 15:28 bug#29193: 26.0.90; Using (thing-at-point 'sexp) in flymake-diag-region might be suboptimal Dmitry Gutov
@ 2020-12-12 11:39 ` Lars Ingebrigtsen
  2020-12-12 23:56   ` Dmitry Gutov
       [not found] ` <handler.29193.D29193.160808016725606.notifdone@debbugs.gnu.org>
  1 sibling, 1 reply; 15+ messages in thread
From: Lars Ingebrigtsen @ 2020-12-12 11:39 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: 29193

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





^ permalink raw reply	[flat|nested] 15+ messages in thread

* bug#29193: 26.0.90; Using (thing-at-point 'sexp) in flymake-diag-region might be suboptimal
  2020-12-12 11:39 ` Lars Ingebrigtsen
@ 2020-12-12 23:56   ` Dmitry Gutov
  2020-12-13 12:51     ` Lars Ingebrigtsen
  0 siblings, 1 reply; 15+ messages in thread
From: Dmitry Gutov @ 2020-12-12 23:56 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: 29193

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.





^ permalink raw reply	[flat|nested] 15+ messages in thread

* bug#29193: 26.0.90; Using (thing-at-point 'sexp) in flymake-diag-region might be suboptimal
  2020-12-12 23:56   ` Dmitry Gutov
@ 2020-12-13 12:51     ` Lars Ingebrigtsen
  2020-12-13 13:19       ` João Távora
  0 siblings, 1 reply; 15+ messages in thread
From: Lars Ingebrigtsen @ 2020-12-13 12:51 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: 29193, João Távora

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





^ permalink raw reply	[flat|nested] 15+ messages in thread

* bug#29193: 26.0.90; Using (thing-at-point 'sexp) in flymake-diag-region might be suboptimal
  2020-12-13 12:51     ` Lars Ingebrigtsen
@ 2020-12-13 13:19       ` João Távora
  2020-12-13 20:55         ` Dmitry Gutov
  0 siblings, 1 reply; 15+ messages in thread
From: João Távora @ 2020-12-13 13:19 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: 29193, Dmitry Gutov

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









^ permalink raw reply related	[flat|nested] 15+ messages in thread

* bug#29193: 26.0.90; Using (thing-at-point 'sexp) in flymake-diag-region might be suboptimal
  2020-12-13 13:19       ` João Távora
@ 2020-12-13 20:55         ` Dmitry Gutov
  2020-12-14 11:03           ` João Távora
  0 siblings, 1 reply; 15+ messages in thread
From: Dmitry Gutov @ 2020-12-13 20:55 UTC (permalink / raw)
  To: João Távora, Lars Ingebrigtsen; +Cc: 29193

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.





^ permalink raw reply	[flat|nested] 15+ messages in thread

* bug#29193: 26.0.90; Using (thing-at-point 'sexp) in flymake-diag-region might be suboptimal
  2020-12-13 20:55         ` Dmitry Gutov
@ 2020-12-14 11:03           ` João Távora
  2020-12-16  0:55             ` Dmitry Gutov
  0 siblings, 1 reply; 15+ messages in thread
From: João Távora @ 2020-12-14 11:03 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: Lars Ingebrigtsen, 29193

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





^ permalink raw reply	[flat|nested] 15+ messages in thread

* bug#29193: 26.0.90; Using (thing-at-point 'sexp) in flymake-diag-region might be suboptimal
  2020-12-14 11:03           ` João Távora
@ 2020-12-16  0:55             ` Dmitry Gutov
  0 siblings, 0 replies; 15+ messages in thread
From: Dmitry Gutov @ 2020-12-16  0:55 UTC (permalink / raw)
  To: João Távora; +Cc: Lars Ingebrigtsen, 29193-done

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





^ permalink raw reply	[flat|nested] 15+ messages in thread

* bug#29193: 26.0.90; Using (thing-at-point 'sexp) in flymake-diag-region might be suboptimal
       [not found] ` <handler.29193.D29193.160808016725606.notifdone@debbugs.gnu.org>
@ 2020-12-17 23:25   ` Glenn Morris
  2020-12-18  2:06     ` Dmitry Gutov
  0 siblings, 1 reply; 15+ messages in thread
From: Glenn Morris @ 2020-12-17 23:25 UTC (permalink / raw)
  To: 29193; +Cc: Dmitry Gutov


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





^ permalink raw reply	[flat|nested] 15+ messages in thread

* bug#29193: 26.0.90; Using (thing-at-point 'sexp) in flymake-diag-region might be suboptimal
  2020-12-17 23:25   ` Glenn Morris
@ 2020-12-18  2:06     ` Dmitry Gutov
  2020-12-18 11:42       ` João Távora
  0 siblings, 1 reply; 15+ messages in thread
From: Dmitry Gutov @ 2020-12-18  2:06 UTC (permalink / raw)
  To: Glenn Morris, 29193

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





^ permalink raw reply	[flat|nested] 15+ messages in thread

* bug#29193: 26.0.90; Using (thing-at-point 'sexp) in flymake-diag-region might be suboptimal
  2020-12-18  2:06     ` Dmitry Gutov
@ 2020-12-18 11:42       ` João Távora
  2020-12-18 15:22         ` Dmitry Gutov
  0 siblings, 1 reply; 15+ messages in thread
From: João Távora @ 2020-12-18 11:42 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: Glenn Morris, 29193

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





^ permalink raw reply	[flat|nested] 15+ messages in thread

* bug#29193: 26.0.90; Using (thing-at-point 'sexp) in flymake-diag-region might be suboptimal
  2020-12-18 11:42       ` João Távora
@ 2020-12-18 15:22         ` Dmitry Gutov
  2020-12-18 15:26           ` João Távora
  0 siblings, 1 reply; 15+ messages in thread
From: Dmitry Gutov @ 2020-12-18 15:22 UTC (permalink / raw)
  To: João Távora; +Cc: Glenn Morris, 29193

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





^ permalink raw reply	[flat|nested] 15+ messages in thread

* bug#29193: 26.0.90; Using (thing-at-point 'sexp) in flymake-diag-region might be suboptimal
  2020-12-18 15:22         ` Dmitry Gutov
@ 2020-12-18 15:26           ` João Távora
  2020-12-18 15:29             ` Dmitry Gutov
  0 siblings, 1 reply; 15+ messages in thread
From: João Távora @ 2020-12-18 15:26 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: Glenn Morris, 29193

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





^ permalink raw reply	[flat|nested] 15+ messages in thread

* bug#29193: 26.0.90; Using (thing-at-point 'sexp) in flymake-diag-region might be suboptimal
  2020-12-18 15:26           ` João Távora
@ 2020-12-18 15:29             ` Dmitry Gutov
  2020-12-18 15:39               ` João Távora
  0 siblings, 1 reply; 15+ messages in thread
From: Dmitry Gutov @ 2020-12-18 15:29 UTC (permalink / raw)
  To: João Távora; +Cc: Glenn Morris, 29193

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.





^ permalink raw reply	[flat|nested] 15+ messages in thread

* bug#29193: 26.0.90; Using (thing-at-point 'sexp) in flymake-diag-region might be suboptimal
  2020-12-18 15:29             ` Dmitry Gutov
@ 2020-12-18 15:39               ` João Távora
  0 siblings, 0 replies; 15+ messages in thread
From: João Távora @ 2020-12-18 15:39 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: Glenn Morris, 29193

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





^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2020-12-18 15:39 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-11-07 15:28 bug#29193: 26.0.90; Using (thing-at-point 'sexp) in flymake-diag-region might be suboptimal Dmitry Gutov
2020-12-12 11:39 ` Lars Ingebrigtsen
2020-12-12 23:56   ` Dmitry Gutov
2020-12-13 12:51     ` Lars Ingebrigtsen
2020-12-13 13:19       ` João Távora
2020-12-13 20:55         ` Dmitry Gutov
2020-12-14 11:03           ` João Távora
2020-12-16  0:55             ` Dmitry Gutov
     [not found] ` <handler.29193.D29193.160808016725606.notifdone@debbugs.gnu.org>
2020-12-17 23:25   ` Glenn Morris
2020-12-18  2:06     ` Dmitry Gutov
2020-12-18 11:42       ` João Távora
2020-12-18 15:22         ` Dmitry Gutov
2020-12-18 15:26           ` João Távora
2020-12-18 15:29             ` Dmitry Gutov
2020-12-18 15:39               ` João Távora

Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.