bug#35496: 27.0.50; smie-blink-matching-open blinks token before point after RET

bug#35496: 27.0.50; smie-blink-matching-open blinks token before point after RET

[Dmitry Gutov]

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

1. Disable show-paren-mode if it's enabled.
2. Evalute the attached .el file (which defined a major mode).
3. Create a new bufferand type M-x foo-mode.
4. Type 'def foo do' (without quotes) and press RET.
5. Cursor will hang around on the first line even after the newline is 
inserted.

Key moments: ?o is not in smie-blink-matching-triggers because the
grammar also defines a closer 'dop'. smie-blink-matching-inners is t
(which it is by default).

Here's an old bug report for elixir-mode which, unfortunately, didn't
reach the end of the investigation before the author opted for a
workaround instead: 
https://github.com/elixir-editors/emacs-elixir/issues/363

What would be the better workaround, by the way? I'm thinking of simply 
disabling smie-blink-matching-inners.

In GNU Emacs 27.0.50 (build 52, x86_64-pc-linux-gnu, GTK+ Version 3.22.30)
  of 2019-04-28 built on zappa
Repository revision: 80822917736edbab77969c4a18dfb8dd20fe3a88
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12001000
System Description: Ubuntu 18.04.2 LTS

[-- Attachment #2: smie-blink-matching-inners.el --]
[-- Type: text/x-emacs-lisp, Size: 470 bytes --]

(require 'smie)

(defvar foo-smie-grammar
  (smie-prec2->grammar
   (smie-bnf->prec2
    '((statements (statement)
                  (statement ";" statements))
      (statement ("def" non-block-expr "do" statements "end")
                 ("def" non-block-expr "COMMA" "dop" non-block-expr))
      (non-block-expr ("STRING"))))))

(define-derived-mode foo-mode prog-mode "Foo"
  (set (make-local-variable 'comment-start) "# ")
  (smie-setup foo-smie-grammar #'ignore))

bug#35496: 27.0.50; smie-blink-matching-open blinks token before point after RET

[Dmitry Gutov]

Stefan, just making sure you read this.

On 29.04.2019 23:57, Dmitry Gutov wrote:
> 1. Disable show-paren-mode if it's enabled.
> 2. Evalute the attached .el file (which defined a major mode).
> 3. Create a new bufferand type M-x foo-mode.
> 4. Type 'def foo do' (without quotes) and press RET.
> 5. Cursor will hang around on the first line even after the newline is 
> inserted.
> 
> Key moments: ?o is not in smie-blink-matching-triggers because the
> grammar also defines a closer 'dop'. smie-blink-matching-inners is t
> (which it is by default).
> 
> Here's an old bug report for elixir-mode which, unfortunately, didn't
> reach the end of the investigation before the author opted for a
> workaround instead: 
> https://github.com/elixir-editors/emacs-elixir/issues/363
> 
> What would be the better workaround, by the way? I'm thinking of simply 
> disabling smie-blink-matching-inners.
> 
> In GNU Emacs 27.0.50 (build 52, x86_64-pc-linux-gnu, GTK+ Version 3.22.30)
>   of 2019-04-28 built on zappa
> Repository revision: 80822917736edbab77969c4a18dfb8dd20fe3a88
> Repository branch: master
> Windowing system distributor 'The X.Org Foundation', version 11.0.12001000
> System Description: Ubuntu 18.04.2 LTS

bug#35496: 27.0.50; smie-blink-matching-open blinks token before point after RET

[Stefan Monnier]

> 1. Disable show-paren-mode if it's enabled.
> 2. Evalute the attached .el file (which defined a major mode).
> 3. Create a new bufferand type M-x foo-mode.
> 4. Type 'def foo do' (without quotes) and press RET.
> 5. Cursor will hang around on the first line even after the newline
> is inserted.

It's not a bug, it's a feature: we can't highlight the matching `def`
when you hit the `o`  because we don't know yet whether you actually
intended to type `do` or a longer identifier, so we postpone the
blinking to the next char.

smie-blink-matching-triggers defaults to ?\s and ?\n so the "next char"
where the blinking can happen is SPC or RET.

Maybe we shouldn't postpone the blinking (i.e. we should add ?o to
smie-blink-matching-triggers)?


        Stefan

bug#35496: 27.0.50; smie-blink-matching-open blinks token before point after RET

[Dmitry Gutov]

On 08.05.2019 4:44, Stefan Monnier wrote:
>> 1. Disable show-paren-mode if it's enabled.
>> 2. Evalute the attached .el file (which defined a major mode).
>> 3. Create a new bufferand type M-x foo-mode.
>> 4. Type 'def foo do' (without quotes) and press RET.
>> 5. Cursor will hang around on the first line even after the newline
>> is inserted.
> 
> It's not a bug, it's a feature: we can't highlight the matching `def`
> when you hit the `o`  because we don't know yet whether you actually
> intended to type `do` or a longer identifier, so we postpone the
> blinking to the next char.

But we don't end up blinking to `def` after RET, we blink to `do`.

There must be an opportunity to check that we don't blink to the 
preceding token.

> smie-blink-matching-triggers defaults to ?\s and ?\n so the "next char"
> where the blinking can happen is SPC or RET.
> 
> Maybe we shouldn't postpone the blinking (i.e. we should add ?o to
> smie-blink-matching-triggers)?

SMIE fills it automatically based on the current set of tokens. If I add 
it myself, yeah, the behavior is better in this case. But I kinda buy 
your reasoning about not having it there (even though it's not a 
panacea: the user can type whatever token, not only ones in the 
smie-closer-alist.

Overall, I feel that the smie-blink-matching-inners might be too much as 
default anyway. So it's not a big deal if elixir-mode has to disable it.

bug#35496: 27.0.50; smie-blink-matching-open blinks token before point after RET

[Stefan Monnier]

>>> 1. Disable show-paren-mode if it's enabled.
>>> 2. Evalute the attached .el file (which defined a major mode).
>>> 3. Create a new bufferand type M-x foo-mode.
>>> 4. Type 'def foo do' (without quotes) and press RET.
>>> 5. Cursor will hang around on the first line even after the newline
>>> is inserted.
>>
>> It's not a bug, it's a feature: we can't highlight the matching `def`
>> when you hit the `o`  because we don't know yet whether you actually
>> intended to type `do` or a longer identifier, so we postpone the
>> blinking to the next char.
>
> But we don't end up blinking to `def` after RET, we blink to `do`.

That's not what I see when I try your recipe: it blinks to the "d" of
"def", as it should, for me (both with Emacs'` master` and with
Debian's Emacs-26.1).

> SMIE fills it automatically based on the current set of tokens.

Indeed, but you can tweak it by hand afterwards.

> If I add it myself, yeah, the behavior is better in this case.

I've been wondering for a while now if it's not just "in this case" but
in general.

> But I kinda buy your reasoning about not having it there (even though
> it's not a panacea: the user can type whatever token, not only ones in
> the smie-closer-alist.

I agree that only considering smie-closer-alist is probably not a great
choice.  It seemed obvious when I wrote it, but I don't know why, and
I can't justify it now.

[ Maybe it's because I was working on octave-mode around that time and
  didn't want to blink on "end" when the user likes to write the
  full-form "endfunction"?  But even that doesn't sound like
  a good justification.  ]

> Overall, I feel that the smie-blink-matching-inners might be too much as
> default anyway.  So it's not a big deal if elixir-mode has to disable it.

I don't think it's specific to elixir-mode but is rather
a user-preference.  Maybe it should default to nil, but I haven't
found a mode where smie-blink-matching-inners should be disabled
*because of the mode* rather than because that's what the user prefers.


        Stefan

bug#35496: 27.0.50; smie-blink-matching-open blinks token before point after RET

[Dmitry Gutov]

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

On 08.05.2019 20:42, Stefan Monnier wrote:

>> But we don't end up blinking to `def` after RET, we blink to `do`.
> 
> That's not what I see when I try your recipe: it blinks to the "d" of
> "def", as it should, for me (both with Emacs'` master` and with
> Debian's Emacs-26.1).

You are right. Sorry about that.

I guess the main problem, both the cause of my misunderstanding and the 
user aggravation, is that smie-blink-matching-open calls 
blink-matching-open on a changed position. As a result the sit-for call 
is issued on the position before the newline instead of after.

Which created both the appearance of sluggishness from the original 
report, and my mistake on which token is highlighted (because the moved 
cursor is much easier to notice than the subtle paren highlighting, at 
least on the theme I'm using).

I've tried to move all checks smie-blink-matching-open into a 
save-excursion and call blink-matching-open from the orignal position, 
but that doesn't work alone, unfortunately, because newline is a 
separate token. Hence the change in blink-matching-open as well. WDYT?

Patch attached.

>> SMIE fills it automatically based on the current set of tokens.
> 
> Indeed, but you can tweak it by hand afterwards.
> 
>> If I add it myself, yeah, the behavior is better in this case.
> 
> I've been wondering for a while now if it's not just "in this case" but
> in general.

I don't know. The highlighting itself is sufficiently unobtrusive here, 
so it could work either way. If the patch I'm proposing seems like 
unwanted complexity, we can avoid it also this way, and even simplify 
more code.

>> But I kinda buy your reasoning about not having it there (even though
>> it's not a panacea: the user can type whatever token, not only ones in
>> the smie-closer-alist.
> 
> I agree that only considering smie-closer-alist is probably not a great
> choice.  It seemed obvious when I wrote it, but I don't know why, and
> I can't justify it now.
> 
> [ Maybe it's because I was working on octave-mode around that time and
>    didn't want to blink on "end" when the user likes to write the
>    full-form "endfunction"?  But even that doesn't sound like
>    a good justification.  ]

I'm guessing that with Elixir's syntax, while the user is typing the 
last two chars in

   def sum, do:

the 'd' in 'def' would blink twice. Which is maybe not ideal either.

>> Overall, I feel that the smie-blink-matching-inners might be too much as
>> default anyway.  So it's not a big deal if elixir-mode has to disable it.
> 
> I don't think it's specific to elixir-mode but is rather
> a user-preference.  Maybe it should default to nil, but I haven't
> found a mode where smie-blink-matching-inners should be disabled
> *because of the mode* rather than because that's what the user prefers.

No argument here. Just saying it's an okay workaround as well.

[-- Attachment #2: smie-blink-matching-do.diff --]
[-- Type: text/x-patch, Size: 5788 bytes --]

diff --git a/lisp/emacs-lisp/smie.el b/lisp/emacs-lisp/smie.el
index f2163b243e..c68e55e0b4 100644
--- a/lisp/emacs-lisp/smie.el
+++ b/lisp/emacs-lisp/smie.el
@@ -997,46 +997,48 @@ smie-blink-matching-open
                                 (eq (char-before) last-command-event)))))
                (memq last-command-event smie-blink-matching-triggers)
                (not (nth 8 (syntax-ppss))))
-      (save-excursion
-        (setq token (funcall smie-backward-token-function))
-        (when (and (eq (point) (1- pos))
-                   (= 1 (length token))
-                   (not (rassoc token smie-closer-alist)))
-          ;; The trigger char is itself a token but is not one of the
-          ;; closers (e.g. ?\; in Octave mode), so go back to the
-          ;; previous token.
-          (setq pos (point))
-          (setq token (funcall smie-backward-token-function)))
-        (when (rassoc token smie-closer-alist)
-          ;; We're after a close token.  Let's still make sure we
-          ;; didn't skip a comment to find that token.
-          (funcall smie-forward-token-function)
-          (when (and (save-excursion
-                       ;; Skip the trigger char, if applicable.
-                       (if (eq (char-after) last-command-event)
-                           (forward-char 1))
-                       (if (eq ?\n last-command-event)
-                           ;; Skip any auto-indentation, if applicable.
-                           (skip-chars-forward " \t"))
-                       (>= (point) pos))
-                     ;; If token ends with a trigger char, don't blink for
-                     ;; anything else than this trigger char, lest we'd blink
-                     ;; both when inserting the trigger char and when
-                     ;; inserting a subsequent trigger char like SPC.
-                     (or (eq (char-before) last-command-event)
-                         (not (memq (char-before)
-                                    smie-blink-matching-triggers)))
-                     ;; FIXME: For octave's "switch ... case ... case" we flash
-                     ;; `switch' at the end of the first `case' and we burp
-                     ;; "mismatch" at the end of the second `case'.
-                     (or smie-blink-matching-inners
-                         (not (numberp (nth 2 (assoc token smie-grammar))))))
-            ;; The major mode might set blink-matching-check-function
-            ;; buffer-locally so that interactive calls to
-            ;; blink-matching-open work right, but let's not presume
-            ;; that's the case.
-            (let ((blink-matching-check-function #'smie-blink-matching-check))
-              (blink-matching-open))))))))
+      (when
+          (save-excursion
+            (setq token (funcall smie-backward-token-function))
+            (when (and (eq (point) (1- pos))
+                       (= 1 (length token))
+                       (not (rassoc token smie-closer-alist)))
+              ;; The trigger char is itself a token but is not one of the
+              ;; closers (e.g. ?\; in Octave mode), so go back to the
+              ;; previous token.
+              (setq pos (point))
+              (setq token (funcall smie-backward-token-function)))
+            (and (rassoc token smie-closer-alist)
+                 (progn
+                   ;; We're after a close token.  Let's still make sure we
+                   ;; didn't skip a comment to find that token.
+                   (funcall smie-forward-token-function)
+                   (and (save-excursion
+                          ;; Skip the trigger char, if applicable.
+                          (if (eq (char-after) last-command-event)
+                              (forward-char 1))
+                          (if (eq ?\n last-command-event)
+                              ;; Skip any auto-indentation, if applicable.
+                              (skip-chars-forward " \t"))
+                          (>= (point) pos))
+                        ;; If token ends with a trigger char, don't blink for
+                        ;; anything else than this trigger char, lest we'd blink
+                        ;; both when inserting the trigger char and when
+                        ;; inserting a subsequent trigger char like SPC.
+                        (or (eq (char-before) last-command-event)
+                            (not (memq (char-before)
+                                       smie-blink-matching-triggers)))
+                        ;; FIXME: For octave's "switch ... case ... case" we flash
+                        ;; `switch' at the end of the first `case' and we burp
+                        ;; "mismatch" at the end of the second `case'.
+                        (or smie-blink-matching-inners
+                            (not (numberp (nth 2 (assoc token smie-grammar)))))))))
+        ;; The major mode might set blink-matching-check-function
+        ;; buffer-locally so that interactive calls to
+        ;; blink-matching-open work right, but let's not presume
+        ;; that's the case.
+        (let ((blink-matching-check-function #'smie-blink-matching-check))
+          (blink-matching-open))))))
 
 (defvar-local smie--matching-block-data-cache nil)
 
diff --git a/lisp/simple.el b/lisp/simple.el
index 4454791ad2..6a077cc55a 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -7648,6 +7648,7 @@ blink-matching-open
                   (condition-case ()
                       (progn
 			(syntax-propertize (point))
+                        (forward-comment -1)
                         (forward-sexp -1)
                         ;; backward-sexp skips backward over prefix chars,
                         ;; so move back to the matching paren.

bug#35496: 27.0.50; smie-blink-matching-open blinks token before point after RET

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> I've tried to move all checks smie-blink-matching-open into a
> save-excursion and call blink-matching-open from the orignal position,
> but that doesn't work alone, unfortunately, because newline is a
> separate token. Hence the change in blink-matching-open as well. WDYT?
>
> Patch attached.

I've just skimmed this thread, but there didn't seem to be any followup
here?  Is this still an issue in the current Emacs?

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

bug#35496: 27.0.50; smie-blink-matching-open blinks token before point after RET

[Dmitry Gutov]

On 23.09.2021 00:34, Lars Ingebrigtsen wrote:
> Dmitry Gutov<dgutov@yandex.ru>  writes:
> 
>> I've tried to move all checks smie-blink-matching-open into a
>> save-excursion and call blink-matching-open from the orignal position,
>> but that doesn't work alone, unfortunately, because newline is a
>> separate token. Hence the change in blink-matching-open as well. WDYT?
>>
>> Patch attached.
> I've just skimmed this thread, but there didn't seem to be any followup
> here?  Is this still an issue in the current Emacs?

Yep. No change here.

Regarding the posted patch, I'm not sure it's doing TRT semantically, so 
I didn't press on it.