bug#37806: 27.0.50; Need to "extend" face-remap.el

[Ergus]

On Sun, Oct 20, 2019 at 02:21:50PM +0200, Stephen Berman wrote:
> On Sat, 19 Oct 2019 20:33:33 +0300 Eli Zaretskii <eliz@gnu.org> wrote:
>
> > > From: Stephen Berman <stephen.berman@gmx.net>
> > > Date: Fri, 18 Oct 2019 11:14:09 +0200
> > >
> > > 0. emacs -Q
> > > 1. C-x C-+ (i.e. M-x text-scale-adjust)
> > > 2. M-x variable-pitch-mode or M-x buffer-face-mode
> > > => Args out of range: [nil :family :foundry :swidth :height :weight :slant 
> > > :underline :inverse :foreground ...], 19
> > >
> > > The error happens because the vector of face attributes defined in
> > > face-remap.el is missing the recently added :extend attribute, so it is
> > > too short.  This also breaks the MELPA package charmap.el (that's where
> > > I hit the error).  The patch below fixes this, though perhaps now would
> > > be a good time to do what the comment above the definition of the vector
> > > says: "This variable should probably be defined in C code where the
> > > actual definitions are available."  Or is this simple fix good enough?
> > >
> > > (The vector is also missing the :distant-foreground attribute, so the
> > > patch adds that as well.  This absence happened to be innocuous because
> > > the first element of the vector is nil, in order to make the attribute
> > > indices match those of the enum lface_attribute_index defined in
> > > dispextern.h, so the vector was long enough; but after the addition of
> > > the :extend attribute, it wasn't anymore (only the indices of the vector
> > > are used in face-remap.el).)
> > >
> > > In GNU Emacs 27.0.50 (build 17, x86_64-pc-linux-gnu, GTK+ Version 3.24.5, cairo 
> > > version 1.16.0)
> > >  of 2019-10-18 built on strobe-lfs84
> > > Repository revision: 2d13a3f68d4724af52e47675bedf60709c7b5171
> > > Repository branch: master
> > > Windowing system distributor 'The X.Org Foundation', version 11.0.12003000
> > > System Description: Linux From Scratch
> > >
> > >
> > > diff --git a/lisp/face-remap.el b/lisp/face-remap.el
> > > index 5cdecb92ee..8e565264fe 100644
> > > --- a/lisp/face-remap.el
> > > +++ b/lisp/face-remap.el
> > > @@ -69,7 +69,7 @@ internal-lisp-face-attributes
> > >    [nil
> > >     :family :foundry :swidth :height :weight :slant :underline :inverse
> > >     :foreground :background :stipple :overline :strike :box
> > > -   :font :inherit :fontset :vector])
> > > +   :font :inherit :fontset :distant :extend :vector])
> > >
> > >  (defun face-attrs-more-relative-p (attrs1 attrs2)
> > >    "Return true if ATTRS1 contains a greater number of relative
> >
> > Thanks.
> >
> > Jimmy, could you please take a look?
>
> This bug (and bug#37824, which is the same) isn't due to the addition of
> the :extend attribute per se, but to that addition increasing the length
> of lface_attribute_index, which makes it necessary to adjust the length
> of internal-lisp-face-attributes, only the indices of which
> face-remap.el actually uses, as I noted above.  Indeed, the following
> patch also fixes this bug:
>
> diff --git a/lisp/face-remap.el b/lisp/face-remap.el
> index 5cdecb92ee..e429752df9 100644
> --- a/lisp/face-remap.el
> +++ b/lisp/face-remap.el
> @@ -66,7 +66,7 @@
> ;; definitions are available.
> ;;
> (defvar internal-lisp-face-attributes
> -  [nil
> +  [nil nil
>    :family :foundry :swidth :height :weight :slant :underline :inverse
>    :foreground :background :stipple :overline :strike :box
>    :font :inherit :fontset :vector])
>
> But then we might as well just use a vector with all nil (or arbitrary)
> elements whose length is LFACE_VECTOR_SIZE (maybe that's even how the
> comment in face-remap.el quoted above could be understood).
>
> The simplest fix is to just add the missing attributes, as my first
> patch does, but then on any future changes to lface_vector_size
> internal-lisp-face-attributes will have to be correspondingly adjusted.
>
> Steve Berman

Yes the problem is just the size of the vector, because actually the
elements in it are wrong as they are not as defined anywhere else.

The proper fix must be to declare the vector in the C side using the
real symbols like:

Lisp_Object internal-lisp-face-attributes[LFACE_VECTOR_SIZE] = {
           Qnil QCfamily, QCfoundry, QCwidth QCheight,
           QCweight, QCslant, QCunderline, QCinverse_video,
           QCforeground, QCbackground, QCstipple, QCoverline,
           QCstrike_through, QCbox, QCfont, QCinherit, QCfontset,
           QCdistant_foreground, QCextend
           };

But using makevector or a similar api from C. This vector can be used in
some other parts in the code where we are duplicating code now, but this
will require a refactor I don't know if it is desired.

So I will submit the simplest fix now and if Eli gives me a green light
I will implement the better version.