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

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 45264 in the body.
You can then email your comments to 45264 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Drew Adams <drew.adams <at> oracle.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 26.3; `face-remap-set-base' seems to be bugged
Date: Tue, 15 Dec 2020 16:31:31 -0800 (PST)

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''

Message #8 received at 45264 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Drew Adams <drew.adams <at> oracle.com>
Cc: 45264 <at> debbugs.gnu.org
Subject: Re: bug#45264: 26.3; `face-remap-set-base' seems to be bugged
Date: Sat, 19 Dec 2020 12:20:04 +0200

> Date: Tue, 15 Dec 2020 16:31:31 -0800 (PST)
> From: Drew Adams <drew.adams <at> 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?

Message #11 received at 45264 <at> debbugs.gnu.org (full text, mbox):

From: Drew Adams <drew.adams <at> oracle.com>
To: Eli Zaretskii <eliz <at> gnu.org>, Drew Adams <drew.adams <at> oracle.com>
Cc: 45264 <at> debbugs.gnu.org
Subject: RE: bug#45264: 26.3; `face-remap-set-base' seems to be bugged
Date: Sat, 19 Dec 2020 10:32:16 -0800 (PST)

> > 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.

Message #16 received at 45264-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Drew Adams <drew.adams <at> oracle.com>
Cc: 45264-done <at> debbugs.gnu.org
Subject: Re: bug#45264: 26.3; `face-remap-set-base' seems to be bugged
Date: Sat, 19 Dec 2020 20:58:02 +0200

> Date: Sat, 19 Dec 2020 10:32:16 -0800 (PST)
> From: Drew Adams <drew.adams <at> oracle.com>
> Cc: 45264 <at> 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.

Message #19 received at 45264-done <at> debbugs.gnu.org (full text, mbox):

From: Drew Adams <drew.adams <at> oracle.com>
To: Eli Zaretskii <eliz <at> gnu.org>, Drew Adams <drew.adams <at> oracle.com>
Cc: 45264-done <at> debbugs.gnu.org
Subject: RE: bug#45264: 26.3; `face-remap-set-base' seems to be bugged
Date: Sat, 19 Dec 2020 11:28:31 -0800 (PST)

> > 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..."

Message #22 received at 45264 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Drew Adams <drew.adams <at> oracle.com>
Cc: 45264 <at> debbugs.gnu.org
Subject: Re: bug#45264: 26.3; `face-remap-set-base' seems to be bugged
Date: Sat, 19 Dec 2020 21:40:06 +0200

> Date: Sat, 19 Dec 2020 11:28:31 -0800 (PST)
> From: Drew Adams <drew.adams <at> oracle.com>
> Cc: 45264-done <at> 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.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.