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

["João Távora" <joaotavora@gmail.com>]

On Fri, Nov 24, 2023 at 9:51 PM Jonas Bernoulli <jonas@bernoul.li> wrote:

> I haven't found any issues beside this off-by-one font-lock issue.

Good.

> So far I have used this beauty:
>
> diff --git a/lisp/emacs-lisp/shorthands.el b/lisp/emacs-lisp/shorthands.el
> @@ -57,7 +57,7 @@ shorthands--mismatch-from-end
>             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))))
> +           finally (return (if (eq (aref str2 (1+ i2)) ?-) (1- i) i))))
>
> Is that good enough?  Depending on how you look at it, this changes what
> is being returned, but IMO this function is a bit murky to begin with.

A bit dodgy, no :-)  Maybe a docstring would shed some light
on what this function is supposed to do, so I'll propose one below.

> The function name is `shorthands--mismatch-from-end', but it returns the
> length of the common suffix, minus one, to account for the separator.
> This change ensures that the separator is accounted for, even if it
> differs between the shorthand and real symbol.
>
> Since this function returns the length of the *matching* suffix after
> the prefixes (including separator), I find it weird that its name
> contains *MISmatch*.

Probably I wanted to emulate what CL's MISMATCH does to some degree.
cl-mismatch exists in Emacs, but it's not available in shorthands.el,
and it seems to be buggy anyway.

You can rename the function shorthands--common-suffix-length if
you want.  I probably meant it to be separator-agnostic function,
and be replaceable by a cl-mismatch some time around 2084,

Now to your problem: I think what you want is to customize element
comparison (CL's MISMATCH allows that, btw).  Here's one way.

diff --git a/lisp/emacs-lisp/shorthands.el b/lisp/emacs-lisp/shorthands.el
index 82200ab88e9..36a862bc037 100644
--- a/lisp/emacs-lisp/shorthands.el
+++ b/lisp/emacs-lisp/shorthands.el
@@ -52,11 +52,18 @@ elisp-shorthand-font-lock-face
   :version "28.1"
   :group 'font-lock-faces)

-(defun shorthands--mismatch-from-end (str1 str2)
-  (cl-loop with l1 = (length str1) with l2 = (length str2)
+(defun shorthands--mismatch-from-end (str1 str2 &optional test)
+  "Compute position of first mismatching element of STR1 and STR2, from end.
+The return value is the reverse index of that element.  If 0, the
+last characters of STR1 and STR2 differ, if 1, the penultimate
+characters differ, and so on.  If optional TEST is passed,
+compare elements using that function, else compare with `eq'."
+  (cl-loop with test = (or test #'eq)
+           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)))
+           while (and (>= i1 0) (>= i2 0)
+                      (funcall test (aref str1 i1) (aref str2 i2)))
            finally (return (1- i))))

 (defun shorthands-font-lock-shorthands (limit)

And now using the following lambda for TEST yields what you want:

(shorthands--mismatch-from-end "s-tail" "long-tail") ;; => 5

(shorthands--mismatch-from-end "s/tail" "long-tail"
                               (lambda (c1 c2)
                                 (or (and (eq c1 ?/) (eq c2 ?-))
                                     (eq c1 c2)))) ;; => also 5

Of course, you can hardcode this test inside the function, no need
for a parameter, and rename the function to whatever you find more
appropriate.  This allows us to keep control over what things
are considered acceptable separators from a font-locking perspective

> It might make more sense to return the length of the shorthand prefix.

I've been away from this code for a couple of years, so feel free to
propose more extensive changes.  As long as they don't increase the
complexity and are suitably tested, I have nothing against.

> Also, have you considered throwing in a
>   (not (string-equal (match-string 1) sname))
>
> to avoid having to call `shorthands--mismatch-from-end' at all?

Where and how this be thrown in?  How would you know what to highlight
in the shorthand printed form?  There can be many shorthand prefixes
in a given file.  But do show some code.

> Maybe you have, but concluded it is cheaper to do a bit too much work
> for non-shorthands, than to effectively repeat some work for shorthands.

Maybe.  Not sure this is more work (string-equal must still compare
every character right?), but, in summary, I'm not married to this
implementation.  I somewhat appreciate that I could still read it
after not having looked  at it for a couple years, but feel free to
change it.

João