bug#67390: 28; shorthands-font-lock-shorthands assumes shorthand uses same separator

[João Távora]

On Wed, Nov 29, 2023 at 9:29 AM Joseph Turner <joseph@ushin.org> wrote:

> > Joseph can you try these variations? They're slightly more exact. Also show 
> > at least one of the large lisp files or tell me how to generate
> > one. If you still don't find any significant slowdown, I think we can merge 
> > your patch.
>
> I'm happy to try Eli's variation if you don't beat me to it ;)

Great.  No I don't think I will beat you to it :-), you have the
machinery setup already.

> However, if we rearrange the values like:
>
> ;; read-symbol-shorthands: (
> ;;   ("he/"  . "hyperdrive-entry-")
> ;;   ("he//" . "hyperdrive-entry--")
> ;;   ("h/"   . "hyperdrive-")
> ;;   ("h//"  . "hyperdrive--"))
>
> then shorthands doesn't add fontification for either "h//" or "he//".
> (This surprised me - I was expecting to see just the "h/" and "he/"
> highlit)
>
> However, I'm starting to wonder whether this is a case of user error.
> Should we avoid overlapping shorthand prefixes?

No, but you should make sure the longer prefixes appear before.
It's not only a font-locking issue, if the shorter shorthand
appears before it could read to an unintended symbol.  If shorthands
are

  (("h/" . "hello-") ("h//" . "hyperdrive--"))

and you type

  "h//warp"

, I'm fairly sure this will be read to

    "hello-/warp"

, which is probably not what you intended

The documentation doesn't mention this, but it should, so patches
welcome for that.

In theory, lread.c could do this sorting for you.  I don't think
this would affect performance, so patches welcome for that,
too.

Anyway, here's another patch that more or less bridges our two patches
and seems to do the right thing every time, even making you aware of
that ordering pitfall.

It could be more performant (or less, or about the same), so if
you can, please include it in your benchmarks.  It does less
work when presented with a "/"-ending shorthand for a "-"-separated
symbol, so I'm starting to think that this "/" idea is a good
convention for shorthands (useful for distinguishing them in
github code, for example).

I've tested it briefly with those 'he/', 'he//', etc cases.

diff --git a/lisp/emacs-lisp/shorthands.el b/lisp/emacs-lisp/shorthands.el
index b0665a55695..69e9e252aee 100644
--- a/lisp/emacs-lisp/shorthands.el
+++ b/lisp/emacs-lisp/shorthands.el
@@ -76,14 +76,19 @@ shorthands-font-lock-shorthands
              (sname (and probe (symbol-name probe)))
              (mismatch (and sname (shorthands--mismatch-from-end
                                    (match-string 1) sname)))
-             (guess (and mismatch (1+ mismatch))))
-        (when guess
-          (when (and (< guess (1- (length (match-string 1))))
-                     ;; In bug#67390 we allow other separators
-                     (eq (char-syntax (aref (match-string 1) guess)) ?_))
-            (setq guess (1+ guess)))
+             probe2)
+        (when mismatch
+          (unless (eq (char-syntax (aref (match-string 1) mismatch)) ?_)
+            (or probe2
+                (setq probe2
+                      (buffer-substring (match-beginning 1)
+                                        (+ (match-beginning 1) mismatch))))
+            (setq mismatch (1- (length
+                                (car (assoc probe2
+                                            read-symbol-shorthands
+                                            (lambda (a b)
(string-prefix-p b a))))))))
           (add-face-text-property (match-beginning 1)
-                                  (+ (match-beginning 1) guess)
+                                  (+ (match-beginning 1) (1+ mismatch))
                                   'elisp-shorthand-font-lock-face))))))

 (font-lock-add-keywords 'emacs-lisp-mode
'((shorthands-font-lock-shorthands)) t)