Re: how to track down "invalid face attribute" errors?

[Felix Dietrich]

Hello Emanuel,

Emanuel Berg <incal@dataswamp.org> writes:

> Stefan Monnier via Users list for the GNU Emacs text editor wrote:
>
>>> Sounds like that error message should be changed to include
>>> what face causes trouble, if possible?
>>
>> IIUC this message comes when the face is not a symbol but
>> a plist of properties, so there's a good chance that
>> printing the face will just say (:foreground nil) and thus
>> won't help you. [...]
>
> Okay, but why are the face used before they even have a name?

Those are *anonymous* faces.  You can attach these to text or overlay
properties.  If I recall correctly, the Emacs Lisp manual recommends
using named faces but recognises that anonymous faces might have some
uses.

> Isn't it better to initialize the faces first, and if it is
> attempted to define them using invalid attributes, then that
> would be an error?

This already happens to a degree for named faces: if you use ‘defface’
to create a face with :foreground set to nil, subsequent calls to
‘face-attribute’ for :foreground on that face will return ‘unspecified’:

#+begin_src emacs-lisp
  (progn
    (defface my/test '((t . (:foreground nil))) "test face")
    (face-attribute 'my/test :foreground))
#+end_src

#+RESULTS:
: unspecified


If I skimmed the code correctly, this is done in
“xfaces.c:internal-set-lisp-face-attribute” around line 3344 (note that
there is also a check to ensure that the :foreground attribute is a
string):

#+NAME: xfaces.c:3344
#+begin_src c
  else if (EQ (attr, QCforeground))
    {
      /* Compatibility with 20.x.  */
      if (NILP (value))
        value = Qunspecified;
      if (!UNSPECIFIEDP (value) && !IGNORE_DEFFACE_P (value))
        {
          /* Don't check for valid color names here because it depends
             on the frame (display) whether the color will be valid
             when the face is realized.  */
          CHECK_STRING (value);
          if (SCHARS (value) == 0)
            signal_error ("Empty foreground color value", value);
        }
#+end_src


In “xfaces.c:merge_face_ref”, around line 2728, on the other hand,
everything but strings is considered an error for the :foreground
attribute:

#+NAME: xfaces.c:2728
#+begin_src c
              else if (EQ (keyword, QCforeground))
                {
                  if (STRINGP (value))
                    to[LFACE_FOREGROUND_INDEX] = value;
                  else
                    err = true;
                }
#+end_src c

Maybe just a missing check to allow NILP for the :foreground value in
“xfaces.c:2728” (completely untested):

diff --git a/src/xfaces.c b/src/xfaces.c
index f7ee19195f..ab624a8d87 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -2727,7 +2727,9 @@ merge_face_ref (struct window *w,
                }
              else if (EQ (keyword, QCforeground))
                {
-                 if (STRINGP (value))
+                 if (NILP (value))
+                   to[LFACE_FOREGROUND_INDEX] = Qunspecified;
+                 else if (STRINGP (value))
                    to[LFACE_FOREGROUND_INDEX] = value;
                  else
                    err = true;

-- 
Felix Dietrich