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

From: Dmitry Gutov <[email-masked]>
Date: Mon, 13 May 2019 03:52:04 +0300

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