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

[João Távora]

On Sat, Nov 25, 2023 at 10:43 PM Joseph Turner <joseph@ushin.org> wrote:
>
> Jonas Bernoulli <jonas@bernoul.li> writes:
>
> > Joseph Turner <joseph@ushin.org> writes:
> >
> >> +                          (car (shorthands--find-if
> >> +                                (lambda (short)
> >> +                                  (string-prefix-p short (match-string 
> >> 1)))
> >> +                                read-symbol-shorthands #'car)))))
> >
> > Or simply:
> >   (car (assoc (match-string 1)
> >               read-symbol-shorthands
> >               #'string-prefix-p))
>
> Much nicer - see patch.  Thanks, Jonas!

So, I had a look at this patch and I think we should compare it
with the patch after my sig, which keeps 'shorthands--mismatch-from-end'
and also fixes this bug.

The main difference I see is that my patch keeps doing one string
comparison, via the mismatch function (which btw is now perfectly
analogous to CL mismatch and thus correctly named).  In the worst case,
Josheph's patch does 1 + N where N is the number of shorthands.  So
this is a fundamental complexity change.

Normally, that would be the end of the story, but here, it isn't.
For two reasons.

My version keeps a behaviour that can be considered buggy.
If a shorthand prefix has a common suffix with the longhand prefix
then that suffix will not be highlighted. Like:

;; Local Variables:
;; read-symbol-shorthands: (("bcrumb-" . "breadcrumb-")
;; End:

Here only "b" would be highlighted, effectively showing the user
how much typing was saved.  Is this wrong?  Does it makes sense
to use shorthands like this?

The other reason why this isn't the end of the story is that even
if we take that bug for granted, the string comparison functions in
Joshep's patch delegate to built-in C  comparison operators, which are
often much, much faster than Elisp.  At least before the advent of native
compilation, it used to be like this. Of course for a large enough  N
number of shorthands, my version wins, but that is probably not very
common either (or is it?  Not very hard to imagine a file making use
of many libraries and shorthanding each of them?)

So, benchmarking it will have to be, I'm afraid, because AFAIK
font-locking is a very performance sensitive area of Emacs.

In the meantime I will push my patch, but keep the bug open to see
if it is worth pushing Joseph's version.

João


diff --git a/lisp/emacs-lisp/shorthands.el b/lisp/emacs-lisp/shorthands.el
index 82200ab88e9..b0665a55695 100644
--- a/lisp/emacs-lisp/shorthands.el
+++ b/lisp/emacs-lisp/shorthands.el
@@ -53,11 +53,16 @@ elisp-shorthand-font-lock-face
   :group 'font-lock-faces)

 (defun shorthands--mismatch-from-end (str1 str2)
+  "Tell index of first mismatch in STR1 and STR2, from end.
+The index is a valid 0-based index on STR1.  Returns nil if STR1
+equals STR2.  Return 0 if STR1 is a suffix of STR2."
   (cl-loop with l1 = (length str1) with l2 = (length str2)
            for i from 1
            for i1 = (- l1 i) for i2 = (- l2 i)
-           while (and (>= i1 0) (>= i2 0) (eq (aref str1 i1) (aref str2 i2)))
-           finally (return (1- i))))
+           while (eq (aref str1 i1) (aref str2 i2))
+           if (zerop i2) return (if (zerop i1) nil i1)
+           if (zerop i1) return 0
+           finally (return i1)))

 (defun shorthands-font-lock-shorthands (limit)
   (when read-symbol-shorthands
@@ -69,10 +74,16 @@ shorthands-font-lock-shorthands
                                                font-lock-string-face)))
                          (intern-soft (match-string 1))))
              (sname (and probe (symbol-name probe)))
-             (mm (and sname (shorthands--mismatch-from-end
-                             (match-string 1) sname))))
-        (unless (or (null mm) (= mm (length sname)))
-          (add-face-text-property (match-beginning 1) (1+ (- (match-end 1) mm))
+             (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)))
+          (add-face-text-property (match-beginning 1)
+                                  (+ (match-beginning 1) guess)
                                   'elisp-shorthand-font-lock-face))))))

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