bug#45264: 26.3; `face-remap-set-base' seems to be bugged

bug#45264: 26.3; `face-remap-set-base' seems to be bugged

[Drew Adams]

See https://emacs.stackexchange.com/a/62301/105

(defface foo '((t (:background "red"))) "...")

(face-remap-set-base 'font-lock-keyword-face 'foo)

The &rest arg SPECS is `(foo)', which is, as required, a list of
(one) face.

But the code actually expects `foo' itself to be a list.  It raises
an error, because it sets SPECS to just `foo' and then tries to
take the car of it.

(while (and (consp specs)
            (not (null (car specs)))
            (null (cdr specs)))
  (setq specs (car specs))) ; <=========

(if (or (null specs)
    (and (eq (car specs) face) ; <=========
         (null (cdr specs))))

Is there a doc bug (both manual and doc string)?  Or is there a code
bug?  Or am I missing something?


In GNU Emacs 26.3 (build 1, x86_64-w64-mingw32)
 of 2019-08-29
Repository revision: 96dd0196c28bc36779584e47fffcca433c9309cd
Windowing system distributor `Microsoft Corp.', version 10.0.18362
Configured using:
 `configure --without-dbus --host=x86_64-w64-mingw32
 --without-compress-install 'CFLAGS=-O2 -static -g3''

bug#45264: 26.3; `face-remap-set-base' seems to be bugged

[Eli Zaretskii]

> Date: Tue, 15 Dec 2020 16:31:31 -0800 (PST)
> From: Drew Adams <drew.adams@oracle.com>
> 
> (defface foo '((t (:background "red"))) "...")
> 
> (face-remap-set-base 'font-lock-keyword-face 'foo)
> 
> The &rest arg SPECS is `(foo)', which is, as required, a list of
> (one) face.
> 
> But the code actually expects `foo' itself to be a list.  It raises
> an error, because it sets SPECS to just `foo' and then tries to
> take the car of it.
> 
> (while (and (consp specs)
>             (not (null (car specs)))
>             (null (cdr specs)))
>   (setq specs (car specs))) ; <=========
> 
> (if (or (null specs)
>     (and (eq (car specs) face) ; <=========
>          (null (cdr specs))))
> 
> Is there a doc bug (both manual and doc string)?  Or is there a code
> bug?  Or am I missing something?

I don't see anything wrong with the documentation yet.  One needs to
know and understand what is a "face spec", and then everything falls
into its place.  The &rest part is also a big hint.

Can I turn the table and ask you why you thought the argument could be
a list of one or more faces?  The doc string says "should FORM a list
of faces", it doesn't say it should BE a list of faces.  And since
when does &rest specify a single argument that is a list?

bug#45264: 26.3; `face-remap-set-base' seems to be bugged

[Drew Adams]

> > Is there a doc bug (both manual and doc string)?  Or is there a code
> > bug?  Or am I missing something?
> 
> I don't see anything wrong with the documentation yet.  One needs to
> know and understand what is a "face spec", and then everything falls
> into its place.  The &rest part is also a big hint.
> 
> Can I turn the table and ask you why you thought the argument could be
> a list of one or more faces?  The doc string says "should FORM a list
> of faces", it doesn't say it should BE a list of faces.  And since
> when does &rest specify a single argument that is a list?

The confusion is not from not understanding what a
face spec is.  I think I know what a face spec is.

The confusion is from the doc not saying explicitly
that each element of SPECS is a face spec, and NOT a
face.  Please consider making that explicit (clear).

1. I didn't suggest (at all) that &rest specifies a
single arg that is a list.  Quite the contrary - the
answer I gave to that SE question explicitly made
that exact point.

2. "why you thought the argument could be a list of
one or more faces?"

The doc string explicitly says that elements of
SPECS can be face names:

  Each list element should be either a face name or...

3. The predicate that tests for a face is `facep',
and it doesn't return non-nil for a face spec.  Its
doc explicitly says that it tests whether its arg
is a "face name", which can be a string or a symbol.

Putting #2 and #3 together, the doc for SPECS does
indeed say that elements of SPECS can be faces.

4. I strongly suggest that you change the language.
"FORM" as a verb here is not clear, and this is
apparently not about faces as arguments; it's about
face specs.  Face specs can be said to define (or
"form") faces, but they are not faces - they don't
satisfy `facep'.
___

Another possibility is perhaps to fix the behavior,
so that it does what its doc says: allow elements
of SPECS to "be either a face name or a property
list of...".  Allow faces, not just face specs.

bug#45264: 26.3; `face-remap-set-base' seems to be bugged

[Eli Zaretskii]

> Date: Sat, 19 Dec 2020 10:32:16 -0800 (PST)
> From: Drew Adams <drew.adams@oracle.com>
> Cc: 45264@debbugs.gnu.org
> 
> The confusion is from the doc not saying explicitly
> that each element of SPECS is a face spec, and NOT a
> face.

SPECS has no "elements".  SPECS stands for arguments to the function
beyond the 1st arg FACE.  Each such argument is either a face name or
a list of attribute/value pairs.

I changed the doc string to be more clear about that.

> 2. "why you thought the argument could be a list of
> one or more faces?"
> 
> The doc string explicitly says that elements of
> SPECS can be face names:
> 
>   Each list element should be either a face name or...

That's after it says that you should consider SPECS as "forming" a
list of elements.

bug#45264: 26.3; `face-remap-set-base' seems to be bugged

[Drew Adams]

> > The confusion is from the doc not saying explicitly
> > that each element of SPECS is a face spec, and NOT a
> > face.
> 
> SPECS has no "elements".  SPECS stands for arguments to the function
> beyond the 1st arg FACE.

As with any &rest, you supply zero or more actual
args that correspond to SPECS.  The function itself
receives a single list argument that corresponds to
SPECS.

In the function body, variable SPECS is a list.
And as the doc says, "Each list element...".

> Each such argument is either a face name or
> a list of attribute/value pairs.

AFAICT, the function doesn't work if such an arg
is a face name.  See what I said about that
previously, please.

> I changed the doc string to be more clear about that.

Great.  Thanks for taking a look.

Please check for the behavior bug I pointed to:
If the doc is correct then the behavior seems
bugged.  It's not true that you can pass face
names, AFAICT.

> > 2. "why you thought the argument could be a list of
> > one or more faces?"
> >
> > The doc string explicitly says that elements of
> > SPECS can be face names:
> >
> >   Each list element should be either a face name or...
> 
> That's after it says that you should consider SPECS as "forming" a
> list of elements.

"Forming" a list of elements is unclear, as I said.

And SPECS is, itself, from the point of view of the
function, a list of elements.  See above.  There is
_nothing_ special about this.  Every &rest parameter
behaves the same in this regard.

BTW, the exact same misleading and inexact text is
used for function `face-map-add-relative'.

For functions `buffer-face-(set|toggle)', however,
we instead say, as we usually do, "Each argument
in SPECS should be a face, i.e., either a face name
or a property list...".  And we explicitly speak of
SPECS as a list (singular): "if SPECS is omitted or
nil..."

bug#45264: 26.3; `face-remap-set-base' seems to be bugged

[Eli Zaretskii]

> Date: Sat, 19 Dec 2020 11:28:31 -0800 (PST)
> From: Drew Adams <drew.adams@oracle.com>
> Cc: 45264-done@debbugs.gnu.org
> 
> > > The confusion is from the doc not saying explicitly
> > > that each element of SPECS is a face spec, and NOT a
> > > face.
> > 
> > SPECS has no "elements".  SPECS stands for arguments to the function
> > beyond the 1st arg FACE.
> 
> As with any &rest, you supply zero or more actual
> args that correspond to SPECS.  The function itself
> receives a single list argument that corresponds to
> SPECS.
> 
> In the function body, variable SPECS is a list.
> And as the doc says, "Each list element...".

The doc string shouldn't explain how the function's body sees SPECS,
it should explain how to provide those args.

> Please check for the behavior bug I pointed to:

If it doesn't work according to the doc string, then yes, it's a bug.