* 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
[parent not found: <handler.29193.D29193.160808016725606.notifdone@debbugs.gnu.org>]
* 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 public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).